All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
@ 2019-12-10  1:40 Dmitry Osipenko
  2019-12-10 12:52 ` Thierry Reding
  2019-12-11  8:11 ` Ulf Hansson
  0 siblings, 2 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-10  1:40 UTC (permalink / raw)
  To: Thierry Reding, Jonathan Hunter, Adrian Hunter, Ulf Hansson
  Cc: linux-mmc, linux-tegra, linux-kernel

All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
In a result high-speed mode isn't enabled for the WiFi card and this
results in a malfunctioning SDIO communication.

 brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
 brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK

Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
the problem, let's do the same in upstream.

The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
which overrides card's info for the TI wl1251 WiFi.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 7bc950520fd9..2ad87da98f2c 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -1501,6 +1501,32 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
 	return ret;
 }
 
+static void sdhci_tegra_init_card(struct mmc_host *mmc, struct mmc_card *card)
+{
+	if (card->type == MMC_TYPE_SDIO) {
+		struct device_node *np = mmc_dev(mmc)->of_node;
+
+		np = of_get_compatible_child(np, "brcm,bcm4329-fmac");
+		if (np) {
+			dev_info(mmc_dev(mmc), "found bcm4329\n");
+
+			/*
+			 * All Tegra20 boards that have embedded BCM4329
+			 * chip need to enable high speed for SDIO, otherwise
+			 * further communication with the card doesn't work
+			 * well.
+			 *
+			 * Later BCM43xx chips do not need this workaround,
+			 * but there is no good way to differentiate chip's
+			 * version at this stage and it doesn't cause any
+			 * harm for the later chips.
+			 */
+			card->cccr.high_speed = 1;
+			of_node_put(np);
+		}
+	}
+}
+
 static int sdhci_tegra_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
@@ -1545,6 +1571,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
 		host->mmc_host_ops.execute_tuning =
 				tegra_sdhci_execute_hw_tuning;
 
+	host->mmc_host_ops.init_card = sdhci_tegra_init_card;
+
 	rc = mmc_of_parse(host->mmc);
 	if (rc)
 		goto err_parse_dt;
-- 
2.24.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-10  1:40 [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi Dmitry Osipenko
@ 2019-12-10 12:52 ` Thierry Reding
  2019-12-10 14:15   ` Dmitry Osipenko
  2019-12-11  8:11 ` Ulf Hansson
  1 sibling, 1 reply; 12+ messages in thread
From: Thierry Reding @ 2019-12-10 12:52 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Jonathan Hunter, Adrian Hunter, Ulf Hansson, linux-mmc,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On Tue, Dec 10, 2019 at 04:40:11AM +0300, Dmitry Osipenko wrote:
> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> In a result high-speed mode isn't enabled for the WiFi card and this
> results in a malfunctioning SDIO communication.
> 
>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
> 
> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> the problem, let's do the same in upstream.
> 
> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> which overrides card's info for the TI wl1251 WiFi.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

This seems like the wrong place to do this. If this is specific to this
WiFi SDIO chip this should be handled at the SDIO card or function
level. It seems like the SDIO infrastructure doesn't currently allow
this because the OF nodes are attached to the card after
mmc_sdio_init_card(), whereas it seems like the quirk is already needed
during mmc_sdio_init_card().

That said, I think we could have some common code that's executed as
part of mmc_attach_sdio() (and before mmc_sdio_init_card()).

Actually, it looks like we already have something like that.
mmc_sdio_init_card() calls mmc_fixup_device() with sdio_fixup_methods
after doing some very basic initialization. Do you know if things start
to go wrong before or after that point? It might be worth looking at
that SDIO fixup array and add something that would override the CCCR
support. That would fix things in a more generic way rather than
requiring every host controller driver to duplicate this quirk.

Thierry

> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 7bc950520fd9..2ad87da98f2c 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -1501,6 +1501,32 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
>  	return ret;
>  }
>  
> +static void sdhci_tegra_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +	if (card->type == MMC_TYPE_SDIO) {
> +		struct device_node *np = mmc_dev(mmc)->of_node;
> +
> +		np = of_get_compatible_child(np, "brcm,bcm4329-fmac");
> +		if (np) {
> +			dev_info(mmc_dev(mmc), "found bcm4329\n");
> +
> +			/*
> +			 * All Tegra20 boards that have embedded BCM4329
> +			 * chip need to enable high speed for SDIO, otherwise
> +			 * further communication with the card doesn't work
> +			 * well.
> +			 *
> +			 * Later BCM43xx chips do not need this workaround,
> +			 * but there is no good way to differentiate chip's
> +			 * version at this stage and it doesn't cause any
> +			 * harm for the later chips.
> +			 */
> +			card->cccr.high_speed = 1;
> +			of_node_put(np);
> +		}
> +	}
> +}
> +
>  static int sdhci_tegra_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *match;
> @@ -1545,6 +1571,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>  		host->mmc_host_ops.execute_tuning =
>  				tegra_sdhci_execute_hw_tuning;
>  
> +	host->mmc_host_ops.init_card = sdhci_tegra_init_card;
> +
>  	rc = mmc_of_parse(host->mmc);
>  	if (rc)
>  		goto err_parse_dt;
> -- 
> 2.24.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-10 12:52 ` Thierry Reding
@ 2019-12-10 14:15   ` Dmitry Osipenko
  2019-12-10 14:32     ` Dmitry Osipenko
  2019-12-10 14:32     ` Thierry Reding
  0 siblings, 2 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-10 14:15 UTC (permalink / raw)
  To: Thierry Reding, Adrian Hunter, Ulf Hansson
  Cc: Jonathan Hunter, linux-mmc, linux-tegra, linux-kernel

10.12.2019 15:52, Thierry Reding пишет:
> On Tue, Dec 10, 2019 at 04:40:11AM +0300, Dmitry Osipenko wrote:
>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>> In a result high-speed mode isn't enabled for the WiFi card and this
>> results in a malfunctioning SDIO communication.
>>
>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>
>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>> the problem, let's do the same in upstream.
>>
>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>> which overrides card's info for the TI wl1251 WiFi.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
> 
> This seems like the wrong place to do this. If this is specific to this
> WiFi SDIO chip this should be handled at the SDIO card or function
> level. It seems like the SDIO infrastructure doesn't currently allow
> this because the OF nodes are attached to the card after
> mmc_sdio_init_card(), whereas it seems like the quirk is already needed
> during mmc_sdio_init_card().
> 
> That said, I think we could have some common code that's executed as
> part of mmc_attach_sdio() (and before mmc_sdio_init_card()).
> 
> Actually, it looks like we already have something like that.
> mmc_sdio_init_card() calls mmc_fixup_device() with sdio_fixup_methods
> after doing some very basic initialization. Do you know if things start
> to go wrong before or after that point? It might be worth looking at
> that SDIO fixup array and add something that would override the CCCR
> support. That would fix things in a more generic way rather than
> requiring every host controller driver to duplicate this quirk.

Hello Thierry,

Thank you very much for the suggestion, looks like indeed it is possible
to make workaround in a generic way.

Ulf / Adrian, will something like this be acceptable:

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 7bd392d55cfa..a6001f210b9e 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -150,6 +150,12 @@ static inline void __maybe_unused
add_limit_rate_quirk(struct mmc_card *card,
 	card->quirk_max_rate = data;
 }

+static inline void __maybe_unused add_high_speed_quirk(struct mmc_card
*card,
+						       int data)
+{
+	card->cccr.high_speed = data;
+}
+
 /*
  * Quirk add/remove for MMC products.
  */
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index 3dba15bccce2..a824c0caa7fb 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -142,6 +142,9 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
 	SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
 		   add_limit_rate_quirk, 150000000),

+	SDIO_FIXUP(SDIO_VENDOR_ID_BROADCOM, SDIO_DEVICE_ID_BROADCOM_4329,
+		   add_high_speed_quirk, 1),
+
 	END_FIXUP
 };

[snip]

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-10 14:15   ` Dmitry Osipenko
@ 2019-12-10 14:32     ` Dmitry Osipenko
  2019-12-10 14:32     ` Thierry Reding
  1 sibling, 0 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-10 14:32 UTC (permalink / raw)
  To: Thierry Reding, Adrian Hunter, Ulf Hansson
  Cc: Jonathan Hunter, linux-mmc, linux-tegra, linux-kernel

10.12.2019 17:15, Dmitry Osipenko пишет:
> 10.12.2019 15:52, Thierry Reding пишет:
>> On Tue, Dec 10, 2019 at 04:40:11AM +0300, Dmitry Osipenko wrote:
>>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>>> In a result high-speed mode isn't enabled for the WiFi card and this
>>> results in a malfunctioning SDIO communication.
>>>
>>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>>
>>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>>> the problem, let's do the same in upstream.
>>>
>>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>>> which overrides card's info for the TI wl1251 WiFi.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>
>> This seems like the wrong place to do this. If this is specific to this
>> WiFi SDIO chip this should be handled at the SDIO card or function
>> level. It seems like the SDIO infrastructure doesn't currently allow
>> this because the OF nodes are attached to the card after
>> mmc_sdio_init_card(), whereas it seems like the quirk is already needed
>> during mmc_sdio_init_card().
>>
>> That said, I think we could have some common code that's executed as
>> part of mmc_attach_sdio() (and before mmc_sdio_init_card()).
>>
>> Actually, it looks like we already have something like that.
>> mmc_sdio_init_card() calls mmc_fixup_device() with sdio_fixup_methods
>> after doing some very basic initialization. Do you know if things start
>> to go wrong before or after that point? It might be worth looking at
>> that SDIO fixup array and add something that would override the CCCR
>> support. That would fix things in a more generic way rather than
>> requiring every host controller driver to duplicate this quirk.
> 
> Hello Thierry,
> 
> Thank you very much for the suggestion, looks like indeed it is possible
> to make workaround in a generic way.
> 
> Ulf / Adrian, will something like this be acceptable:
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 7bd392d55cfa..a6001f210b9e 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -150,6 +150,12 @@ static inline void __maybe_unused
> add_limit_rate_quirk(struct mmc_card *card,
>  	card->quirk_max_rate = data;
>  }
> 
> +static inline void __maybe_unused add_high_speed_quirk(struct mmc_card
> *card,
> +						       int data)
> +{
> +	card->cccr.high_speed = data;
> +}
> +
>  /*
>   * Quirk add/remove for MMC products.
>   */
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 3dba15bccce2..a824c0caa7fb 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -142,6 +142,9 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
>  	SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
>  		   add_limit_rate_quirk, 150000000),
> 
> +	SDIO_FIXUP(SDIO_VENDOR_ID_BROADCOM, SDIO_DEVICE_ID_BROADCOM_4329,
> +		   add_high_speed_quirk, 1),
> +
>  	END_FIXUP
>  };
> 
> [snip]

On second thought, perhaps it's not very universal to change card's CCCR
and this should be a better variant:

diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
index 7bd392d55cfa..b5e44fcda7fb 100644
--- a/drivers/mmc/core/card.h
+++ b/drivers/mmc/core/card.h
@@ -222,4 +222,9 @@ static inline int mmc_card_broken_hpi(const struct
mmc_card *c)
 	return c->quirks & MMC_QUIRK_BROKEN_HPI;
 }

+static inline int mmc_card_need_high_speed_toggle(const struct mmc_card *c)
+{
+	return c->quirks & MMC_QUIRK_HIGH_SPEED_CARD;
+}
+
 #endif
diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
index 3dba15bccce2..c9af62a1d44b 100644
--- a/drivers/mmc/core/quirks.h
+++ b/drivers/mmc/core/quirks.h
@@ -142,6 +142,9 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
 	SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
 		   add_limit_rate_quirk, 150000000),

+	SDIO_FIXUP(SDIO_VENDOR_ID_BROADCOM, SDIO_DEVICE_ID_BROADCOM_4329,
+		   add_quirk, MMC_QUIRK_HIGH_SPEED_CARD),
+
 	END_FIXUP
 };

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5158..ac12c7631ec5 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -323,7 +323,7 @@ static int mmc_sdio_switch_hs(struct mmc_card *card,
int enable)
 	if (!(card->host->caps & MMC_CAP_SD_HIGHSPEED))
 		return 0;

-	if (!card->cccr.high_speed)
+	if (!mmc_card_need_high_speed_toggle(card) && !card->cccr.high_speed)
 		return 0;

 	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_SPEED, 0, &speed);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index cf3780a6ccc4..06f697e6d002 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -269,6 +269,7 @@ struct mmc_card {
 #define MMC_QUIRK_BROKEN_IRQ_POLLING	(1<<11)	/* Polling SDIO_CCCR_INTx
could create a fake interrupt */
 #define MMC_QUIRK_TRIM_BROKEN	(1<<12)		/* Skip trim */
 #define MMC_QUIRK_BROKEN_HPI	(1<<13)		/* Disable broken HPI support */
+#define MMC_QUIRK_HIGH_SPEED_CARD	(1<<14)	/* Card is high-speed capable */

 	bool			reenable_cmdq;	/* Re-enable Command Queue */

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-10 14:15   ` Dmitry Osipenko
  2019-12-10 14:32     ` Dmitry Osipenko
@ 2019-12-10 14:32     ` Thierry Reding
  1 sibling, 0 replies; 12+ messages in thread
From: Thierry Reding @ 2019-12-10 14:32 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Adrian Hunter, Ulf Hansson, Jonathan Hunter, linux-mmc,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]

On Tue, Dec 10, 2019 at 05:15:58PM +0300, Dmitry Osipenko wrote:
> 10.12.2019 15:52, Thierry Reding пишет:
> > On Tue, Dec 10, 2019 at 04:40:11AM +0300, Dmitry Osipenko wrote:
> >> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> >> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> >> In a result high-speed mode isn't enabled for the WiFi card and this
> >> results in a malfunctioning SDIO communication.
> >>
> >>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
> >>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
> >>
> >> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> >> the problem, let's do the same in upstream.
> >>
> >> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> >> which overrides card's info for the TI wl1251 WiFi.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> > 
> > This seems like the wrong place to do this. If this is specific to this
> > WiFi SDIO chip this should be handled at the SDIO card or function
> > level. It seems like the SDIO infrastructure doesn't currently allow
> > this because the OF nodes are attached to the card after
> > mmc_sdio_init_card(), whereas it seems like the quirk is already needed
> > during mmc_sdio_init_card().
> > 
> > That said, I think we could have some common code that's executed as
> > part of mmc_attach_sdio() (and before mmc_sdio_init_card()).
> > 
> > Actually, it looks like we already have something like that.
> > mmc_sdio_init_card() calls mmc_fixup_device() with sdio_fixup_methods
> > after doing some very basic initialization. Do you know if things start
> > to go wrong before or after that point? It might be worth looking at
> > that SDIO fixup array and add something that would override the CCCR
> > support. That would fix things in a more generic way rather than
> > requiring every host controller driver to duplicate this quirk.
> 
> Hello Thierry,
> 
> Thank you very much for the suggestion, looks like indeed it is possible
> to make workaround in a generic way.
> 
> Ulf / Adrian, will something like this be acceptable:
> 
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 7bd392d55cfa..a6001f210b9e 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -150,6 +150,12 @@ static inline void __maybe_unused
> add_limit_rate_quirk(struct mmc_card *card,
>  	card->quirk_max_rate = data;
>  }
> 
> +static inline void __maybe_unused add_high_speed_quirk(struct mmc_card
> *card,
> +						       int data)
> +{
> +	card->cccr.high_speed = data;
> +}
> +
>  /*
>   * Quirk add/remove for MMC products.
>   */
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 3dba15bccce2..a824c0caa7fb 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -142,6 +142,9 @@ static const struct mmc_fixup sdio_fixup_methods[] = {
>  	SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
>  		   add_limit_rate_quirk, 150000000),
> 
> +	SDIO_FIXUP(SDIO_VENDOR_ID_BROADCOM, SDIO_DEVICE_ID_BROADCOM_4329,
> +		   add_high_speed_quirk, 1),
> +
>  	END_FIXUP
>  };
> 

Looks good to me:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-10  1:40 [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi Dmitry Osipenko
  2019-12-10 12:52 ` Thierry Reding
@ 2019-12-11  8:11 ` Ulf Hansson
  2019-12-11 15:46   ` Dmitry Osipenko
  1 sibling, 1 reply; 12+ messages in thread
From: Ulf Hansson @ 2019-12-11  8:11 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> In a result high-speed mode isn't enabled for the WiFi card and this
> results in a malfunctioning SDIO communication.

Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?

>
>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>
> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> the problem, let's do the same in upstream.
>
> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> which overrides card's info for the TI wl1251 WiFi.

This is a temporary solution and should be replaced by doing the DT
parsing during

So, yes, let's see if we can use a card quirk instead. That's the first option.

A second option is simply to parse the DT subnode for a new DT
property during mmc_sdio_init_card(). Along the lines of what we do
for the broken-hpi DT binding for eMMC.

Kind regards
Uffe

>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mmc/host/sdhci-tegra.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> index 7bc950520fd9..2ad87da98f2c 100644
> --- a/drivers/mmc/host/sdhci-tegra.c
> +++ b/drivers/mmc/host/sdhci-tegra.c
> @@ -1501,6 +1501,32 @@ static int sdhci_tegra_add_host(struct sdhci_host *host)
>         return ret;
>  }
>
> +static void sdhci_tegra_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +       if (card->type == MMC_TYPE_SDIO) {
> +               struct device_node *np = mmc_dev(mmc)->of_node;
> +
> +               np = of_get_compatible_child(np, "brcm,bcm4329-fmac");
> +               if (np) {
> +                       dev_info(mmc_dev(mmc), "found bcm4329\n");
> +
> +                       /*
> +                        * All Tegra20 boards that have embedded BCM4329
> +                        * chip need to enable high speed for SDIO, otherwise
> +                        * further communication with the card doesn't work
> +                        * well.
> +                        *
> +                        * Later BCM43xx chips do not need this workaround,
> +                        * but there is no good way to differentiate chip's
> +                        * version at this stage and it doesn't cause any
> +                        * harm for the later chips.
> +                        */
> +                       card->cccr.high_speed = 1;
> +                       of_node_put(np);
> +               }
> +       }
> +}
> +
>  static int sdhci_tegra_probe(struct platform_device *pdev)
>  {
>         const struct of_device_id *match;
> @@ -1545,6 +1571,8 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
>                 host->mmc_host_ops.execute_tuning =
>                                 tegra_sdhci_execute_hw_tuning;
>
> +       host->mmc_host_ops.init_card = sdhci_tegra_init_card;
> +
>         rc = mmc_of_parse(host->mmc);
>         if (rc)
>                 goto err_parse_dt;
> --
> 2.24.0
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-11  8:11 ` Ulf Hansson
@ 2019-12-11 15:46   ` Dmitry Osipenko
  2019-12-11 16:10     ` Ulf Hansson
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-11 15:46 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

Hello Ulf,

11.12.2019 11:11, Ulf Hansson пишет:
> On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>> In a result high-speed mode isn't enabled for the WiFi card and this
>> results in a malfunctioning SDIO communication.
> 
> Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?

Yes, the SDIO_SPEED_SHS bit is set.

>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>
>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>> the problem, let's do the same in upstream.
>>
>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>> which overrides card's info for the TI wl1251 WiFi.
> 
> This is a temporary solution and should be replaced by doing the DT
> parsing during
> 
> So, yes, let's see if we can use a card quirk instead. That's the first option.
> 
> A second option is simply to parse the DT subnode for a new DT
> property during mmc_sdio_init_card(). Along the lines of what we do
> for the broken-hpi DT binding for eMMC.

Let's try the first option. My understanding is that the problem affects
only the specific model of the WiFi chip and it's not a board-specific
problem. I'll add Broadcom driver people to CC for the next version of
the patch, maybe they'll have something to say.

[snip]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-11 15:46   ` Dmitry Osipenko
@ 2019-12-11 16:10     ` Ulf Hansson
  2019-12-11 16:29       ` Dmitry Osipenko
  0 siblings, 1 reply; 12+ messages in thread
From: Ulf Hansson @ 2019-12-11 16:10 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

On Wed, 11 Dec 2019 at 16:46, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Hello Ulf,
>
> 11.12.2019 11:11, Ulf Hansson пишет:
> > On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> >> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> >> In a result high-speed mode isn't enabled for the WiFi card and this
> >> results in a malfunctioning SDIO communication.
> >
> > Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?
>
> Yes, the SDIO_SPEED_SHS bit is set.
>
> >>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
> >>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
> >>
> >> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> >> the problem, let's do the same in upstream.
> >>
> >> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> >> which overrides card's info for the TI wl1251 WiFi.
> >
> > This is a temporary solution and should be replaced by doing the DT
> > parsing during
> >
> > So, yes, let's see if we can use a card quirk instead. That's the first option.
> >
> > A second option is simply to parse the DT subnode for a new DT
> > property during mmc_sdio_init_card(). Along the lines of what we do
> > for the broken-hpi DT binding for eMMC.
>
> Let's try the first option. My understanding is that the problem affects
> only the specific model of the WiFi chip and it's not a board-specific
> problem. I'll add Broadcom driver people to CC for the next version of
> the patch, maybe they'll have something to say.

Okay, sounds reasonable. By looking at your latest attempt for a fix,
I have two minor nitpicks, otherwise it looks good.

The nitpicks:
I suggest to rename MMC_QUIRK_HIGH_SPEED_CARD to MMC_QUIRK_HIGH_SPEED
and mmc_card_need_high_speed_toggle() to mmc_card_quirk_hs().

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-11 16:10     ` Ulf Hansson
@ 2019-12-11 16:29       ` Dmitry Osipenko
  2019-12-12 14:23         ` Dmitry Osipenko
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-11 16:29 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

11.12.2019 19:10, Ulf Hansson пишет:
> On Wed, 11 Dec 2019 at 16:46, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Hello Ulf,
>>
>> 11.12.2019 11:11, Ulf Hansson пишет:
>>> On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>>>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>>>> In a result high-speed mode isn't enabled for the WiFi card and this
>>>> results in a malfunctioning SDIO communication.
>>>
>>> Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?
>>
>> Yes, the SDIO_SPEED_SHS bit is set.
>>
>>>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>>>
>>>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>>>> the problem, let's do the same in upstream.
>>>>
>>>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>>>> which overrides card's info for the TI wl1251 WiFi.
>>>
>>> This is a temporary solution and should be replaced by doing the DT
>>> parsing during
>>>
>>> So, yes, let's see if we can use a card quirk instead. That's the first option.
>>>
>>> A second option is simply to parse the DT subnode for a new DT
>>> property during mmc_sdio_init_card(). Along the lines of what we do
>>> for the broken-hpi DT binding for eMMC.
>>
>> Let's try the first option. My understanding is that the problem affects
>> only the specific model of the WiFi chip and it's not a board-specific
>> problem. I'll add Broadcom driver people to CC for the next version of
>> the patch, maybe they'll have something to say.
> 
> Okay, sounds reasonable. By looking at your latest attempt for a fix,
> I have two minor nitpicks, otherwise it looks good.
> 
> The nitpicks:
> I suggest to rename MMC_QUIRK_HIGH_SPEED_CARD to MMC_QUIRK_HIGH_SPEED
> and mmc_card_need_high_speed_toggle() to mmc_card_quirk_hs().

I'll take it into account, thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-11 16:29       ` Dmitry Osipenko
@ 2019-12-12 14:23         ` Dmitry Osipenko
  2019-12-12 15:07           ` Ulf Hansson
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-12 14:23 UTC (permalink / raw)
  To: Ulf Hansson, Thierry Reding
  Cc: Jonathan Hunter, Adrian Hunter, linux-mmc, linux-tegra,
	Linux Kernel Mailing List

11.12.2019 19:29, Dmitry Osipenko пишет:
> 11.12.2019 19:10, Ulf Hansson пишет:
>> On Wed, 11 Dec 2019 at 16:46, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>
>>> Hello Ulf,
>>>
>>> 11.12.2019 11:11, Ulf Hansson пишет:
>>>> On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>>
>>>>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>>>>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>>>>> In a result high-speed mode isn't enabled for the WiFi card and this
>>>>> results in a malfunctioning SDIO communication.
>>>>
>>>> Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?
>>>
>>> Yes, the SDIO_SPEED_SHS bit is set.
>>>
>>>>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>>>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>>>>
>>>>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>>>>> the problem, let's do the same in upstream.
>>>>>
>>>>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>>>>> which overrides card's info for the TI wl1251 WiFi.
>>>>
>>>> This is a temporary solution and should be replaced by doing the DT
>>>> parsing during
>>>>
>>>> So, yes, let's see if we can use a card quirk instead. That's the first option.
>>>>
>>>> A second option is simply to parse the DT subnode for a new DT
>>>> property during mmc_sdio_init_card(). Along the lines of what we do
>>>> for the broken-hpi DT binding for eMMC.
>>>
>>> Let's try the first option. My understanding is that the problem affects
>>> only the specific model of the WiFi chip and it's not a board-specific
>>> problem. I'll add Broadcom driver people to CC for the next version of
>>> the patch, maybe they'll have something to say.
>>
>> Okay, sounds reasonable. By looking at your latest attempt for a fix,
>> I have two minor nitpicks, otherwise it looks good.
>>
>> The nitpicks:
>> I suggest to rename MMC_QUIRK_HIGH_SPEED_CARD to MMC_QUIRK_HIGH_SPEED
>> and mmc_card_need_high_speed_toggle() to mmc_card_quirk_hs().
> 
> I'll take it into account, thanks.

Looks like I managed to figure out what's really going on:

  1. The BCM4329 doc clearly states that High Speed is supported, see
page 49 (Section 11: WLAN Interfaces, SDIO v1.2)

https://www.cypress.com/file/298626/download

  2. I googled for performance results of the BCM4329 SDIO WiFi and came
to a conclusion that ~40 Mbit/s is a realistic maximum of the WiFi-data
throughput for NVIDIA Tegra20 boards due to antenna configuration
limitations and whatever.

  3. The Tegra's SDHCI clock is pre-configured to 48MHz at the time of
kernel's boot-up.

  4. IIUC, the maximum clock rate for the legacy SD signaling mode is
~25MHz and that is more than enough for a 4-lane SDIO data-bus that
allows up to 100 Mbit/s for the WiFi which is capped to 40 Mbit/s anyways.

  5. Apparently MMC core doesn't limit the clock rate for the Normal
Speed cards.


So, I added "max-frequency = <25000000>;" to the SDHCI node of the
board's device-tree and ta-da! WiFi works absolutely fine without the
quirk! Thus the SDIO card quirk isn't really needed and I'm dropping it
for now.

Ulf, do you know if it's a bug or a feature of the MMC core that it
doesn't limit clock rate for the Normal Speed cards?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-12 14:23         ` Dmitry Osipenko
@ 2019-12-12 15:07           ` Ulf Hansson
  2019-12-12 17:31             ` Dmitry Osipenko
  0 siblings, 1 reply; 12+ messages in thread
From: Ulf Hansson @ 2019-12-12 15:07 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

On Thu, 12 Dec 2019 at 15:23, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 11.12.2019 19:29, Dmitry Osipenko пишет:
> > 11.12.2019 19:10, Ulf Hansson пишет:
> >> On Wed, 11 Dec 2019 at 16:46, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>>
> >>> Hello Ulf,
> >>>
> >>> 11.12.2019 11:11, Ulf Hansson пишет:
> >>>> On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>>>>
> >>>>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
> >>>>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
> >>>>> In a result high-speed mode isn't enabled for the WiFi card and this
> >>>>> results in a malfunctioning SDIO communication.
> >>>>
> >>>> Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?
> >>>
> >>> Yes, the SDIO_SPEED_SHS bit is set.
> >>>
> >>>>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
> >>>>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
> >>>>>
> >>>>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
> >>>>> the problem, let's do the same in upstream.
> >>>>>
> >>>>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
> >>>>> which overrides card's info for the TI wl1251 WiFi.
> >>>>
> >>>> This is a temporary solution and should be replaced by doing the DT
> >>>> parsing during
> >>>>
> >>>> So, yes, let's see if we can use a card quirk instead. That's the first option.
> >>>>
> >>>> A second option is simply to parse the DT subnode for a new DT
> >>>> property during mmc_sdio_init_card(). Along the lines of what we do
> >>>> for the broken-hpi DT binding for eMMC.
> >>>
> >>> Let's try the first option. My understanding is that the problem affects
> >>> only the specific model of the WiFi chip and it's not a board-specific
> >>> problem. I'll add Broadcom driver people to CC for the next version of
> >>> the patch, maybe they'll have something to say.
> >>
> >> Okay, sounds reasonable. By looking at your latest attempt for a fix,
> >> I have two minor nitpicks, otherwise it looks good.
> >>
> >> The nitpicks:
> >> I suggest to rename MMC_QUIRK_HIGH_SPEED_CARD to MMC_QUIRK_HIGH_SPEED
> >> and mmc_card_need_high_speed_toggle() to mmc_card_quirk_hs().
> >
> > I'll take it into account, thanks.
>
> Looks like I managed to figure out what's really going on:
>
>   1. The BCM4329 doc clearly states that High Speed is supported, see
> page 49 (Section 11: WLAN Interfaces, SDIO v1.2)
>
> https://www.cypress.com/file/298626/download
>
>   2. I googled for performance results of the BCM4329 SDIO WiFi and came
> to a conclusion that ~40 Mbit/s is a realistic maximum of the WiFi-data
> throughput for NVIDIA Tegra20 boards due to antenna configuration
> limitations and whatever.

Okay.

>
>   3. The Tegra's SDHCI clock is pre-configured to 48MHz at the time of
> kernel's boot-up.
>
>   4. IIUC, the maximum clock rate for the legacy SD signaling mode is
> ~25MHz and that is more than enough for a 4-lane SDIO data-bus that
> allows up to 100 Mbit/s for the WiFi which is capped to 40 Mbit/s anyways.

Yes, I see.

>
>   5. Apparently MMC core doesn't limit the clock rate for the Normal
> Speed cards.

It should, else it's a bug (I would be really surprised if that's the
case, but who knows).

>
>
> So, I added "max-frequency = <25000000>;" to the SDHCI node of the
> board's device-tree and ta-da! WiFi works absolutely fine without the
> quirk! Thus the SDIO card quirk isn't really needed and I'm dropping it
> for now.
>
> Ulf, do you know if it's a bug or a feature of the MMC core that it
> doesn't limit clock rate for the Normal Speed cards?

It should limit the speed, else it's a bug. Can you perhaps check what
the requested clock rate is via some debug prints in the host ops
->set_ios()? And also what the real rate becomes after dividers.

If it's not a bug in the core, I suspect that there may be generic
problem dealing with initialization frequencies for sdhci-tegra.

For example, mmc_rescan_try_freq() tries to initialize the SDIO card
at 400KHz, then 300, then 200 then 100 (in that order, and note
*KHz*). When a frequency is successful, initialization continues and
later on the clock rate should be increased to 25MHz, for legacy speed
mode.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
  2019-12-12 15:07           ` Ulf Hansson
@ 2019-12-12 17:31             ` Dmitry Osipenko
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Osipenko @ 2019-12-12 17:31 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Thierry Reding, Jonathan Hunter, Adrian Hunter, linux-mmc,
	linux-tegra, Linux Kernel Mailing List

12.12.2019 18:07, Ulf Hansson пишет:
> On Thu, 12 Dec 2019 at 15:23, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 11.12.2019 19:29, Dmitry Osipenko пишет:
>>> 11.12.2019 19:10, Ulf Hansson пишет:
>>>> On Wed, 11 Dec 2019 at 16:46, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>>
>>>>> Hello Ulf,
>>>>>
>>>>> 11.12.2019 11:11, Ulf Hansson пишет:
>>>>>> On Tue, 10 Dec 2019 at 02:40, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>>>>
>>>>>>> All Tegra20 boards that have embedded Broadcom WiFi SDIO chip are affected
>>>>>>> by a problem where WiFi chip reports CCCR v1.10, while it should v1.20.
>>>>>>> In a result high-speed mode isn't enabled for the WiFi card and this
>>>>>>> results in a malfunctioning SDIO communication.
>>>>>>
>>>>>> Does that also mean SDIO_SPEED_SHS bit is set when reading SDIO_CCCR_SPEED?
>>>>>
>>>>> Yes, the SDIO_SPEED_SHS bit is set.
>>>>>
>>>>>>>  brcmfmac: brcmf_sdio_readframes: read 304 bytes from channel 1 failed: -84
>>>>>>>  brcmfmac: brcmf_sdio_rxfail: abort command, terminate frame, send NAK
>>>>>>>
>>>>>>> Downstream kernels are overriding card's CCCR info in SDHCI driver to fix
>>>>>>> the problem, let's do the same in upstream.
>>>>>>>
>>>>>>> The change is inspired by omap_hsmmc_init_card() of OMAP's HSMMC driver,
>>>>>>> which overrides card's info for the TI wl1251 WiFi.
>>>>>>
>>>>>> This is a temporary solution and should be replaced by doing the DT
>>>>>> parsing during
>>>>>>
>>>>>> So, yes, let's see if we can use a card quirk instead. That's the first option.
>>>>>>
>>>>>> A second option is simply to parse the DT subnode for a new DT
>>>>>> property during mmc_sdio_init_card(). Along the lines of what we do
>>>>>> for the broken-hpi DT binding for eMMC.
>>>>>
>>>>> Let's try the first option. My understanding is that the problem affects
>>>>> only the specific model of the WiFi chip and it's not a board-specific
>>>>> problem. I'll add Broadcom driver people to CC for the next version of
>>>>> the patch, maybe they'll have something to say.
>>>>
>>>> Okay, sounds reasonable. By looking at your latest attempt for a fix,
>>>> I have two minor nitpicks, otherwise it looks good.
>>>>
>>>> The nitpicks:
>>>> I suggest to rename MMC_QUIRK_HIGH_SPEED_CARD to MMC_QUIRK_HIGH_SPEED
>>>> and mmc_card_need_high_speed_toggle() to mmc_card_quirk_hs().
>>>
>>> I'll take it into account, thanks.
>>
>> Looks like I managed to figure out what's really going on:
>>
>>   1. The BCM4329 doc clearly states that High Speed is supported, see
>> page 49 (Section 11: WLAN Interfaces, SDIO v1.2)
>>
>> https://www.cypress.com/file/298626/download
>>
>>   2. I googled for performance results of the BCM4329 SDIO WiFi and came
>> to a conclusion that ~40 Mbit/s is a realistic maximum of the WiFi-data
>> throughput for NVIDIA Tegra20 boards due to antenna configuration
>> limitations and whatever.
> 
> Okay.
> 
>>
>>   3. The Tegra's SDHCI clock is pre-configured to 48MHz at the time of
>> kernel's boot-up.
>>
>>   4. IIUC, the maximum clock rate for the legacy SD signaling mode is
>> ~25MHz and that is more than enough for a 4-lane SDIO data-bus that
>> allows up to 100 Mbit/s for the WiFi which is capped to 40 Mbit/s anyways.
> 
> Yes, I see.
> 
>>
>>   5. Apparently MMC core doesn't limit the clock rate for the Normal
>> Speed cards.
> 
> It should, else it's a bug (I would be really surprised if that's the
> case, but who knows).
> 
>>
>>
>> So, I added "max-frequency = <25000000>;" to the SDHCI node of the
>> board's device-tree and ta-da! WiFi works absolutely fine without the
>> quirk! Thus the SDIO card quirk isn't really needed and I'm dropping it
>> for now.
>>
>> Ulf, do you know if it's a bug or a feature of the MMC core that it
>> doesn't limit clock rate for the Normal Speed cards?
> 
> It should limit the speed, else it's a bug. Can you perhaps check what
> the requested clock rate is via some debug prints in the host ops
> ->set_ios()? And also what the real rate becomes after dividers.
> 
> If it's not a bug in the core, I suspect that there may be generic
> problem dealing with initialization frequencies for sdhci-tegra.
> 
> For example, mmc_rescan_try_freq() tries to initialize the SDIO card
> at 400KHz, then 300, then 200 then 100 (in that order, and note
> *KHz*). When a frequency is successful, initialization continues and
> later on the clock rate should be increased to 25MHz, for legacy speed
> mode.

I made the following change:

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5158..d37b61223290 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -372,12 +372,16 @@ static unsigned mmc_sdio_get_max_clock(struct
mmc_card *card)
                 * mandatory.
                 */
                max_dtr = 50000000;
+               dev_err(mmc_dev(card->host), "fixed max_dtr %u\n", max_dtr);
        } else {
                max_dtr = card->cis.max_dtr;
+               dev_err(mmc_dev(card->host), "card max_dtr %u\n", max_dtr);
        }

-       if (card->type == MMC_TYPE_SD_COMBO)
+       if (card->type == MMC_TYPE_SD_COMBO) {
                max_dtr = min(max_dtr, mmc_sd_get_max_clock(card));
+               dev_err(mmc_dev(card->host), "combo max_dtr %u\n", max_dtr);
+       }

        return max_dtr;
 }
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 7bc950520fd9..3833be5ceeb5 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -730,6 +730,8 @@ static void tegra_sdhci_set_clock(struct sdhci_host
*host, unsigned int clock)
        struct sdhci_tegra *tegra_host = sdhci_pltfm_priv(pltfm_host);
        unsigned long host_clk;

+       dev_err(mmc_dev(host->mmc), "%s %u\n", __func__, clock);
+
        if (!clock)
                return sdhci_set_clock(host, clock);
---

and got the following log:

 sdhci-pltfm: SDHCI platform and OF driver helper
 sdhci-tegra c8000000.sdhci: allocated mmc-pwrseq
 mmc0: Missing autocal timeout 3v3-pad drvs
 mmc0: Missing autocal timeout 3v3-pad drvs
 mmc0: Missing autocal timeout 1v8-pad drvs
 mmc0: Missing autocal timeout 1v8-pad drvs
 mmc0: Invalid maximum block size, assuming 512 bytes
 sdhci-tegra c8000000.sdhci: tegra_sdhci_set_clock 0
 sdhci-tegra c8000000.sdhci: tegra_sdhci_set_clock 843750
 mmc0: SDHCI controller on c8000000.sdhci [c8000000.sdhci] using ADMA
 mmc0: queuing unknown CIS tuple 0x80 (50 bytes)
 mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
 mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
 mmc0: queuing unknown CIS tuple 0x02 (1 bytes)
 sdhci-tegra c8000000.sdhci: card max_dtr 50000000
 sdhci-tegra c8000000.sdhci: tegra_sdhci_set_clock 50000000
 mmc0: new SDIO card at address 0001
 brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip
BCM4329/32
 ...

which tells that MMC core doesn't limit Normal Speed, assuming that card
reports an adequate max_dtr value.

The following MMC core change works:

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5158..da1e28892831 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -373,7 +373,7 @@ static unsigned mmc_sdio_get_max_clock(struct
mmc_card *card)
                 */
                max_dtr = 50000000;
        } else {
-               max_dtr = card->cis.max_dtr;
+               max_dtr = min(card->cis.max_dtr, 25000000u);
        }

        if (card->type == MMC_TYPE_SD_COMBO)

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-12-12 17:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10  1:40 [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi Dmitry Osipenko
2019-12-10 12:52 ` Thierry Reding
2019-12-10 14:15   ` Dmitry Osipenko
2019-12-10 14:32     ` Dmitry Osipenko
2019-12-10 14:32     ` Thierry Reding
2019-12-11  8:11 ` Ulf Hansson
2019-12-11 15:46   ` Dmitry Osipenko
2019-12-11 16:10     ` Ulf Hansson
2019-12-11 16:29       ` Dmitry Osipenko
2019-12-12 14:23         ` Dmitry Osipenko
2019-12-12 15:07           ` Ulf Hansson
2019-12-12 17:31             ` Dmitry Osipenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.