On Wed, 10 Nov 2021 at 17:36, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote: > > Hi Ulf, > > > Am 09.11.2021 um 21:01 schrieb Ulf Hansson <ulf.hansson@xxxxxxxxxx>: > > > > On Tue, 9 Nov 2021 at 11:58, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote: > >> > >> Hi Ulf, > >> > >>> Am 08.11.2021 um 16:33 schrieb Ulf Hansson <ulf.hansson@xxxxxxxxxx>: > >>> > >>> On Fri, 5 Nov 2021 at 10:06, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> 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. That's great! Thanks for testing, again! > > > > >> > >> 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. Yes, I can pick up my patch first. As it's not really fixing a problem, but rather preparing for your series to work better, I don't think we need to care about stable backports, at least for now. If you re-submit before rc1, then just include my patch early in your series. > > BR and thanks, > Nikolaus Kind regards Uffe
