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

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

[Jonas Karlman]

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.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
This fixes booting from eMMC on nanopc-t6-rk3588 and quartzpro64-rk3588
that probably broke with commit 6de9d7b2f13c ("rockchip: rk35xx: Enable
eMMC HS200 mode by default").
---
 drivers/mmc/mmc-uclass.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
index 328456831dd2..1349da72b102 100644
--- a/drivers/mmc/mmc-uclass.c
+++ b/drivers/mmc/mmc-uclass.c
@@ -251,9 +251,9 @@ int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg)
 	if (dev_read_bool(dev, "mmc-hs200-1_2v"))
 		cfg->host_caps |= MMC_CAP(MMC_HS_200);
 	if (dev_read_bool(dev, "mmc-hs400-1_8v"))
-		cfg->host_caps |= MMC_CAP(MMC_HS_400);
+		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
 	if (dev_read_bool(dev, "mmc-hs400-1_2v"))
-		cfg->host_caps |= MMC_CAP(MMC_HS_400);
+		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
 	if (dev_read_bool(dev, "mmc-hs400-enhanced-strobe"))
 		cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
 
-- 
2.43.2

[PATCH 2/2] mmc: Add support for the no-mmc-hs400 prop

[Jonas Karlman]

The linux commit f722e650d965 ("mmc: core: add support for disabling
HS400 mode via DT") added support for a no-mmc-hs400 prop.

Add support for the no-mmc-hs400 prop to disable HS400 host caps.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/mmc/mmc-uclass.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
index 1349da72b102..1b454e8ec758 100644
--- a/drivers/mmc/mmc-uclass.c
+++ b/drivers/mmc/mmc-uclass.c
@@ -256,6 +256,9 @@ int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg)
 		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
 	if (dev_read_bool(dev, "mmc-hs400-enhanced-strobe"))
 		cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
+	if (dev_read_bool(dev, "no-mmc-hs400"))
+		cfg->host_caps &= ~(MMC_CAP(MMC_HS_400) |
+				    MMC_CAP(MMC_HS_400_ES));
 
 	if (dev_read_bool(dev, "non-removable")) {
 		cfg->host_caps |= MMC_CAP_NONREMOVABLE;
-- 
2.43.2

Re: [PATCH 2/2] mmc: Add support for the no-mmc-hs400 prop

[Dragan Simic]

On 2024-04-08 23:06, Jonas Karlman wrote:
> The linux commit f722e650d965 ("mmc: core: add support for disabling
> HS400 mode via DT") added support for a no-mmc-hs400 prop.
> 
> Add support for the no-mmc-hs400 prop to disable HS400 host caps.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

The description above should use "Linux" instead od "linux", but that's
perhaps not worth sending the v2.

Otherwise, looking good to me.  Great job catching this!

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  drivers/mmc/mmc-uclass.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index 1349da72b102..1b454e8ec758 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -256,6 +256,9 @@ int mmc_of_parse(struct udevice *dev, struct
> mmc_config *cfg)
>  		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
>  	if (dev_read_bool(dev, "mmc-hs400-enhanced-strobe"))
>  		cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
> +	if (dev_read_bool(dev, "no-mmc-hs400"))
> +		cfg->host_caps &= ~(MMC_CAP(MMC_HS_400) |
> +				    MMC_CAP(MMC_HS_400_ES));
> 
>  	if (dev_read_bool(dev, "non-removable")) {
>  		cfg->host_caps |= MMC_CAP_NONREMOVABLE;

RE: [PATCH 2/2] mmc: Add support for the no-mmc-hs400 prop

[Jaehoon Chung]

Hi

> -----Original Message-----
> From: Jonas Karlman <jonas@kwiboo.se>
> Sent: Tuesday, April 9, 2024 6:06 AM
> To: Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>; u-boot@lists.denx.de
> Subject: [PATCH 2/2] mmc: Add support for the no-mmc-hs400 prop
> 
> The linux commit f722e650d965 ("mmc: core: add support for disabling
> HS400 mode via DT") added support for a no-mmc-hs400 prop.
> 
> Add support for the no-mmc-hs400 prop to disable HS400 host caps.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/mmc-uclass.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index 1349da72b102..1b454e8ec758 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -256,6 +256,9 @@ int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg)
>  		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
>  	if (dev_read_bool(dev, "mmc-hs400-enhanced-strobe"))
>  		cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);
> +	if (dev_read_bool(dev, "no-mmc-hs400"))
> +		cfg->host_caps &= ~(MMC_CAP(MMC_HS_400) |
> +				    MMC_CAP(MMC_HS_400_ES));
> 
>  	if (dev_read_bool(dev, "non-removable")) {
>  		cfg->host_caps |= MMC_CAP_NONREMOVABLE;
> --
> 2.43.2

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

[Dragan Simic]

Hello Jonas,

On 2024-04-08 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.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

The description above should use "Linux" instead od "linux", but that's
perhaps not worth sending the v2.

Otherwise, looking good to me.  Great job catching this!

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
> This fixes booting from eMMC on nanopc-t6-rk3588 and quartzpro64-rk3588
> that probably broke with commit 6de9d7b2f13c ("rockchip: rk35xx: Enable
> eMMC HS200 mode by default").
> ---
>  drivers/mmc/mmc-uclass.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index 328456831dd2..1349da72b102 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -251,9 +251,9 @@ int mmc_of_parse(struct udevice *dev, struct
> mmc_config *cfg)
>  	if (dev_read_bool(dev, "mmc-hs200-1_2v"))
>  		cfg->host_caps |= MMC_CAP(MMC_HS_200);
>  	if (dev_read_bool(dev, "mmc-hs400-1_8v"))
> -		cfg->host_caps |= MMC_CAP(MMC_HS_400);
> +		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
>  	if (dev_read_bool(dev, "mmc-hs400-1_2v"))
> -		cfg->host_caps |= MMC_CAP(MMC_HS_400);
> +		cfg->host_caps |= MMC_CAP(MMC_HS_400) | MMC_CAP(MMC_HS_200);
>  	if (dev_read_bool(dev, "mmc-hs400-enhanced-strobe"))
>  		cfg->host_caps |= MMC_CAP(MMC_HS_400_ES);

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

[Quentin Schulz]

Hi Jonas,

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.

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 :)

Reviewed-by: Quentin Schulz <quentin.schulz@theobrma-systems.com>

Cheers,
Quentin

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

[Jonas Karlman]

Hi Quentin,

On 2024-04-09 17:27, Quentin Schulz wrote:
> Hi Jonas,
> 
> 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.
> 
> 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.

The original Linux commit [1] mention:
"Specially, if host can support HS400, it means that host can also
support HS200."

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c373eb489b27b268c9b8c267b212d10864bc8cdd

Regards,
Jonas

> 
> Reviewed-by: Quentin Schulz <quentin.schulz@theobrma-systems.com>
> 
> Cheers,
> Quentin

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

[Quentin Schulz]

Hi Jonas,

On 4/9/24 17:58, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2024-04-09 17:27, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> 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.
>>
>> 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 :) )

Cheers,
Quentin

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

[Jonas Karlman]

Hi Quentin,

On 2024-04-09 18:02, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 4/9/24 17:58, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2024-04-09 17:27, Quentin Schulz wrote:
>>> Hi Jonas,
>>>
>>> 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.
>>>
>>> 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.

I will add a patch that adds the missing mmc-hs200 props to affected
rk3588 boards, nanopc-t4 and quartzpro64 in v2 of the "rockchip: rk35xx:
Miscellaneous fixes and updates" series.

Also turns out the issue with those boards was because of my other "mmc:
rockchip_sdhci: Revert 4 blocks PIO mode read limit for RK35xx" patch,
so will need to rework that revert some more before posting a v2 of that
patch.

For this patch it is fully up to the maintainers if U-Boot wants to
mimic Linux kernel or not.

Regards,
Jonas

> 
> (well maybe it isn't a hack per-se, but for lack of more info on that, I 
> call the kernel implementation this :) )
> 
> Cheers,
> Quentin

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

[Dragan Simic]

Hello Jonas,

On 2024-04-09 18:30, Jonas Karlman wrote:
> 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.
>>>> 
>>>> 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.
> 
> I will add a patch that adds the missing mmc-hs200 props to affected
> rk3588 boards, nanopc-t4 and quartzpro64 in v2 of the "rockchip: 
> rk35xx:
> Miscellaneous fixes and updates" series.
> 
> Also turns out the issue with those boards was because of my other 
> "mmc:
> rockchip_sdhci: Revert 4 blocks PIO mode read limit for RK35xx" patch,
> so will need to rework that revert some more before posting a v2 of 
> that
> patch.
> 
> For this patch it is fully up to the maintainers if U-Boot wants to
> mimic Linux kernel or not.

I think that the logic used in the Linux kernel should be followed,
because one of the goals should be to add as few "touches" to the
upstream DT files in U-Boot as possible.

If the kernel binding patch that I mentioned in my earlier email [1]
becomes accepted, that should be another reason to do so.

[1] 
https://lore.kernel.org/u-boot/9b62bbedf2c6d52b76a8ce1ce57dd35d@manjaro.org/

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

[Quentin Schulz]

Hi Dragan,

On 4/9/24 21:30, Dragan Simic wrote:
> Hello Jonas,
> 
> On 2024-04-09 18:30, Jonas Karlman wrote:
>> 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.
>>>>>
>>>>> 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.
>>
>> I will add a patch that adds the missing mmc-hs200 props to affected
>> rk3588 boards, nanopc-t4 and quartzpro64 in v2 of the "rockchip: rk35xx:
>> Miscellaneous fixes and updates" series.
>>
>> Also turns out the issue with those boards was because of my other "mmc:
>> rockchip_sdhci: Revert 4 blocks PIO mode read limit for RK35xx" patch,
>> so will need to rework that revert some more before posting a v2 of that
>> patch.
>>
>> For this patch it is fully up to the maintainers if U-Boot wants to
>> mimic Linux kernel or not.
> 
> I think that the logic used in the Linux kernel should be followed,
> because one of the goals should be to add as few "touches" to the
> upstream DT files in U-Boot as possible.
> 

I was suggesting to fix the upstream DT files as well.

Cheers,
Quentin

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

[Dragan Simic]

Hello Quentin,

On 2024-04-10 10:47, Quentin Schulz wrote:
> On 4/9/24 21:30, Dragan Simic wrote:
>> On 2024-04-09 18:30, Jonas Karlman wrote:
>>> On 2024-04-09 18:02, Quentin Schulz wrote:
>>>> On 4/9/24 17:58, Jonas Karlman wrote:
>>>>> 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.
>>> 
>>> I will add a patch that adds the missing mmc-hs200 props to affected
>>> rk3588 boards, nanopc-t4 and quartzpro64 in v2 of the "rockchip: 
>>> rk35xx:
>>> Miscellaneous fixes and updates" series.
>>> 
>>> Also turns out the issue with those boards was because of my other 
>>> "mmc:
>>> rockchip_sdhci: Revert 4 blocks PIO mode read limit for RK35xx" 
>>> patch,
>>> so will need to rework that revert some more before posting a v2 of 
>>> that
>>> patch.
>>> 
>>> For this patch it is fully up to the maintainers if U-Boot wants to
>>> mimic Linux kernel or not.
>> 
>> I think that the logic used in the Linux kernel should be followed,
>> because one of the goals should be to add as few "touches" to the
>> upstream DT files in U-Boot as possible.
> 
> I was suggesting to fix the upstream DT files as well.

I see, but I think there's no need for that, as I already explained
further in my other response.

Re: [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.

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

[Quentin Schulz]

Hi Dragan,

On 4/9/24 21:28, Dragan Simic wrote:

[...]

> 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.
> 

So why do we have those properties specified in board DTSes instead of 
in the SoC DTSI? Logic would want us to have this defined in one place 
only. I assume the issue is that even if the eMMC chip itself says it 
supports HS400 but the HW routing or some other issue make it impossible 
to use, we need a way to disable it from the DT for that board?

> 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.
> 

That's true for the IP block which is self-contained in the SoC, but 
it's forgetting about the other part, the eMMC chip/card. It depends on 
the HW routing, where mistakes/limitations can happen. And I don't think 
we have a mechanism today to disable the modes set in the MMC controller 
for a given MMC card from DT (aside from /delete-property/ in board files).

> 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.

I for sure do not have enough knowledge in MMC to argue more than I just 
did, so having people with more experience/knowledge have a look at this 
would make sense, let's see what they have to say :)

Cheers,
Quentin

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

[Dragan Simic]

Hello Quentin,

On 2024-04-10 10:56, Quentin Schulz wrote:
> On 4/9/24 21:28, Dragan Simic wrote:
> 
>> 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.
> 
> So why do we have those properties specified in board DTSes instead of
> in the SoC DTSI? Logic would want us to have this defined in one place
> only. I assume the issue is that even if the eMMC chip itself says it
> supports HS400 but the HW routing or some other issue make it
> impossible to use, we need a way to disable it from the DT for that
> board?

Yes, we're having that defined in board DTs because some boards
aren't made well enough to provide the required signal integrity
for HS400, for example, so only HS200 may work well, despite the
fact that the SoC the board is based on supports HS400.

Some boards, such as the Pine64 RockPro64, are miswired so the
Data Strobe signal doesn't even reach the eMMC chip, rendering
HS400 impossible.  Other boards, such as the one found inside
the Pine64 PinePhone, provide wrong voltage to the eMMC, so only
DDR52 can work with no hardware modifications.

>> 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.
> 
> That's true for the IP block which is self-contained in the SoC, but
> it's forgetting about the other part, the eMMC chip/card. It depends
> on the HW routing, where mistakes/limitations can happen. And I don't
> think we have a mechanism today to disable the modes set in the MMC
> controller for a given MMC card from DT (aside from /delete-property/
> in board files).

Sure, but the same "lower speeds work" rule still applies, because
if a board limits the speed to HS200, due to signal integrity issues,
DDR52 is virtually guaranteed to work as well.  If some particular
eMMC chip supports that speed, of course.

Though, there are bugs in the Linux kernel when it comes to limiting
the board to DDR52, for example, using the DT properties.  I've already
spent some time fixing those issues, and I hope I'll have the patches
submitted to the mailing list rather soon.

>> 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.
> 
> I for sure do not have enough knowledge in MMC to argue more than I
> just did, so having people with more experience/knowledge have a look
> at this would make sense, let's see what they have to say :)

Sure, having more opinions is always good.

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

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Sent: Wednesday, April 10, 2024 5:57 PM
> To: Dragan Simic <dsimic@manjaro.org>
> Cc: Jonas Karlman <jonas@kwiboo.se>; Peng Fan <peng.fan@nxp.com>; Jaehoon Chung
> <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>; u-boot@lists.denx.de
> Subject: Re: [PATCH 1/2] mmc: Imply HS200 cap with mmc-hs400 prop to match linux
> 
> Hi Dragan,
> 
> On 4/9/24 21:28, Dragan Simic wrote:
> 
> [...]
> 
> > 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.
> >

I didn't see the above mail in my mailbox. So I can miss something.

> 
> So why do we have those properties specified in board DTSes instead of
> in the SoC DTSI? Logic would want us to have this defined in one place
> only. I assume the issue is that even if the eMMC chip itself says it
> supports HS400 but the HW routing or some other issue make it impossible
> to use, we need a way to disable it from the DT for that board?
> 
> > 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.
> >
> 
> That's true for the IP block which is self-contained in the SoC, but
> it's forgetting about the other part, the eMMC chip/card. It depends on
> the HW routing, where mistakes/limitations can happen. And I don't think
> we have a mechanism today to disable the modes set in the MMC controller
> for a given MMC card from DT (aside from /delete-property/ in board files).

The both opinions make sense.
But, It doesn't set to all capabilities when nodes has mmc-hs400-* property.

That's why it's describing to each property is because they want to clarify only which mode they use.

AFAIK, I can't remember exactly, there were some boards that even though HS400 is working fine, 
but HS200 was not working fine. (It's depends on which IP board is using.)

There were too many cases not to work fine because of *HW* design. 
eMMC and Host IP were supporting the HS400/200 and all modes, but there was a problem of handling clock.
So it couldn't use HS200/400 and other dual modes.

And We needs to know if it's working fine. 
If we want to use hs400 mode, but board can be working to other mode without any error.
Of course, we can see a mode as log. But it's at least approach to limit.

Best Regards,
Jaehoon Chung

> 
> > 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.
> 
> I for sure do not have enough knowledge in MMC to argue more than I just
> did, so having people with more experience/knowledge have a look at this
> would make sense, let's see what they have to say :)
> 
> Cheers,
> Quentin

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

[Dragan Simic]

Hello Jaehoon,

On 2024-04-17 01:33, Jaehoon Chung wrote:
>> -----Original Message-----
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> Sent: Wednesday, April 10, 2024 5:57 PM
>> To: Dragan Simic <dsimic@manjaro.org>
>> Cc: Jonas Karlman <jonas@kwiboo.se>; Peng Fan <peng.fan@nxp.com>; 
>> Jaehoon Chung
>> <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>; 
>> u-boot@lists.denx.de
>> Subject: Re: [PATCH 1/2] mmc: Imply HS200 cap with mmc-hs400 prop to 
>> match linux
>> 
>> On 4/9/24 21:28, Dragan Simic wrote:
>> 
>> [...]
>> 
>> > 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.
> 
> I didn't see the above mail in my mailbox. So I can miss something.

It seems that, for some reason, the mail server(s) for @samsung.com
don't like the IP address of the @manjaro.org mail server, so my email
messages to you get rejected.

>> So why do we have those properties specified in board DTSes instead of
>> in the SoC DTSI? Logic would want us to have this defined in one place
>> only. I assume the issue is that even if the eMMC chip itself says it
>> supports HS400 but the HW routing or some other issue make it 
>> impossible
>> to use, we need a way to disable it from the DT for that board?
>> 
>> > 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.
>> >
>> 
>> That's true for the IP block which is self-contained in the SoC, but
>> it's forgetting about the other part, the eMMC chip/card. It depends 
>> on
>> the HW routing, where mistakes/limitations can happen. And I don't 
>> think
>> we have a mechanism today to disable the modes set in the MMC 
>> controller
>> for a given MMC card from DT (aside from /delete-property/ in board 
>> files).
> 
> The both opinions make sense.
> But, It doesn't set to all capabilities when nodes has mmc-hs400-* 
> property.
> 
> That's why it's describing to each property is because they want to
> clarify only which mode they use.
> 
> AFAIK, I can't remember exactly, there were some boards that even
> though HS400 is working fine,
> but HS200 was not working fine. (It's depends on which IP board is 
> using.)

That's really good to know, thanks.

> There were too many cases not to work fine because of *HW* design.
> eMMC and Host IP were supporting the HS400/200 and all modes, but
> there was a problem of handling clock.
> So it couldn't use HS200/400 and other dual modes.

Yes, there are all kinds of eMMC signal integrity issues on different
designs.  There are also similar issues with different microSD cards,
causing some of them not to work in SDR104 mode on a particular board,
for example.

> And We needs to know if it's working fine.
> If we want to use hs400 mode, but board can be working to other mode
> without any error.
> Of course, we can see a mode as log. But it's at least approach to 
> limit.
> 
>> > 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.
>> 
>> I for sure do not have enough knowledge in MMC to argue more than I 
>> just
>> did, so having people with more experience/knowledge have a look at 
>> this
>> would make sense, let's see what they have to say :)

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

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Sent: Wednesday, April 10, 2024 12:27 AM
> To: Jonas Karlman <jonas@kwiboo.se>; Peng Fan <peng.fan@nxp.com>; Jaehoon Chung
> <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> Subject: Re: [PATCH 1/2] mmc: Imply HS200 cap with mmc-hs400 prop to match linux
> 
> Hi Jonas,
> 
> 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.
> 
> 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 :)

I want to follow the linux kernel. 

> 
> Reviewed-by: Quentin Schulz <quentin.schulz@theobrma-systems.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> 
> Cheers,
> Quentin