[PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured

[PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

On some new i.MX platforms, hardware guidelines recommend using identical
pin configurations for SDR50/DDR50 (100MHz) and SDR104/HS400 (200MHz)
modes. But defining two identical pinctrl for 100MHz and 200MHz in dts
creates redundancy. In this case, omit explicit 100MHz configuration,
driver will inherit 100MHz pinctrl from 200MHz.

Preserves existing behavior if 100MHz is configured but 200MHz not (e.g,
imx8mp-navq.dts usdhc1 supports SDR50/DDR50 but SDR104/HS400 not).

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index f206b562a6e3..dfd8be5000c8 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1810,6 +1810,9 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 						ESDHC_PINCTRL_STATE_100MHZ);
 		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
 						ESDHC_PINCTRL_STATE_200MHZ);
+
+		if (IS_ERR_OR_NULL(imx_data->pins_100mhz))
+			imx_data->pins_100mhz = imx_data->pins_200mhz;
 	}
 
 	/* call to generic mmc_of_parse to support additional capabilities */
-- 
2.34.1

Re: [PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured

[Ahmad Fatoum]

Hi Luke,

Thanks for your patch.

On 21.05.25 13:20, ziniu.wang_1@nxp.com wrote:
> From: Luke Wang <ziniu.wang_1@nxp.com>
> 
> On some new i.MX platforms, hardware guidelines recommend using identical
> pin configurations for SDR50/DDR50 (100MHz) and SDR104/HS400 (200MHz)
> modes. But defining two identical pinctrl for 100MHz and 200MHz in dts
> creates redundancy.

I am not convinced this is an improvement. If 200mhz is missing, it's understood
that it's not supported. Now if 100mhz is missing, it means something different
depending on whether state_200mhz exists or not. This is more mental overhead
than having:

  pinctrl-names = "default", "state_100mhz", "state_200mhz";
  pinctrl-0 = <&pinctrl_usdhc1>;        
  pinctrl-1 = <&pinctrl_usdhc1_uhs>;   
  pinctrl-2 = <&pinctrl_usdhc1_uhs>;

where it's directly evident that you share pinctrl states.

> In this case, omit explicit 100MHz configuration,
> driver will inherit 100MHz pinctrl from 200MHz.
> 
> Preserves existing behavior if 100MHz is configured but 200MHz not (e.g,
> imx8mp-navq.dts usdhc1 supports SDR50/DDR50 but SDR104/HS400 not).

This conflicts with the binding, which doesn't allow for state_200mhz
to exist without state_100mhz, so that would need adaptation.

As noted before though, I don't think we really need to change anything
here though.

Thanks,
Ahmad

> 
> Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index f206b562a6e3..dfd8be5000c8 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1810,6 +1810,9 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>  						ESDHC_PINCTRL_STATE_100MHZ);
>  		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>  						ESDHC_PINCTRL_STATE_200MHZ);
> +
> +		if (IS_ERR_OR_NULL(imx_data->pins_100mhz))
> +			imx_data->pins_100mhz = imx_data->pins_200mhz;
>  	}
>  
>  	/* call to generic mmc_of_parse to support additional capabilities */


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured

[Luke Wang]

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: Wednesday, May 21, 2025 7:45 PM
> To: Luke Wang <ziniu.wang_1@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; adrian.hunter@intel.com; ulf.hansson@linaro.org;
> linux-mmc@vger.kernel.org
> Cc: imx@lists.linux.dev; dl-S32 <S32@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; festevam@gmail.com; linux-arm-
> kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from
> pins_200mhz when unconfigured
>
> [You don't often get email from a.fatoum@pengutronix.de. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> Hi Luke,
>
> Thanks for your patch.
>
> On 21.05.25 13:20, ziniu.wang_1@nxp.com wrote:
> > From: Luke Wang <ziniu.wang_1@nxp.com>
> >
> > On some new i.MX platforms, hardware guidelines recommend using
> identical
> > pin configurations for SDR50/DDR50 (100MHz) and SDR104/HS400
> (200MHz)
> > modes. But defining two identical pinctrl for 100MHz and 200MHz in dts
> > creates redundancy.
>
> I am not convinced this is an improvement. If 200mhz is missing, it's
> understood
> that it's not supported. Now if 100mhz is missing, it means something
> different
> depending on whether state_200mhz exists or not. This is more mental
> overhead
> than having:
>
>   pinctrl-names = "default", "state_100mhz", "state_200mhz";
>   pinctrl-0 = <&pinctrl_usdhc1>;
>   pinctrl-1 = <&pinctrl_usdhc1_uhs>;
>   pinctrl-2 = <&pinctrl_usdhc1_uhs>;
>

Hi Ahmad,

This way looks better. Will use pinctrl_usdhc1_uhs for both state_100mhz and state_200mhz if there are identical.

Thanks
Luke

> where it's directly evident that you share pinctrl states.
>
> > In this case, omit explicit 100MHz configuration,
> > driver will inherit 100MHz pinctrl from 200MHz.
> >
> > Preserves existing behavior if 100MHz is configured but 200MHz not (e.g,
> > imx8mp-navq.dts usdhc1 supports SDR50/DDR50 but SDR104/HS400 not).
>
> This conflicts with the binding, which doesn't allow for state_200mhz
> to exist without state_100mhz, so that would need adaptation.
>
> As noted before though, I don't think we really need to change anything
> here though.
>
> Thanks,
> Ahmad
>
> >
> > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> > index f206b562a6e3..dfd8be5000c8 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -1810,6 +1810,9 @@ sdhci_esdhc_imx_probe_dt(struct
> platform_device *pdev,
> >                                               ESDHC_PINCTRL_STATE_100MHZ);
> >               imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
> >                                               ESDHC_PINCTRL_STATE_200MHZ);
> > +
> > +             if (IS_ERR_OR_NULL(imx_data->pins_100mhz))
> > +                     imx_data->pins_100mhz = imx_data->pins_200mhz;
> >       }
> >
> >       /* call to generic mmc_of_parse to support additional capabilities */
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       |
> http://www.p/
> engutronix.de%2F&data=05%7C02%7Cziniu.wang_1%40nxp.com%7Cd8145bf
> 22a114de4256f08dd985ced07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C638834247059505189%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0
> eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpb
> CIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=tXhmm%2Bd3rGbtfl2VorcnL
> OvfZ8NcVFc0EJ%2B4fTxYJns%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured

[Frank Li]

On Wed, May 21, 2025 at 07:20:42PM +0800, ziniu.wang_1@nxp.com wrote:
> From: Luke Wang <ziniu.wang_1@nxp.com>
>
> On some new i.MX platforms, hardware guidelines recommend using identical
> pin configurations for SDR50/DDR50 (100MHz) and SDR104/HS400 (200MHz)
> modes. But defining two identical pinctrl for 100MHz and 200MHz in dts
> creates redundancy. In this case, omit explicit 100MHz configuration,
> driver will inherit 100MHz pinctrl from 200MHz.

It is quite strange inherit low freq setting from high freq setting.

Orignal method that decide support SDR50/DDR50/SDR104/HS400 abuse the
pinctrl state usage.

Frank

>
> Preserves existing behavior if 100MHz is configured but 200MHz not (e.g,
> imx8mp-navq.dts usdhc1 supports SDR50/DDR50 but SDR104/HS400 not).
>
> Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index f206b562a6e3..dfd8be5000c8 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1810,6 +1810,9 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
>  						ESDHC_PINCTRL_STATE_100MHZ);
>  		imx_data->pins_200mhz = pinctrl_lookup_state(imx_data->pinctrl,
>  						ESDHC_PINCTRL_STATE_200MHZ);
> +
> +		if (IS_ERR_OR_NULL(imx_data->pins_100mhz))
> +			imx_data->pins_100mhz = imx_data->pins_200mhz;
>  	}
>
>  	/* call to generic mmc_of_parse to support additional capabilities */
> --
> 2.34.1
>