diff mbox

[3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

Message ID 6110875.0k1HlSKhzn@wuerfel (mailing list archive)
State Rejected, archived
Delegated to: Stephen Boyd
Headers show

Commit Message

Arnd Bergmann May 30, 2016, 1:16 p.m. UTC 
This is a rewrite of an earlier patch from Yangbo Lu, adding a quirk
for the NXP QorIQ T4240 in the detection of the host device version.

Unfortunately, this device cannot be detected using the compatible
string, as we have to support existing DTS files that use the generic
"fsl,t4240-esdhc" identifier but that have other host versions that
are correctly detected.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>


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

Comments

Crystal Wood June 2, 2016, 1:11 a.m. UTC | #1 
On Mon, 2016-05-30 at 15:16 +0200, Arnd Bergmann wrote:
> This is a rewrite of an earlier patch from Yangbo Lu, adding a quirk
> for the NXP QorIQ T4240 in the detection of the host device version.
> 
> Unfortunately, this device cannot be detected using the compatible
> string, as we have to support existing DTS files that use the generic
> "fsl,t4240-esdhc" identifier but that have other host versions that
> are correctly detected.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of
> -esdhc.c
> index 3f34d354f1fc..1d4814fe4cb2 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -73,14 +73,16 @@ 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;
>  
>  	if (spec_reg == SDHCI_HOST_VERSION)
> -		ret = value & 0xffff;
> -	else
> -		ret = (value >> shift) & 0xffff;
> -	return ret;
> +		return esdhc->vendor_ver << SDHCI_VENDOR_VER_SHIFT |
> +		       esdhc->spec_ver;
> +
> +	return (value >> shift) & 0xffff;
>  }
>  
>  static u8 esdhc_readb_fixup(struct sdhci_host *host,
> @@ -562,16 +564,32 @@ static const struct sdhci_pltfm_data
> sdhci_esdhc_le_pdata = {
>  	.ops = &sdhci_esdhc_le_ops,
>  };
>  
> +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> SDHCI_SPEC_200)
> +static const struct soc_device_attribute esdhc_t4240_quirk = {
> +	/* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> */
> +	{ .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> +	  .data = (void *)(uintptr_t)(T4240_HOST_VER) },

Why should this code need to care that the string begins with "T4"?  This
creates dual maintenance if that were to change.  It's also broken because
T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
-2.0" and thus with these patches it would incorrectly show up as "P series
(0x824000)".  The compatible string of this node was never meant to be a key
for choosing a string to describe the system to userspace.

0x824000 is a magic number which should be represented symbolically.

If T4240 is affected, then so are the reduced-core variants T4160 and T4080,
but 0x824000 doesn't match them (Yangbo's patch had the same problem).  And
please don't respond with "0x824*"

You also didn't strip out the E bit of SVR which indicates encryption
capability and nothing else (Yangbo's patch did not have this problem because
it used SVR_SOC_VER).

What happens if the revision condition is more complicated, such as <= 0x20
with 0x21 being fine?  Multiple quirk entries where before we had as simple
comparison?

I fail to see how this approach is an improvement (much less one that needs to
hold up a patchset that is fixing a problem and is not touching any generic
code).  Why does this need to be a string?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 2, 2016, 8:52 a.m. UTC | #2 
On Wednesday, June 1, 2016 8:11:14 PM CEST Scott Wood wrote:
> > +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> > SDHCI_SPEC_200)
> > +static const struct soc_device_attribute esdhc_t4240_quirk = {
> > +     /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> > */
> > +     { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> > +       .data = (void *)(uintptr_t)(T4240_HOST_VER) },
> 
> Why should this code need to care that the string begins with "T4"?  This
> creates dual maintenance if that were to change.  It's also broken because
> T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
> -2.0" and thus with these patches it would incorrectly show up as "P series
> (0x824000)".  The compatible string of this node was never meant to be a key
> for choosing a string to describe the system to userspace.

This is an artifact of not knowing the specific SoC name, and we can change
that by looking up the name from the SVR value in the soc_device driver.
 
> 0x824000 is a magic number which should be represented symbolically.

Sure, feel free to change the format of the soc_device string in any
name, I just converted the information I had available in the comments.
The only thing that is important here is that the string matches what
the soc_device driver calls it.

> If T4240 is affected, then so are the reduced-core variants T4160 and T4080,
> but 0x824000 doesn't match them (Yangbo's patch had the same problem).  And
> please don't respond with "0x824*"
> 
> You also didn't strip out the E bit of SVR which indicates encryption
> capability and nothing else (Yangbo's patch did not have this problem because
> it used SVR_SOC_VER).

Ok, that should be easy enough to fix in the soc_device driver.

> What happens if the revision condition is more complicated, such as <= 0x20
> with 0x21 being fine?  Multiple quirk entries where before we had as simple
> comparison?

I guess yes. I would really hope that there is no need to use this interface
pervasively, it's really just to work around the cases where there is no
way to pass the information in DT otherwise.

> I fail to see how this approach is an improvement (much less one that needs to
> hold up a patchset that is fixing a problem and is not touching any generic
> code).  Why does this need to be a string?

A string is what user space gets in /sys/devices/soc/*, and we already have
code that does the same things there to work around quirks, here we just
use the same interface in a completely generic way. Note that not every
SoC family uses numbers in the same way, some have multiple subrevisions,
some have names etc.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Crystal Wood June 11, 2016, 2 a.m. UTC | #3 
On Thu, 2016-06-02 at 10:52 +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 8:11:14 PM CEST Scott Wood wrote:
> > > +#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) |
> > > SDHCI_SPEC_200)
> > > +static const struct soc_device_attribute esdhc_t4240_quirk = {
> > > +     /* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200
> > > */
> > > +     { .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
> > > +       .data = (void *)(uintptr_t)(T4240_HOST_VER) },
> > 
> > Why should this code need to care that the string begins with "T4"?  This
> > creates dual maintenance if that were to change.  It's also broken because
> > T4240 has compatible = "fsl,t4240-device-config", "fsl,qoriq-device-config
> > -2.0" and thus with these patches it would incorrectly show up as "P
> > series
> > (0x824000)".  The compatible string of this node was never meant to be a
> > key
> > for choosing a string to describe the system to userspace.
> 
> This is an artifact of not knowing the specific SoC name, and we can change
> that by looking up the name from the SVR value in the soc_device driver.

...or we could keep it simple and just match the number.

> > 0x824000 is a magic number which should be represented symbolically.
> 
> Sure, feel free to change the format of the soc_device string in any
> name,

That's not what I was asking for...  The match should be numeric but the
knowledge of what the number is should come from a symbolic #define.

> > If T4240 is affected, then so are the reduced-core variants T4160 and
> > T4080,
> > but 0x824000 doesn't match them (Yangbo's patch had the same problem). 
> >  And
> > please don't respond with "0x824*"
> > 
> > You also didn't strip out the E bit of SVR which indicates encryption
> > capability and nothing else (Yangbo's patch did not have this problem
> > because
> > it used SVR_SOC_VER).
> 
> Ok, that should be easy enough to fix in the soc_device driver.

No, because the soc_device driver doesn't know whether the consumer of the ID
cares about the E bit.

> > What happens if the revision condition is more complicated, such as <=
> > 0x20
> > with 0x21 being fine?  Multiple quirk entries where before we had as
> > simple
> > comparison?
> 
> I guess yes. I would really hope that there is no need to use this interface
> pervasively, it's really just to work around the cases where there is no
> way to pass the information in DT otherwise.

How does putting it in the DT work when you have multiple versions of the same
SoC, some of which have the bug and some which don't?

> > I fail to see how this approach is an improvement (much less one that
> > needs to
> > hold up a patchset that is fixing a problem and is not touching any
> > generic
> > code).  Why does this need to be a string?
> 
> A string is what user space gets in /sys/devices/soc/*,

It is rare that the kernel accesses information in the exact same way that
userspace does.  And once we expose this to userspace we're stuck with it, so
exporting anything other than a simple number is even less desirable.

>  and we already have
> code that does the same things there to work around quirks, here we just
> use the same interface in a completely generic way. Note that not every
> SoC family uses numbers in the same way, some have multiple subrevisions,
> some have names etc.

Where is the need for a "completely generic way" for one piece of vendor
-specific code to get information that is inherently specific to that vendor,
that is supplied by code specific to that vendor?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 3f34d354f1fc..1d4814fe4cb2 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -73,14 +73,16 @@  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;
 
 	if (spec_reg == SDHCI_HOST_VERSION)
-		ret = value & 0xffff;
-	else
-		ret = (value >> shift) & 0xffff;
-	return ret;
+		return esdhc->vendor_ver << SDHCI_VENDOR_VER_SHIFT |
+		       esdhc->spec_ver;
+
+	return (value >> shift) & 0xffff;
 }
 
 static u8 esdhc_readb_fixup(struct sdhci_host *host,
@@ -562,16 +564,32 @@  static const struct sdhci_pltfm_data sdhci_esdhc_le_pdata = {
 	.ops = &sdhci_esdhc_le_ops,
 };
 
+#define T4240_HOST_VER ((VENDOR_V_23 << SDHCI_VENDOR_VER_SHIFT) | SDHCI_SPEC_200)
+static const struct soc_device_attribute esdhc_t4240_quirk = {
+	/* T4240 revision < 0x20 uses vendor version 23, SDHCI version 200 */
+	{ .soc_id = "T4*(0x824000)", .revision = "0x[01]?",
+	  .data = (void *)(uintptr_t)(T4240_HOST_VER) },
+	{ },
+};
+
 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;
 
 	pltfm_host = sdhci_priv(host);
 	esdhc = sdhci_pltfm_priv(pltfm_host);
 
 	host_ver = sdhci_readw(host, SDHCI_HOST_VERSION);
+
+	if (of_device_is_compatible(pdev->dev.of_node, "fsl,t4240-esdhc")) {
+		struct soc_device_attribute *match;
+
+		match = soc_device_match(&esdhc_t4240_quirk);
+		if (match)
+			host_ver = (uintptr_t)match->data;
+	}
+
 	esdhc->vendor_ver = (host_ver & SDHCI_VENDOR_VER_MASK) >>
 			     SDHCI_VENDOR_VER_SHIFT;
 	esdhc->spec_ver = host_ver & SDHCI_SPEC_VER_MASK;