| Message ID | 3ca9a3099d86d631235b6c03ae260bc581cc8d60.1636103151.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: core: extend mmc_fixup_device and transplant ti,wl1251 quirks from to be retired omap_hsmmc | expand |
On Fri, 5 Nov 2021 at 10:06, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > The TiWi WL1251 WiFi chip needs special setup of the sdio > interface before it can be probed. > > So far, this is done in omap_hsmmc_init_card() in omap_hsmmc.c > which makes it useable only if connected to omap devices > which use the omap_hsmmc. The OpenPandora is the most promient > example. > > There are plans to switch to a newer sdhci-omap driver and > retire omap_hsmmc. Hence this quirk must be reworked or moved > somewhere else. Ideally to some location that is not dependent > on the specific SoC mmc host driver. > > This is achieved by the new mmc_fixup_device() option introduced > by ("mmc: allow to match the device tree to apply quirks") to match > through device tree compatible string. > > This quirk will be called early right after where host->ops->init_card() > and thus omap_hsmmc_init_card() was previously called. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > drivers/mmc/core/card.h | 19 +++++++++++++++++++ > drivers/mmc/core/quirks.h | 7 +++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h > index 089ede71d3150..20c8dfd6831cf 100644 > --- a/drivers/mmc/core/card.h > +++ b/drivers/mmc/core/card.h > @@ -168,6 +168,25 @@ static inline void __maybe_unused add_limit_rate_quirk(struct mmc_card *card, > card->quirk_max_rate = data; > } > > +static inline void __maybe_unused wl1251_quirk(struct mmc_card *card, > + int data) > +{ > + /* > + * We have TI wl1251 attached to this mmc. Pass this > + * information to the SDIO core because it can't be > + * probed by normal methods. > + */ > + > + dev_info(card->host->parent, "found wl1251\n"); > + 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 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) - and see if that allows us to drop the assignment of "card->ocr = 0x80;" above. Would you mind doing some tests for this? If that works, we should add some comments about it above, I think. > +} > + > /* > * Quirk add/remove for MMC products. > */ > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h > index 41c418527199c..e9813f1f8b23c 100644 > --- a/drivers/mmc/core/quirks.h > +++ b/drivers/mmc/core/quirks.h > @@ -146,7 +146,14 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = { > END_FIXUP > }; > > +static const char *__maybe_unused wl1251_compatible_list[] = { > + "ti,wl1251", > + NULL > +}; > + > static const struct mmc_fixup __maybe_unused sdio_card_init_methods[] = { > + SDIO_FIXUP_COMPATIBLE(wl1251_compatible_list, wl1251_quirk, 0), > + > END_FIXUP > }; > Kind regards Uffe
> 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: >> >> The TiWi WL1251 WiFi chip needs special setup of the sdio >> interface before it can be probed. >> >> So far, this is done in omap_hsmmc_init_card() in omap_hsmmc.c >> which makes it useable only if connected to omap devices >> which use the omap_hsmmc. The OpenPandora is the most promient >> example. >> >> There are plans to switch to a newer sdhci-omap driver and >> retire omap_hsmmc. Hence this quirk must be reworked or moved >> somewhere else. Ideally to some location that is not dependent >> on the specific SoC mmc host driver. >> >> This is achieved by the new mmc_fixup_device() option introduced >> by ("mmc: allow to match the device tree to apply quirks") to match >> through device tree compatible string. >> >> This quirk will be called early right after where host->ops->init_card() >> and thus omap_hsmmc_init_card() was previously called. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> drivers/mmc/core/card.h | 19 +++++++++++++++++++ >> drivers/mmc/core/quirks.h | 7 +++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h >> index 089ede71d3150..20c8dfd6831cf 100644 >> --- a/drivers/mmc/core/card.h >> +++ b/drivers/mmc/core/card.h >> @@ -168,6 +168,25 @@ static inline void __maybe_unused add_limit_rate_quirk(struct mmc_card *card, >> card->quirk_max_rate = data; >> } >> >> +static inline void __maybe_unused wl1251_quirk(struct mmc_card *card, >> + int data) >> +{ >> + /* >> + * We have TI wl1251 attached to this mmc. Pass this >> + * information to the SDIO core because it can't be >> + * probed by normal methods. >> + */ >> + >> + dev_info(card->host->parent, "found wl1251\n"); >> + 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 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) - and see if that > allows us to drop the assignment of "card->ocr = 0x80;" above. Would > you mind doing some tests for this? > > If that works, we should add some comments about it above, I think. Will try before posting next version [PATCH v1].
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 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 [ 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 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
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. 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? > > 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. Kind regards Uffe [1] https://patchwork.kernel.org/project/linux-mmc/patch/20211109192547.28679-1-ulf.hansson@linaro.org/
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
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
> Am 10.11.2021 um 18:03 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > On Wed, 10 Nov 2021 at 17:36, H. Nikolaus Schaller <hns@goldelico.com> wrote: >> >> >> 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. Ok, I'll submit a v2 asap (isn't much work since I have your patch already in my test branch). BR and thanks, Nikolaus
diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h index 089ede71d3150..20c8dfd6831cf 100644 --- a/drivers/mmc/core/card.h +++ b/drivers/mmc/core/card.h @@ -168,6 +168,25 @@ static inline void __maybe_unused add_limit_rate_quirk(struct mmc_card *card, card->quirk_max_rate = data; } +static inline void __maybe_unused wl1251_quirk(struct mmc_card *card, + int data) +{ + /* + * We have TI wl1251 attached to this mmc. Pass this + * information to the SDIO core because it can't be + * probed by normal methods. + */ + + dev_info(card->host->parent, "found wl1251\n"); + 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; +} + /* * Quirk add/remove for MMC products. */ diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h index 41c418527199c..e9813f1f8b23c 100644 --- a/drivers/mmc/core/quirks.h +++ b/drivers/mmc/core/quirks.h @@ -146,7 +146,14 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = { END_FIXUP }; +static const char *__maybe_unused wl1251_compatible_list[] = { + "ti,wl1251", + NULL +}; + static const struct mmc_fixup __maybe_unused sdio_card_init_methods[] = { + SDIO_FIXUP_COMPATIBLE(wl1251_compatible_list, wl1251_quirk, 0), + END_FIXUP };
The TiWi WL1251 WiFi chip needs special setup of the sdio interface before it can be probed. So far, this is done in omap_hsmmc_init_card() in omap_hsmmc.c which makes it useable only if connected to omap devices which use the omap_hsmmc. The OpenPandora is the most promient example. There are plans to switch to a newer sdhci-omap driver and retire omap_hsmmc. Hence this quirk must be reworked or moved somewhere else. Ideally to some location that is not dependent on the specific SoC mmc host driver. This is achieved by the new mmc_fixup_device() option introduced by ("mmc: allow to match the device tree to apply quirks") to match through device tree compatible string. This quirk will be called early right after where host->ops->init_card() and thus omap_hsmmc_init_card() was previously called. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- drivers/mmc/core/card.h | 19 +++++++++++++++++++ drivers/mmc/core/quirks.h | 7 +++++++ 2 files changed, 26 insertions(+)