[RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

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(+)

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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Ulf Hansson]

+ 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
>

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

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/

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

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/

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Ulf Hansson]

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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

> 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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Jérôme Pouiller]

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...


-- 
Jérôme Pouiller

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Ulf Hansson]

[...]

> > 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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

> 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
> 

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Ulf Hansson]

[...]

> >>>
> >>> 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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[Jérôme Pouiller]

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. 



-- 
Jérôme Pouiller

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

> 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

Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc

[H. Nikolaus Schaller]

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