[PATCH 1/2] mmc: Imply HS200 cap with mmc-hs400 prop to match linux

[Dragan Simic]

Hello Quentin,

On 2024-04-09 18:02, Quentin Schulz wrote:
> On 4/9/24 17:58, Jonas Karlman wrote:
>> On 2024-04-09 17:27, Quentin Schulz wrote:
>>> On 4/8/24 23:06, Jonas Karlman wrote:
>>>> eMMC nodes in linux device tree files typically only contain a 
>>>> mmc-hs400
>>>> prop to signal support for both HS400 and HS200. However, U-Boot 
>>>> require
>>>> an explicit mmc-hs200 prop to signal support for the HS200 mode.
>>>>   > Fix this by follow linux and imply HS200 cap when HS400 cap is 
>>>> signaled
>>>> using a mmc-hs400 prop.
>>> 
>>> Technically speaking, the DT binding should be the one and only 
>>> source
>>> of truth and should be implementation-agnostic.
>>> 
>>> There it says:
>>> """
>>>     mmc-hs400-1_2v:
>>>       $ref: /schemas/types.yaml#/definitions/flag
>>>       description:
>>>         eMMC HS400 mode (1.2V I/O) is supported.
>>> 
>>>     mmc-hs400-1_8v:
>>>       $ref: /schemas/types.yaml#/definitions/flag
>>>       description:
>>>         eMMC HS400 mode (1.8V I/O) is supported.
>>> """
>>> 
>>> So I'd say, the DTs should be fixed to add mmc-hs200 as well wherever 
>>> it
>>> makes sense.

Good point, but I think that the descriptions in this Linux kernel
binding should be fixed instead, to eliminate this ambiguity.  I'll
explain this further below.

>>> The point of the DT/DT binding is to be system-agnostic and
>>> representative of the **HW** implementation. At least that's what the 
>>> DT
>>> people want it to be.
>>> 
>>> If the eMMC standard doesn't allow to have HS400 without HS200, then 
>>> I
>>> think this change is acceptable as is, because it is the reality of 
>>> the
>>> HW standard. Couldn't find this implied in the standard though (but I
>>> just skimmed through).
>>> 
>>> It's also quite surprising, as it's not because the eMMC works with
>>> HS400 that it necessarily does with HS200 or that it's desired (EMI,
>>> signal integrity/stability, etc...)?
>>> 
>>> Now, it wouldn't be the first time U-Boot follows whatever is done in
>>> Linux, so... up to you/the maintainers :)
>> 
>> Agree that implying HS200 does not fully make sense, however it was 
>> part
>> of the original Linux binding when HS400 was added in v3.16-rc1 [1] so 
>> I
>> think that this is the expected behavior and changing it may be an ABI
>> breakage.
> 
> I'm not advocating undoing the kernel "hack", but rather make it so
> that we add hs200 to DTs where it's actually supported instead of
> doing the same hack the kernel does. In that case, we wouldn't need
> the hack anymore.
> 
> (well maybe it isn't a hack per-se, but for lack of more info on that,
> I call the kernel implementation this :) )

Let's keep in mind that the troublesome DT properties describe the
capabilities of the MMC controller and the board, not the capabilities
of the MMC storage device.  As we know, eMMC devices provide automatic
detection capabilities, to allow the host to determine its supported
modes, and match them with the ones the host is configured to support.
It's all described in the JEDEC standards.

Having that in mind, I find the approach in the Linux kernel rather
reasonable, because I highly doubt that some MMC controllers support,
for example, HS400 without supporting DDR52, or HS400 without supporting
DDR52.  A reasonable approach for an MMC IP block is to make it capable
of supporting all the speeds below its highest supported speed, to make
itself capable of supporting more, if not all, MMC storage devices.

In fact, I'll probably go ahead and submit a Linux kernel patch that
updates the descriptions in the binding, to hopefully eliminate any
ambiguities like these.  I hope you agree.