| Message ID | 20240408210619.3749231-1-jonas@kwiboo.se |
|---|---|
| State | Accepted |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | [1/2] mmc: Imply HS200 cap with mmc-hs400 prop to match linux | expand |
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);
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
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
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
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
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.
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/
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
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
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.
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.
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
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
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 :)
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);
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(-)