Re: [PATCH V2 1/3] mmc: sdhci-pltfm: Add DT properties to set various QUIRKS

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux