Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Date:   Wed, 04 Jul 2018 15:18:51 +0200
From:   Stefan Agner <stefan@agner.ch>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     Adrian Hunter <adrian.hunter@intel.com>,
        Fabio Estevam <fabio.estevam@nxp.com>,
        Haibo Chen <haibo.chen@nxp.com>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        Michael Trimarchi <michael@amarulasolutions.com>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        linux-mmc@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] mmc: sdhci: add quirk to prevent higher speed modes
In-Reply-To: <CAPDyKFrDMmMu1kuJdPDyyGKu6b0dR2MF2U4Rnb4hoHAcwNYtuw@mail.gmail.com>
References: <20180628081331.13051-1-stefan@agner.ch>
 <20180628081331.13051-3-stefan@agner.ch>
 <CAPDyKFqPaRRuXdSD50q=et-OvJ=emdbq-XWyeddkiuC5SjuLig@mail.gmail.com>
 <76c619e6267c5f0adfda80d0fdba3c6b@agner.ch>
 <CAPDyKFrjSadT0T2+zWhpq-E5kS1bm01y4R+LfyHyhjFBuK_hLQ@mail.gmail.com>
 <ffea342896629cf57b78661b04f60eda@agner.ch>
 <CAPDyKFrDMmMu1kuJdPDyyGKu6b0dR2MF2U4Rnb4hoHAcwNYtuw@mail.gmail.com>
Message-ID: <6cf2fd635bc65de33edcdf401b035a67@agner.ch>
User-Agent: Roundcube Webmail/1.3.4
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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