Add SDHC common driver - #114752
Conversation
978c7ab to
b6a8052
Compare
| int sdhc_host_props_init(struct sdhc_host_props *props, const struct sdhc_config *cfg) | ||
| { | ||
| props->max_current_330 = cfg->max_current_330; | ||
| props->max_current_300 = cfg->max_current_300; | ||
| props->max_current_180 = cfg->max_current_180; | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
this function should be a inline function, it only copies 3 vars
There was a problem hiding this comment.
For now, it is...
Let me implement full support for sdhc properties defined in binding file. Introducing some debug messages are better. And in the future I think this function will handle more things.
| # Copyright 2022 NXP | ||
| # SPDX-FileCopyrightText: Copyright 2022, 2026 NXP | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| if(CONFIG_SDHC) |
There was a problem hiding this comment.
instead remove this, it is not needed, as this cmakefile is already only conditionally included:
Line 89 in bf3339d
There was a problem hiding this comment.
Thanks. Will do that.
b6a8052 to
05a2658
Compare
|
Updated to v2. Changes include,
Thanks. |
|
@danieldegrasse Could you help to review when you get any chance? |
danieldegrasse
left a comment
There was a problem hiding this comment.
I agree that we should consider a common SDHC driver- when I initially proposed this subsystem I felt it did not make sense, but so many vendors either reuse IP from Synopsis or Cadence or follow the SD Host Controller Spec (A2) pretty closely that I do think a common driver is reasonable.
That being said, I think this driver needs a bit more logic in order to move forwards. At a minimum lets target implementing a shared "transfer" function that uses the registers in the SD host controller specification.
I'd like to have an architecture where the SD subsystem implements a common SDHC driver, with public functions that other drivers can utilize. This way IP blocks that are partially compliant can use the common functions where possible, and write their own for other cases. Similar to the can_mcan.c driver in the CAN subsystem
| property-blocklist: | ||
| - bus-width |
There was a problem hiding this comment.
I'm not following here. Why not just removing bus-width prop from the list props below ?
There was a problem hiding this comment.
I didn't remove the property in this file, because it had different default value with in sdhc.yaml.
Keeping it here can ensure native/out-of-tree platforms not affected.
Thanks.
mathieuchopstm
left a comment
There was a problem hiding this comment.
There seems to be confusion between:
- hardware-independent operations implemented similarly/identically across drivers
- hardware implementations complying with the SD Host Controller Simplified Specification (SD Specifications Part A2)
The former can be refactored into a common implementation (as done in ADC and other subsystems); the latter can be covered by a unique driver, potentially layered like the Bosch M-CAN driver as pointed out by @danieldegrasse.
But the two concepts are orthogonal so I'm not sure I understand what this PR is trying to do, because it seems to conflate the two? Clearly, it is not reasonable to force SDHC-Specification-compliant-controller-only fields in the common API/structures: this should be opt-in for the drivers which are interested.
Vendor SDHC hosts were actually designed per common specification. There may be specific implementation in hardware but most should be compatible.
The SD Host Controller Simplified Specification is not binding:
The Secure Digital (SD) Host Standard Specification is the SD Association's (SDA) guideline for designing SD Host Controllers and related vendor products.
Within the scope of the SD Associations adherence to this specification is not mandatory.
From a rough look at tree, I'm estimating ~50% of IPs covered by existing drivers do not comply with the specification; the claim that "most hardware complies" doesn't seem accurate.
| unsigned int adma3_support: 1; /**< ADMA3 support */ | ||
| unsigned int vdd2_180_support: 1; /**< 1.8V VDD2 support */ | ||
| unsigned int _rsvd_61: 3; /**< Reserved */ | ||
| uint64_t timeout_clk_freq: 6; /**< Timeout clock frequency */ |
There was a problem hiding this comment.
As pointed out by SonarCloud: bitfields must have type int, signed int or (in this case) unsigned int (support for other types is implementation-defined).
There was a problem hiding this comment.
Will drop the changes.
Thanks.
You're right. I initially didn't realize Then we may need a sdhc_common driver for common APIs, and a sdhc_standard driver for standard controller APIs. Thank you very much. |
1db419a to
f6e4271
Compare
|
Updated to v4. Changes include,
Thanks a lot. |
|
mathieuchopstm
left a comment
There was a problem hiding this comment.
Seems like a much better direction 🙂
There was a problem hiding this comment.
FWIW, this could remain as just:
power-delay-ms:
required: trueNot mandatory - removal is fine too.
There was a problem hiding this comment.
Thanks, let's remove it.
Because for required property, it should not have default value in common yaml.
| bus-width: | ||
| type: int | ||
| description: | | ||
| SDHC data bus width. |
There was a problem hiding this comment.
Nit: SDHC data bus width (in bits).
There was a problem hiding this comment.
Thanks, will update.
There was a problem hiding this comment.
In commit titled drivers: sdhc: add common driver: prefer "common code" to "driver" since this is not directly related to actual hardware
There was a problem hiding this comment.
Thanks, will rephrase.
| * Version 3.00 | ||
| * @{ | ||
| */ | ||
| #define SDHC_REG_SDMA_SYS_ADDR_ARG2 0x000 /**< SDMA System Address / Argument 2 Register */ |
There was a problem hiding this comment.
Nit: indicating register sizes would be nice
e.g.:
/**< SDMA System Address / Argument 2 Register (u32) */
There was a problem hiding this comment.
Thanks. Will add that.
| help | ||
| Enable SDHC scatter-gather API. | ||
|
|
||
| config SDHC_STANDARD |
There was a problem hiding this comment.
Thanks, will update.
| #define SDHC_REG_CAPABILITIES_CM_MASK (0xffULL << 48) | ||
| #define SDHC_REG_CAPABILITIES_CM_SHIFT 48 | ||
| /** @} */ | ||
|
|
There was a problem hiding this comment.
It is probably wiser to define:
struct sdhc_standard_host_common_config {
struct sdhc_common_config subsys_common;
DEVICE_MMIO_NAMED(xxx);
};which would replace the sdhc_common_config for spec-compliant controllers. These functions would then consume the usual const struct device *.
There was a problem hiding this comment.
As I understand, DEVICE_MMIO_NAMED_ROM() must be put in dev->config, nesting will not be workable.
So, will keep struct sdhc_standard_host for vendor drivers to use.
Vendor driver initialize it like registering itself as standard host.
Thanks.
There was a problem hiding this comment.
sdhc_standard.c -> sdhc_standard_core.c; ditto for .h file
There was a problem hiding this comment.
Thanks, will update.
| uint32_t sdhc_standard_read32(struct sdhc_standard_host *host, uint32_t reg); | ||
| void sdhc_standard_write32(struct sdhc_standard_host *host, uint32_t reg, uint32_t value); | ||
| uint64_t sdhc_standard_read64(struct sdhc_standard_host *host, uint32_t reg); | ||
| void sdhc_standard_write64(struct sdhc_standard_host *host, uint32_t reg, uint64_t value); |
There was a problem hiding this comment.
Since that's intended to be called by the core itself, these should be kept private. But I think this is overkill, see comment in iMX driver.
There was a problem hiding this comment.
In vendor drivers, they may have registers accessing for specific process.
Then they can reuse IO access functions and registers definition.
Without these APIs exported, vendor drivers also could implement that (maybe efficient or redundant).
But exporting them provides an available way to choose.
My thought is from Linux :)
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.h#L731
Thanks.
| * - imx_usdhc_reg32_read | ||
| * - imx_usdhc_reg32_write | ||
| */ | ||
| static uint64_t imx_usdhc_reg32_read(const struct device *dev, uint32_t reg, uint8_t reg_bytes) |
There was a problem hiding this comment.
This code is overall pretty generic. I think it should be in the core driver rather than here.
Here's my proposal:
-
add a Kconfig option
SDHC_STANDARD_CORE_MMIO_SIZE8/16/32(/64) - or1/2/4(/8)- Indicates the size for all accesses to the HC MMIO interface
- (maybe separate
booloptions instead of a singleintoption is better)- Downside: all controllers in a system must support the same access size
- This can be turned into a property in the future if necessary...
-
Implement these MMIO wrappers in the core driver
- For now, only
CONFIG_SDHC_STANDARD_CORE_MMIO_SIZE=32support is sufficient (BUILD_ASSERT()it)
- For now, only
#if CONFIG_SDHC_STANDARD_CORE_MMIO_SIZE == 32
uint8_t sdhc_std_core_read8(mem_addr_t reg)
{
return (uint8_t)sys_read32(reg);
}
/* ...all others, always using sys_read32()/sys_write32()... */
#else
#error Unsupported MMIO size
#endif - Perform all I/O directly in the driver
- using
dev->config->sdhc_std_common.mmioas base address - c.f. comment about having a common config structure
- using
This should be sufficiently extensible to allow support for platforms where MMIO is larger (64-bit) or smaller (16-bit/8-bit) in the future if it ever becomes necessary.
There was a problem hiding this comment.
EDIT: see #114752 (comment)
Just go full raw 32-bit I/O, not need for this abstraction layer at all. Controllers which don't support 32-bit operations are non-compliant with the Spec.
There was a problem hiding this comment.
Thanks a lot. That's really good discussion. The specification is sometime hard to understand...
Let use raw 32-bit I/O, for standard registers with different width.
Thanks.
| /* There is only lower 32-bit capabilities register on usdhc */ | ||
| usdhc_caps = imx_usdhc_reg32_read(dev, reg, 4); |
There was a problem hiding this comment.
The issue seems to rather be that the uSDHC capabilities register(s) are not compliant with the Specification. Using the core driver to parse them seems like the wrong approach: just read the register and decode it from this driver instead?
In any case, this approach doesn't seem clean at all.
There was a problem hiding this comment.
In fact, browsing quickly through the i.MX93 Applications Processor Reference Manual, it seems there are other registers which don't match the Standard Specification...
There was a problem hiding this comment.
There are several, NXP's uSDHC isn't really a compliant controller IMO. I believe that the Synopsis SD controller IP (and the one from Cadence as well) may be fully compliant, but NXP's only partially complies with the spec. This was one of the reasons I didn't write a generic SD controller driver when I introduced the subsystem- NXP wasn't compliant with the spec.
@yangbolu1991 is there any part of the controller that is fully compliant? It looks like the registers related to command arguments and responses are the same as the SDHC spec- I haven't checked the ADMA2 descriptor format though.
There was a problem hiding this comment.
@danieldegrasse @mathieuchopstm
Unfortunately, I only have experience on NXP uSDHC/eSDHC by now.
They are partial compatible with spec. There are some different with register/bit/process.
The difficulty to implement a full standard host driver is we don't have compliant platform to verify.
But with the basic driver framework here, more people can work on this gradually.
For the registers/bits which are not fully compliant, I think vendor driver can choose to handle totally in itself, or choose to do some io access fixup which can still reusing standard driver. That's the way linux using.
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-esdhc-imx.c#L1533
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-of-esdhc.c#L1266
I think no need to estimate how much a controller is complaint with spec.
For standard driver, we may leave any callback in process to let vendor driver to do by itself if needs in the future.
For vendor driver, Just feel free to choose to reuse some APIs if could, or handle by itself.
Thanks.
|
|
||
| LOG_MODULE_REGISTER(sdhc_standard, CONFIG_SDHC_LOG_LEVEL); | ||
|
|
||
| uint8_t sdhc_standard_read8(struct sdhc_standard_host *host, uint32_t reg) |
There was a problem hiding this comment.
In the SDHC spec, it states the following:
Implementation Note: Table 1-4 implies that the Host Driver should align register accesses on address boundaries matching the number of bytes in the access. That is, single byte accesses may be aligned on any offset within the register set; word (double byte) accesses should be aligned on two-byte offsets; and double-word (quad byte) accesses should be aligned on four-byte offsets. According to the feature (3), the Host Driver can always access Buffer Data Port register with double-word access.
Could we just restrict accesses to 32 bit, so we can always use sys_read32?
There was a problem hiding this comment.
For more context, the feature (3) it refers to is:
Details
(3) Buffer Control with Block Size
The buffer preserves data up to the block size specified by the Block Size register. Following
definitions of controlling buffer enable the Host Driver to access the Buffer Data Port register
repeatedly with 32-bit width regardless of block size.
In case of write operation, the buffer accumulates the data written through the Buffer Data Port
register. When the buffer pointer reaches the block size, Buffer Write Enable in the Present State
register changes 1 to 0. It means no more data can be written to the buffer. Excess data of the last
write is ignored. For example, if just lower 2 bytes data can be written to the buffer and a 32-bit
(4-byte) block of data is written to the Buffer Data Port register, the lower 2 bytes of data is written to
the buffer and the upper 2 bytes is ignored. Every time Buffer Write Enable changes 0 to 1, it
means a next block of data can be written to the buffer. A new blocks write shall always start from
BE[00] position. After that, a block of data can be written to the buffer without checking Buffer Write
Enable.
In case of read operation, every time Buffer Read Enable in the Present State register changes 0
to 1, a block of data can be read through the Buffer Data Port register. A new block read shall
always start from BE[00] position. After that a block of data can be read from the buffer without
checking Buffer Read Enable. Excess data of the last read is ignored. For example, if just lower 2
bytes of data are left in the buffer and a 32-bit (4-byte) read is performed, the lower 2 bytes is valid
but the upper 2 bytes is undefined. When the buffer pointer reaches block size, Buffer Read
Enable changes 1 to 0. It means no more data can be read from the buffer.
(In Version 4.20 of the specification, this is titled (8) Buffer Control with Block Size but the implementation note still reads feature (3) - presumably an error)
With this context, it seems like this note should be understood as Double-word accesses to the Buffer Data Port [if supported by the implementation] will always be valid - as opposed to e.g., byte accesses which must be done in ascending order (see (2) Sequential and continuous access). In particular, I don't think this note should be interpreted as the Host Controller shall support double-word accesses...
...but looking through the specification again, I found the following paragraph:
SD Host Controller Simplified Specification Version 4.20
1.2 Register Map
The standard register map is classified in 18 parts listed below.
The Host Controller shall support byte, word and double word accesses to these registers.
Reserved bits in all registers shall be fixed to zero.
The Host Controller shall ignore writes to reserved bits; however, the Host Driver should write them as zero to ensure compatibility with possible future revisions to this Specification.
In older versions of the specification
SD Host Controller Simplified Specification Version 3.00
1.2 Register Map
The standard register map is classified in 12 parts listed below.
The Host Controller shall support byte, word and double word accesses to these registers.
Reserved bits in all registers shall be fixed to zero.
The Host Controller shall ignore writes to reserved bits; however, the Host Driver should write them as zero to ensure compatibility with possible future revisions to this Specification.
SD Host Controller Simplified Specification Version 2.00
1.2 Register Map
The standard register map is classified in 9 parts listed below.
The Host Controller shall support byte, word and double word accesses to these registers.
Reserved bits in all registers shall be fixed to zero.
The Host Controller shall ignore writes to reserved bits; however, the Host Driver should write them as zero to ensure compatibility with possible future revisions to this Specification.
I was actually wondering if the Specification imposed any such restriction (or if a controller supporting only 16-bit accesses would be valid for example) and must have missed this paragraph.
With this in mind, full +1 from me on using only native 32-bit I/O operations since support of these operations is required by the specification.
(that's what I proposed in #114752 (comment) but as extensible mechanism, which we should not need given this - except to support 16-bit platforms, perhaps... 🙂)
There was a problem hiding this comment.
Thanks a lot for the good discussion.
Let's use sys_read32 in default in standard driver for any width registers.
But still leave 8/16/32/64 access callbacks if vendor driver want their implementation, or do some fixup for register complaint.
| /* There is only lower 32-bit capabilities register on usdhc */ | ||
| usdhc_caps = imx_usdhc_reg32_read(dev, reg, 4); |
There was a problem hiding this comment.
There are several, NXP's uSDHC isn't really a compliant controller IMO. I believe that the Synopsis SD controller IP (and the one from Cadence as well) may be fully compliant, but NXP's only partially complies with the spec. This was one of the reasons I didn't write a generic SD controller driver when I introduced the subsystem- NXP wasn't compliant with the spec.
@yangbolu1991 is there any part of the controller that is fully compliant? It looks like the registers related to command arguments and responses are the same as the SDHC spec- I haven't checked the ADMA2 descriptor format though.
Kept unified max-current-xxx property in sdhc.yaml. Dopped them in other binding files. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Kept unified power-delay-ms property in sdhc.yaml. Dopped power-delay-ms in other binding files. - If host requires it, add it in dts node. - If not, there is default value used. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Added common bus-width property in sdhc.yaml. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Renamed sdhc_helpers.h to sdhc_common.h for more usage in the future. The SDHC needs a common c file for vendor drivers reusing, and this file can be as its header file. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Added sdhc_common.c for common code. For now, it provided only one common API, for SDHC device tree properties initialization. Vendor drivers can convert to use sdhc_common_config and sdhc_common_dt_props_init API. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Added standard controller driver per SD Specifications Part A2 SD Host Controller Simplified Specification (Version 3.00) which matches current sdhc and sd subsystem features support. It can be updated in the future for example to v4.20 if new features are required to support. For now, the driver provided standard registers definition and access APIs, also provided a sdhc_standard_capabilites_init API for host capabilities initialization. The host controller driver needs to initialize a sdhc_standard_host structure variable, then it can reuse the standard controller driver APIs. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Converted imx_usdhc to use common API to init properties. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
Converted imx_usdhc to use standard driver APIs for capabilities init. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
|
@danieldegrasse @mathieuchopstm Updated to v5. Changes include,
Thanks. |
|
It seems the twister issue existed on main code base. @VynDragon May I know if you are the right person to fix it? Thanks.
|



This PR added sdhc common driver and sdhc standard controller driver.
And converted imx_usdhc to use them.
sdhc common driver
Added sdhc_common driver. For now, it provided only one
common API, for SDHC device tree properties initialization.
Vendor drivers can convert to use sdhc_common_config and
sdhc_common_dt_props_init API.
sdhc standard controller driver
Added standard controller driver per SD Specifications Part A2
SD Host Controller Simplified Specification (Version 3.00).
For now, the driver provided standard registers definition and
access APIs, also provided a sdhc_standard_capabilites_init API
for host capabilities initialization.
The host controller driver needs to initialize a sdhc_standard_host
structure variable, then it can reuse the standard controller
driver APIs.
Thanks.