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? > >> 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? >> } >> #else >> void sdhci_get_of_property(struct platform_device *pdev) {} >> -- >> 1.9.0.138.g2de3478 >> > > Kind regards > Uffe Thanks Al -- 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
