[PATCH 0/3] Add SDHCI driver support for NXP S32G2

[PATCH 0/3] Add SDHCI driver support for NXP S32G2

[Chester Lin]

Hello,

This is a patch series for initial sdhci support on NXP S32G2 platforms.
In the previous work[1], only basic DTs and serial ports are supported for
minimum hardware booting. Here we want to add a mmc-host node and add
support in the sdhci-esdhc-imx driver so that S32G2 can also mount file
system from SDCard but not just ramdisk. As the first step, we choose fixed
clocks to fulfill the requirements the mmc host driver needs, and they will
be replaced by ARM SCMI clock protocol (0x14) once the SCMI feature are
added into S32G2 DT later.

This patchset has been verified with NXP downstream firmware blobs [ver:
bsp27/28/29/30], such as TF-A[2] and U-Boot[3] (BL33) on CodeAurora.

Thanks,
Chester

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/freescale/s32g2.dtsi
[2]: https://source.codeaurora.org/external/autobsps32/arm-trusted-firmware/
[3]: https://source.codeaurora.org/external/autobsps32/u-boot/

Chester Lin (3):
  dt-bindings: mmc: fsl-imx-esdhc: add NXP S32G2 support
  mmc: sdhci-esdhc-imx: add NXP S32G2 support
  arm64: dts: s32g2: add USDHC support

 .../bindings/mmc/fsl-imx-esdhc.yaml           |  1 +
 arch/arm64/boot/dts/freescale/s32g2.dtsi      | 32 +++++++++++++++++++
 .../arm64/boot/dts/freescale/s32g274a-evb.dts |  4 +++
 .../boot/dts/freescale/s32g274a-rdb2.dts      |  4 +++
 drivers/mmc/host/sdhci-esdhc-imx.c            | 17 ++++++++--
 5 files changed, 56 insertions(+), 2 deletions(-)

-- 
2.30.0

[PATCH 1/3] dt-bindings: mmc: fsl-imx-esdhc: add NXP S32G2 support

[Chester Lin]

Add support for the SDHC binding of S32G2.

Signed-off-by: Chester Lin <clin@suse.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
index a3412f221104..19621a2f8beb 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
@@ -34,6 +34,7 @@ properties:
           - fsl,imx6ull-usdhc
           - fsl,imx7d-usdhc
           - fsl,imx7ulp-usdhc
+          - nxp,s32g2-usdhc
       - items:
           - enum:
               - fsl,imx8mm-usdhc
-- 
2.30.0

[RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support

[Chester Lin]

Support the SDHCI controller found on NXP S32G2 platform. The new flag
ESDHC_FLAG_SKIP_ERR004536 is used because the hardware erratum bit is not
applicable for S32G2.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index f18d169bc8ff..d0f7d46a0354 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -196,6 +196,9 @@
  */
 #define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
 
+/* ERR004536 is not applicable for the IP  */
+#define ESDHC_FLAG_SKIP_ERR004536	BIT(17)
+
 enum wp_types {
 	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
 	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
@@ -289,6 +292,13 @@ static const struct esdhc_soc_data usdhc_imx7d_data = {
 			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
 };
 
+static struct esdhc_soc_data usdhc_s32g2_data = {
+	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
+			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
+			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
+			| ESDHC_FLAG_SKIP_ERR004536,
+};
+
 static struct esdhc_soc_data usdhc_imx7ulp_data = {
 	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
@@ -347,6 +357,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
 	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
 	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
 	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
+	{ .compatible = "nxp,s32g2-usdhc", .data = &usdhc_s32g2_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids);
@@ -1359,8 +1370,10 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 		 * erratum ESDHC_FLAG_ERR004536 fix for MX6Q TO1.2 and MX6DL
 		 * TO1.1, it's harmless for MX6SL
 		 */
-		writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
-			host->ioaddr + 0x6c);
+		if (!(imx_data->socdata->flags & ESDHC_FLAG_SKIP_ERR004536)) {
+			writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
+				host->ioaddr + 0x6c);
+		}
 
 		/* disable DLL_CTRL delay line settings */
 		writel(0x0, host->ioaddr + ESDHC_DLL_CTRL);
-- 
2.30.0

RE: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support

[Bough Chen]

[-- Attachment #1: Type: text/plain, Size: 3486 bytes --]

> -----Original Message-----
> From: Chester Lin [mailto:clin@suse.com]
> Sent: 2021年10月21日 15:14
> To: Ulf Hansson <ulf.hansson@linaro.org>; dl-S32 <S32@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; Bough Chen <haibo.chen@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; linux-mmc@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Radu-nicolae Pirea (OSS)
> <radu-nicolae.pirea@oss.nxp.com>; Andreas Färber <afaerber@suse.de>;
> Matthias Brugger <mbrugger@suse.com>; Ivan T . Ivanov <iivanov@suse.de>;
> Lee, Chun-Yi <jlee@suse.com>; Chester Lin <clin@suse.com>
> Subject: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support
> 
> Support the SDHCI controller found on NXP S32G2 platform. The new flag
> ESDHC_FLAG_SKIP_ERR004536 is used because the hardware erratum bit is
> not applicable for S32G2.

What's this bit7 definition on S32G2 usdhc?  Any issue if clear bit 7?

Best Regards
Haibo Chen
> 
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index f18d169bc8ff..d0f7d46a0354 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -196,6 +196,9 @@
>   */
>  #define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
> 
> +/* ERR004536 is not applicable for the IP  */
> +#define ESDHC_FLAG_SKIP_ERR004536	BIT(17)
> +
>  enum wp_types {
>  	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
>  	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
> @@ -289,6 +292,13 @@ static const struct esdhc_soc_data
> usdhc_imx7d_data = {
>  			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
>  };
> 
> +static struct esdhc_soc_data usdhc_s32g2_data = {
> +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
> +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> +			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
> +			| ESDHC_FLAG_SKIP_ERR004536,
> +};
> +
>  static struct esdhc_soc_data usdhc_imx7ulp_data = {
>  	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200 @@ -347,6
> +357,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
>  	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
>  	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
>  	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
> +	{ .compatible = "nxp,s32g2-usdhc", .data = &usdhc_s32g2_data, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids); @@ -1359,8 +1370,10 @@
> static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>  		 * erratum ESDHC_FLAG_ERR004536 fix for MX6Q TO1.2 and MX6DL
>  		 * TO1.1, it's harmless for MX6SL
>  		 */
> -		writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> -			host->ioaddr + 0x6c);
> +		if (!(imx_data->socdata->flags & ESDHC_FLAG_SKIP_ERR004536)) {
> +			writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> +				host->ioaddr + 0x6c);
> +		}
> 
>  		/* disable DLL_CTRL delay line settings */
>  		writel(0x0, host->ioaddr + ESDHC_DLL_CTRL);
> --
> 2.30.0


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9551 bytes --]

Re: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support

[Chester Lin]

Hi NXP S32 and i.MX Linux teams,

On Thu, Oct 21, 2021 at 03:13:32PM +0800, Chester Lin wrote:
> Support the SDHCI controller found on NXP S32G2 platform. The new flag
> ESDHC_FLAG_SKIP_ERR004536 is used because the hardware erratum bit is not
> applicable for S32G2.
> 
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index f18d169bc8ff..d0f7d46a0354 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -196,6 +196,9 @@
>   */
>  #define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
>  
> +/* ERR004536 is not applicable for the IP  */
> +#define ESDHC_FLAG_SKIP_ERR004536	BIT(17)
> +
>  enum wp_types {
>  	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
>  	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
> @@ -289,6 +292,13 @@ static const struct esdhc_soc_data usdhc_imx7d_data = {
>  			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
>  };
>  
> +static struct esdhc_soc_data usdhc_s32g2_data = {
> +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
> +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> +			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
> +			| ESDHC_FLAG_SKIP_ERR004536,
> +};
> +
>  static struct esdhc_soc_data usdhc_imx7ulp_data = {
>  	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> @@ -347,6 +357,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
>  	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
>  	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
>  	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
> +	{ .compatible = "nxp,s32g2-usdhc", .data = &usdhc_s32g2_data, },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids);
> @@ -1359,8 +1370,10 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
>  		 * erratum ESDHC_FLAG_ERR004536 fix for MX6Q TO1.2 and MX6DL
>  		 * TO1.1, it's harmless for MX6SL
>  		 */
> -		writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> -			host->ioaddr + 0x6c);
> +		if (!(imx_data->socdata->flags & ESDHC_FLAG_SKIP_ERR004536)) {
> +			writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> +				host->ioaddr + 0x6c);
> +		}

Hope you don't might that I raise this question here. Is it really necessary
to unconditionally apply the erratum bit even if some SoCs might not need this
workaround? From the S32 implementation in CodeAurora[1], I noticed that this
bit is not required by S32V/S32G so I wonder if there's any better way to
refine this part?

Thanks,
Chester

[1] https://source.codeaurora.org/external/autobsps32/linux/tree/drivers/mmc/host/sdhci-esdhc-imx.c?h=release/bsp30.0-5.4-rt#n1268

>  
>  		/* disable DLL_CTRL delay line settings */
>  		writel(0x0, host->ioaddr + ESDHC_DLL_CTRL);
> -- 
> 2.30.0
> 

RE: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support

[Bough Chen]

[-- Attachment #1: Type: text/plain, Size: 4849 bytes --]

> -----Original Message-----
> From: Chester Lin [mailto:clin@suse.com]
> Sent: 2021年10月21日 15:31
> To: Ulf Hansson <ulf.hansson@linaro.org>; dl-S32 <S32@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; Bough Chen <haibo.chen@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; linux-mmc@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Radu-nicolae Pirea (OSS)
> <radu-nicolae.pirea@oss.nxp.com>; Andreas Färber <afaerber@suse.de>;
> Matthias Brugger <mbrugger@suse.com>; Ivan T . Ivanov <iivanov@suse.de>;
> Lee, Chun-Yi <jlee@suse.com>
> Subject: Re: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support
> 
> Hi NXP S32 and i.MX Linux teams,
> 
> On Thu, Oct 21, 2021 at 03:13:32PM +0800, Chester Lin wrote:
> > Support the SDHCI controller found on NXP S32G2 platform. The new flag
> > ESDHC_FLAG_SKIP_ERR004536 is used because the hardware erratum bit is
> > not applicable for S32G2.
> >
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index f18d169bc8ff..d0f7d46a0354 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -196,6 +196,9 @@
> >   */
> >  #define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
> >
> > +/* ERR004536 is not applicable for the IP  */
> > +#define ESDHC_FLAG_SKIP_ERR004536	BIT(17)
> > +
> >  enum wp_types {
> >  	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
> >  	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
> > @@ -289,6 +292,13 @@ static const struct esdhc_soc_data
> usdhc_imx7d_data = {
> >  			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
> >  };
> >
> > +static struct esdhc_soc_data usdhc_s32g2_data = {
> > +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
> > +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> > +			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
> > +			| ESDHC_FLAG_SKIP_ERR004536,
> > +};
> > +
> >  static struct esdhc_soc_data usdhc_imx7ulp_data = {
> >  	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200 @@ -347,6
> +357,7 @@
> > static const struct of_device_id imx_esdhc_dt_ids[] = {
> >  	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
> >  	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
> >  	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
> > +	{ .compatible = "nxp,s32g2-usdhc", .data = &usdhc_s32g2_data, },
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids); @@ -1359,8 +1370,10
> @@
> > static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
> >  		 * erratum ESDHC_FLAG_ERR004536 fix for MX6Q TO1.2 and MX6DL
> >  		 * TO1.1, it's harmless for MX6SL
> >  		 */
> > -		writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> > -			host->ioaddr + 0x6c);
> > +		if (!(imx_data->socdata->flags & ESDHC_FLAG_SKIP_ERR004536)) {
> > +			writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> > +				host->ioaddr + 0x6c);
> > +		}
> 
> Hope you don't might that I raise this question here. Is it really necessary to
> unconditionally apply the erratum bit even if some SoCs might not need this
> workaround? From the S32 implementation in CodeAurora[1], I noticed that
> this bit is not required by S32V/S32G so I wonder if there's any better way to
> refine this part?
> 

I confirmed with IP owner before, for SoC contain this errata fixup, clear this bit 7
will enable the fixup, set the bit 7 will disable the fixup.
For SoC which do not contain this errata fixup, this bit 7 has no definition.
So it's okay to clear this bit 7 unconditionally.

Best Regards
Haibo Chen


> Thanks,
> Chester
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.
> codeaurora.org%2Fexternal%2Fautobsps32%2Flinux%2Ftree%2Fdrivers%2Fm
> mc%2Fhost%2Fsdhci-esdhc-imx.c%3Fh%3Drelease%2Fbsp30.0-5.4-rt%23n1268
> &amp;data=04%7C01%7Chaibo.chen%40nxp.com%7Cec36a273354b4b66c45b
> 08d99464c449%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63770
> 3982782606697%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=hvzsy
> 3W%2FpKXBmXxS8%2F73huzb157a%2FuHa5G4lFWj5ABQ%3D&amp;reserved=
> 0
> 
> >
> >  		/* disable DLL_CTRL delay line settings */
> >  		writel(0x0, host->ioaddr + ESDHC_DLL_CTRL);
> > --
> > 2.30.0
> >


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9551 bytes --]

Re: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support

[Chester Lin]

Hi Haibo,

On Thu, Oct 21, 2021 at 08:00:43AM +0000, Bough Chen wrote:
> > -----Original Message-----
> > From: Chester Lin [mailto:clin@suse.com]
> > Sent: 2021年10月21日 15:31
> > To: Ulf Hansson <ulf.hansson@linaro.org>; dl-S32 <S32@nxp.com>; dl-linux-imx
> > <linux-imx@nxp.com>; Bough Chen <haibo.chen@nxp.com>; Aisheng Dong
> > <aisheng.dong@nxp.com>; linux-mmc@vger.kernel.org
> > Cc: Rob Herring <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>;
> > Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; Radu-nicolae Pirea (OSS)
> > <radu-nicolae.pirea@oss.nxp.com>; Andreas Färber <afaerber@suse.de>;
> > Matthias Brugger <mbrugger@suse.com>; Ivan T . Ivanov <iivanov@suse.de>;
> > Lee, Chun-Yi <jlee@suse.com>
> > Subject: Re: [RFC PATCH 2/3] mmc: sdhci-esdhc-imx: add NXP S32G2 support
> > 
> > Hi NXP S32 and i.MX Linux teams,
> > 
> > On Thu, Oct 21, 2021 at 03:13:32PM +0800, Chester Lin wrote:
> > > Support the SDHCI controller found on NXP S32G2 platform. The new flag
> > > ESDHC_FLAG_SKIP_ERR004536 is used because the hardware erratum bit is
> > > not applicable for S32G2.
> > >
> > > Signed-off-by: Chester Lin <clin@suse.com>
> > > ---
> > >  drivers/mmc/host/sdhci-esdhc-imx.c | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > index f18d169bc8ff..d0f7d46a0354 100644
> > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > > @@ -196,6 +196,9 @@
> > >   */
> > >  #define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
> > >
> > > +/* ERR004536 is not applicable for the IP  */
> > > +#define ESDHC_FLAG_SKIP_ERR004536	BIT(17)
> > > +
> > >  enum wp_types {
> > >  	ESDHC_WP_NONE,		/* no WP, neither controller nor gpio */
> > >  	ESDHC_WP_CONTROLLER,	/* mmc controller internal WP */
> > > @@ -289,6 +292,13 @@ static const struct esdhc_soc_data
> > usdhc_imx7d_data = {
> > >  			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
> > >  };
> > >
> > > +static struct esdhc_soc_data usdhc_s32g2_data = {
> > > +	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
> > > +			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> > > +			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
> > > +			| ESDHC_FLAG_SKIP_ERR004536,
> > > +};
> > > +
> > >  static struct esdhc_soc_data usdhc_imx7ulp_data = {
> > >  	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> > >  			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200 @@ -347,6
> > +357,7 @@
> > > static const struct of_device_id imx_esdhc_dt_ids[] = {
> > >  	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
> > >  	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
> > >  	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
> > > +	{ .compatible = "nxp,s32g2-usdhc", .data = &usdhc_s32g2_data, },
> > >  	{ /* sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids); @@ -1359,8 +1370,10
> > @@
> > > static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
> > >  		 * erratum ESDHC_FLAG_ERR004536 fix for MX6Q TO1.2 and MX6DL
> > >  		 * TO1.1, it's harmless for MX6SL
> > >  		 */
> > > -		writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> > > -			host->ioaddr + 0x6c);
> > > +		if (!(imx_data->socdata->flags & ESDHC_FLAG_SKIP_ERR004536)) {
> > > +			writel(readl(host->ioaddr + 0x6c) & ~BIT(7),
> > > +				host->ioaddr + 0x6c);
> > > +		}
> > 
> > Hope you don't might that I raise this question here. Is it really necessary to
> > unconditionally apply the erratum bit even if some SoCs might not need this
> > workaround? From the S32 implementation in CodeAurora[1], I noticed that
> > this bit is not required by S32V/S32G so I wonder if there's any better way to
> > refine this part?
> > 
> 
> I confirmed with IP owner before, for SoC contain this errata fixup, clear this bit 7
> will enable the fixup, set the bit 7 will disable the fixup.
> For SoC which do not contain this errata fixup, this bit 7 has no definition.
> So it's okay to clear this bit 7 unconditionally.
> 

Thanks for your reply. If I understand correctly, this bit should be almost the
same even if the IP can be used in different SoCs. Actually I haven't found an
issue even if I have tried to clear the bit-7 on S32G274A although this patch
just gets the driver working on S32G, which still has limited functions [e.g.
pins_100mhz and pins_200mhz are missing]. I just wonder if any case should
avoid touching this bit since the s32 downstream kernel has a specific handling
for this part.

@NXP S32 team:
Please let us know if any concern about this bit.

Thanks,
Chester
> 
> 
> > Thanks,
> > Chester
> > 
> > [1]
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.
> > codeaurora.org%2Fexternal%2Fautobsps32%2Flinux%2Ftree%2Fdrivers%2Fm
> > mc%2Fhost%2Fsdhci-esdhc-imx.c%3Fh%3Drelease%2Fbsp30.0-5.4-rt%23n1268
> > &amp;data=04%7C01%7Chaibo.chen%40nxp.com%7Cec36a273354b4b66c45b
> > 08d99464c449%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63770
> > 3982782606697%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=hvzsy
> > 3W%2FpKXBmXxS8%2F73huzb157a%2FuHa5G4lFWj5ABQ%3D&amp;reserved=
> > 0
> > 
> > >
> > >  		/* disable DLL_CTRL delay line settings */
> > >  		writel(0x0, host->ioaddr + ESDHC_DLL_CTRL);
> > > --
> > > 2.30.0
> > >
> 

[PATCH 3/3] arm64: dts: s32g2: add USDHC support

[Chester Lin]

Add a mmc node to support USDHC on NXP S32G2 platforms.

Signed-off-by: Chester Lin <clin@suse.com>
---
 arch/arm64/boot/dts/freescale/s32g2.dtsi      | 32 +++++++++++++++++++
 .../arm64/boot/dts/freescale/s32g274a-evb.dts |  4 +++
 .../boot/dts/freescale/s32g274a-rdb2.dts      |  4 +++
 3 files changed, 40 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
index 59ea8a25aa4c..19e2e2561374 100644
--- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
@@ -79,6 +79,26 @@ psci {
 		};
 	};
 
+	clocks {
+		usdhc_clk_module: usdhc_clk_module {
+			compatible = "fixed-clock";
+			clock-frequency = <133333333>;
+			#clock-cells = <0>;
+		};
+
+		usdhc_clk_ahb: usdhc_clk_ahb {
+			compatible = "fixed-clock";
+			clock-frequency = <400000000>;
+			#clock-cells = <0>;
+		};
+
+		usdhc_clk_core: usdhc_clk_core {
+			compatible = "fixed-clock";
+			clock-frequency = <400000000>;
+			#clock-cells = <0>;
+		};
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -109,6 +129,18 @@ uart2: serial@402bc000 {
 			status = "disabled";
 		};
 
+		usdhc0: mmc@402f0000 {
+			compatible = "nxp,s32g2-usdhc";
+			reg = <0x402f0000 0x1000>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			bus-width = <8>;
+			clocks = <&usdhc_clk_module>, <&usdhc_clk_ahb>,
+				 <&usdhc_clk_core>;
+			clock-names = "ipg", "ahb", "per";
+			no-1-8-v;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@50800000 {
 			compatible = "arm,gic-v3";
 			reg = <0x50800000 0x10000>,
diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
index 9118d8d2ee01..89428f1883d9 100644
--- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
+++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
@@ -32,3 +32,7 @@ memory@80000000 {
 &uart0 {
 	status = "okay";
 };
+
+&usdhc0 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
index e05ee854cdf5..30eae51121de 100644
--- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
+++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
@@ -38,3 +38,7 @@ &uart0 {
 &uart1 {
 	status = "okay";
 };
+
+&usdhc0 {
+	status = "okay";
+};
-- 
2.30.0

Re: [PATCH 3/3] arm64: dts: s32g2: add USDHC support

[Radu Nicolae Pirea (NXP OSS)]

Hi Chester,

On Thu, 2021-10-21 at 15:13 +0800, Chester Lin wrote:
> Add a mmc node to support USDHC on NXP S32G2 platforms.
> 
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  arch/arm64/boot/dts/freescale/s32g2.dtsi      | 32
> +++++++++++++++++++
>  .../arm64/boot/dts/freescale/s32g274a-evb.dts |  4 +++
>  .../boot/dts/freescale/s32g274a-rdb2.dts      |  4 +++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> index 59ea8a25aa4c..19e2e2561374 100644
> --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> @@ -79,6 +79,26 @@ psci {
>                 };
>         };
>  
> +       clocks {
> +               usdhc_clk_module: usdhc_clk_module {
> +                       compatible = "fixed-clock";
> +                       clock-frequency = <133333333>;
> +                       #clock-cells = <0>;
> +               };
> +
> +               usdhc_clk_ahb: usdhc_clk_ahb {
> +                       compatible = "fixed-clock";
> +                       clock-frequency = <400000000>;
> +                       #clock-cells = <0>;
> +               };
> +
> +               usdhc_clk_core: usdhc_clk_core {
> +                       compatible = "fixed-clock";
> +                       clock-frequency = <400000000>;
> +                       #clock-cells = <0>;
> +               };

Adding the clock bindings as fixed-clock doesn't describe the hardware.
Using fixed-clock is suitable for quartz crystals and oscillators. Here
we should have the bindings to the clock driver. Are you planning to
submit such driver soon or you will add here more fixed clocks every
time you add a peripheral in the dts?

Cheers.
Radu P.

> +       };
> +
>         soc {
>                 compatible = "simple-bus";
>                 #address-cells = <1>;
> @@ -109,6 +129,18 @@ uart2: serial@402bc000 {
>                         status = "disabled";
>                 };
>  
> +               usdhc0: mmc@402f0000 {
> +                       compatible = "nxp,s32g2-usdhc";
> +                       reg = <0x402f0000 0x1000>;
> +                       interrupts = <GIC_SPI 36
> IRQ_TYPE_LEVEL_HIGH>;
> +                       bus-width = <8>;
> +                       clocks = <&usdhc_clk_module>,
> <&usdhc_clk_ahb>,
> +                                <&usdhc_clk_core>;
> +                       clock-names = "ipg", "ahb", "per";
> +                       no-1-8-v;
> +                       status = "disabled";
> +               };
> +
>                 gic: interrupt-controller@50800000 {
>                         compatible = "arm,gic-v3";
>                         reg = <0x50800000 0x10000>,
> diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> index 9118d8d2ee01..89428f1883d9 100644
> --- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> +++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> @@ -32,3 +32,7 @@ memory@80000000 {
>  &uart0 {
>         status = "okay";
>  };
> +
> +&usdhc0 {
> +       status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> index e05ee854cdf5..30eae51121de 100644
> --- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> +++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> @@ -38,3 +38,7 @@ &uart0 {
>  &uart1 {
>         status = "okay";
>  };
> +
> +&usdhc0 {
> +       status = "okay";
> +};

Re: [PATCH 3/3] arm64: dts: s32g2: add USDHC support

[Chester Lin]

Hi Radu,

On Thu, Oct 21, 2021 at 04:32:52PM +0300, Radu Nicolae Pirea (NXP OSS) wrote:
> Hi Chester,
> 
> On Thu, 2021-10-21 at 15:13 +0800, Chester Lin wrote:
> > Add a mmc node to support USDHC on NXP S32G2 platforms.
> > 
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> >  arch/arm64/boot/dts/freescale/s32g2.dtsi      | 32
> > +++++++++++++++++++
> >  .../arm64/boot/dts/freescale/s32g274a-evb.dts |  4 +++
> >  .../boot/dts/freescale/s32g274a-rdb2.dts      |  4 +++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > index 59ea8a25aa4c..19e2e2561374 100644
> > --- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > @@ -79,6 +79,26 @@ psci {
> >                 };
> >         };
> >  
> > +       clocks {
> > +               usdhc_clk_module: usdhc_clk_module {
> > +                       compatible = "fixed-clock";
> > +                       clock-frequency = <133333333>;
> > +                       #clock-cells = <0>;
> > +               };
> > +
> > +               usdhc_clk_ahb: usdhc_clk_ahb {
> > +                       compatible = "fixed-clock";
> > +                       clock-frequency = <400000000>;
> > +                       #clock-cells = <0>;
> > +               };
> > +
> > +               usdhc_clk_core: usdhc_clk_core {
> > +                       compatible = "fixed-clock";
> > +                       clock-frequency = <400000000>;
> > +                       #clock-cells = <0>;
> > +               };
> 
> Adding the clock bindings as fixed-clock doesn't describe the hardware.
> Using fixed-clock is suitable for quartz crystals and oscillators. Here
> we should have the bindings to the clock driver. Are you planning to
> submit such driver soon or you will add here more fixed clocks every
> time you add a peripheral in the dts?
> 

Yes, I'm planning to add real clock bindings but not just fixed rates I observed
from EVB and RDB2 boards. Since the upstream support is based on the psci
method and TF-A, I noticed that we can use SCMI clocks [+smc transport driver,
e.g. "arm,scmi-smc"] as clock inputs for most IPs on S32G, and AFAICS the
downstream TF-A[1] has infrastructure to support SCMI clock protocl
[protocol-id: 0x14, arm,smc-id: 0xc20000fe].

However it will take much time to verify and upstream all SCMI-related stuff
such as clock dt-bindings, so I am thinking of using fixed-clock as the first
step to have initial support before we can have all SCMI bindings get accepted
by upstream.

Please feel free to let me know if any suggestions.

Regards,
Chester

[1] https://source.codeaurora.org/external/autobsps32/arm-trusted-firmware/tree/plat/nxp/s32g/s32g_svc.c?h=release/bsp30.0-2.3

> Cheers.
> Radu P.
> 
> > +       };
> > +
> >         soc {
> >                 compatible = "simple-bus";
> >                 #address-cells = <1>;
> > @@ -109,6 +129,18 @@ uart2: serial@402bc000 {
> >                         status = "disabled";
> >                 };
> >  
> > +               usdhc0: mmc@402f0000 {
> > +                       compatible = "nxp,s32g2-usdhc";
> > +                       reg = <0x402f0000 0x1000>;
> > +                       interrupts = <GIC_SPI 36
> > IRQ_TYPE_LEVEL_HIGH>;
> > +                       bus-width = <8>;
> > +                       clocks = <&usdhc_clk_module>,
> > <&usdhc_clk_ahb>,
> > +                                <&usdhc_clk_core>;
> > +                       clock-names = "ipg", "ahb", "per";
> > +                       no-1-8-v;
> > +                       status = "disabled";
> > +               };
> > +
> >                 gic: interrupt-controller@50800000 {
> >                         compatible = "arm,gic-v3";
> >                         reg = <0x50800000 0x10000>,
> > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> > b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> > index 9118d8d2ee01..89428f1883d9 100644
> > --- a/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> > +++ b/arch/arm64/boot/dts/freescale/s32g274a-evb.dts
> > @@ -32,3 +32,7 @@ memory@80000000 {
> >  &uart0 {
> >         status = "okay";
> >  };
> > +
> > +&usdhc0 {
> > +       status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> > b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> > index e05ee854cdf5..30eae51121de 100644
> > --- a/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> > +++ b/arch/arm64/boot/dts/freescale/s32g274a-rdb2.dts
> > @@ -38,3 +38,7 @@ &uart0 {
> >  &uart1 {
> >         status = "okay";
> >  };
> > +
> > +&usdhc0 {
> > +       status = "okay";
> > +};
> 
> 

Re: [PATCH 0/3] Add SDHCI driver support for NXP S32G2

[Ulf Hansson]

On Thu, 21 Oct 2021 at 09:13, Chester Lin <clin@suse.com> wrote:
>
> Hello,
>
> This is a patch series for initial sdhci support on NXP S32G2 platforms.
> In the previous work[1], only basic DTs and serial ports are supported for
> minimum hardware booting. Here we want to add a mmc-host node and add
> support in the sdhci-esdhc-imx driver so that S32G2 can also mount file
> system from SDCard but not just ramdisk. As the first step, we choose fixed
> clocks to fulfill the requirements the mmc host driver needs, and they will
> be replaced by ARM SCMI clock protocol (0x14) once the SCMI feature are
> added into S32G2 DT later.
>
> This patchset has been verified with NXP downstream firmware blobs [ver:
> bsp27/28/29/30], such as TF-A[2] and U-Boot[3] (BL33) on CodeAurora.
>
> Thanks,
> Chester
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/freescale/s32g2.dtsi
> [2]: https://source.codeaurora.org/external/autobsps32/arm-trusted-firmware/
> [3]: https://source.codeaurora.org/external/autobsps32/u-boot/
>
> Chester Lin (3):
>   dt-bindings: mmc: fsl-imx-esdhc: add NXP S32G2 support
>   mmc: sdhci-esdhc-imx: add NXP S32G2 support
>   arm64: dts: s32g2: add USDHC support
>
>  .../bindings/mmc/fsl-imx-esdhc.yaml           |  1 +
>  arch/arm64/boot/dts/freescale/s32g2.dtsi      | 32 +++++++++++++++++++
>  .../arm64/boot/dts/freescale/s32g274a-evb.dts |  4 +++
>  .../boot/dts/freescale/s32g274a-rdb2.dts      |  4 +++
>  drivers/mmc/host/sdhci-esdhc-imx.c            | 17 ++++++++--
>  5 files changed, 56 insertions(+), 2 deletions(-)
>

Patch 1 and 2, applied for next, thanks!

Kind regards
Uffe