diff mbox series

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

Message ID 20250521112042.266111-1-ziniu.wang_1@nxp.com
State New
Headers show
Series mmc: sdhci-esdhc-imx: inherit pins_100mhz from pins_200mhz when unconfigured | expand

Commit Message

Luke Wang May 21, 2025, 11:20 a.m. UTC 
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(+)

Comments

Frank Li May 21, 2025, 3:46 p.m. UTC | #1 
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
>
Luke Wang May 22, 2025, 4:47 a.m. UTC | #2 
> -----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 |
diff mbox series

Patch

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 */