| Message ID | 8ecc5c79c1dd0627d570ede31e18c860786cacca.1633519499.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc | expand |
+ Jerome On Wed, 6 Oct 2021 at 13:25, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > The TiWi 5 WiFi module 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. > > Analysis has shown that omap_hsmmc_init_card() is called > through the host->ops->init_card hook which itself > is called in three generic locations: > > mmc_init_card() > mmc_sd_init_card() > mmc_sdio_init_card() > > All these functions share a call to mmc_select_card() shortly > after running the init hook and therefore I assume that > a good place transplanting the special wl1251 handling is > mmc_select_card() - unless we want to copy and maintain the > code to three different places. > > After this quirk has been moved there, we can remove > omap_hsmmc_init_card() in omap_hsmmc.c in a separate patch. > Indeed the plan is to remove omap_hsmmc.c completely. > > A future development path to generalize could be to make > the code not depend on compatible = "ti,wl1251" but check > for optional device tree properties (non-std-sdio, bus width, > vendor, device, blksize, max_dtr, ocr) which can be defined > for any child device of the mmd/sd port needing such special > setup. I wouldn't go that path, simply because it may look like we encourage vendors to deviate from the SDIO spec. :-) At least for now, matching on the compatible string and applying card quirks makes perfect sense to me. > > 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") > Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # on OpenPandora > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> As a matter of fact, the similar problem that you are looking to address (applying card quirks based on DT compatibility strings), is partly being taken care of in another series [1], being discussed right now. I think the solution for the ti,wl1251 should be based upon that too. Please have a look and see if you can play with that!? Kind regards Uffe [1] [RFC PATCH 0/2] mmc: allow to rely on the DT to apply quirks https://lore.kernel.org/lkml/20211014143031.1313783-1-Jerome.Pouiller@silabs.com/ > --- > drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index 0c54858e89c0..6f9b96be9fe6 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -7,6 +7,7 @@ > > #include <linux/slab.h> > #include <linux/export.h> > +#include <linux/of.h> > #include <linux/types.h> > #include <linux/scatterlist.h> > > @@ -107,6 +108,35 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card) > > int mmc_select_card(struct mmc_card *card) > { > + if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { > + struct device_node *np = card->host->parent->of_node; > + > + /* > + * REVISIT: should be made more general > + * e.g. by expanding the DT bindings of child nodes to > + * optionally 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 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; > + of_node_put(np); > + } > + } > > return _mmc_select_card(card->host, card); > } > -- > 2.33.0 >
Hi Uf, + Tony, linux-omap list > Am 26.10.2021 um 19:12 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > + Jerome > > On Wed, 6 Oct 2021 at 13:25, H. Nikolaus Schaller <hns@goldelico.com> wrote: >> >> The TiWi 5 WiFi module 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. >> >> Analysis has shown that omap_hsmmc_init_card() is called >> through the host->ops->init_card hook which itself >> is called in three generic locations: >> >> mmc_init_card() >> mmc_sd_init_card() >> mmc_sdio_init_card() >> >> All these functions share a call to mmc_select_card() shortly >> after running the init hook and therefore I assume that >> a good place transplanting the special wl1251 handling is >> mmc_select_card() - unless we want to copy and maintain the >> code to three different places. >> >> After this quirk has been moved there, we can remove >> omap_hsmmc_init_card() in omap_hsmmc.c in a separate patch. >> Indeed the plan is to remove omap_hsmmc.c completely. >> >> A future development path to generalize could be to make >> the code not depend on compatible = "ti,wl1251" but check >> for optional device tree properties (non-std-sdio, bus width, >> vendor, device, blksize, max_dtr, ocr) which can be defined >> for any child device of the mmd/sd port needing such special >> setup. > > I wouldn't go that path, simply because it may look like we encourage > vendors to deviate from the SDIO spec. :-) Well, that ti,wl1251 chip is so old [1] that probably the SDIO spec did deviate from what the vendor thought it will look like :) [1] https://www.ti.com/lit/ml/swmt009a/swmt009a.pdf > At least for now, matching on the compatible string and applying card > quirks makes perfect sense to me. Yes, that is how it already was in the omal_hsmmc driver quirks. > >> >> 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") >> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # on OpenPandora >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > > As a matter of fact, the similar problem that you are looking to > address (applying card quirks based on DT compatibility strings), is > partly being taken care of in another series [1], being discussed > right now. I think the solution for the ti,wl1251 should be based upon > that too. Please have a look and see if you can play with that!? That is interesting. Yes, maybe it can be the basis. At least for finding the chip and driver. BR and thanks, Nikolaus > > Kind regards > Uffe > > [1] > [RFC PATCH 0/2] mmc: allow to rely on the DT to apply quirks > https://lore.kernel.org/lkml/20211014143031.1313783-1-Jerome.Pouiller@silabs.com/
Hi Ulf, > Am 26.10.2021 um 20:08 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > Hi Uf, >> >> As a matter of fact, the similar problem that you are looking to >> address (applying card quirks based on DT compatibility strings), is >> partly being taken care of in another series [1], being discussed >> right now. I think the solution for the ti,wl1251 should be based upon >> that too. Please have a look and see if you can play with that!? > > That is interesting. > Yes, maybe it can be the basis. At least for finding the chip and driver. I have done a first experiment. It seems as if the series [1] does the opposite of what we need... It just skips entries in struct mmc_fixup if the DT does *not* match. This new match is not even tried in the wl1251 case since card->cis.vendor and card->cis.device are not properly initialized when mmc_fixup_device() is called. (in the upstream code the init_card function sets these and also sets MMC_QUIRK_NONSTD_SDIO to early abort before sdio_read_cccr, sdio_read_common_cis, and mmc_fixup_device). What I don't get from the code is how cis.vendor or cis.device can be initialized from device tree for a specific device. As far as I see it can only be checked for and some quirks can be set from a table if vendor and device read from the CIS registers do match. Instead, we want to match DT and define some values for an otherwise unknown device (i.e. we can't match by vendor or other methods) to help to initialize the interface. So in mmc_fixup_device it is too late and we need something running earlier, based purely on device tree information... BR and thanks, Nikolaus > [1] > [RFC PATCH 0/2] mmc: allow to rely on the DT to apply quirks > https://lore.kernel.org/lkml/20211014143031.1313783-1-Jerome.Pouiller@silabs.com/
On Wed, 27 Oct 2021 at 19:01, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Hi Ulf, > > > Am 26.10.2021 um 20:08 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > > > Hi Uf, > >> > >> As a matter of fact, the similar problem that you are looking to > >> address (applying card quirks based on DT compatibility strings), is > >> partly being taken care of in another series [1], being discussed > >> right now. I think the solution for the ti,wl1251 should be based upon > >> that too. Please have a look and see if you can play with that!? > > > > That is interesting. > > Yes, maybe it can be the basis. At least for finding the chip and driver. > > I have done a first experiment. > > It seems as if the series [1] does the opposite of what we need... It just > skips entries in struct mmc_fixup if the DT does *not* match. Ohh, I didn't look that close. In that case the code isn't doing what it *should*. The point is really to match on the compatible string and then add quirks if that is found. Let me have a closer look - and for sure, I am willing to help if needed. > > This new match is not even tried in the wl1251 case since card->cis.vendor > and card->cis.device are not properly initialized when mmc_fixup_device() is called. > (in the upstream code the init_card function sets these and also sets MMC_QUIRK_NONSTD_SDIO > to early abort before sdio_read_cccr, sdio_read_common_cis, and mmc_fixup_device). We can call mmc_fixup_device() more than once during initialization and provide different struct mmc_fixup* - if that is needed. > > What I don't get from the code is how cis.vendor or cis.device can be > initialized from device tree for a specific device. As far as I see it can > only be checked for and some quirks can be set from a table if vendor and > device read from the CIS registers do match. Yes. I thought that should be possible, but maybe it is not? > > Instead, we want to match DT and define some values for an otherwise unknown > device (i.e. we can't match by vendor or other methods) to help to initialize > the interface. So in mmc_fixup_device it is too late and we need something > running earlier, based purely on device tree information... Okay, I will have a closer look. Maybe we need to extend the card quirks interface a bit to make it suitable for this case too. > > BR and thanks, > Nikolaus > > > > [1] > > [RFC PATCH 0/2] mmc: allow to rely on the DT to apply quirks > > https://lore.kernel.org/lkml/20211014143031.1313783-1-Jerome.Pouiller@silabs.com/ > Kind regards Uffe
> Am 27.10.2021 um 23:31 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > On Wed, 27 Oct 2021 at 19:01, H. Nikolaus Schaller <hns@goldelico.com> wrote: >> >> Hi Ulf, >> >>> Am 26.10.2021 um 20:08 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>> >>> Hi Uf, >>>> >>>> As a matter of fact, the similar problem that you are looking to >>>> address (applying card quirks based on DT compatibility strings), is >>>> partly being taken care of in another series [1], being discussed >>>> right now. I think the solution for the ti,wl1251 should be based upon >>>> that too. Please have a look and see if you can play with that!? >>> >>> That is interesting. >>> Yes, maybe it can be the basis. At least for finding the chip and driver. >> >> I have done a first experiment. >> >> It seems as if the series [1] does the opposite of what we need... It just >> skips entries in struct mmc_fixup if the DT does *not* match. > > Ohh, I didn't look that close. In that case the code isn't doing what > it *should*. The point is really to match on the compatible string and > then add quirks if that is found. That is what I had expected. > > Let me have a closer look - and for sure, I am willing to help if needed. > >> >> This new match is not even tried in the wl1251 case since card->cis.vendor >> and card->cis.device are not properly initialized when mmc_fixup_device() is called. >> (in the upstream code the init_card function sets these and also sets MMC_QUIRK_NONSTD_SDIO >> to early abort before sdio_read_cccr, sdio_read_common_cis, and mmc_fixup_device). > > We can call mmc_fixup_device() more than once during initialization > and provide different struct mmc_fixup* - if that is needed. Ah, looks good. > >> >> What I don't get from the code is how cis.vendor or cis.device can be >> initialized from device tree for a specific device. As far as I see it can >> only be checked for and some quirks can be set from a table if vendor and >> device read from the CIS registers do match. > > Yes. I thought that should be possible, but maybe it is not? It seems to be a hen or egg issue here. MMC_QUIRK_NONSTD_SDIO should be set before we can match by vendor and device or compatible. But it can't be set late. > >> >> Instead, we want to match DT and define some values for an otherwise unknown >> device (i.e. we can't match by vendor or other methods) to help to initialize >> the interface. So in mmc_fixup_device it is too late and we need something >> running earlier, based purely on device tree information... > > Okay, I will have a closer look. Maybe we need to extend the card > quirks interface a bit to make it suitable for this case too. Combining your suggestions we could do roughly: in mmc_sdio_init_card(): if (host->ops->init_card) host->ops->init_card(host, card); else mmc_fixup_device(host, sdio_prepare_fixups_methods); Next we need a location for the sdio_prepare_fixups_methods table and functions. For "ti,wl1251" we would then provide the entry in the table and a function doing the setup. But where should these be defined? Likely not in a header file like quirks.h? But there is no quirks.c. Best regards, Nikolaus
Hi Nikolaus, On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: > > Am 27.10.2021 um 23:31 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > On Wed, 27 Oct 2021 at 19:01, H. Nikolaus Schaller <hns@goldelico.com> wrote: > >>> Am 26.10.2021 um 20:08 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > >>>> As a matter of fact, the similar problem that you are looking to > >>>> address (applying card quirks based on DT compatibility strings), is > >>>> partly being taken care of in another series [1], being discussed > >>>> right now. I think the solution for the ti,wl1251 should be based upon > >>>> that too. Please have a look and see if you can play with that!? > >>> > >>> That is interesting. > >>> Yes, maybe it can be the basis. At least for finding the chip and driver. > >> > >> I have done a first experiment. > >> > >> It seems as if the series [1] does the opposite of what we need... It just > >> skips entries in struct mmc_fixup if the DT does *not* match. > > > > Ohh, I didn't look that close. In that case the code isn't doing what > > it *should*. The point is really to match on the compatible string and > > then add quirks if that is found. > > That is what I had expected. Note I have not tested this code. My primary goal was to submit the idea. I think I will be able to send a true PR next week. > > Let me have a closer look - and for sure, I am willing to help if needed. I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() should be mmc_fixup_of_compatible_match(), sorry. [...] > >> What I don't get from the code is how cis.vendor or cis.device can be > >> initialized from device tree for a specific device. As far as I see it can > >> only be checked for and some quirks can be set from a table if vendor and > >> device read from the CIS registers do match. > > > > Yes. I thought that should be possible, but maybe it is not? > > It seems to be a hen or egg issue here. MMC_QUIRK_NONSTD_SDIO should be set > before we can match by vendor and device or compatible. But it can't be set > late. I think you can add a new fixup table that could be applied earlier (as you do in your suggestion below). > >> Instead, we want to match DT and define some values for an otherwise unknown > >> device (i.e. we can't match by vendor or other methods) to help to initialize > >> the interface. So in mmc_fixup_device it is too late and we need something > >> running earlier, based purely on device tree information... > > > > Okay, I will have a closer look. Maybe we need to extend the card > > quirks interface a bit to make it suitable for this case too. > > Combining your suggestions we could do roughly: > > in mmc_sdio_init_card(): > > if (host->ops->init_card) > host->ops->init_card(host, card); > else > mmc_fixup_device(host, sdio_prepare_fixups_methods); I think I mostly agree, but why you don't call mmc_fixup_device() if init_card is defined? (BTW, mmc_fixup_device() takes a card as first parameter) > Next we need a location for the sdio_prepare_fixups_methods table and functions. > > For "ti,wl1251" we would then provide the entry in the table and a function doing > the setup. But where should these be defined? Likely not in a header file like > quirks.h? But there is no quirks.c. I think you can place your function in drivers/mmc/core/card.h. There are already add_quirk(), add_limit_rate_quirk(), add_quirk_mmc(), etc...
[...] > > Combining your suggestions we could do roughly: > > > > in mmc_sdio_init_card(): > > > > if (host->ops->init_card) > > host->ops->init_card(host, card); > > else > > mmc_fixup_device(host, sdio_prepare_fixups_methods); > > I think I mostly agree, but why you don't call mmc_fixup_device() if > init_card is defined? (BTW, mmc_fixup_device() takes a card as > first parameter) Ack. > > > > Next we need a location for the sdio_prepare_fixups_methods table and functions. > > > > For "ti,wl1251" we would then provide the entry in the table and a function doing > > the setup. But where should these be defined? Likely not in a header file like > > quirks.h? But there is no quirks.c. > > I think you can place your function in drivers/mmc/core/card.h. There are > already add_quirk(), add_limit_rate_quirk(), add_quirk_mmc(), etc... FYI: I don't mind adding a quirks.c, if that makes sense. Kind regards Uffe
Hi Jérôme, > Am 28.10.2021 um 10:59 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: > > Hi Nikolaus, > > On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: > >>> Let me have a closer look - and for sure, I am willing to help if needed. > > I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() > should be mmc_fixup_of_compatible_match(), sorry. Ok, I see. One more question: how can I specify "ti,wl1251" in some struct mmc_fixup table? Does it need another macro like MMC_FIXUP() or SDIO_FIXUP() to set the .name field? >> >> Combining your suggestions we could do roughly: >> >> in mmc_sdio_init_card(): >> >> if (host->ops->init_card) >> host->ops->init_card(host, card); >> else >> mmc_fixup_device(host, sdio_prepare_fixups_methods); > > I think I mostly agree, but why you don't call mmc_fixup_device() if > init_card is defined? (BTW, mmc_fixup_device() takes a card as > first parameter) Because I want to get rid of init_card. It is host specific and not client specific. > > >> Next we need a location for the sdio_prepare_fixups_methods table and functions. >> >> For "ti,wl1251" we would then provide the entry in the table and a function doing >> the setup. But where should these be defined? Likely not in a header file like >> quirks.h? But there is no quirks.c. > > I think you can place your function in drivers/mmc/core/card.h. There are > already add_quirk(), add_limit_rate_quirk(), add_quirk_mmc(), etc... Ok. Would be some add_wl1251_quirk() then. BR and thanks, Nikolaus
> Am 28.10.2021 um 11:40 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > Hi Jérôme, > >> Am 28.10.2021 um 10:59 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: >> >> Hi Nikolaus, >> >> On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: >> >>>> Let me have a closer look - and for sure, I am willing to help if needed. >> >> I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() >> should be mmc_fixup_of_compatible_match(), sorry. > > Ok, I see. > > One more question: how can I specify "ti,wl1251" in some struct mmc_fixup table? > Does it need another macro like MMC_FIXUP() or SDIO_FIXUP() to set the .name > field? > >>> >>> Combining your suggestions we could do roughly: >>> >>> in mmc_sdio_init_card(): >>> >>> if (host->ops->init_card) >>> host->ops->init_card(host, card); >>> else >>> mmc_fixup_device(host, sdio_prepare_fixups_methods); >> >> I think I mostly agree, but why you don't call mmc_fixup_device() if >> init_card is defined? (BTW, mmc_fixup_device() takes a card as >> first parameter) > > Because I want to get rid of init_card. It is host specific and not client > specific. Ah, on a second though we can do that independently. Either there is some init_card - or something in the fixup tables. Why not both... So the else clause is not needed. And you are right, the first parameter should be card`. > >> >> >>> Next we need a location for the sdio_prepare_fixups_methods table and functions. >>> >>> For "ti,wl1251" we would then provide the entry in the table and a function doing >>> the setup. But where should these be defined? Likely not in a header file like >>> quirks.h? But there is no quirks.c. >> >> I think you can place your function in drivers/mmc/core/card.h. There are >> already add_quirk(), add_limit_rate_quirk(), add_quirk_mmc(), etc... > > Ok. Would be some add_wl1251_quirk() then. > > BR and thanks, > Nikolaus >
[...] > >>> > >>> Combining your suggestions we could do roughly: > >>> > >>> in mmc_sdio_init_card(): > >>> > >>> if (host->ops->init_card) > >>> host->ops->init_card(host, card); > >>> else > >>> mmc_fixup_device(host, sdio_prepare_fixups_methods); > >> > >> I think I mostly agree, but why you don't call mmc_fixup_device() if > >> init_card is defined? (BTW, mmc_fixup_device() takes a card as > >> first parameter) > > > > Because I want to get rid of init_card. It is host specific and not client > > specific. > > Ah, on a second though we can do that independently. Either there is > some init_card - or something in the fixup tables. Why not both... > So the else clause is not needed. I agree, I definitely want to get rid of ->init_card() as well, but let's deal with that from changes on top. [...] Kind regards Uffe
On Thursday 28 October 2021 11:40:59 CEST H. Nikolaus Schaller wrote: > > Am 28.10.2021 um 10:59 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: > > On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: > > > >>> Let me have a closer look - and for sure, I am willing to help if needed. > > > > I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() > > should be mmc_fixup_of_compatible_match(), sorry. > > Ok, I see. > > One more question: how can I specify "ti,wl1251" in some struct mmc_fixup table? > Does it need another macro like MMC_FIXUP() or SDIO_FIXUP() to set the .name > field? yes, I didn't provide it with my RFC.
> Am 28.10.2021 um 11:55 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: > > On Thursday 28 October 2021 11:40:59 CEST H. Nikolaus Schaller wrote: >>> Am 28.10.2021 um 10:59 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: >>> On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: >>> >>>>> Let me have a closer look - and for sure, I am willing to help if needed. >>> >>> I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() >>> should be mmc_fixup_of_compatible_match(), sorry. >> >> Ok, I see. >> >> One more question: how can I specify "ti,wl1251" in some struct mmc_fixup table? >> Does it need another macro like MMC_FIXUP() or SDIO_FIXUP() to set the .name >> field? > > yes, I didn't provide it with my RFC. I see. Starts to look like a good plan and we just have to execute it. Please notify if you have a new version to work with (it isn't urgent since the transplantation is only needed if omap_hsmmc is retired which depends on merge of the new driver). BR and thanks, Nikolaus
Hi Jerome and Ulf, > Am 28.10.2021 um 12:07 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > > >> Am 28.10.2021 um 11:55 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: >> >> On Thursday 28 October 2021 11:40:59 CEST H. Nikolaus Schaller wrote: >>>> Am 28.10.2021 um 10:59 schrieb Jérôme Pouiller <jerome.pouiller@silabs.com>: >>>> On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote: >>>> >>>>>> Let me have a closer look - and for sure, I am willing to help if needed. >>>> >>>> I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_match() >>>> should be mmc_fixup_of_compatible_match(), sorry. I did take a deeper look and contrary to my first analyses your original patches are correct. I did not completely understand how it was intended to be used and how to match the compatible entry. >>> >>> Ok, I see. >>> >>> One more question: how can I specify "ti,wl1251" in some struct mmc_fixup table? >>> Does it need another macro like MMC_FIXUP() or SDIO_FIXUP() to set the .name >>> field? >> >> yes, I didn't provide it with my RFC. Now I was able to make a version that works on the OpenPandora. It does not yet use a macro to define the mmc_fixup table, but that can be easily changed. > > I see. > > Starts to look like a good plan and we just have to execute it. > > Please notify if you have a new version to work with (it isn't urgent since the > transplantation is only needed if omap_hsmmc is retired which depends on merge > of the new driver). I'll send an RFCv2 for the ti,wl1251 quirks now. Best regards and thanks, Nikolaus
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index 0c54858e89c0..6f9b96be9fe6 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -7,6 +7,7 @@ #include <linux/slab.h> #include <linux/export.h> +#include <linux/of.h> #include <linux/types.h> #include <linux/scatterlist.h> @@ -107,6 +108,35 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card) int mmc_select_card(struct mmc_card *card) { + if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { + struct device_node *np = card->host->parent->of_node; + + /* + * REVISIT: should be made more general + * e.g. by expanding the DT bindings of child nodes to + * optionally 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 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; + of_node_put(np); + } + } return _mmc_select_card(card->host, card); }
The TiWi 5 WiFi module 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. Analysis has shown that omap_hsmmc_init_card() is called through the host->ops->init_card hook which itself is called in three generic locations: mmc_init_card() mmc_sd_init_card() mmc_sdio_init_card() All these functions share a call to mmc_select_card() shortly after running the init hook and therefore I assume that a good place transplanting the special wl1251 handling is mmc_select_card() - unless we want to copy and maintain the code to three different places. After this quirk has been moved there, we can remove omap_hsmmc_init_card() in omap_hsmmc.c in a separate patch. Indeed the plan is to remove omap_hsmmc.c completely. A future development path to generalize could be to make the code not depend on compatible = "ti,wl1251" but check for optional device tree properties (non-std-sdio, bus width, vendor, device, blksize, max_dtr, ocr) which can be defined for any child device of the mmd/sd port needing such special setup. 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") Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # on OpenPandora Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)