* [RFC v4 0/6] mmc: core: extend mmc_fixup_device and transplant ti,wl1251 quirks from to be retired omap_hsmmc
@ 2021-11-05 9:05 H. Nikolaus Schaller
2021-11-05 9:05 ` [RFC v4 1/6] mmc: core: rewrite mmc_fixup_device() H. Nikolaus Schaller
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
RFC V4 2021-11-05 10:05:51:
* remove const from char *const * (Ulf Hansson <ulf.hansson@linaro.org>)
* use for_each_child_of_node() to scan compatible children (Ulf Hansson <ulf.hansson@linaro.org>)
(see: https://lore.kernel.org/lkml/CAPDyKFpr0kpRXoUACNNSwe8pL1S9wJPjnX+GFGS1PNezKCDYzQ@mail.gmail.com/)
RFC V3 2021-11-03 14:00:13:
* patches have been split into smaller ones a little further
* propose a new macro for setup of device tree compatible quirks
* directly include patches by jerome.pouiller@silabs.com
in this series
RFC V2 2021-11-01 10:24:26:
* reworked to not misuse mmc_select_card() but add a call to
mmc_fixup_device() right after where host->ops->init_card
was called before to apply the wl1251 specific quirks.
Device tree matching is done by a new table passed to mmc_fixup_device().
suggested by: ulf.hansson@linaro.org
based on patches by: jerome.pouiller@silabs.com
RFC V1 2021-10-06 13:24:13:
H. Nikolaus Schaller (4):
mmc: core: provide macro and table to match the device tree to apply
quirks
mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
mmc: host: omap_hsmmc: revert special init for wl1251
Jérôme Pouiller (2):
mmc: core: rewrite mmc_fixup_device()
mmc: core: allow to match the device tree to apply quirks
drivers/mmc/core/card.h | 37 +++++++++++++++++++
drivers/mmc/core/mmc.c | 1 +
drivers/mmc/core/quirks.h | 69 ++++++++++++++++++++++++++---------
drivers/mmc/core/sd.c | 2 +
drivers/mmc/core/sdio.c | 1 +
drivers/mmc/host/omap_hsmmc.c | 36 ------------------
6 files changed, 93 insertions(+), 53 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC v4 1/6] mmc: core: rewrite mmc_fixup_device()
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 ` 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
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Currently, mmc_fixup_device() is a bit difficult to read because of
particularly long condition.
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/mmc/core/quirks.h | 41 +++++++++++++++++++++++----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index d68e6e513a4f4..c7ef2d14b359f 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -152,22 +152,29 @@ static inline void mmc_fixup_device(struct mmc_card *card,
u64 rev = cid_rev_card(card);
for (f = table; f->vendor_fixup; f++) {
- if ((f->manfid == CID_MANFID_ANY ||
- f->manfid == card->cid.manfid) &&
- (f->oemid == CID_OEMID_ANY ||
- f->oemid == card->cid.oemid) &&
- (f->name == CID_NAME_ANY ||
- !strncmp(f->name, card->cid.prod_name,
- sizeof(card->cid.prod_name))) &&
- (f->cis_vendor == card->cis.vendor ||
- f->cis_vendor == (u16) SDIO_ANY_ID) &&
- (f->cis_device == card->cis.device ||
- f->cis_device == (u16) SDIO_ANY_ID) &&
- (f->ext_csd_rev == EXT_CSD_REV_ANY ||
- f->ext_csd_rev == card->ext_csd.rev) &&
- rev >= f->rev_start && rev <= f->rev_end) {
- dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
- f->vendor_fixup(card, f->data);
- }
+ if (f->manfid != CID_MANFID_ANY &&
+ f->manfid != card->cid.manfid)
+ continue;
+ if (f->oemid != CID_OEMID_ANY &&
+ f->oemid != card->cid.oemid)
+ continue;
+ if (f->name != CID_NAME_ANY &&
+ strncmp(f->name, card->cid.prod_name,
+ sizeof(card->cid.prod_name)))
+ continue;
+ if (f->cis_vendor != (u16)SDIO_ANY_ID &&
+ f->cis_vendor != card->cis.vendor)
+ continue;
+ if (f->cis_device != (u16)SDIO_ANY_ID &&
+ f->cis_device != card->cis.device)
+ continue;
+ if (f->ext_csd_rev != EXT_CSD_REV_ANY &&
+ f->ext_csd_rev != card->ext_csd.rev)
+ continue;
+ if (rev < f->rev_start || rev > f->rev_end)
+ continue;
+
+ dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
+ f->vendor_fixup(card, f->data);
}
}
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
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 ` H. Nikolaus Schaller
2021-11-05 14:27 ` Jérôme Pouiller
2021-11-05 9:05 ` [RFC v4 3/6] mmc: core: provide macro and table " H. Nikolaus Schaller
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel
From: Jérôme Pouiller <jerome.pouiller@silabs.com>
MMC subsystem provides a way to apply quirks when a device match some
properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
comply with the SDIO specification and does not provide reliable VID/PID
(eg. Silabs WF200).
So, the drivers for these devices rely on device tree to identify the
device.
This patch allows the MMC to also rely on the device tree to apply a
quirk.
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
drivers/mmc/core/card.h | 3 +++
drivers/mmc/core/quirks.h | 17 +++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 7bd392d55cfa5..a3204c19861a4 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -59,6 +59,9 @@ struct mmc_fixup {
/* for MMC cards */
unsigned int ext_csd_rev;
+ /* Match against functions declared in device tree */
+ const char **of_compatible;
+
void (*vendor_fixup)(struct mmc_card *card, int data);
int data;
};
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index c7ef2d14b359f..ee591fd8aeca2 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -10,6 +10,7 @@
*
*/
+#include <linux/of.h>
#include <linux/mmc/sdio_ids.h>
#include "card.h"
@@ -145,6 +146,19 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = {
END_FIXUP
};
+static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
+ const char *const *compat_list)
+{
+ struct device_node *np;
+
+ for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
+ if (of_device_compatible_match(np, compat_list))
+ return true;
+ }
+
+ return false;
+}
+
static inline void mmc_fixup_device(struct mmc_card *card,
const struct mmc_fixup *table)
{
@@ -173,6 +187,9 @@ static inline void mmc_fixup_device(struct mmc_card *card,
continue;
if (rev < f->rev_start || rev > f->rev_end)
continue;
+ if (f->of_compatible &&
+ !mmc_fixup_of_compatible_match(card, f->of_compatible))
+ continue;
dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
f->vendor_fixup(card, f->data);
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC v4 3/6] mmc: core: provide macro and table to match the device tree to apply quirks
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 9:05 ` 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
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
This (initially empty) table allows to match quirks early based
on .compatible of the child node of some mmc/sdio interface.
A new macro SDIO_FIXUP_COMPATIBLE makes the definition readable.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/mmc/core/card.h | 15 +++++++++++++++
drivers/mmc/core/quirks.h | 4 ++++
2 files changed, 19 insertions(+)
diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index a3204c19861a4..089ede71d3150 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -122,6 +122,21 @@ struct mmc_fixup {
_vendor, _device, \
_fixup, _data, EXT_CSD_REV_ANY) \
+#define SDIO_FIXUP_COMPATIBLE(_compatible_list, _fixup, _data) \
+ { \
+ .name = CID_NAME_ANY, \
+ .manfid = CID_MANFID_ANY, \
+ .oemid = CID_OEMID_ANY, \
+ .rev_start = 0, \
+ .rev_end = -1ull, \
+ .cis_vendor = SDIO_ANY_ID, \
+ .cis_device = SDIO_ANY_ID, \
+ .vendor_fixup = (_fixup), \
+ .data = (_data), \
+ .ext_csd_rev = EXT_CSD_REV_ANY, \
+ .of_compatible = _compatible_list, \
+ }
+
#define cid_rev(hwrev, fwrev, year, month) \
(((u64) hwrev) << 40 | \
((u64) fwrev) << 32 | \
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index ee591fd8aeca2..41c418527199c 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -146,6 +146,10 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = {
END_FIXUP
};
+static const struct mmc_fixup __maybe_unused sdio_card_init_methods[] = {
+ END_FIXUP
+};
+
static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
const char *const *compat_list)
{
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
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
` (2 preceding siblings ...)
2021-11-05 9:05 ` [RFC v4 3/6] mmc: core: provide macro and table " H. Nikolaus Schaller
@ 2021-11-05 9:05 ` H. Nikolaus Schaller
2021-11-08 15:08 ` Ulf Hansson
2021-11-08 15:39 ` Jérôme Pouiller
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-05 9:05 ` [RFC v4 6/6] mmc: host: omap_hsmmc: revert special init for wl1251 H. Nikolaus Schaller
5 siblings, 2 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
This allows to add quirks based on device tree instead of having
card specific code in the host ops.
We call it just after where host->ops->init_card() can be optionally
called.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/mmc/core/mmc.c | 1 +
drivers/mmc/core/sd.c | 2 ++
drivers/mmc/core/sdio.c | 1 +
3 files changed, 4 insertions(+)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 29e58ffae3797..19cd138acaec9 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1634,6 +1634,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
*/
if (host->ops->init_card)
host->ops->init_card(host, card);
+ mmc_fixup_device(card, sdio_card_init_methods);
/*
* For native busses: set card RCA and quit open drain mode.
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 4646b7a03db6b..0d174fdf47164 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -23,6 +23,7 @@
#include "host.h"
#include "bus.h"
#include "mmc_ops.h"
+#include "quirks.h"
#include "sd.h"
#include "sd_ops.h"
@@ -1427,6 +1428,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
*/
if (host->ops->init_card)
host->ops->init_card(host, card);
+ mmc_fixup_device(card, sdio_card_init_methods);
/*
* For native busses: get card RCA and quit open drain mode.
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 68edf7a615be5..cf8ee66990508 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -707,6 +707,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
*/
if (host->ops->init_card)
host->ops->init_card(host, card);
+ mmc_fixup_device(card, sdio_card_init_methods);
/*
* If the host and card support UHS-I mode request the card
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
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
` (3 preceding siblings ...)
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-05 9:05 ` H. Nikolaus Schaller
2021-11-08 15:33 ` Ulf Hansson
2021-11-05 9:05 ` [RFC v4 6/6] mmc: host: omap_hsmmc: revert special init for wl1251 H. Nikolaus Schaller
5 siblings, 1 reply; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
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;
+}
+
/*
* 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
};
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC v4 6/6] mmc: host: omap_hsmmc: revert special init for wl1251
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
` (4 preceding siblings ...)
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-05 9:05 ` H. Nikolaus Schaller
5 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-05 9:05 UTC (permalink / raw)
To: Ulf Hansson, Jérôme Pouiller, Avri Altman, Shawn Lin,
Linus Walleij, Tony Lindgren, Bean Huo
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
Replaces: commit f6498b922e57 ("mmc: host: omap_hsmmc: add code for special init of wl1251 to get rid of pandora_wl1251_init_card")
Requires: commit ("mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc")
After moving the wl1251 quirks from omap_hsmmc_init_card() to wl1251_quirk()
and sdio_card_init_methods[] we can remove omap_hsmmc_init_card() completely.
This also removes the specialization on the combination of omap_hsmmc and wl1251.
Related-to: commit f6498b922e57 ("mmc: host: omap_hsmmc: add code for special init of wl1251 to get rid of pandora_wl1251_init_card")
Related-to: commit 2398c41d6432 ("omap: pdata-quirks: remove openpandora quirks for mmc3 and wl1251")
Related-to: commit f9d50fef4b64 ("ARM: OMAP2+: omap3-pandora: add wifi support")
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/mmc/host/omap_hsmmc.c | 36 -----------------------------------
1 file changed, 36 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 6960e305e0a39..9749639ea8977 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1504,41 +1504,6 @@ static void omap_hsmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
omap_hsmmc_set_bus_mode(host);
}
-static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card)
-{
- struct omap_hsmmc_host *host = mmc_priv(mmc);
-
- if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
- struct device_node *np = mmc_dev(mmc)->of_node;
-
- /*
- * REVISIT: should be moved to sdio core and made more
- * general e.g. by expanding the DT bindings of child nodes
- * to provide a mechanism to provide this information:
- * Documentation/devicetree/bindings/mmc/mmc-card.txt
- */
-
- np = of_get_compatible_child(np, "ti,wl1251");
- if (np) {
- /*
- * We have TI wl1251 attached to MMC3. Pass this
- * information to the SDIO core because it can't be
- * probed by normal methods.
- */
-
- dev_info(host->dev, "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;
- of_node_put(np);
- }
- }
-}
-
static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
{
struct omap_hsmmc_host *host = mmc_priv(mmc);
@@ -1667,7 +1632,6 @@ static struct mmc_host_ops omap_hsmmc_ops = {
.set_ios = omap_hsmmc_set_ios,
.get_cd = mmc_gpio_get_cd,
.get_ro = mmc_gpio_get_ro,
- .init_card = omap_hsmmc_init_card,
.enable_sdio_irq = omap_hsmmc_enable_sdio_irq,
};
--
2.33.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
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
0 siblings, 1 reply; 23+ messages in thread
From: Jérôme Pouiller @ 2021-11-05 14:27 UTC (permalink / raw)
To: Ulf Hansson, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, H. Nikolaus Schaller
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel
On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> MMC subsystem provides a way to apply quirks when a device match some
> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> comply with the SDIO specification and does not provide reliable VID/PID
> (eg. Silabs WF200).
>
> So, the drivers for these devices rely on device tree to identify the
> device.
>
> This patch allows the MMC to also rely on the device tree to apply a
> quirk.
>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
Thank you for to have taken care of that (Maybe, you would like to add a
"Co-developed-by:" tag).
> ---
> drivers/mmc/core/card.h | 3 +++
> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 7bd392d55cfa5..a3204c19861a4 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -59,6 +59,9 @@ struct mmc_fixup {
> /* for MMC cards */
> unsigned int ext_csd_rev;
>
> + /* Match against functions declared in device tree */
> + const char **of_compatible;
> +
> void (*vendor_fixup)(struct mmc_card *card, int data);
> int data;
> };
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index c7ef2d14b359f..ee591fd8aeca2 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -10,6 +10,7 @@
> *
> */
>
> +#include <linux/of.h>
> #include <linux/mmc/sdio_ids.h>
>
> #include "card.h"
> @@ -145,6 +146,19 @@ static const struct mmc_fixup __maybe_unused sdio_fixup_methods[] = {
> END_FIXUP
> };
>
> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> + const char *const *compat_list)
> +{
> + struct device_node *np;
> +
> + for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
> + if (of_device_compatible_match(np, compat_list))
> + return true;
Intel robot complains about of_device_compatible_match():
ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!
I think we have to add this line:
EXPORT_SYMBOL(of_device_compatible_match);
in drivers/of/base.c
> + }
> +
> + return false;
> +}
> +
> static inline void mmc_fixup_device(struct mmc_card *card,
> const struct mmc_fixup *table)
> {
> @@ -173,6 +187,9 @@ static inline void mmc_fixup_device(struct mmc_card *card,
> continue;
> if (rev < f->rev_start || rev > f->rev_end)
> continue;
> + if (f->of_compatible &&
> + !mmc_fixup_of_compatible_match(card, f->of_compatible))
> + continue;
>
> dev_dbg(&card->dev, "calling %ps\n", f->vendor_fixup);
> f->vendor_fixup(card, f->data);
> --
> 2.33.0
>
>
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
2021-11-05 14:27 ` Jérôme Pouiller
@ 2021-11-06 14:31 ` H. Nikolaus Schaller
2021-11-08 15:00 ` Ulf Hansson
0 siblings, 1 reply; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-06 14:31 UTC (permalink / raw)
To: Jérôme Pouiller
Cc: Ulf Hansson, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, notasas, linux-mmc, linux-kernel,
letux-kernel, kernel
Hi Jérôme,
> Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
>
> On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
>> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>>
>> MMC subsystem provides a way to apply quirks when a device match some
>> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
>> comply with the SDIO specification and does not provide reliable VID/PID
>> (eg. Silabs WF200).
>>
>> So, the drivers for these devices rely on device tree to identify the
>> device.
>>
>> This patch allows the MMC to also rely on the device tree to apply a
>> quirk.
>>
>> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> Thank you for to have taken care of that (Maybe, you would like to add a
> "Co-developed-by:" tag).
Well, I just have taken your and Ulf's proposal and done 90% copy&paste.
So there wasn't much development, just editing...
>
>
>> ---
>> drivers/mmc/core/card.h | 3 +++
>> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
>> + const char *const *compat_list)
>> +{
>> + struct device_node *np;
>> +
>> + for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
>> + if (of_device_compatible_match(np, compat_list))
>> + return true;
>
> Intel robot complains about of_device_compatible_match():
>
> ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!
>
> I think we have to add this line:
>
> EXPORT_SYMBOL(of_device_compatible_match);
>
> in drivers/of/base.c
I had seen the krobot message as well but could not figure out
what it meant...
But with your hint it indeed looks like an omission in drivers/of/base.c
Having something exported in include/linux/of.h but code not
marked EXPORT_SYMBOL...
That needs a separate patch. I'll add one with a
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
and some Fixes: tag. Since it has a different audience I think
I should post it separately.
BTW: krobot noted the same issue for mmc_of_find_child_device()
in drivers/mmc/core/core.c (which we do not touch in this series).
But maybe it should be fixed as well.
So let's wait for more comments and then I may distribute a [PATCH v1].
Or should I do a [PATCH v5] to continue version counting?
BR,
Nikolaus
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
2021-11-06 14:31 ` H. Nikolaus Schaller
@ 2021-11-08 15:00 ` Ulf Hansson
2021-11-08 15:34 ` Jérôme Pouiller
0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2021-11-08 15:00 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, notasas, linux-mmc, linux-kernel,
letux-kernel, kernel
On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> Hi Jérôme,
>
> > Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
> >
> > On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> >> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >>
> >> MMC subsystem provides a way to apply quirks when a device match some
> >> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> >> comply with the SDIO specification and does not provide reliable VID/PID
> >> (eg. Silabs WF200).
> >>
> >> So, the drivers for these devices rely on device tree to identify the
> >> device.
> >>
> >> This patch allows the MMC to also rely on the device tree to apply a
> >> quirk.
> >>
> >> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >
> > Thank you for to have taken care of that (Maybe, you would like to add a
> > "Co-developed-by:" tag).
>
> Well, I just have taken your and Ulf's proposal and done 90% copy&paste.
> So there wasn't much development, just editing...
>
> >
> >
> >> ---
> >> drivers/mmc/core/card.h | 3 +++
> >> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
> >> 2 files changed, 20 insertions(+)
> >>
> >> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> >> + const char *const *compat_list)
After a second thought, I am not sure we really need a list of
compatibles here. The quirks we may want to apply should be specific
per device and most likely not shared among a family of devices, don't
you think?
> >> +{
> >> + struct device_node *np;
> >> +
> >> + for_each_child_of_node(mmc_dev(card->host)->of_node, np) {
> >> + if (of_device_compatible_match(np, compat_list))
> >> + return true;
> >
> > Intel robot complains about of_device_compatible_match():
> >
> > ERROR: modpost: "of_device_compatible_match" [drivers/mmc/core/mmc_core.ko] undefined!
> >
> > I think we have to add this line:
> >
> > EXPORT_SYMBOL(of_device_compatible_match);
> >
> > in drivers/of/base.c
If we change to use one compatible string, rather than a list - then
we can use of_device_is_compatible() instead, which is already
properly exported with EXPORT_SYMBOL().
>
> I had seen the krobot message as well but could not figure out
> what it meant...
>
> But with your hint it indeed looks like an omission in drivers/of/base.c
> Having something exported in include/linux/of.h but code not
> marked EXPORT_SYMBOL...
>
> That needs a separate patch. I'll add one with a
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> and some Fixes: tag. Since it has a different audience I think
> I should post it separately.
>
> BTW: krobot noted the same issue for mmc_of_find_child_device()
> in drivers/mmc/core/core.c (which we do not touch in this series).
> But maybe it should be fixed as well.
If there is an existing problem, please send a separate fix/patch for that.
>
> So let's wait for more comments and then I may distribute a [PATCH v1].
> Or should I do a [PATCH v5] to continue version counting?
I suggest moving from RFC into using PATCH v1, as this isn't really an
RFC any more.
>
> BR,
> Nikolaus
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
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
1 sibling, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2021-11-08 15:08 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, notasas, linux-mmc, linux-kernel,
letux-kernel, kernel
On Fri, 5 Nov 2021 at 10:06, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> This allows to add quirks based on device tree instead of having
> card specific code in the host ops.
>
> We call it just after where host->ops->init_card() can be optionally
> called.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/mmc/core/mmc.c | 1 +
> drivers/mmc/core/sd.c | 2 ++
> drivers/mmc/core/sdio.c | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 29e58ffae3797..19cd138acaec9 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1634,6 +1634,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> */
> if (host->ops->init_card)
> host->ops->init_card(host, card);
> + mmc_fixup_device(card, sdio_card_init_methods);
>
> /*
> * For native busses: set card RCA and quit open drain mode.
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4646b7a03db6b..0d174fdf47164 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -23,6 +23,7 @@
> #include "host.h"
> #include "bus.h"
> #include "mmc_ops.h"
> +#include "quirks.h"
> #include "sd.h"
> #include "sd_ops.h"
>
> @@ -1427,6 +1428,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> */
> if (host->ops->init_card)
> host->ops->init_card(host, card);
> + mmc_fixup_device(card, sdio_card_init_methods);
>
> /*
> * For native busses: get card RCA and quit open drain mode.
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 68edf7a615be5..cf8ee66990508 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -707,6 +707,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
> */
> if (host->ops->init_card)
> host->ops->init_card(host, card);
> + mmc_fixup_device(card, sdio_card_init_methods);
>
> /*
> * If the host and card support UHS-I mode request the card
> --
> 2.33.0
>
As the quirk is for SDIO cards, we don't need to call
mmc_fixup_device(card, sdio_card_init_methods) - other than from
mmc_sdio_init_card(). Additionally, for sd/mmc we should not be using
'sdio_card_init_methods'.
That said, it looks also reasonable to me, to squash $subject patch with patch3.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
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
0 siblings, 2 replies; 23+ messages in thread
From: Ulf Hansson @ 2021-11-08 15:33 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, notasas, linux-mmc, linux-kernel,
letux-kernel, kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
2021-11-08 15:00 ` Ulf Hansson
@ 2021-11-08 15:34 ` Jérôme Pouiller
2021-11-08 16:01 ` H. Nikolaus Schaller
0 siblings, 1 reply; 23+ messages in thread
From: Jérôme Pouiller @ 2021-11-08 15:34 UTC (permalink / raw)
To: H. Nikolaus Schaller, Ulf Hansson
Cc: Avri Altman, Shawn Lin, Linus Walleij, Tony Lindgren, Bean Huo,
notasas, linux-mmc, linux-kernel, letux-kernel, kernel
On Monday 8 November 2021 16:00:02 CET Ulf Hansson wrote:
> On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >
> > Hi Jérôme,
> >
> > > Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
> > >
> > > On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
> > >> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > >>
> > >> MMC subsystem provides a way to apply quirks when a device match some
> > >> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
> > >> comply with the SDIO specification and does not provide reliable VID/PID
> > >> (eg. Silabs WF200).
> > >>
> > >> So, the drivers for these devices rely on device tree to identify the
> > >> device.
> > >>
> > >> This patch allows the MMC to also rely on the device tree to apply a
> > >> quirk.
> > >>
> > >> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
> > >> ---
> > >> drivers/mmc/core/card.h | 3 +++
> > >> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
> > >> 2 files changed, 20 insertions(+)
> > >>
> > >> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
> > >> + const char *const *compat_list)
>
> After a second thought, I am not sure we really need a list of
> compatibles here. The quirks we may want to apply should be specific
> per device and most likely not shared among a family of devices, don't
> you think?
Indeed. I dislike to have to declare a list of compatible device (see
wl1251_compatible_list in patch 5) outside of the fixup list.
If I have several devices, I prefer to copy-paste a few lines in the
mmc_fixup list (for the WFX driver, I have 4 devices to declare).
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
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 15:39 ` Jérôme Pouiller
2021-11-08 16:04 ` H. Nikolaus Schaller
1 sibling, 1 reply; 23+ messages in thread
From: Jérôme Pouiller @ 2021-11-08 15:39 UTC (permalink / raw)
To: Ulf Hansson, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, H. Nikolaus Schaller
Cc: notasas, linux-mmc, linux-kernel, letux-kernel, kernel,
H. Nikolaus Schaller
On Friday 5 November 2021 10:05:49 CET H. Nikolaus Schaller wrote:
> This allows to add quirks based on device tree instead of having
> card specific code in the host ops.
>
> We call it just after where host->ops->init_card() can be optionally
> called.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
[...]
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 68edf7a615be5..cf8ee66990508 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -707,6 +707,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
> */
> if (host->ops->init_card)
> host->ops->init_card(host, card);
> + mmc_fixup_device(card, sdio_card_init_methods);
sdio_read_common_cis(card) is called a bit after this line. I think it
will overwrite all the card->cis fields. This does not conflict with what
your are doing in wl1251_quirk()?
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 2/6] mmc: core: allow to match the device tree to apply quirks
2021-11-08 15:34 ` Jérôme Pouiller
@ 2021-11-08 16:01 ` H. Nikolaus Schaller
0 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-08 16:01 UTC (permalink / raw)
To: Jérôme Pouiller, Ulf Hansson
Cc: Avri Altman, Shawn Lin, Linus Walleij, Tony Lindgren, Bean Huo,
Gražvydas Ignotas, linux-mmc, linux-kernel,
Discussions about the Letux Kernel, kernel
> Am 08.11.2021 um 16:34 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
>
> On Monday 8 November 2021 16:00:02 CET Ulf Hansson wrote:
>> On Sat, 6 Nov 2021 at 15:31, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>
>>> Hi Jérôme,
>>>
>>>> Am 05.11.2021 um 15:27 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
>>>>
>>>> On Friday 5 November 2021 10:05:47 CET H. Nikolaus Schaller wrote:
>>>>> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>>>>>
>>>>> MMC subsystem provides a way to apply quirks when a device match some
>>>>> properties (VID, PID, etc...) Unfortunately, some SDIO devices does not
>>>>> comply with the SDIO specification and does not provide reliable VID/PID
>>>>> (eg. Silabs WF200).
>>>>>
>>>>> So, the drivers for these devices rely on device tree to identify the
>>>>> device.
>>>>>
>>>>> This patch allows the MMC to also rely on the device tree to apply a
>>>>> quirk.
>>>>>
>>>>> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> [...]
>
>>>>> ---
>>>>> drivers/mmc/core/card.h | 3 +++
>>>>> drivers/mmc/core/quirks.h | 17 +++++++++++++++++
>>>>> 2 files changed, 20 insertions(+)
>>>>>
>>>>> +static inline bool mmc_fixup_of_compatible_match(struct mmc_card *card,
>>>>> + const char *const *compat_list)
>>
>> After a second thought, I am not sure we really need a list of
>> compatibles here. The quirks we may want to apply should be specific
>> per device and most likely not shared among a family of devices, don't
>> you think?
>
> Indeed. I dislike to have to declare a list of compatible device (see
> wl1251_compatible_list in patch 5) outside of the fixup list.
>
> If I have several devices, I prefer to copy-paste a few lines in the
> mmc_fixup list (for the WFX driver, I have 4 devices to declare).
Agreed. It makes the macro easier to use.
BR and thanks,
Nikolaus
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
2021-11-08 15:39 ` Jérôme Pouiller
@ 2021-11-08 16:04 ` H. Nikolaus Schaller
0 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-08 16:04 UTC (permalink / raw)
To: Jérôme Pouiller
Cc: Ulf Hansson, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
> Am 08.11.2021 um 16:39 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>:
>
> On Friday 5 November 2021 10:05:49 CET H. Nikolaus Schaller wrote:
>> This allows to add quirks based on device tree instead of having
>> card specific code in the host ops.
>>
>> We call it just after where host->ops->init_card() can be optionally
>> called.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>
> [...]
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 68edf7a615be5..cf8ee66990508 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -707,6 +707,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>> */
>> if (host->ops->init_card)
>> host->ops->init_card(host, card);
>> + mmc_fixup_device(card, sdio_card_init_methods);
>
> sdio_read_common_cis(card) is called a bit after this line. I think it
> will overwrite all the card->cis fields. This does not conflict with what
> your are doing in wl1251_quirk()?
No, because the wl1251_quirk sets MMC_QUIRK_NONSTD_SDIO which
skips reading CIS. The key issue with the wl1251 seems to be
that it reports random CIS tuples if we try to probe without
quirks (I have no further idea about the wl1251 than moving the
quirks around...).
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 4/6] mmc: core: add new calls to mmc_fixup_device(sdio_card_init_methods)
2021-11-08 15:08 ` Ulf Hansson
@ 2021-11-08 16:07 ` H. Nikolaus Schaller
0 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-08 16:07 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
> Am 08.11.2021 um 16:08 schrieb Ulf Hansson <ulf.hansson@linaro.org>:
>
> On Fri, 5 Nov 2021 at 10:06, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>
>> This allows to add quirks based on device tree instead of having
>> card specific code in the host ops.
>>
>> We call it just after where host->ops->init_card() can be optionally
>> called.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/mmc/core/mmc.c | 1 +
>> drivers/mmc/core/sd.c | 2 ++
>> drivers/mmc/core/sdio.c | 1 +
>> 3 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 29e58ffae3797..19cd138acaec9 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1634,6 +1634,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>> */
>> if (host->ops->init_card)
>> host->ops->init_card(host, card);
>> + mmc_fixup_device(card, sdio_card_init_methods);
>>
>> /*
>> * For native busses: set card RCA and quit open drain mode.
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 4646b7a03db6b..0d174fdf47164 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -23,6 +23,7 @@
>> #include "host.h"
>> #include "bus.h"
>> #include "mmc_ops.h"
>> +#include "quirks.h"
>> #include "sd.h"
>> #include "sd_ops.h"
>>
>> @@ -1427,6 +1428,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
>> */
>> if (host->ops->init_card)
>> host->ops->init_card(host, card);
>> + mmc_fixup_device(card, sdio_card_init_methods);
>>
>> /*
>> * For native busses: get card RCA and quit open drain mode.
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 68edf7a615be5..cf8ee66990508 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -707,6 +707,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>> */
>> if (host->ops->init_card)
>> host->ops->init_card(host, card);
>> + mmc_fixup_device(card, sdio_card_init_methods);
>>
>> /*
>> * If the host and card support UHS-I mode request the card
>> --
>> 2.33.0
>>
>
> As the quirk is for SDIO cards, we don't need to call
> mmc_fixup_device(card, sdio_card_init_methods) - other than from
> mmc_sdio_init_card().
Ok. Well, the old code did have some logic for some SD_COMBO.
But I have no idea if that is needed.
> Additionally, for sd/mmc we should not be using
> 'sdio_card_init_methods'.
Ok ,I see.
>
> That said, it looks also reasonable to me, to squash $subject patch with patch3.
Ok.
BR and thanks,
Nikolaus
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
2021-11-08 15:33 ` Ulf Hansson
@ 2021-11-08 16:08 ` H. Nikolaus Schaller
2021-11-09 10:58 ` H. Nikolaus Schaller
1 sibling, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-08 16:08 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
> 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].
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
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
1 sibling, 1 reply; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-09 10:58 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
2021-11-09 10:58 ` H. Nikolaus Schaller
@ 2021-11-09 20:01 ` Ulf Hansson
2021-11-10 16:36 ` H. Nikolaus Schaller
0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2021-11-09 20:01 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
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/
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
2021-11-09 20:01 ` Ulf Hansson
@ 2021-11-10 16:36 ` H. Nikolaus Schaller
2021-11-10 17:03 ` Ulf Hansson
0 siblings, 1 reply; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-10 16:36 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
2021-11-10 16:36 ` H. Nikolaus Schaller
@ 2021-11-10 17:03 ` Ulf Hansson
2021-11-10 17:09 ` H. Nikolaus Schaller
0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2021-11-10 17:03 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC v4 5/6] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
2021-11-10 17:03 ` Ulf Hansson
@ 2021-11-10 17:09 ` H. Nikolaus Schaller
0 siblings, 0 replies; 23+ messages in thread
From: H. Nikolaus Schaller @ 2021-11-10 17:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jérôme Pouiller, Avri Altman, Shawn Lin, Linus Walleij,
Tony Lindgren, Bean Huo, Gražvydas Ignotas, linux-mmc,
linux-kernel, letux-kernel, kernel
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2021-11-10 17:09 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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).