| Message ID | 20250407222702.2199047-1-jm@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA | expand |
Hi Judith, On Mon, Apr 07, 2025 at 05:27:00PM -0500, Judith Mendez wrote: > For all TI devices, timing was closed For Legacy and HS modes in > half cycle timing, where data is launched on the negative edge of > clock and latched on the following positive edge of clock. The > switch to full cycle timing happens when any of HIGH_SPEED_ENA, > V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. > > Currently HIGH_SPEED_ENA is set for HS modes and violates timing > requirements for TI devices so add a .set_hs_ena callback in > sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. > > There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a > specific Kingston eMMC due to the sequencing when enumerating to > HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not > set V1P8_SIGNAL_ENA be default. This fix was previously merged in > the kernel, but was reverted due to the "heuristics for enabling > the quirk"[0]. The new implementation applies the quirk based-off of > bus width, which should not be an issue since there is no internal > LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only > effect timing for MMC0 interface. > > [0] https://lore.kernel.org/linux-mmc/20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com/ > > Judith Mendez (2): > PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops > mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch > > drivers/mmc/host/sdhci.c | 55 +++++++++++++++++++++------------- > drivers/mmc/host/sdhci.h | 2 ++ > drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 20 deletions(-) > > -- > 2.49.0 > Thanks for the patches. We are currently experiencing this issue on the AM62 Solo SoC (hardware: Toradex Verdin AM62 Solo 512 MB), with the latest kernel 6.15-rc1. I tested your patches (added both on top of 6.15-rc1) and I can still see the issue, when the kernel boots: root@verdin-am62-15412807:~# dmesg | grep -i mmc1 [ 1.912123] mmc1: CQHCI version 5.10 [ 1.985262] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit [ 2.186954] mmc1: error -110 whilst initialising SD card [ 2.620625] mmc1: error -110 whilst initialising SD card [ 2.997642] mmc1: error -110 whilst initialising SD card [ 3.337071] mmc1: error -110 whilst initialising SD card This does not happen if I use commit 941a7abd4666 ("mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch"), as you described. Is there anything I missing or should test to make it work? Cheers, Hiago.
Hi Hiago, On 4/11/25 8:03 AM, Hiago De Franco wrote: > Hi Judith, > > On Mon, Apr 07, 2025 at 05:27:00PM -0500, Judith Mendez wrote: >> For all TI devices, timing was closed For Legacy and HS modes in >> half cycle timing, where data is launched on the negative edge of >> clock and latched on the following positive edge of clock. The >> switch to full cycle timing happens when any of HIGH_SPEED_ENA, >> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. >> >> Currently HIGH_SPEED_ENA is set for HS modes and violates timing >> requirements for TI devices so add a .set_hs_ena callback in >> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. >> >> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a >> specific Kingston eMMC due to the sequencing when enumerating to >> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not >> set V1P8_SIGNAL_ENA be default. This fix was previously merged in >> the kernel, but was reverted due to the "heuristics for enabling >> the quirk"[0]. The new implementation applies the quirk based-off of >> bus width, which should not be an issue since there is no internal >> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only >> effect timing for MMC0 interface. >> >> [0] https://lore.kernel.org/linux-mmc/20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com/ >> >> Judith Mendez (2): >> PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops >> mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch >> >> drivers/mmc/host/sdhci.c | 55 +++++++++++++++++++++------------- >> drivers/mmc/host/sdhci.h | 2 ++ >> drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++++++++++++ >> 3 files changed, 85 insertions(+), 20 deletions(-) >> >> -- >> 2.49.0 >> > > Thanks for the patches. We are currently experiencing this issue on the > AM62 Solo SoC (hardware: Toradex Verdin AM62 Solo 512 MB), with the > latest kernel 6.15-rc1. I tested your patches (added both on top of > 6.15-rc1) and I can still see the issue, when the kernel boots: > > root@verdin-am62-15412807:~# dmesg | grep -i mmc1 > [ 1.912123] mmc1: CQHCI version 5.10 > [ 1.985262] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit > [ 2.186954] mmc1: error -110 whilst initialising SD card > [ 2.620625] mmc1: error -110 whilst initialising SD card > [ 2.997642] mmc1: error -110 whilst initialising SD card > [ 3.337071] mmc1: error -110 whilst initialising SD card > > This does not happen if I use commit 941a7abd4666 ("mmc: sdhci_am654: > Add sdhci_am654_start_signal_voltage_switch"), as you described. > > Is there anything I missing or should test to make it work? The reason that patches fixes SD init for you is because in original patch, quirk was applied for both SD and eMMC with the exception of SD for am64x SoC. This patch only applies the quirk for eMMC. We cannot use the original implementation because the logic applying the quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x based boards will only have vmmc node since there is an internal LDO that is used to switch IO signal voltage instead of vqmmc. However, SolidRun HimmingBoard-T board has a different implementation on voltage regulator nodes in DT and the quirk applied to them as well and breaks their SD boot. So we now only apply the quirk for eMMC. Without this quirk applied to SD, am62x SD will continue having some stability issues. ~ Judith
On Fri, Apr 11, 2025 at 11:37:14AM -0500, Judith Mendez wrote: > > The reason that patches fixes SD init for you is because in original patch, > quirk was applied for both SD and eMMC with the exception of SD for am64x > SoC. This patch only applies the quirk for eMMC. > > We cannot use the original implementation because the logic applying the > quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x > based boards will only have vmmc node since there is an internal LDO that is > used to switch IO signal voltage instead of vqmmc. However, SolidRun > HimmingBoard-T board has a different implementation on voltage regulator > nodes in DT and the quirk applied to them as well and breaks their SD boot. > So we now only apply the quirk for eMMC. Without this quirk applied to SD, > am62x SD will continue having some stability issues. > > ~ Judith I got the idea now, thanks for the explanation, I missed in the original patches this was only about the eMMC and *not* the SD card. Is there any plans to send patches to the SD card as well? About that, I was wondering if instead of checking the bus_width to apply the fix for the eMMC, we could set a devicetree property to explicity apply the quirk. This way, we could also decide to apply it on the SD node without checking any bus width. As example: diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi index 1ea8f64b1b3b..c4423c09e809 100644 --- a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi @@ -1466,6 +1466,7 @@ &sdhci1 { vmmc-supply = <®_sdhc1_vmmc>; vqmmc-supply = <®_sdhc1_vqmmc>; ti,fails-without-test-cd; + ti,suppress-v1p8-ena; status = "disabled"; }; diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 4e1156a2f1b8..a0485c2bb549 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -87,7 +87,6 @@ #define CLOCK_TOO_SLOW_HZ 50000000 #define SDHCI_AM654_AUTOSUSPEND_DELAY -1 #define RETRY_TUNING_MAX 10 -#define BUS_WIDTH_8 8 /* Command Queue Host Controller Interface Base address */ #define SDHCI_AM654_CQE_BASE_ADDR 0x200 @@ -844,7 +843,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, struct device *dev = &pdev->dev; int drv_strength; int ret; - u32 bus_width; if (sdhci_am654->flags & DLL_PRESENT) { ret = device_property_read_u32(dev, "ti,trm-icp", @@ -886,9 +884,11 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, if (device_property_read_bool(dev, "ti,fails-without-test-cd")) sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST; - /* Suppress V1P8_SIGNAL_ENA for eMMC */ - device_property_read_u32(dev, "bus-width", &bus_width); - if (bus_width == BUS_WIDTH_8) + /* + * Suppress V1P8_SIGNAL_ENA for MMC devices if ti,suppress-v1p8-ena + * flag is present. + */ + if (device_property_read_bool(dev, "ti,suppress-v1p8-ena")) sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA; sdhci_get_of_property(pdev); Which makes our Kingston SD card work again: root@verdin-am62-15412807:~# dmesg | grep -i mmc1 [ 1.897055] mmc1: CQHCI version 5.10 [ 2.043673] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit [ 2.260610] mmc1: new UHS-I speed SDR104 SDHC card at address 0001 [ 2.268150] mmcblk1: mmc1:0001 SD32G 29.1 GiB Sorry if I missed something, would also be a valid solution? Cheers, Hiago.
Hi Hilago, On 4/11/25 2:48 PM, Hiago De Franco wrote: > On Fri, Apr 11, 2025 at 11:37:14AM -0500, Judith Mendez wrote: >> >> The reason that patches fixes SD init for you is because in original patch, >> quirk was applied for both SD and eMMC with the exception of SD for am64x >> SoC. This patch only applies the quirk for eMMC. >> >> We cannot use the original implementation because the logic applying the >> quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x >> based boards will only have vmmc node since there is an internal LDO that is >> used to switch IO signal voltage instead of vqmmc. However, SolidRun >> HimmingBoard-T board has a different implementation on voltage regulator >> nodes in DT and the quirk applied to them as well and breaks their SD boot. >> So we now only apply the quirk for eMMC. Without this quirk applied to SD, >> am62x SD will continue having some stability issues. >> >> ~ Judith > > I got the idea now, thanks for the explanation, I missed in the original > patches this was only about the eMMC and *not* the SD card. Is there any > plans to send patches to the SD card as well? > > About that, I was wondering if instead of checking the bus_width to > apply the fix for the eMMC, we could set a devicetree property to > explicity apply the quirk. This way, we could also decide to apply it on > the SD node without checking any bus width. As example: > > diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi > index 1ea8f64b1b3b..c4423c09e809 100644 > --- a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi > @@ -1466,6 +1466,7 @@ &sdhci1 { > vmmc-supply = <®_sdhc1_vmmc>; > vqmmc-supply = <®_sdhc1_vqmmc>; > ti,fails-without-test-cd; > + ti,suppress-v1p8-ena; > status = "disabled"; > }; > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 4e1156a2f1b8..a0485c2bb549 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -87,7 +87,6 @@ > #define CLOCK_TOO_SLOW_HZ 50000000 > #define SDHCI_AM654_AUTOSUSPEND_DELAY -1 > #define RETRY_TUNING_MAX 10 > -#define BUS_WIDTH_8 8 > > /* Command Queue Host Controller Interface Base address */ > #define SDHCI_AM654_CQE_BASE_ADDR 0x200 > @@ -844,7 +843,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, > struct device *dev = &pdev->dev; > int drv_strength; > int ret; > - u32 bus_width; > > if (sdhci_am654->flags & DLL_PRESENT) { > ret = device_property_read_u32(dev, "ti,trm-icp", > @@ -886,9 +884,11 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, > if (device_property_read_bool(dev, "ti,fails-without-test-cd")) > sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST; > > - /* Suppress V1P8_SIGNAL_ENA for eMMC */ > - device_property_read_u32(dev, "bus-width", &bus_width); > - if (bus_width == BUS_WIDTH_8) > + /* > + * Suppress V1P8_SIGNAL_ENA for MMC devices if ti,suppress-v1p8-ena > + * flag is present. > + */ > + if (device_property_read_bool(dev, "ti,suppress-v1p8-ena")) > sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA; > > sdhci_get_of_property(pdev); > > Which makes our Kingston SD card work again: > > root@verdin-am62-15412807:~# dmesg | grep -i mmc1 > [ 1.897055] mmc1: CQHCI version 5.10 > [ 2.043673] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit > [ 2.260610] mmc1: new UHS-I speed SDR104 SDHC card at address 0001 > [ 2.268150] mmcblk1: mmc1:0001 SD32G 29.1 GiB > > Sorry if I missed something, would also be a valid solution? Actually this was one of the previous implementations, I should have that original patch somewhere. My understanding was that we do not like adding new DT properties if we can find a way to apply the quirk in the driver. If this implementation flies with the maintainers, then we can go back to DT property implementation. Adrian, is this fine with you? ~ Judith
Hi Judith, On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: > Actually this was one of the previous implementations, I should have that > original patch somewhere. My understanding was that we do not like adding > new DT properties if we can find a way to apply the quirk in the driver. Got it, makes sense. This will work fine with eMMC, but for the SD card I am not seeing other option. Maybe we could use both implementations? diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 4e1156a2f1b8..db8ee66e76d8 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -888,7 +888,7 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, /* Suppress V1P8_SIGNAL_ENA for eMMC */ device_property_read_u32(dev, "bus-width", &bus_width); - if (bus_width == BUS_WIDTH_8) + if (bus_width == BUS_WIDTH_8 || device_property_read_bool(dev, "ti,suppress-v1p8-ena")) sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA; So the driver applies the quirk for eMMC and we set the DT property for SD card. Not sure which one is the best, but maybe it is another option. Let's see what Adrian also thinks about that. > > If this implementation flies with the maintainers, then we can go back to DT > property implementation. > > Adrian, is this fine with you? > > ~ Judith Cheers, Hiago.
Hello Judith On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: > My understanding was that we do not like adding new DT properties if > we can find a way to apply the quirk in the driver. ... > If this implementation flies with the maintainers, then we can go back to DT > property implementation. Not sure if this is clear, but this patch is NOT working according to our tests, we would need to fix it in a different way. Francesco > > > > > >
Hi Hiago, On 4/12/25 8:20 AM, Hiago De Franco wrote: > Hi Judith, > > On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: >> Actually this was one of the previous implementations, I should have that >> original patch somewhere. My understanding was that we do not like adding >> new DT properties if we can find a way to apply the quirk in the driver. > > Got it, makes sense. This will work fine with eMMC, but for the SD card > I am not seeing other option. Maybe we could use both implementations? > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 4e1156a2f1b8..db8ee66e76d8 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -888,7 +888,7 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev, > > /* Suppress V1P8_SIGNAL_ENA for eMMC */ > device_property_read_u32(dev, "bus-width", &bus_width); > - if (bus_width == BUS_WIDTH_8) > + if (bus_width == BUS_WIDTH_8 || device_property_read_bool(dev, "ti,suppress-v1p8-ena")) > sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA; > > So the driver applies the quirk for eMMC and we set the DT property for > SD card. Not sure which one is the best, but maybe it is another option. > Let's see what Adrian also thinks about that. I like it, sure lets see what Adrian says, thanks for your suggestion. ~ Judith
Hi Francesco, On 4/14/25 1:51 AM, Francesco Dolcini wrote: > Hello Judith > > On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: >> My understanding was that we do not like adding new DT properties if >> we can find a way to apply the quirk in the driver. > ... > >> If this implementation flies with the maintainers, then we can go back to DT >> property implementation. > > Not sure if this is clear, but this patch is NOT working according to > our tests, we would need to fix it in a different way. Not sure if you are following the thread with Hiago, but we are fixing it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD card which allows us to have granular control on when v1p8 is suppressed for MMC1. With this method, we make sure the quirk is never applied to your board (; ~ Judith
On Mon, Apr 14, 2025 at 09:37:53AM -0500, Judith Mendez wrote: > Hi Francesco, > > On 4/14/25 1:51 AM, Francesco Dolcini wrote: > > Hello Judith > > > > On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: > > > My understanding was that we do not like adding new DT properties if > > > we can find a way to apply the quirk in the driver. > > ... > > > > > If this implementation flies with the maintainers, then we can go back to DT > > > property implementation. > > > > Not sure if this is clear, but this patch is NOT working according to > > our tests, we would need to fix it in a different way. > > Not sure if you are following the thread with Hiago, but we are fixing > it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD > card which allows us to have granular control on when v1p8 is suppressed > for MMC1. > > With this method, we make sure the quirk is never applied to your board (; My board needs the quirk ;-) It was not clear to me that this change was going to be added to this patch, because without it's not working. Thanks for the clarification. Francesco
Hi Francesco, On 4/14/25 9:57 AM, Francesco Dolcini wrote: > On Mon, Apr 14, 2025 at 09:37:53AM -0500, Judith Mendez wrote: >> Hi Francesco, >> >> On 4/14/25 1:51 AM, Francesco Dolcini wrote: >>> Hello Judith >>> >>> On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote: >>>> My understanding was that we do not like adding new DT properties if >>>> we can find a way to apply the quirk in the driver. >>> ... >>> >>>> If this implementation flies with the maintainers, then we can go back to DT >>>> property implementation. >>> >>> Not sure if this is clear, but this patch is NOT working according to >>> our tests, we would need to fix it in a different way. >> >> Not sure if you are following the thread with Hiago, but we are fixing >> it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD >> card which allows us to have granular control on when v1p8 is suppressed >> for MMC1. >> >> With this method, we make sure the quirk is never applied to your board (; > > My board needs the quirk ;-) > It was not clear to me that this change was going to be added to this > patch, because without it's not working. > > Thanks for the clarification. Understood, Ok seems like we are just waiting on Adrian or another MMC maintainer to comment before sending v2 with Hiago's suggestion, thanks. ~ Judith
Hello Adrian, On 4/7/25 5:27 PM, Judith Mendez wrote: > For all TI devices, timing was closed For Legacy and HS modes in > half cycle timing, where data is launched on the negative edge of > clock and latched on the following positive edge of clock. The > switch to full cycle timing happens when any of HIGH_SPEED_ENA, > V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. > > Currently HIGH_SPEED_ENA is set for HS modes and violates timing > requirements for TI devices so add a .set_hs_ena callback in > sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. > > There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a > specific Kingston eMMC due to the sequencing when enumerating to > HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not > set V1P8_SIGNAL_ENA be default. This fix was previously merged in > the kernel, but was reverted due to the "heuristics for enabling > the quirk"[0]. The new implementation applies the quirk based-off of > bus width, which should not be an issue since there is no internal > LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only > effect timing for MMC0 interface. On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA, Ulf requested a change [0] which I am planning to do for v2. But I was hoping to get your opinion on whether Hiago's suggestion [1] is doable so I can add that as well to v2. Thanks for your attention. [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/ [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/ ~ Judith
On 16/04/25 19:59, Judith Mendez wrote: > Hello Adrian, > > On 4/7/25 5:27 PM, Judith Mendez wrote: >> For all TI devices, timing was closed For Legacy and HS modes in >> half cycle timing, where data is launched on the negative edge of >> clock and latched on the following positive edge of clock. The >> switch to full cycle timing happens when any of HIGH_SPEED_ENA, >> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. >> >> Currently HIGH_SPEED_ENA is set for HS modes and violates timing >> requirements for TI devices so add a .set_hs_ena callback in >> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. >> >> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a >> specific Kingston eMMC due to the sequencing when enumerating to >> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not >> set V1P8_SIGNAL_ENA be default. This fix was previously merged in >> the kernel, but was reverted due to the "heuristics for enabling >> the quirk"[0]. The new implementation applies the quirk based-off of >> bus width, which should not be an issue since there is no internal >> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only >> effect timing for MMC0 interface. > > > On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA, > Ulf requested a change [0] which I am planning to do for v2. But I was > hoping to get your opinion on whether Hiago's suggestion [1] is doable > so I can add that as well to v2. Thanks for your attention. > > > [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/ > [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/ > Sorry for the slow reply - been a bit distracted. I'll look at it properly tomorrow, but noticed sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL and SDHCI_CTRL_HISPD. Can you make use of that instead of a .set_hs_ena callback?
On 16/04/25 22:11, Adrian Hunter wrote: > On 16/04/25 19:59, Judith Mendez wrote: >> Hello Adrian, >> >> On 4/7/25 5:27 PM, Judith Mendez wrote: >>> For all TI devices, timing was closed For Legacy and HS modes in >>> half cycle timing, where data is launched on the negative edge of >>> clock and latched on the following positive edge of clock. The >>> switch to full cycle timing happens when any of HIGH_SPEED_ENA, >>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. >>> >>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing >>> requirements for TI devices so add a .set_hs_ena callback in >>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. >>> >>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a >>> specific Kingston eMMC due to the sequencing when enumerating to >>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not >>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in >>> the kernel, but was reverted due to the "heuristics for enabling >>> the quirk"[0]. The new implementation applies the quirk based-off of >>> bus width, which should not be an issue since there is no internal >>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only >>> effect timing for MMC0 interface. >> >> >> On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA, >> Ulf requested a change [0] which I am planning to do for v2. But I was >> hoping to get your opinion on whether Hiago's suggestion [1] is doable >> so I can add that as well to v2. Thanks for your attention. >> >> >> [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/ >> [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/ >> > > Sorry for the slow reply - been a bit distracted. > > I'll look at it properly tomorrow, but noticed > sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL > and SDHCI_CTRL_HISPD. Can you make use of that instead of > a .set_hs_ena callback? Patch 1 continues to look unneeded because sdhci_am654_write_b() seems to do the same thing. WRT patch 2, the suggestion to add a DT property and check the bus width seems fine to me. DT properties can be added to identify "broken" hardware that cannot be identified by the compatible string.
Hi Adrian, On 4/17/25 7:03 AM, Adrian Hunter wrote: > On 16/04/25 22:11, Adrian Hunter wrote: >> On 16/04/25 19:59, Judith Mendez wrote: >>> Hello Adrian, >>> >>> On 4/7/25 5:27 PM, Judith Mendez wrote: >>>> For all TI devices, timing was closed For Legacy and HS modes in >>>> half cycle timing, where data is launched on the negative edge of >>>> clock and latched on the following positive edge of clock. The >>>> switch to full cycle timing happens when any of HIGH_SPEED_ENA, >>>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set. >>>> >>>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing >>>> requirements for TI devices so add a .set_hs_ena callback in >>>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode. >>>> >>>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a >>>> specific Kingston eMMC due to the sequencing when enumerating to >>>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not >>>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in >>>> the kernel, but was reverted due to the "heuristics for enabling >>>> the quirk"[0]. The new implementation applies the quirk based-off of >>>> bus width, which should not be an issue since there is no internal >>>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only >>>> effect timing for MMC0 interface. >>> >>> >>> On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA, >>> Ulf requested a change [0] which I am planning to do for v2. But I was >>> hoping to get your opinion on whether Hiago's suggestion [1] is doable >>> so I can add that as well to v2. Thanks for your attention. >>> >>> >>> [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/ >>> [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/ >>> >> >> Sorry for the slow reply - been a bit distracted. >> >> I'll look at it properly tomorrow, but noticed >> sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL >> and SDHCI_CTRL_HISPD. Can you make use of that instead of >> a .set_hs_ena callback? > > Patch 1 continues to look unneeded because sdhci_am654_write_b() > seems to do the same thing. You are right, I will drop the first patch then. > > WRT patch 2, the suggestion to add a DT property and check the bus > width seems fine to me. DT properties can be added to identify > "broken" hardware that cannot be identified by the compatible > string. > Cool, will add this functionality to v2, thanks ~ Judith