On Fri, Nov 13, 2015 at 4:31 AM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
> On 12 November 2015 at 20:44, Alan Cooper <alcoop...@gmail.com> wrote:
>> On Thu, Nov 12, 2015 at 4:35 AM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
>>> On 6 November 2015 at 19:56, Al Cooper <alcoop...@gmail.com> 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 <alcoop...@gmail.com>
>>>> ---
>>>> 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.
As a possible first step, what about the following:
Change sdhci_add_host() to take an additional parameter that, if not
NULL, is a pointer to a data structure that contains overrides for
CAPS and CAPS1 in the form of a mask/value pair. These pairs would be
used by sdhci_add_host() to modify any field after reading the CAPS
and CAPS1 registers. With this in place it looks like the following
QUIRKS would no longer be needed because they could be handled in the
variant drivers using overrides:
SDHCI_QUIRK_FORCE_DMA
SDHCI_QUIRK_BROKEN_DMA
SDHCI_QUIRK_BROKEN_ADMA
SDHCI_QUIRK_FORCE_BLK_SZ_2048
SDHCI_QUIRK_MISSING_CAPS
SDHCI_QUIRK2_NO_1_8_V
SDHCI_QUIRK2_BROKEN_DDR50
SDHCI_QUIRK2_BROKEN_64_BIT_DMA
I would add device tree properties for the overrides to my variant
driver. I could also add the overrides as module params. There are
also many drivers that hook the sdhci_readl() routine to modify the
CAPS fields that could be changed to use the overrides.
With this change, my variant driver that supports more than 10
different MIPS/ARM SoC's would be very small with all the SoC
differences handled using overrides in the device tree and without the
use of any QUIRKS.
>
>>
>>>> }
>>>> #else
>>>> void sdhci_get_of_property(struct platform_device *pdev) {}
>>>> --
>>>> 1.9.0.138.g2de3478
>>>>
>>>
>>> Kind regards
>>> Uffe
>>
>> Thanks
>> Al
>
> Kind regards
> Uffe
Thanks
Al
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
