[v6,5/5] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

Commit Message

Yangbo Lu March 9, 2016, 10:08 a.m. UTC

The eSDHC of T4240-R1.0-R2.0 has incorrect vender version and spec version.
Acturally the right version numbers should be VVN=0x13 and SVN = 0x1.
This patch adds the GUTS driver support for eSDHC driver to get SVR(System
version register). And fix host version to avoid that incorrect version
numbers break down the ADMA data transfer.

Signed-off-by: Yangbo Lu <[e-mail]>
Acked-by: Ulf Hansson <[e-mail]>
---
Changes for v2:
	- Got SVR through iomap instead of dts
Changes for v3:
	- Managed GUTS through syscon instead of iomap in eSDHC driver
Changes for v4:
	- Got SVR by GUTS driver instead of SYSCON
Changes for v5:
	- Changed to get SVR through API fsl_guts_get_svr()
	- Combined patch 4, patch 5 and patch 6 into one
Changes for v6:
	- Added 'Acked-by: Ulf Hansson'
---
 drivers/mmc/host/Kconfig          |  1 +
 drivers/mmc/host/sdhci-of-esdhc.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Arnd Bergmann March 13, 2016, 10:26 p.m. UTC | #1

On Wednesday 09 March 2016 18:08:51 Yangbo Lu wrote:
> @@ -567,10 +580,20 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
>         struct sdhci_pltfm_host *pltfm_host;
>         struct sdhci_esdhc *esdhc;
>         u16 host_ver;
> +       u32 svr;
>  
>         pltfm_host = sdhci_priv(host);
>         esdhc = sdhci_pltfm_priv(pltfm_host);
>  
> +       fsl_guts_init();
> +       svr = fsl_guts_get_svr();
> +       if (svr) {
> +               esdhc->soc_ver = SVR_SOC_VER(svr);
> +               esdhc->soc_rev = SVR_REV(svr);
> +       } else {
> +               dev_err(&pdev->dev, "Failed to get SVR value!\n");
> +       }
> +

This makes the driver non-portable. Better identify the specific workarounds
based on the compatible string for this device, or add a boolean DT property
for the quirk.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yangbo Lu March 14, 2016, 7:29 a.m. UTC | #2

> -----Original Message-----
> From: Arnd Bergmann [mailto:[e-mail]]
> Sent: Monday, March 14, 2016 6:26 AM
> To: [e-mail]
> Cc: Yangbo Lu; [e-mail]; linux-arm-
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail]-
> foundation.org; [e-mail]; [e-mail];
> [e-mail]; Zhao Qiang; Russell King; Bhupesh Sharma; Joerg
> Roedel; Santosh Shilimkar; Scott Wood; Rob Herring; Claudiu Manoil; Kumar
> Gala; Yang-Leo Li; Xiaobo Xie
> Subject: Re: [v6, 5/5] mmc: sdhci-of-esdhc: fix host version for T4240-
> R1.0-R2.0
> 
> On Wednesday 09 March 2016 18:08:51 Yangbo Lu wrote:
> > @@ -567,10 +580,20 @@ static void esdhc_init(struct platform_device
> *pdev, struct sdhci_host *host)
> >         struct sdhci_pltfm_host *pltfm_host;
> >         struct sdhci_esdhc *esdhc;
> >         u16 host_ver;
> > +       u32 svr;
> >
> >         pltfm_host = sdhci_priv(host);
> >         esdhc = sdhci_pltfm_priv(pltfm_host);
> >
> > +       fsl_guts_init();
> > +       svr = fsl_guts_get_svr();
> > +       if (svr) {
> > +               esdhc->soc_ver = SVR_SOC_VER(svr);
> > +               esdhc->soc_rev = SVR_REV(svr);
> > +       } else {
> > +               dev_err(&pdev->dev, "Failed to get SVR value!\n");
> > +       }
> > +
> 
> This makes the driver non-portable. Better identify the specific
> workarounds based on the compatible string for this device, or add a
> boolean DT property for the quirk.
> 
> 	Arnd

[Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS in v1 before.
https://patchwork.kernel.org/patch/6834221/

We don't have a separate DTS file for each revision of an SOC and if we did, we'd constantly have people using the wrong one.
In addition, the device tree is stable ABI and errata are often discovered after device tree are deployed.
See the link for details.

So we decide to read SVR from the device-config/guts MMIO block other than using DTS.
Thanks.




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Scott Wood March 14, 2016, 5:45 p.m. UTC | #3

On 03/14/2016 02:29 AM, Yangbo Lu wrote:
>> -----Original Message-----
>> From: Arnd Bergmann [mailto:[e-mail]]
>> Sent: Monday, March 14, 2016 6:26 AM
>> To: [e-mail]
>> Cc: Yangbo Lu; [e-mail]; linux-arm-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]; [e-mail]-
>> foundation.org; [e-mail]; [e-mail];
>> [e-mail]; Zhao Qiang; Russell King; Bhupesh Sharma; Joerg
>> Roedel; Santosh Shilimkar; Scott Wood; Rob Herring; Claudiu Manoil; Kumar
>> Gala; Yang-Leo Li; Xiaobo Xie
>> Subject: Re: [v6, 5/5] mmc: sdhci-of-esdhc: fix host version for T4240-
>> R1.0-R2.0
>>
>> On Wednesday 09 March 2016 18:08:51 Yangbo Lu wrote:
>>> @@ -567,10 +580,20 @@ static void esdhc_init(struct platform_device
>> *pdev, struct sdhci_host *host)
>>>         struct sdhci_pltfm_host *pltfm_host;
>>>         struct sdhci_esdhc *esdhc;
>>>         u16 host_ver;
>>> +       u32 svr;
>>>
>>>         pltfm_host = sdhci_priv(host);
>>>         esdhc = sdhci_pltfm_priv(pltfm_host);
>>>
>>> +       fsl_guts_init();
>>> +       svr = fsl_guts_get_svr();
>>> +       if (svr) {
>>> +               esdhc->soc_ver = SVR_SOC_VER(svr);
>>> +               esdhc->soc_rev = SVR_REV(svr);
>>> +       } else {
>>> +               dev_err(&pdev->dev, "Failed to get SVR value!\n");
>>> +       }
>>> +
>>
>> This makes the driver non-portable. Better identify the specific
>> workarounds based on the compatible string for this device, or add a
>> boolean DT property for the quirk.
>>
>> 	Arnd
> 
> [Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS in v1 before.
> https://patchwork.kernel.org/patch/6834221/
> 
> We don’t have a separate DTS file for each revision of an SOC and if we did, we'd constantly have people using the wrong one.
> In addition, the device tree is stable ABI and errata are often discovered after device tree are deployed.
> See the link for details.
> 
> So we decide to read SVR from the device-config/guts MMIO block other than using DTS.
> Thanks.

Also note that this driver is already only for fsl-specific hardware,
and it will still work even if fsl_guts doesn't find anything to bind to
-- it just wouldn't be able to detect errata based on SVR in that case.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring (Arm) March 17, 2016, 5:01 p.m. UTC | #4

On Mon, Mar 14, 2016 at 05:45:43PM +0000, Scott Wood wrote:
> On 03/14/2016 02:29 AM, Yangbo Lu wrote:
> >> -----Original Message-----
> >> From: Arnd Bergmann [mailto:[e-mail]]
> >> Sent: Monday, March 14, 2016 6:26 AM
> >> To: [e-mail]
> >> Cc: Yangbo Lu; [e-mail]; linux-arm-
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail]; [e-mail]-
> >> foundation.org; [e-mail]; [e-mail];
> >> [e-mail]; Zhao Qiang; Russell King; Bhupesh Sharma; Joerg
> >> Roedel; Santosh Shilimkar; Scott Wood; Rob Herring; Claudiu Manoil; Kumar
> >> Gala; Yang-Leo Li; Xiaobo Xie
> >> Subject: Re: [v6, 5/5] mmc: sdhci-of-esdhc: fix host version for T4240-
> >> R1.0-R2.0
> >>
> >> On Wednesday 09 March 2016 18:08:51 Yangbo Lu wrote:
> >>> @@ -567,10 +580,20 @@ static void esdhc_init(struct platform_device
> >> *pdev, struct sdhci_host *host)
> >>>         struct sdhci_pltfm_host *pltfm_host;
> >>>         struct sdhci_esdhc *esdhc;
> >>>         u16 host_ver;
> >>> +       u32 svr;
> >>>
> >>>         pltfm_host = sdhci_priv(host);
> >>>         esdhc = sdhci_pltfm_priv(pltfm_host);
> >>>
> >>> +       fsl_guts_init();
> >>> +       svr = fsl_guts_get_svr();
> >>> +       if (svr) {
> >>> +               esdhc->soc_ver = SVR_SOC_VER(svr);
> >>> +               esdhc->soc_rev = SVR_REV(svr);
> >>> +       } else {
> >>> +               dev_err(&pdev->dev, "Failed to get SVR value!\n");
> >>> +       }
> >>> +
> >>
> >> This makes the driver non-portable. Better identify the specific
> >> workarounds based on the compatible string for this device, or add a
> >> boolean DT property for the quirk.
> >>
> >> 	Arnd
> > 
> > [Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS in v1 before.
> > https://patchwork.kernel.org/patch/6834221/
> > 
> > We don’t have a separate DTS file for each revision of an SOC and if we did, we'd constantly have people using the wrong one.
> > In addition, the device tree is stable ABI and errata are often discovered after device tree are deployed.
> > See the link for details.
> > 
> > So we decide to read SVR from the device-config/guts MMIO block other than using DTS.
> > Thanks.
> 
> Also note that this driver is already only for fsl-specific hardware,
> and it will still work even if fsl_guts doesn't find anything to bind to
> -- it just wouldn't be able to detect errata based on SVR in that case.

IIRC, it is the same IP block as i.MX and Arnd's point is this won't 
even compile on !PPC. It is things like this that prevent sharing the 
driver. Dealing with Si revs is a common problem. We should have a 
common solution. There is soc_device for this purpose.

OTOH, the integration differences may be enough that trying to have a 
common driver with i.MX would not be worth it.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arnd Bergmann March 17, 2016, 5:06 p.m. UTC | #5

On Thursday 17 March 2016 12:01:01 Rob Herring wrote:
> On Mon, Mar 14, 2016 at 05:45:43PM +0000, Scott Wood wrote:

> > >> This makes the driver non-portable. Better identify the specific
> > >> workarounds based on the compatible string for this device, or add a
> > >> boolean DT property for the quirk.
> > >>
> > >>    Arnd
> > > 
> > > [Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS in v1 before.
> > > https://patchwork.kernel.org/patch/6834221/
> > > 
> > > We don’t have a separate DTS file for each revision of an SOC and if we did, we'd constantly have people using the wrong one.
> > > In addition, the device tree is stable ABI and errata are often discovered after device tree are deployed.
> > > See the link for details.
> > > 
> > > So we decide to read SVR from the device-config/guts MMIO block other than using DTS.
> > > Thanks.
> > 
> > Also note that this driver is already only for fsl-specific hardware,
> > and it will still work even if fsl_guts doesn't find anything to bind to
> > -- it just wouldn't be able to detect errata based on SVR in that case.
> 
> IIRC, it is the same IP block as i.MX and Arnd's point is this won't 
> even compile on !PPC. It is things like this that prevent sharing the 
> driver.

I think the first four patches take care of building for ARM,
but the problem remains if you want to enable COMPILE_TEST as
we need for certain automated checking.

> Dealing with Si revs is a common problem. We should have a 
> common solution. There is soc_device for this purpose.

Exactly. The last time this came up, I think we agreed to implement a
helper using glob_match() on the soc_device strings. Unfortunately
this hasn't happened then, but I'd still prefer that over yet another
vendor-specific way of dealing with the generic issue.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Scott Wood March 18, 2016, 6:28 p.m. UTC | #6

On 03/17/2016 12:06 PM, Arnd Bergmann wrote:
> On Thursday 17 March 2016 12:01:01 Rob Herring wrote:
>> On Mon, Mar 14, 2016 at 05:45:43PM +0000, Scott Wood wrote:
> 
>>>>> This makes the driver non-portable. Better identify the specific
>>>>> workarounds based on the compatible string for this device, or add a
>>>>> boolean DT property for the quirk.
>>>>>
>>>>>    Arnd
>>>>
>>>> [Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS in v1 before.
>>>> https://patchwork.kernel.org/patch/6834221/
>>>>
>>>> We don’t have a separate DTS file for each revision of an SOC and if we did, we'd constantly have people using the wrong one.
>>>> In addition, the device tree is stable ABI and errata are often discovered after device tree are deployed.
>>>> See the link for details.
>>>>
>>>> So we decide to read SVR from the device-config/guts MMIO block other than using DTS.
>>>> Thanks.
>>>
>>> Also note that this driver is already only for fsl-specific hardware,
>>> and it will still work even if fsl_guts doesn't find anything to bind to
>>> -- it just wouldn't be able to detect errata based on SVR in that case.
>>
>> IIRC, it is the same IP block as i.MX and Arnd's point is this won't 
>> even compile on !PPC. It is things like this that prevent sharing the 
>> driver.

The whole point of using the MMIO SVR instead of the PPC SPR is so that
it will work on ARM...  The guts driver should build on any platform as
long as OF is enabled, and if it doesn't find a node to bind to it will
return 0 for SVR, and the eSDHC driver will continue (after printing an
error that should be removed) without the ability to test for errata
based on SVR.

> I think the first four patches take care of building for ARM,
> but the problem remains if you want to enable COMPILE_TEST as
> we need for certain automated checking.

What specific problem is there with COMPILE_TEST?

>> Dealing with Si revs is a common problem. We should have a 
>> common solution. There is soc_device for this purpose.
> 
> Exactly. The last time this came up, I think we agreed to implement a
> helper using glob_match() on the soc_device strings. Unfortunately
> this hasn't happened then, but I'd still prefer that over yet another
> vendor-specific way of dealing with the generic issue.

soc_device would require encoding the SVR as a string and then decoding
the string, which is more complicated and error prone than having
platform-specific code test a platform-specific number.  And when would
it get registered on arm64, which doesn't have platform code?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yangbo Lu March 25, 2016, 6:43 a.m. UTC | #7

> -----Original Message-----
> From: Scott Wood
> Sent: Saturday, March 19, 2016 2:28 AM
> To: Arnd Bergmann; Rob Herring
> Cc: Yangbo Lu; [e-mail]; [e-mail];
> [e-mail]; Zhao Qiang; Russell King; Bhupesh Sharma;
> [e-mail]; Joerg Roedel; Kumar Gala; linux-
> [e-mail]; [e-mail]; Yang-Leo Li;
> [e-mail]; [e-mail]; Claudiu
> Manoil; Santosh Shilimkar; Xiaobo Xie; [e-mail]; linux-
> [e-mail]
> Subject: Re: [v6, 5/5] mmc: sdhci-of-esdhc: fix host version for T4240-
> R1.0-R2.0
> 
> On 03/17/2016 12:06 PM, Arnd Bergmann wrote:
> > On Thursday 17 March 2016 12:01:01 Rob Herring wrote:
> >> On Mon, Mar 14, 2016 at 05:45:43PM +0000, Scott Wood wrote:
> >
> >>>>> This makes the driver non-portable. Better identify the specific
> >>>>> workarounds based on the compatible string for this device, or add
> >>>>> a boolean DT property for the quirk.
> >>>>>
> >>>>>    Arnd
> >>>>
> >>>> [Lu Yangbo-B47093] Hi Arnd, we did have a discussion about using DTS
> in v1 before.
> >>>> https://patchwork.kernel.org/patch/6834221/
> >>>>
> >>>> We don't have a separate DTS file for each revision of an SOC and if
> we did, we'd constantly have people using the wrong one.
> >>>> In addition, the device tree is stable ABI and errata are often
> discovered after device tree are deployed.
> >>>> See the link for details.
> >>>>
> >>>> So we decide to read SVR from the device-config/guts MMIO block
> other than using DTS.
> >>>> Thanks.
> >>>
> >>> Also note that this driver is already only for fsl-specific
> >>> hardware, and it will still work even if fsl_guts doesn't find
> >>> anything to bind to
> >>> -- it just wouldn't be able to detect errata based on SVR in that
> case.
> >>
> >> IIRC, it is the same IP block as i.MX and Arnd's point is this won't
> >> even compile on !PPC. It is things like this that prevent sharing the
> >> driver.
> 
> The whole point of using the MMIO SVR instead of the PPC SPR is so that
> it will work on ARM...  The guts driver should build on any platform as
> long as OF is enabled, and if it doesn't find a node to bind to it will
> return 0 for SVR, and the eSDHC driver will continue (after printing an
> error that should be removed) without the ability to test for errata
> based on SVR.
> 
> > I think the first four patches take care of building for ARM, but the
> > problem remains if you want to enable COMPILE_TEST as we need for
> > certain automated checking.
> 
> What specific problem is there with COMPILE_TEST?
> 
> >> Dealing with Si revs is a common problem. We should have a common
> >> solution. There is soc_device for this purpose.
> >
> > Exactly. The last time this came up, I think we agreed to implement a
> > helper using glob_match() on the soc_device strings. Unfortunately
> > this hasn't happened then, but I'd still prefer that over yet another
> > vendor-specific way of dealing with the generic issue.
> 
> soc_device would require encoding the SVR as a string and then decoding
> the string, which is more complicated and error prone than having
> platform-specific code test a platform-specific number.  And when would
> it get registered on arm64, which doesn't have platform code?
> 
> -Scott

[Lu Yangbo-B47093] Hi Arnd, could you answer Scott's questions?
If you don't oppose this patch, I'd like to rework a new version for merging.
Thanks.

:)




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 04feea8..5743b05 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -142,6 +142,7 @@  config MMC_SDHCI_OF_ESDHC
 	depends on MMC_SDHCI_PLTFM
 	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
 	select MMC_SDHCI_IO_ACCESSORS
+	select FSL_GUTS
 	help
 	  This selects the Freescale eSDHC controller support.
 
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 3f34d35..68cc020 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -18,6 +18,8 @@ 
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/fsl/svr.h>
+#include <linux/fsl/guts.h>
 #include <linux/mmc/host.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
@@ -28,6 +30,8 @@ 
 struct sdhci_esdhc {
 	u8 vendor_ver;
 	u8 spec_ver;
+	u32 soc_ver;
+	u8 soc_rev;
 };
 
 /**
@@ -73,6 +77,8 @@  static u32 esdhc_readl_fixup(struct sdhci_host *host,
 static u16 esdhc_readw_fixup(struct sdhci_host *host,
 				     int spec_reg, u32 value)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_esdhc *esdhc = sdhci_pltfm_priv(pltfm_host);
 	u16 ret;
 	int shift = (spec_reg & 0x2) * 8;
 
@@ -80,6 +86,13 @@  static u16 esdhc_readw_fixup(struct sdhci_host *host,
 		ret = value & 0xffff;
 	else
 		ret = (value >> shift) & 0xffff;
+
+	/* Workaround for T4240-R1.0-R2.0 eSDHC which has incorrect
+	 * vendor version and spec version information.
+	 */
+	if ((spec_reg == SDHCI_HOST_VERSION) &&
+	    (esdhc->soc_ver == SVR_T4240) && (esdhc->soc_rev <= 0x20))
+		ret = (VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200;
 	return ret;
 }
 
@@ -567,10 +580,20 @@  static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_esdhc *esdhc;
 	u16 host_ver;
+	u32 svr;
 
 	pltfm_host = sdhci_priv(host);
 	esdhc = sdhci_pltfm_priv(pltfm_host);
 
+	fsl_guts_init();
+	svr = fsl_guts_get_svr();
+	if (svr) {
+		esdhc->soc_ver = SVR_SOC_VER(svr);
+		esdhc->soc_rev = SVR_REV(svr);
+	} else {
+		dev_err(&pdev->dev, "Failed to get SVR value!\n");
+	}
+
 	host_ver = sdhci_readw(host, SDHCI_HOST_VERSION);
 	esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
 			     SDHCI_VENDOR_VER_SHIFT;