| Message ID | 11a8a0c8-a5b1-8f38-a139-97172ab7be68@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: meson-gx: increase power-up delay | expand |
On 14.03.2023 18:24, Marc Gonzalez wrote: > With the default power-up delay, on small kernels, the host probes > too soon, and mmc_send_io_op_cond() times out. > Looking at mmc_power_up() and how power_delay_ms is used I wonder what you mean with "host probes too soon". Are you sure that the additional delay is needed for the Amlogic MMC block IP in general? Or could it be that your issue is caused by a specific regulator and you need to add a delay there? > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > Stress-tested with 80 cold boots, checking every time > mmc2: new ultra high speed SDR50 SDIO card at address 0001 > IIUC, this will also slow down SD & MMC probing, > but an additional 20 ms seems acceptable? > --- > drivers/mmc/host/meson-gx-mmc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 6e5ea0213b477..73ecbcf588c65 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -1182,6 +1182,7 @@ static int meson_mmc_probe(struct platform_device *pdev) > mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev); > if (!mmc) > return -ENOMEM; > + mmc->ios.power_delay_ms = 20; > host = mmc_priv(mmc); > host->mmc = mmc; > host->dev = &pdev->dev;
On 14/03/2023 20:45, Heiner Kallweit wrote: > On 14.03.2023 18:24, Marc Gonzalez wrote: > >> With the default power-up delay, on small kernels, the host probes >> too soon, and mmc_send_io_op_cond() times out. > > Looking at mmc_power_up() and how power_delay_ms is used > I wonder what you mean with "host probes too soon". Hello Heiner, Thanks for your interest in my patch! :) I should have added a link to the thread that led to the patch. https://patchwork.kernel.org/project/linux-wireless/patch/c1a215cf-94be-871b-2a8a-3cc381588f83@free.fr/ Start at "I have run into another issue." Basically, I have an S905X2-based board. I built a small kernel for it (with only a few drivers), that boots really fast. mmc2 (SDIO controller hooked to WiFi chip) would not probe at all, unless I added lots of printks. Basically, calling mmc_send_io_op_cond() too soon after the controller has been reset leads to the CMD5 request timing out. > Are you sure that the additional delay is needed for the Amlogic MMC > block IP in general? Or could it be that your issue is caused by > a specific regulator and you need to add a delay there? The eternal question... I have only one type of board. (Actually, I have a reference design that is slightly different, so I should test on that one as well.) In vendor kernels, they add delays to the WiFi drivers. Maybe they have run into the issue, and they're just fixing the symptom? Default value for ios.power_delay_ms is 10 ms. msleep(ios.power_delay_ms) is called twice in mmc_power_up(). So raising the delay from 10 to 20 adds 20 ms to the latency of initializing SDIO/SD/MMC controllers. Would you be willing to test if the problem manifests on your board? Regards
On Wed 15 Mar 2023 at 11:20, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 14/03/2023 20:45, Heiner Kallweit wrote: > >> On 14.03.2023 18:24, Marc Gonzalez wrote: >> >>> With the default power-up delay, on small kernels, the host probes >>> too soon, and mmc_send_io_op_cond() times out. >> >> Looking at mmc_power_up() and how power_delay_ms is used >> I wonder what you mean with "host probes too soon". > > Hello Heiner, > > Thanks for your interest in my patch! :) > > I should have added a link to the thread that led to the patch. > https://patchwork.kernel.org/project/linux-wireless/patch/c1a215cf-94be-871b-2a8a-3cc381588f83@free.fr/ > Start at "I have run into another issue." > > Basically, I have an S905X2-based board. > I built a small kernel for it (with only a few drivers), that boots really fast. > > mmc2 (SDIO controller hooked to WiFi chip) would not probe at all, > unless I added lots of printks. > Basically, calling mmc_send_io_op_cond() too soon after the controller > has been reset leads to the CMD5 request timing out. > > I tend to agree with Heiner here. This patch is backing a contraint only reported on your design in the driver of every AML SoC supported, for every MMC controller. I think you should look first in your vmmc and vqmmc regulators and their setup times. "fixed-regulator" have properties which might be interesting to you, like * startup-delay-us * off-on-delay-us >> Are you sure that the additional delay is needed for the Amlogic MMC >> block IP in general? Or could it be that your issue is caused by >> a specific regulator and you need to add a delay there? > > The eternal question... > > I have only one type of board. (Actually, I have a reference design > that is slightly different, so I should test on that one as well.) > > In vendor kernels, they add delays to the WiFi drivers. > Maybe they have run into the issue, and they're just fixing the symptom? > > Default value for ios.power_delay_ms is 10 ms. > msleep(ios.power_delay_ms) is called twice in mmc_power_up(). > So raising the delay from 10 to 20 adds 20 ms > to the latency of initializing SDIO/SD/MMC controllers. > > Would you be willing to test if the problem manifests on your board? > > Regards
On 15.03.2023 16:27, Jerome Brunet wrote: > > On Wed 15 Mar 2023 at 11:20, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > >> On 14/03/2023 20:45, Heiner Kallweit wrote: >> >>> On 14.03.2023 18:24, Marc Gonzalez wrote: >>> >>>> With the default power-up delay, on small kernels, the host probes >>>> too soon, and mmc_send_io_op_cond() times out. >>> >>> Looking at mmc_power_up() and how power_delay_ms is used >>> I wonder what you mean with "host probes too soon". >> >> Hello Heiner, >> >> Thanks for your interest in my patch! :) >> >> I should have added a link to the thread that led to the patch. >> https://patchwork.kernel.org/project/linux-wireless/patch/c1a215cf-94be-871b-2a8a-3cc381588f83@free.fr/ >> Start at "I have run into another issue." >> >> Basically, I have an S905X2-based board. >> I built a small kernel for it (with only a few drivers), that boots really fast. >> >> mmc2 (SDIO controller hooked to WiFi chip) would not probe at all, >> unless I added lots of printks. >> Basically, calling mmc_send_io_op_cond() too soon after the controller >> has been reset leads to the CMD5 request timing out. >> >> > > I tend to agree with Heiner here. > This patch is backing a contraint only reported on your design in the > driver of every AML SoC supported, for every MMC controller. > > I think you should look first in your vmmc and vqmmc regulators and > their setup times. > > "fixed-regulator" have properties which might be interesting to you, > like > * startup-delay-us > * off-on-delay-us > >>> Are you sure that the additional delay is needed for the Amlogic MMC >>> block IP in general? Or could it be that your issue is caused by >>> a specific regulator and you need to add a delay there? >> >> The eternal question... >> >> I have only one type of board. (Actually, I have a reference design >> that is slightly different, so I should test on that one as well.) >> Let me ask few more questions: You said that the issue is with a SDIO card. How about eMMC and sdcard, does the issue occur for them too? Then you mention "too soon after reset", but add a delay to power-up. If the delay would be needed after reset, then shouldn't it be in meson_mmc_probe() after the call to device_reset_optional()? >> In vendor kernels, they add delays to the WiFi drivers. >> Maybe they have run into the issue, and they're just fixing the symptom? >> >> Default value for ios.power_delay_ms is 10 ms. >> msleep(ios.power_delay_ms) is called twice in mmc_power_up(). >> So raising the delay from 10 to 20 adds 20 ms >> to the latency of initializing SDIO/SD/MMC controllers. >> >> Would you be willing to test if the problem manifests on your board? >> >> Regards >
On Wed, 15 Mar 2023 at 16:31, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Wed 15 Mar 2023 at 11:20, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > > > On 14/03/2023 20:45, Heiner Kallweit wrote: > > > >> On 14.03.2023 18:24, Marc Gonzalez wrote: > >> > >>> With the default power-up delay, on small kernels, the host probes > >>> too soon, and mmc_send_io_op_cond() times out. > >> > >> Looking at mmc_power_up() and how power_delay_ms is used > >> I wonder what you mean with "host probes too soon". > > > > Hello Heiner, > > > > Thanks for your interest in my patch! :) > > > > I should have added a link to the thread that led to the patch. > > https://patchwork.kernel.org/project/linux-wireless/patch/c1a215cf-94be-871b-2a8a-3cc381588f83@free.fr/ > > Start at "I have run into another issue." > > > > Basically, I have an S905X2-based board. > > I built a small kernel for it (with only a few drivers), that boots really fast. > > > > mmc2 (SDIO controller hooked to WiFi chip) would not probe at all, > > unless I added lots of printks. > > Basically, calling mmc_send_io_op_cond() too soon after the controller > > has been reset leads to the CMD5 request timing out. > > > > > > I tend to agree with Heiner here. > This patch is backing a contraint only reported on your design in the > driver of every AML SoC supported, for every MMC controller. > > I think you should look first in your vmmc and vqmmc regulators and > their setup times. > > "fixed-regulator" have properties which might be interesting to you, > like > * startup-delay-us > * off-on-delay-us If the problem is regulator specific, this would be the correct thing to do. Although, if the problem is pwrseq specific, like that we need a delay after enabling the clock and asserting the GPIO enable pin for the WiFi chip, then we have the "post-power-on-delay-ms" of the pwrseq node to play with instead. > > >> Are you sure that the additional delay is needed for the Amlogic MMC > >> block IP in general? Or could it be that your issue is caused by > >> a specific regulator and you need to add a delay there? > > > > The eternal question... > > > > I have only one type of board. (Actually, I have a reference design > > that is slightly different, so I should test on that one as well.) > > > > In vendor kernels, they add delays to the WiFi drivers. > > Maybe they have run into the issue, and they're just fixing the symptom? > > > > Default value for ios.power_delay_ms is 10 ms. > > msleep(ios.power_delay_ms) is called twice in mmc_power_up(). > > So raising the delay from 10 to 20 adds 20 ms > > to the latency of initializing SDIO/SD/MMC controllers. > > > > Would you be willing to test if the problem manifests on your board? > > > > Regards > Kind regards Uffe
On 15/03/2023 22:14, Heiner Kallweit wrote: > Let me ask few more questions: > > You said that the issue is with a SDIO card. How about eMMC and sdcard, > does the issue occur for them too? I have never seen an issue with eMMC not probing on this board. (I have not tested the SD card slot, as I'm using it for serial.) I did a (probably silly) test by setting mmc->ios.power_delay_ms = 0 in the driver, and logging relevant events (for timing). Here's a boot log where both mmc1 & mmc2 probe correctly: [ 0.879968] >ffe03000.sd [ 0.880320] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 0.891604] +mmc2 [ 0.895179] -mmc2 [ 0.896873] +mmc2 [ 0.898829] <ffe03000.sd [ 0.951755] Q [ 1.049162] >ffe07000.mmc [ 1.049602] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.061378] +mmc1 [ 1.061666] -mmc1 [ 1.062841] +mmc1 [ 1.063532] <ffe07000.mmc [ 1.072056] Q [ 1.079669] R [ 1.123210] mmc2: new ultra high speed SDR50 SDIO card at address 0001 [ 1.193650] mmc1: new HS200 MMC card at address 0001 [ 1.198553] mmcblk1: mmc1:0001 SCA16G 14.7 GiB [ 1.205777] brcmf_sdio_probe_attach: AFTER [ 1.209853] mmcblk1boot0: mmc1:0001 SCA16G 4.00 MiB [ 1.211218] mmcblk1boot1: mmc1:0001 SCA16G 4.00 MiB [ 1.212254] mmcblk1rpmb: mmc1:0001 SCA16G 4.00 MiB, chardev (246:0) Analysis for mmc2 [ 0.879968] ENTER meson_mmc_probe() [ 0.880320] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 0.891604] ENTER mmc_power_up() [ 0.895179] EXIT mmc_power_up() [ 0.896873] ENTER mmc_power_up() => premature exit [ 0.898829] EXIT meson_mmc_probe() [ 0.951755] ENTER mmc_attach_sdio() [ 1.123210] mmc2: new ultra high speed SDR50 SDIO card at address 0001 mmc_attach_sdio() is called 60 ms after entering mmc_power_up() Analysis for mmc1 [ 1.049162] ENTER meson_mmc_probe() [ 1.049602] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.061378] ENTER mmc_power_up() [ 1.061666] EXIT mmc_power_up() [ 1.062841] ENTER mmc_power_up() [ 1.063532] EXIT meson_mmc_probe() => premature exit [ 1.072056] ENTER mmc_attach_sdio() [ 1.079669] ENTER mmc_attach_mmc() [ 1.193650] mmc1: new HS200 MMC card at address 0001 mmc_attach_mmc() is called 18 ms after entering mmc_power_up() Here's a boot log where mmc2 *DOES NOT* probe correctly: [ 0.875868] >ffe03000.sd [ 0.876196] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 0.881407] +mmc2 [ 0.886832] -mmc2 [ 0.888544] +mmc2 [ 0.894688] <ffe03000.sd [ 0.901587] Q [ 0.908937] R [ 0.913466] mmc2: YO no device WTF [ 1.042609] >ffe07000.mmc [ 1.042910] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.053460] +mmc1 [ 1.053694] -mmc1 [ 1.055039] +mmc1 [ 1.056956] <ffe07000.mmc [ 1.064126] Q [ 1.075205] R [ 1.197676] mmc1: new HS200 MMC card at address 0001 [ 1.198337] mmcblk1: mmc1:0001 SCA16G 14.7 GiB [ 1.203947] mmcblk1boot0: mmc1:0001 SCA16G 4.00 MiB [ 1.207858] mmcblk1boot1: mmc1:0001 SCA16G 4.00 MiB [ 1.212593] mmcblk1rpmb: mmc1:0001 SCA16G 4.00 MiB, chardev (246:0) In this instance, for mmc2, mmc_attach_sdio() is called 20 ms after entering mmc_power_up() and CMD5 times out (probe failure). > Then you mention "too soon after reset", but add a delay to power-up. > If the delay would be needed after reset, then shouldn't it be in > meson_mmc_probe() after the call to device_reset_optional()? The first mmc_power_up() is called from: [ 0.916446] mmc_power_up+0x2c/0xe4 [ 0.926186] mmc_start_host+0x94/0xa0 [ 0.926197] mmc_add_host+0x84/0xf0 [ 0.926205] meson_mmc_probe+0x374/0x3f4 [ 0.933086] platform_probe+0x68/0xe0 [ 0.940159] really_probe+0xbc/0x2f0 [ 0.948008] __driver_probe_device+0x78/0xe0 [ 0.955080] driver_probe_device+0xd8/0x160 [ 0.955087] __driver_attach_async_helper+0x4c/0xc0 [ 0.965689] async_run_entry_fn+0x34/0xe0 [ 0.965699] process_one_work+0x1cc/0x320 [ 0.972847] worker_thread+0x14c/0x450 [ 0.972855] kthread+0x10c/0x110 [ 1.018906] ret_from_fork+0x10/0x20 The second mmc_power_up() (prematurely exited) is called from: [ 1.061686] mmc_power_up+0x2c/0xe4 [ 1.065136] mmc_rescan+0x18c/0x310 [ 1.068586] process_one_work+0x1cc/0x320 [ 1.072553] worker_thread+0x14c/0x450 [ 1.076262] kthread+0x10c/0x110 [ 1.086528] ret_from_fork+0x10/0x20 I'm confused about device_reset_optional() vs mmc_power_up(). Do they both reset the controller? Regards
On 16/03/2023 13:32, Marc Gonzalez wrote: > On 15/03/2023 22:14, Heiner Kallweit wrote: > >> Then you mention "too soon after reset", but add a delay to power-up. >> If the delay would be needed after reset, then shouldn't it be in >> meson_mmc_probe() after the call to device_reset_optional()? > > The first mmc_power_up() is called from: > > [ 0.916446] mmc_power_up+0x2c/0xe4 > [ 0.926186] mmc_start_host+0x94/0xa0 > [ 0.926197] mmc_add_host+0x84/0xf0 > [ 0.926205] meson_mmc_probe+0x374/0x3f4 > [ 0.933086] platform_probe+0x68/0xe0 > [ 0.940159] really_probe+0xbc/0x2f0 > [ 0.948008] __driver_probe_device+0x78/0xe0 > [ 0.955080] driver_probe_device+0xd8/0x160 > [ 0.955087] __driver_attach_async_helper+0x4c/0xc0 > [ 0.965689] async_run_entry_fn+0x34/0xe0 > [ 0.965699] process_one_work+0x1cc/0x320 > [ 0.972847] worker_thread+0x14c/0x450 > [ 0.972855] kthread+0x10c/0x110 > [ 1.018906] ret_from_fork+0x10/0x20 > > > The second mmc_power_up() (prematurely exited) is called from: > > [ 1.061686] mmc_power_up+0x2c/0xe4 > [ 1.065136] mmc_rescan+0x18c/0x310 > [ 1.068586] process_one_work+0x1cc/0x320 > [ 1.072553] worker_thread+0x14c/0x450 > [ 1.076262] kthread+0x10c/0x110 > [ 1.086528] ret_from_fork+0x10/0x20 > > > I'm confused about device_reset_optional() vs mmc_power_up(). > Do they both reset the controller? NB: adding 500 ms delay after device_reset_optional() DOES NOT fix the issue: [ 0.875494] >ffe03000.sd [ 0.875823] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 1.027111] >ffe07000.mmc [ 1.027430] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.405170] +mmc2 [ 1.405389] -mmc2 [ 1.405413] <ffe03000.sd [ 1.405500] +mmc2 [ 1.412957] Q [ 1.420317] R [ 1.422590] mmc2: YO no device WTF We enter mmc_power_up at 1.405170 We enter mmc_attach_sdio at 1.412957 Empirically, 8 ms is not enough time for the SDIO bus to be ready. Reverting to default power_delay_ms. [ 0.879842] >ffe03000.sd [ 0.880189] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 1.031635] >ffe07000.mmc [ 1.031958] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.405284] +mmc2 [ 1.430499] -mmc2 [ 1.430525] <ffe03000.sd [ 1.430616] +mmc2 [ 1.438065] Q [ 1.445404] R [ 1.447677] mmc2: YO no device WTF We enter mmc_power_up at 1.405284 We enter mmc_attach_sdio at 1.438065 Empirically, 33 ms is not enough time for the SDIO bus to be ready. Increasing power_delay_ms to 15. [ 0.879788] >ffe03000.sd [ 0.880092] meson-gx-mmc ffe03000.sd: allocated mmc-pwrseq [ 1.031437] >ffe07000.mmc [ 1.031767] meson-gx-mmc ffe07000.mmc: allocated mmc-pwrseq [ 1.405262] +mmc2 [ 1.442981] -mmc2 [ 1.443007] <ffe03000.sd [ 1.443099] +mmc2 [ 1.450549] Q [ 1.565950] +mmc1 [ 1.584306] mmc2: new ultra high speed SDR50 SDIO card at address 0001 We enter mmc_power_up at 1.405262 We enter mmc_attach_sdio at 1.450549 Empirically, 45 ms *is* enough time for the SDIO bus to be ready. My gut feeling is that the problem with SDIO devices exists on all boards. (I am well aware that I may be wrong.) Would you be willing to test a small kernel? I can provide a kernel defconfig and/or a buildroot defconfig. Regards
On 16/03/2023 12:59, Ulf Hansson wrote: > If the problem is regulator specific, this would be the correct thing to do. > > Although, if the problem is pwrseq specific, like that we need a delay > after enabling the clock and asserting the GPIO enable pin for the > WiFi chip, then we have the "post-power-on-delay-ms" of the pwrseq > node to play with instead. Heiner, Jerome, Uffe, Amlogic DT maintainers, Perhaps tweaking the driver for every user is too intrusive. Another solution would be to factorize all boards that implement the reference design SDIO-based RF (WiFi + BT using brcm). Then we can add post-power-on-delay-ms = 20 in sdio_pwrseq. NB: there's vddio_ao1v8 vs vddao_1v8 in sei510. Is my addition OK? RFC prototype: diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts index e3bb6df42ff3e..42b5dcf358912 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts @@ -6,6 +6,7 @@ /dts-v1/; #include "meson-g12a.dtsi" +#include "meson-g12a-ref-design-brcm-rf.dtsi" #include <dt-bindings/gpio/meson-g12a-gpio.h> #include <dt-bindings/sound/meson-g12a-tohdmitx.h> @@ -53,13 +54,6 @@ emmc_pwrseq: emmc-pwrseq { reset-gpios = <&gpio BOOT_12 GPIO_ACTIVE_LOW>; }; - sdio_pwrseq: sdio-pwrseq { - compatible = "mmc-pwrseq-simple"; - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; - clocks = <&wifi32k>; - clock-names = "ext_clock"; - }; - ao_5v: regulator-ao_5v { compatible = "regulator-fixed"; regulator-name = "AO_5V"; @@ -182,13 +176,6 @@ codec { }; }; }; - - wifi32k: wifi32k { - compatible = "pwm-clock"; - #clock-cells = <0>; - clock-frequency = <32768>; - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ - }; }; &arb { @@ -299,37 +286,6 @@ &saradc { vref-supply = <&vddao_1v8>; }; -/* SDIO */ -&sd_emmc_a { - status = "okay"; - pinctrl-0 = <&sdio_pins>; - pinctrl-1 = <&sdio_clk_gate_pins>; - pinctrl-names = "default", "clk-gate"; - #address-cells = <1>; - #size-cells = <0>; - - bus-width = <4>; - cap-sd-highspeed; - sd-uhs-sdr50; - max-frequency = <100000000>; - - non-removable; - disable-wp; - - /* WiFi firmware requires power to be kept while in suspend */ - keep-power-in-suspend; - - mmc-pwrseq = <&sdio_pwrseq>; - - vmmc-supply = <&vddao_3v3>; - vqmmc-supply = <&vddao_1v8>; - - brcmf: wifi@1 { - reg = <1>; - compatible = "brcm,bcm4329-fmac"; - }; -}; - /* SD card */ &sd_emmc_b { status = "okay"; diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi new file mode 100644 index 0000000000000..e462324596964 --- /dev/null +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. + */ + +#include <dt-bindings/gpio/meson-g12a-gpio.h> + +/ { + sdio_pwrseq: sdio-pwrseq { + compatible = "mmc-pwrseq-simple"; + reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; + clocks = <&wifi32k>; + clock-names = "ext_clock"; + }; + + wifi32k: wifi32k { + compatible = "pwm-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ + }; +}; + +/* SDIO */ +&sd_emmc_a { + status = "okay"; + pinctrl-0 = <&sdio_pins>; + pinctrl-1 = <&sdio_clk_gate_pins>; + pinctrl-names = "default", "clk-gate"; + #address-cells = <1>; + #size-cells = <0>; + + bus-width = <4>; + cap-sd-highspeed; + sd-uhs-sdr50; + max-frequency = <100000000>; + + non-removable; + disable-wp; + + /* WiFi firmware requires power to be kept while in suspend */ + keep-power-in-suspend; + + mmc-pwrseq = <&sdio_pwrseq>; + + vmmc-supply = <&vddao_3v3>; + vqmmc-supply = <&vddao_1v8>; + + brcmf: wifi@1 { + reg = <1>; + compatible = "brcm,bcm4329-fmac"; + }; +}; diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts index 23b790c6469d3..e12aeb956b7d7 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts @@ -6,6 +6,7 @@ /dts-v1/; #include "meson-g12a.dtsi" +#include "meson-g12a-ref-design-brcm-rf.dtsi" #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/gpio/meson-g12a-gpio.h> @@ -96,6 +97,15 @@ emmc_1v8: regulator-emmc_1v8 { regulator-always-on; }; + vddao_1v8: regulator-vddao_1v8 { + compatible = "regulator-fixed"; + regulator-name = "VDDAO_1V8"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <&vddao_3v3>; + regulator-always-on; + }; + vddao_3v3: regulator-vddao_3v3 { compatible = "regulator-fixed"; regulator-name = "VDDAO_3V3"; @@ -143,20 +153,6 @@ vddio_ao1v8: regulator-vddio_ao1v8 { regulator-always-on; }; - sdio_pwrseq: sdio-pwrseq { - compatible = "mmc-pwrseq-simple"; - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; - clocks = <&wifi32k>; - clock-names = "ext_clock"; - }; - - wifi32k: wifi32k { - compatible = "pwm-clock"; - #clock-cells = <0>; - clock-frequency = <32768>; - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ - }; - sound { compatible = "amlogic,axg-sound-card"; model = "SEI510"; @@ -375,37 +371,6 @@ &saradc { vref-supply = <&vddio_ao1v8>; }; -/* SDIO */ -&sd_emmc_a { - status = "okay"; - pinctrl-0 = <&sdio_pins>; - pinctrl-1 = <&sdio_clk_gate_pins>; - pinctrl-names = "default", "clk-gate"; - #address-cells = <1>; - #size-cells = <0>; - - bus-width = <4>; - cap-sd-highspeed; - sd-uhs-sdr50; - max-frequency = <100000000>; - - non-removable; - disable-wp; - - /* WiFi firmware requires power to be kept while in suspend */ - keep-power-in-suspend; - - mmc-pwrseq = <&sdio_pwrseq>; - - vmmc-supply = <&vddao_3v3>; - vqmmc-supply = <&vddio_ao1v8>; - - brcmf: wifi@1 { - reg = <1>; - compatible = "brcm,bcm4329-fmac"; - }; -}; - /* SD card */ &sd_emmc_b { status = "okay"; diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts index b2bb94981838f..68a8876386115 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts @@ -6,6 +6,7 @@ /dts-v1/; #include "meson-g12a.dtsi" +#include "meson-g12a-ref-design-brcm-rf.dtsi" #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/gpio/meson-g12a-gpio.h> #include <dt-bindings/sound/meson-g12a-tohdmitx.h> @@ -60,13 +61,6 @@ emmc_pwrseq: emmc-pwrseq { reset-gpios = <&gpio BOOT_12 GPIO_ACTIVE_LOW>; }; - sdio_pwrseq: sdio-pwrseq { - compatible = "mmc-pwrseq-simple"; - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; - clocks = <&wifi32k>; - clock-names = "ext_clock"; - }; - flash_1v8: regulator-flash_1v8 { compatible = "regulator-fixed"; regulator-name = "FLASH_1V8"; @@ -226,13 +220,6 @@ codec { }; }; }; - - wifi32k: wifi32k { - compatible = "pwm-clock"; - #clock-cells = <0>; - clock-frequency = <32768>; - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ - }; }; &arb { @@ -391,37 +378,6 @@ &usb { dr_mode = "host"; }; -/* SDIO */ -&sd_emmc_a { - status = "okay"; - pinctrl-0 = <&sdio_pins>; - pinctrl-1 = <&sdio_clk_gate_pins>; - pinctrl-names = "default", "clk-gate"; - #address-cells = <1>; - #size-cells = <0>; - - bus-width = <4>; - cap-sd-highspeed; - sd-uhs-sdr50; - max-frequency = <100000000>; - - non-removable; - disable-wp; - - /* WiFi firmware requires power to be kept while in suspend */ - keep-power-in-suspend; - - mmc-pwrseq = <&sdio_pwrseq>; - - vmmc-supply = <&vddao_3v3>; - vqmmc-supply = <&vddao_1v8>; - - brcmf: wifi@1 { - reg = <1>; - compatible = "brcm,bcm4329-fmac"; - }; -}; - /* SD card */ &sd_emmc_b { status = "okay";
> On 16 Mar 2023, at 8:32 pm, Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > > On 16/03/2023 12:59, Ulf Hansson wrote: > >> If the problem is regulator specific, this would be the correct thing to do. >> >> Although, if the problem is pwrseq specific, like that we need a delay >> after enabling the clock and asserting the GPIO enable pin for the >> WiFi chip, then we have the "post-power-on-delay-ms" of the pwrseq >> node to play with instead. > > Heiner, Jerome, Uffe, Amlogic DT maintainers, > > Perhaps tweaking the driver for every user is too intrusive. > > Another solution would be to factorize all boards that implement > the reference design SDIO-based RF (WiFi + BT using brcm). > > Then we can add post-power-on-delay-ms = 20 in sdio_pwrseq. > > NB: there's vddio_ao1v8 vs vddao_1v8 in sei510. > Is my addition OK? In LibreELEC we use a fairly minimal kernel defconfig; things are booting fast compared to most desktop distro configs. I see occasional wireless issues with cheap STB boards where SDIO speeds need to be reduced or UHS modes need to be removed to keep things stable, but I don’t see SDIO probe issues with any of the 30+ boards I do random testing with. The only SDIO drama in recent times have been Heiner's IRQ changes (which are now resolved). Are you using a real SEI510 board? ..I ask because we get lots of users with random no-name Android boxes who try lots of dtb files until they come across something that works; and there is a recurring trend where users have been using SEI510 or SEI620 with poor results, and odd issues magically go away once they are persuaded to use something else. So I have the impression those specific device-trees aren’t well suited to typical reference design clones. FWIW, I’ve also had thoughts about factoring aspects of WiFi support into a few common dtsi files as there are increasing numbers of boxes/boards that use Realtek or Qualcomm chips in circulation (due to post-covid chip supply and pricing troubles). I would leave the sd_mmc_a node in the board files since this is common (and in my experience works the same everywhere) while moving the BT bits that need ‘compatible’ and GPIO details and the SDIO ID poke and compatible (which isn’t really needed) to dtsi files. My goal would be to make the copy/paste assembly of a new Android box dts even easier than it already is. If you don’t have an SEI510 board I have a hunch you’d be better off with another device-tree as the source/base for your board. If you’re able to share the schematics (privately if preferred) I’d be interested in seeing what other dtb's could be used; and perhaps getting you to boot a LibreELEC test image on your board to see whether my codebase gives same or different results? Christian > RFC prototype: > > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts > index e3bb6df42ff3e..42b5dcf358912 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts > +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-radxa-zero.dts > @@ -6,6 +6,7 @@ > /dts-v1/; > > #include "meson-g12a.dtsi" > +#include "meson-g12a-ref-design-brcm-rf.dtsi" > #include <dt-bindings/gpio/meson-g12a-gpio.h> > #include <dt-bindings/sound/meson-g12a-tohdmitx.h> > > @@ -53,13 +54,6 @@ emmc_pwrseq: emmc-pwrseq { > reset-gpios = <&gpio BOOT_12 GPIO_ACTIVE_LOW>; > }; > > - sdio_pwrseq: sdio-pwrseq { > - compatible = "mmc-pwrseq-simple"; > - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; > - clocks = <&wifi32k>; > - clock-names = "ext_clock"; > - }; > - > ao_5v: regulator-ao_5v { > compatible = "regulator-fixed"; > regulator-name = "AO_5V"; > @@ -182,13 +176,6 @@ codec { > }; > }; > }; > - > - wifi32k: wifi32k { > - compatible = "pwm-clock"; > - #clock-cells = <0>; > - clock-frequency = <32768>; > - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ > - }; > }; > > &arb { > @@ -299,37 +286,6 @@ &saradc { > vref-supply = <&vddao_1v8>; > }; > > -/* SDIO */ > -&sd_emmc_a { > - status = "okay"; > - pinctrl-0 = <&sdio_pins>; > - pinctrl-1 = <&sdio_clk_gate_pins>; > - pinctrl-names = "default", "clk-gate"; > - #address-cells = <1>; > - #size-cells = <0>; > - > - bus-width = <4>; > - cap-sd-highspeed; > - sd-uhs-sdr50; > - max-frequency = <100000000>; > - > - non-removable; > - disable-wp; > - > - /* WiFi firmware requires power to be kept while in suspend */ > - keep-power-in-suspend; > - > - mmc-pwrseq = <&sdio_pwrseq>; > - > - vmmc-supply = <&vddao_3v3>; > - vqmmc-supply = <&vddao_1v8>; > - > - brcmf: wifi@1 { > - reg = <1>; > - compatible = "brcm,bcm4329-fmac"; > - }; > -}; > - > /* SD card */ > &sd_emmc_b { > status = "okay"; > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi > new file mode 100644 > index 0000000000000..e462324596964 > --- /dev/null > +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-ref-design-brcm-rf.dtsi > @@ -0,0 +1,53 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Copyright (c) 2018 Amlogic, Inc. All rights reserved. > + */ > + > +#include <dt-bindings/gpio/meson-g12a-gpio.h> > + > +/ { > + sdio_pwrseq: sdio-pwrseq { > + compatible = "mmc-pwrseq-simple"; > + reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; > + clocks = <&wifi32k>; > + clock-names = "ext_clock"; > + }; > + > + wifi32k: wifi32k { > + compatible = "pwm-clock"; > + #clock-cells = <0>; > + clock-frequency = <32768>; > + pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ > + }; > +}; > + > +/* SDIO */ > +&sd_emmc_a { > + status = "okay"; > + pinctrl-0 = <&sdio_pins>; > + pinctrl-1 = <&sdio_clk_gate_pins>; > + pinctrl-names = "default", "clk-gate"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + bus-width = <4>; > + cap-sd-highspeed; > + sd-uhs-sdr50; > + max-frequency = <100000000>; > + > + non-removable; > + disable-wp; > + > + /* WiFi firmware requires power to be kept while in suspend */ > + keep-power-in-suspend; > + > + mmc-pwrseq = <&sdio_pwrseq>; > + > + vmmc-supply = <&vddao_3v3>; > + vqmmc-supply = <&vddao_1v8>; > + > + brcmf: wifi@1 { > + reg = <1>; > + compatible = "brcm,bcm4329-fmac"; > + }; > +}; > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts > index 23b790c6469d3..e12aeb956b7d7 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts > +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-sei510.dts > @@ -6,6 +6,7 @@ > /dts-v1/; > > #include "meson-g12a.dtsi" > +#include "meson-g12a-ref-design-brcm-rf.dtsi" > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/gpio/meson-g12a-gpio.h> > @@ -96,6 +97,15 @@ emmc_1v8: regulator-emmc_1v8 { > regulator-always-on; > }; > > + vddao_1v8: regulator-vddao_1v8 { > + compatible = "regulator-fixed"; > + regulator-name = "VDDAO_1V8"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + vin-supply = <&vddao_3v3>; > + regulator-always-on; > + }; > + > vddao_3v3: regulator-vddao_3v3 { > compatible = "regulator-fixed"; > regulator-name = "VDDAO_3V3"; > @@ -143,20 +153,6 @@ vddio_ao1v8: regulator-vddio_ao1v8 { > regulator-always-on; > }; > > - sdio_pwrseq: sdio-pwrseq { > - compatible = "mmc-pwrseq-simple"; > - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; > - clocks = <&wifi32k>; > - clock-names = "ext_clock"; > - }; > - > - wifi32k: wifi32k { > - compatible = "pwm-clock"; > - #clock-cells = <0>; > - clock-frequency = <32768>; > - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ > - }; > - > sound { > compatible = "amlogic,axg-sound-card"; > model = "SEI510"; > @@ -375,37 +371,6 @@ &saradc { > vref-supply = <&vddio_ao1v8>; > }; > > -/* SDIO */ > -&sd_emmc_a { > - status = "okay"; > - pinctrl-0 = <&sdio_pins>; > - pinctrl-1 = <&sdio_clk_gate_pins>; > - pinctrl-names = "default", "clk-gate"; > - #address-cells = <1>; > - #size-cells = <0>; > - > - bus-width = <4>; > - cap-sd-highspeed; > - sd-uhs-sdr50; > - max-frequency = <100000000>; > - > - non-removable; > - disable-wp; > - > - /* WiFi firmware requires power to be kept while in suspend */ > - keep-power-in-suspend; > - > - mmc-pwrseq = <&sdio_pwrseq>; > - > - vmmc-supply = <&vddao_3v3>; > - vqmmc-supply = <&vddio_ao1v8>; > - > - brcmf: wifi@1 { > - reg = <1>; > - compatible = "brcm,bcm4329-fmac"; > - }; > -}; > - > /* SD card */ > &sd_emmc_b { > status = "okay"; > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts > index b2bb94981838f..68a8876386115 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts > +++ b/arch/arm64/boot/dts/amlogic/meson-g12a-x96-max.dts > @@ -6,6 +6,7 @@ > /dts-v1/; > > #include "meson-g12a.dtsi" > +#include "meson-g12a-ref-design-brcm-rf.dtsi" > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/gpio/meson-g12a-gpio.h> > #include <dt-bindings/sound/meson-g12a-tohdmitx.h> > @@ -60,13 +61,6 @@ emmc_pwrseq: emmc-pwrseq { > reset-gpios = <&gpio BOOT_12 GPIO_ACTIVE_LOW>; > }; > > - sdio_pwrseq: sdio-pwrseq { > - compatible = "mmc-pwrseq-simple"; > - reset-gpios = <&gpio GPIOX_6 GPIO_ACTIVE_LOW>; > - clocks = <&wifi32k>; > - clock-names = "ext_clock"; > - }; > - > flash_1v8: regulator-flash_1v8 { > compatible = "regulator-fixed"; > regulator-name = "FLASH_1V8"; > @@ -226,13 +220,6 @@ codec { > }; > }; > }; > - > - wifi32k: wifi32k { > - compatible = "pwm-clock"; > - #clock-cells = <0>; > - clock-frequency = <32768>; > - pwms = <&pwm_ef 0 30518 0>; /* PWM_E at 32.768KHz */ > - }; > }; > > &arb { > @@ -391,37 +378,6 @@ &usb { > dr_mode = "host"; > }; > > -/* SDIO */ > -&sd_emmc_a { > - status = "okay"; > - pinctrl-0 = <&sdio_pins>; > - pinctrl-1 = <&sdio_clk_gate_pins>; > - pinctrl-names = "default", "clk-gate"; > - #address-cells = <1>; > - #size-cells = <0>; > - > - bus-width = <4>; > - cap-sd-highspeed; > - sd-uhs-sdr50; > - max-frequency = <100000000>; > - > - non-removable; > - disable-wp; > - > - /* WiFi firmware requires power to be kept while in suspend */ > - keep-power-in-suspend; > - > - mmc-pwrseq = <&sdio_pwrseq>; > - > - vmmc-supply = <&vddao_3v3>; > - vqmmc-supply = <&vddao_1v8>; > - > - brcmf: wifi@1 { > - reg = <1>; > - compatible = "brcm,bcm4329-fmac"; > - }; > -}; > - > /* SD card */ > &sd_emmc_b { > status = "okay"; > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 6e5ea0213b477..73ecbcf588c65 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -1182,6 +1182,7 @@ static int meson_mmc_probe(struct platform_device *pdev) mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev); if (!mmc) return -ENOMEM; + mmc->ios.power_delay_ms = 20; host = mmc_priv(mmc); host->mmc = mmc; host->dev = &pdev->dev;
With the default power-up delay, on small kernels, the host probes too soon, and mmc_send_io_op_cond() times out. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- Stress-tested with 80 cold boots, checking every time mmc2: new ultra high speed SDR50 SDIO card at address 0001 IIUC, this will also slow down SD & MMC probing, but an additional 20 ms seems acceptable? --- drivers/mmc/host/meson-gx-mmc.c | 1 + 1 file changed, 1 insertion(+)