lkml.org 
[lkml]   [2019]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
SubjectRe: [PATCH v1] sdhci: tegra: Add workaround for Broadcom WiFi
From
Date
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)
\
 
 \ /
  Last update: 2019-12-12 18:32    [W:0.045 / U:5.680 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site