Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes

[Stefan Agner <stefan@agner.ch>]

On 04.07.2018 13:16, Ulf Hansson wrote:
> On 4 July 2018 at 12:55, Stefan Agner <stefan@agner.ch> wrote:
>> On 04.07.2018 12:07, Ulf Hansson wrote:
>>> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote:
>>>> On 02.07.2018 16:36, Ulf Hansson wrote:
>>>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote:
>>>>>> Some hosts are capable of running higher speed modes but do not
>>>>>> have the board support for it. Introduce a quirk which prevents
>>>>>> the stack from using modes running at 100MHz or faster.
>>>>>
>>>>> To cap the freq, use the DT property "max-frequency". To enable
>>>>> certain speed modes, use the corresponding speed mode binding. For
>>>>> example "sd-uhs-sdr*" and "mmc-hs200*".
>>>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt
>>>>
>>>> I had bad experience with max-frequency: Some higher speed modes seem
>>>> not to work reliably if constraint to low frequencies. E.g. we had lots
>>>> of devices fail in practise with HS400@100MHz... So it is doing what it
>>>> should, but it just seems that higher speed modes do not necessarily run
>>>> well with lower frequencies...
>>>>
>>>> So I'd rather prefer to limit speed modes as it is done right now.
>>>>
>>>>>
>>>>> In case the sdhci cap register, doesn't reflect the board support
>>>>> properly, such that you may want to disable some speed modes, then you
>>>>> may benefit from using the DT properties "sdhci-caps*.
>>>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt
>>>>
>>>> Hm, yeah I guess something like
>>>>
>>>> sdhci-caps-mask =  /bits/ 64 <((SDHCI_SUPPORT_SDR104 |
>>>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)>
>>>>
>>>> would come close.
>>>>
>>>> But it does not restrict MMC modes such as HS200/HS400. There seem to be
>>>> no mmc-caps...
>>>
>>> Right.
>>>
>>> The solution to fix this, should be to *not* set those DT properties,
>>> like "mmc-hs*" for example. That should work, no?
>>>
>>
>> The controller does not make use of the dt modes so far, so I can't not
>> set those properties...
> 
> Then where are the corresponding caps for the eMMC speed modes being set?
> 
> Can't you just avoid setting them?
> 

That was the right question:

On closer look, the higher speed MMC modes are actually not set at all!

So just using sdhci-caps-mask = <0x7 0x0>; (which masks
SDR104/SDR50/DDR50) is doing the job as far as I can tell.

>>
>>>>
>>>>
>>>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not
>>>> restrict modes correctly. Currently the driver checks whether >=100MHz
>>>> pinctrl settings are available, and if not uses the quirk to restrict
>>>> higher speed modes. Removing that would break device tree backward
>>>> compatibility...
>>>
>>> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather
>>> how the pinctrl setting becomes interpreted when setting the quirk.
>>>
>>
>> Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just
>> not the quirk this driver needs.
>>
>> I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl
>> state according to uhs mode") chose the wrong quirk from the
>> beginning...
> 
> That's seems reasonable! But it's been there since 3.13, so I guess we
> have to think about backwards compatibility issues, as you stated.
> 

It seems that the driver already fakes
SDHCI_CAPABILITIES/SDHCI_CAPABILITIES_1 in esdhc_readl_le, so instead
using the sdhci-caps-mask we could implement the work around there. Not
very clean, but backward compatible and everything self contained in the
driver.

--
Stefan

>>
>> Afaict, the quirk needed here does not exist.
> 
> [...]
> 
> Kind regards
> Uffe