| Message ID | 20191210014011.21987-1-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] sdhci: tegra: Add workaround for Broadcom WiFi | expand |
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 >
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]
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 */
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>
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 >
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]
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
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.
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?
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
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)
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;
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(+)