linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: "H. Nikolaus Schaller" <hns@goldelico.com>
Cc: "Jérôme Pouiller" <Jerome.Pouiller@silabs.com>,
	"Avri Altman" <avri.altman@wdc.com>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Tony Lindgren" <tony@atomide.com>,
	"Bean Huo" <beanhuo@micron.com>,
	"Gražvydas Ignotas" <notasas@gmail.com>,
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
	letux-kernel@openphoenux.org, kernel@pyra-handheld.com
Subject: Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
Date: Wed, 10 Nov 2021 18:03:01 +0100	[thread overview]
Message-ID: <CAPDyKFqsBkWBajYvS2DXsHzO01-hgQp7YuzTs61N7vmLkZLgKg@mail.gmail.com> (raw)
In-Reply-To: <B6ECEECF-EC1D-431E-B4E4-915B29E31AEE@goldelico.com>

On Wed, 10 Nov 2021 at 17:36, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> 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.

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

  reply	other threads:[~2021-11-10 17:03 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-05  9:05 [RFC v4 0/6] mmc: core: extend mmc_fixup_device and transplant ti,wl1251 quirks from to be retired omap_hsmmc H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 1/6] mmc: core: rewrite mmc_fixup_device() H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks H. Nikolaus Schaller
2021-11-05 14:27   ` Jérôme Pouiller
2021-11-06 14:31     ` H. Nikolaus Schaller
2021-11-08 15:00       ` Ulf Hansson
2021-11-08 15:34         ` Jérôme Pouiller
2021-11-08 16:01           ` H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 3/6] mmc: core: provide macro and table " H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods) H. Nikolaus Schaller
2021-11-08 15:08   ` Ulf Hansson
2021-11-08 16:07     ` H. Nikolaus Schaller
2021-11-08 15:39   ` Jérôme Pouiller
2021-11-08 16:04     ` H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc H. Nikolaus Schaller
2021-11-08 15:33   ` Ulf Hansson
2021-11-08 16:08     ` H. Nikolaus Schaller
2021-11-09 10:58     ` H. Nikolaus Schaller
2021-11-09 20:01       ` Ulf Hansson
2021-11-10 16:36         ` H. Nikolaus Schaller
2021-11-10 17:03           ` Ulf Hansson [this message]
2021-11-10 17:09             ` H. Nikolaus Schaller
2021-11-05  9:05 ` [RFC v4 6/6] mmc: host: omap_hsmmc: revert special init for wl1251 H. Nikolaus Schaller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPDyKFqsBkWBajYvS2DXsHzO01-hgQp7YuzTs61N7vmLkZLgKg@mail.gmail.com \
    --to=ulf.hansson@linaro.org \
    --cc=Jerome.Pouiller@silabs.com \
    --cc=avri.altman@wdc.com \
    --cc=beanhuo@micron.com \
    --cc=hns@goldelico.com \
    --cc=kernel@pyra-handheld.com \
    --cc=letux-kernel@openphoenux.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=notasas@gmail.com \
    --cc=shawn.lin@rock-chips.com \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).