On 12 November 2015 at 20:44, Alan Cooper <alcooperx@xxxxxxxxx> wrote: > On Thu, Nov 12, 2015 at 4:35 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 6 November 2015 at 19:56, Al Cooper <alcooperx@xxxxxxxxx> wrote: >>> Add support for "broken-ddr50", "broken-64-bit-dma" >>> and "broken-timeout-value" device tree properties. >>> The properties will cause the corresponding quirks bits to be set. >>> This allows some of the platform specific QUIRKS setting to be >>> moved out of the driver and into the Device Tree node. >>> >>> Signed-off-by: Al Cooper <alcooperx@xxxxxxxxx> >>> --- >>> drivers/mmc/host/sdhci-pltfm.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c >>> index 87fb5ea..cc0730c 100644 >>> --- a/drivers/mmc/host/sdhci-pltfm.c >>> +++ b/drivers/mmc/host/sdhci-pltfm.c >>> @@ -90,10 +90,14 @@ void sdhci_get_of_property(struct platform_device *pdev) >>> if (of_get_property(np, "no-1-8-v", NULL)) >>> host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >>> >>> + if (of_get_property(np, "broken-64-bit-dma", NULL)) >>> + host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA; >>> + >> >> To me this is yet another step in the wrong direction of sdhci. >> >> Not only have we added a quirk which we likely could avoided, but now >> you also suggest to add a DT binding for it. >> >> Please try to avoid this, we shouldn't need to add one DT binding per >> quirk, right? > > The BRCMSTB SDHCI driver needs to support a mix of existing and future > ARM and MIPS SoC's on a variety of boards, and many of the > combinations have issues. In older separate MIPS/ARM internal versions > of the driver, issues were handled by a combination of QUIRKS and the > use of custom SDHCI registers that allowed the Host CAPS registers to > be changed on a per chip basis. While working on a unified driver for > 4.1, I realized that almost all the issues solved by overriding the > CAPs registers could also be done using an existing QUIRK and if I had > device tree properties that set the QUIRKs, that all the chip/board > specific knowledge would end up in the device tree node instead of in > the driver. This also allowed me to stop using the non-standard CAPS > override registers that tended to change per chip. This approach also > allows the driver to work, without change, on future chips/boards as > long as they don't have any new issues and they have the proper device > tree node. > > Would there be any objection to me continuing this approach if I moved > all the functionality into the sdhci-brcm.c driver and left sdhci.c > and sdhci-pltfm.c untouched? Let me elaborate a bit on what I think you/we need to do. The problem with the quirks is that these exists because of how the sdhci code is designed. Sdhci core should have been implemented as a pure library providing a set of common functions. Today it's more of a driver than a library. All, or at least most of the variant specific code, should have been dealt from each of the sdhci variant drivers, instead of via quirks and sdhci callbacks. If you didn't notice my recent statement regarding the above on linux-mmc, let's me say it again. I have a reached a point where I think sdhci has become unmaintainable (and non-optimized coding wise) and I am therefore encouraging people to stop adding new quirks or sdhci callbacks, but instead "libraryfy" sdhci. I would really appreciate if you could help moving sdhci in that direction! That said, you won't be able to leave the sdhci core code untouched, because that would in principle mean that you will need to duplicate much of the sdhci code into your sdhci-brcm driver. Instead, try to split-up/re-factor common sdhci code into library functions, which your sdhci-brcm driver can use. I guess one of the hardest part in this effort will be to not break existing sdhci variant drivers. Let's do our best in that effort, but we will rely on more people to participate in testing/coding. > >> >>> if (of_device_is_compatible(np, "fsl,p2020-rev1-esdhc")) >>> host->quirks |= SDHCI_QUIRK_BROKEN_DMA; >>> >>> - if (of_device_is_compatible(np, "fsl,p2020-esdhc") || >>> + if (of_get_property(np, "broken-timeout-value", NULL) || >>> + of_device_is_compatible(np, "fsl,p2020-esdhc") || >>> of_device_is_compatible(np, "fsl,p1010-esdhc") || >>> of_device_is_compatible(np, "fsl,t4240-esdhc") || >>> of_device_is_compatible(np, "fsl,mpc8536-esdhc")) >>> @@ -106,6 +110,8 @@ void sdhci_get_of_property(struct platform_device *pdev) >>> >>> if (of_find_property(np, "enable-sdio-wakeup", NULL)) >>> host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ; >>> + if (of_find_property(np, "broken-ddr50", NULL)) >>> + host->quirks2 |= SDHCI_QUIRK2_BROKEN_DDR50; >> >> Same comment as above. >> >> Let me also refer to the response from Scott Branden to the earlier >> version of this patchset. >> http://permalink.gmane.org/gmane.linux.kernel.mmc/34614 >> >> Perhaps we should invent a sdhci library function, which each sdhci >> variant can use to set capabilities through. Typically useful for >> those variants that have a non-reliable capabilities register. >> > > Do you mean something more than using SDHCI_QUIRK_MISSING_CAPS which > allows the driver to supply the 2 CAPS registers instead of having > sdhci.c get them by reading the Host CAPS registers? Yes, something like that, but preferably without needing to use *any* quirk. >From an sdhci core perspective there could be a library function which parses the Host CAPS register. Some sdhci variants could use that function and some couldn't since the informations in the CAPS register isn't reliable. > >>> } >>> #else >>> void sdhci_get_of_property(struct platform_device *pdev) {} >>> -- >>> 1.9.0.138.g2de3478 >>> >> >> Kind regards >> Uffe > > Thanks > Al Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
