lkml.org 
[lkml]   [2021]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
From
Date
Hi Ulf,

> Am 09.11.2021 um 21:01 schrieb Ulf Hansson <ulf.hansson@linaro.org>:
>
> On Tue, 9 Nov 2021 at 11:58, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>
>> Hi Ulf,
>>
>>> Am 08.11.2021 um 16:33 schrieb Ulf Hansson <ulf.hansson@linaro.org>:
>>>
>>> On Fri, 5 Nov 2021 at 10:06, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>>
>>>> + card->quirks |= MMC_QUIRK_NONSTD_SDIO;
>>>> + card->cccr.wide_bus = 1;
>>>> + card->cis.vendor = 0x104c;
>>>> + card->cis.device = 0x9066;
>>>> + card->cis.blksize = 512;
>>>> + card->cis.max_dtr = 24000000;
>>>> + card->ocr = 0x80;
>>>
>>> In the past, we discussed a bit around why card->ocr needs to be set here.
>>>
>>> The reason could very well be that the DTS file is specifying the
>>> vmmc-supply with 1.8V fixed regulator, which seems wrong to me.
>>
>> I have checked with the schematics but the wlan_en regulator-fixed is just a GPIO
>> controlling some pin of the wifi chip.
>>
>> I guess it enables some regulator or power switch inside the wifi module which
>> has unknown voltage.
>>
>> We can interpret this as two sequential power-switches. The first one controlled
>> by the gpio-register bit and switches gpio power to the gpio pad of the SoC. The second
>> one switches the battery voltage to the internal circuits of the wifi module.
>>
>> The GPIO itself is on 1.8V VIO level which seems to be the reason for the min/max.
>>
>> Now it is a little arbitrary what the DTS describes: the gpio voltage or the unknown
>> internal voltage of the second switch.
>>
>> So from hardware perspective the min/max values are irrelevant.
>
> I completely agree with you! That's also why I earlier suggested
> moving to use an mmc-pwrseq node
> (Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml), that
> would allow a better description of the HW.

Basically yes.

> Nevertheless, the important point is that the mmc core gets a valid
> host->ocr_avail to work with during card initialization. And in this
> case, it's probably good enough to model this via changing the
> regulator-min|max-microvolt to get a proper value from the
> "regulator".
>
>>
>>>
>>> I would be very interested to know if we would change
>>> "regulator-min|max-microvolt" of the regulator in the DTS, into
>>> somewhere in between 2700000-3600000 (2.7-3.6V)
>>
>> Ok, if the mmc driver does something with these values it may have indeed an influence.
>>
>>> - and see if that
>>> allows us to drop the assignment of "card->ocr = 0x80;" above. Would
>>> you mind doing some tests for this?
>>
>> Well, with min/max=3.3V and no ocr I get:
>>
>> [ 2.765136] omap_hsmmc 480ad000.mmc: card claims to support voltages below defined range
>> [ 2.776367] omap_hsmmc 480ad000.mmc: found wl1251
>> [ 2.782287] mmc2: new SDIO card at address 0001
>
> That's really great information! During the first initialization
> attempt, things are working fine and the SDIO card gets properly
> detected.
>
>> [ 10.874237] omap_hsmmc 480ad000.mmc: could not set regulator OCR (-22)
>> [ 10.945373] wl1251_sdio: probe of mmc2:0001:1 failed with error -16
>
> It looks like the card is being re-initialized when it's time to probe
> with the SDIO func driver. This makes sense, assuming it's been
> powered off via runtime PM (the "cap-power-off-card" DT property
> should be set in the DTS for this card's slot).
>
> I looked a bit closer to understand the problem above and then I
> realized why the card->ocr is being set from omap_hsmmc ->init_card()
> callback. It's most likely because the mmc core in
> mmc_sdio_init_card() doesn't save the card->ocr when
> MMC_QUIRK_NONSTD_SDIO is set. Instead it becomes the responsibility
> for the ->init_card() callback to do it, which seems wrong to me.
>
> Note that the card->ocr is being used when re-initializing the SDIO card.
>
> I have just sent a patch [1], would you mind trying it, in combination
> with not assigning card->ocr in $subject patch?

Yes, it works! I have not even played with the wlan_en regulator voltage.

>
>>
>> Adding back card->ocr = 0x80 (and keeping 3.3V for min/max) shows exactly the same.
>>
>> Only min/max 1.8V + OCR works:
>>
>> [ 2.824188] mmc2: new SDIO card at address 0001
>> [ 2.806518] omap_hsmmc 480ad000.mmc: card claims to support voltages below defined range
>> [ 2.815979] omap_hsmmc 480ad000.mmc: found wl1251
>> [ 10.981018] omap_hsmmc 480ad000.mmc: found wl1251
>> [ 11.018280] wl1251: using dedicated interrupt line
>> [ 11.321136] wl1251: loaded
>> [ 11.378601] wl1251: initialized
>> [ 14.521759] omap_hsmmc 480ad000.mmc: found wl1251
>> [ 38.680725] omap_hsmmc 480ad000.mmc: found wl1251
>> [ 39.646942] wl1251: 151 tx blocks at 0x3b788, 35 rx blocks at 0x3a780
>> [ 39.654785] wl1251: firmware booted (Rev 4.0.4.3.7)
>>
>> Therefore I also tried the 4th combination: min/max 1.8V and no ocr quirk and it fails again.
>>
>> Finally I tried setting min to 2.7V and max to 3.6V. This ends up in
>>
>> [ 0.402648] reg-fixed-voltage fixed-regulator-wg7210_en: Fixed regulator specified with variable voltages
>>
>> So it seems that we need both: min/max = 1.8V and OCR. A little unexpected since I had expected
>> that min/max is completely irrelevant.
>>
>>> If that works, we should add some comments about it above, I think.
>>
>> So at the moment no change for [PATCH v1] which I can now send out.
>>
>> BR and thanks,
>> Nikolaus
>>
>
> Thanks a lot for doing these tests! If I am right, it looks like we
> should be able to skip assigning card->ocr for this quirk, but let's
> see.

Indeed we can. That is great.

Now the question is how to handle the dependency on your patch.
Somehow we must ensure that it is merged before my $subject patch.
Even if someone decides to backport this to stable.

BR and thanks,
Nikolaus
\
 
 \ /
  Last update: 2021-11-10 17:37    [W:0.851 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site