Re: [PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
From: Scott Wood <oss@buserror.net>
Date: 2016-06-02 01:11:40
Also in:
linux-arm-kernel, linux-clk, linux-devicetree, linux-i2c, linux-iommu, linux-mmc, linuxppc-dev, lkml
On Mon, 2016-05-30 at 15:16 +0200, Arnd Bergmann wrote:
quoted hunk ↗ jump to hunk
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_datasdhci_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