* [PATCH] mmc: meson-gx: increase power-up delay @ 2023-03-14 17:24 Marc Gonzalez 2023-03-14 19:45 ` Heiner Kallweit 0 siblings, 1 reply; 9+ messages in thread From: Marc Gonzalez @ 2023-03-14 17:24 UTC (permalink / raw) To: Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Pierre-Hugues Husson, Heiner Kallweit, Rong Chen, Yang Yingliang Cc: MMC, AML 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(+) 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; -- 2.25.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-14 17:24 [PATCH] mmc: meson-gx: increase power-up delay Marc Gonzalez @ 2023-03-14 19:45 ` Heiner Kallweit 2023-03-15 10:20 ` Marc Gonzalez 0 siblings, 1 reply; 9+ messages in thread From: Heiner Kallweit @ 2023-03-14 19:45 UTC (permalink / raw) To: Marc Gonzalez, Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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; _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-14 19:45 ` Heiner Kallweit @ 2023-03-15 10:20 ` Marc Gonzalez 2023-03-15 15:27 ` Jerome Brunet 0 siblings, 1 reply; 9+ messages in thread From: Marc Gonzalez @ 2023-03-15 10:20 UTC (permalink / raw) To: Heiner Kallweit, Ulf Hansson, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-15 10:20 ` Marc Gonzalez @ 2023-03-15 15:27 ` Jerome Brunet 2023-03-15 21:14 ` Heiner Kallweit 2023-03-16 11:59 ` Ulf Hansson 0 siblings, 2 replies; 9+ messages in thread From: Jerome Brunet @ 2023-03-15 15:27 UTC (permalink / raw) To: Marc Gonzalez, Heiner Kallweit, Ulf Hansson, Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-15 15:27 ` Jerome Brunet @ 2023-03-15 21:14 ` Heiner Kallweit 2023-03-16 12:32 ` Marc Gonzalez 2023-03-16 11:59 ` Ulf Hansson 1 sibling, 1 reply; 9+ messages in thread From: Heiner Kallweit @ 2023-03-15 21:14 UTC (permalink / raw) To: Jerome Brunet, Marc Gonzalez, Ulf Hansson, Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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 > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-15 21:14 ` Heiner Kallweit @ 2023-03-16 12:32 ` Marc Gonzalez 2023-03-16 13:02 ` Marc Gonzalez 0 siblings, 1 reply; 9+ messages in thread From: Marc Gonzalez @ 2023-03-16 12:32 UTC (permalink / raw) To: Heiner Kallweit, Jerome Brunet, Ulf Hansson, Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-16 12:32 ` Marc Gonzalez @ 2023-03-16 13:02 ` Marc Gonzalez 0 siblings, 0 replies; 9+ messages in thread From: Marc Gonzalez @ 2023-03-16 13:02 UTC (permalink / raw) To: Heiner Kallweit, Jerome Brunet, Ulf Hansson, Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang Cc: MMC, AML 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-15 15:27 ` Jerome Brunet 2023-03-15 21:14 ` Heiner Kallweit @ 2023-03-16 11:59 ` Ulf Hansson 2023-03-16 16:32 ` Marc Gonzalez 1 sibling, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2023-03-16 11:59 UTC (permalink / raw) To: Heiner Kallweit, Marc Gonzalez, Jerome Brunet Cc: Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang, MMC, AML 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mmc: meson-gx: increase power-up delay 2023-03-16 11:59 ` Ulf Hansson @ 2023-03-16 16:32 ` Marc Gonzalez 0 siblings, 0 replies; 9+ messages in thread From: Marc Gonzalez @ 2023-03-16 16:32 UTC (permalink / raw) To: Ulf Hansson, Heiner Kallweit, Jerome Brunet Cc: Neil Armstrong, Kevin Hilman, Martin Blumenstingl, Pierre-Hugues Husson, Rong Chen, Yang Yingliang, MMC, AML 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"; _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-16 16:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-14 17:24 [PATCH] mmc: meson-gx: increase power-up delay Marc Gonzalez 2023-03-14 19:45 ` Heiner Kallweit 2023-03-15 10:20 ` Marc Gonzalez 2023-03-15 15:27 ` Jerome Brunet 2023-03-15 21:14 ` Heiner Kallweit 2023-03-16 12:32 ` Marc Gonzalez 2023-03-16 13:02 ` Marc Gonzalez 2023-03-16 11:59 ` Ulf Hansson 2023-03-16 16:32 ` Marc Gonzalez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox