| Message ID | TYZPR01MB5556B56D834E02F41C44D81DC95FA@TYZPR01MB5556.apcprd01.prod.exchangelabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: meson-mx-sdhc: Fix initialization frozen issue | expand |
Hello, first of all: thank you for this patch! On Mon, Jun 19, 2023 at 7:36 PM Ziyang Huang <hzyitc@outlook.com> wrote: > > Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending > HS CMD13") set HS clock (52MHz) before switching to HS mode. For this > freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). > Then we set rx_clk_phase to 11 or 15 which is out of range and make > hardware frozen. After we send command request, no irq will be > interrupted and the mmc driver will keep to wait for request finished, > even durning rebooting. I think this is the exact same problem I reported some days ago: [0] Ulf is questioning whether we properly support 52MHz clocks correctly, so I think you're onto something! So this is an excellent finding! I can confirm that using rx_clk_phase of 1 makes my Odroid-C1 eMMC work again :-) > So let's set a common value - 1 just for initialization. Then let > meson_mx_sdhc_execute_tuning() to find the accurate value for data > transfer. As far as I know unconditionally using value 1 can negatively affect other devices. I'm assuming that you're testing on an Odroid-C1 or similar board with HS200 eMMC: On those SoC + eMMC combinations we do support. But on other boards (for example Meson8b EC-100 / Endless Mini) there's no HS200 support because the eMMC is connected with 3.3V IO lines. So tuning is not executed there (if I recall correctly). What do you think about adding a special case for the 51MHz "actual clock rate" and adding a comment that it was found by manual testing? For some reason (that I don't understand) Amlogic's vendor driver maxes out at 47.22MHz (presumably because they limit themselves to using FCLK_DIV3 as input only - but I don't get why...). Best regards, Martin [0] https://lore.kernel.org/linux-amlogic/CAFBinCD0RT0p-jk86W0JuMT3ufohRh1RqWCcM35DKZJpuc10HQ@mail.gmail.com/ [1] https://lore.kernel.org/linux-amlogic/CAPDyKFpS-UwiaRPMqSpX0mNPrS5p=yJzu3g0=pGyCkWHSYyqWg@mail.gmail.com/
Hello, On Mon, Jun 19, 2023 at 9:51 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hello, > > first of all: thank you for this patch! > > On Mon, Jun 19, 2023 at 7:36 PM Ziyang Huang <hzyitc@outlook.com> wrote: > > > > Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending > > HS CMD13") set HS clock (52MHz) before switching to HS mode. For this > > freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). > > Then we set rx_clk_phase to 11 or 15 which is out of range and make > > hardware frozen. After we send command request, no irq will be > > interrupted and the mmc driver will keep to wait for request finished, > > even durning rebooting. > I think this is the exact same problem I reported some days ago: [0] > Ulf is questioning whether we properly support 52MHz clocks correctly, > so I think you're onto something! > > So this is an excellent finding! I can confirm that using rx_clk_phase > of 1 makes my Odroid-C1 eMMC work again :-) > > > So let's set a common value - 1 just for initialization. Then let > > meson_mx_sdhc_execute_tuning() to find the accurate value for data > > transfer. > As far as I know unconditionally using value 1 can negatively affect > other devices. > I'm assuming that you're testing on an Odroid-C1 or similar board with > HS200 eMMC: > On those SoC + eMMC combinations we do support. But on other boards > (for example Meson8b EC-100 / Endless Mini) there's no HS200 support > because the eMMC is connected with 3.3V IO lines. So tuning is not > executed there (if I recall correctly). > > What do you think about adding a special case for the 51MHz "actual > clock rate" and adding a comment that it was found by manual testing? > For some reason (that I don't understand) Amlogic's vendor driver > maxes out at 47.22MHz (presumably because they limit themselves to > using FCLK_DIV3 as input only - but I don't get why...). Did you have the chance to look into my comment? I would like to hear your opinion on this topic! Best regards, Martin
在 2023/6/20 3:51, Martin Blumenstingl 写道: > Hello, > > first of all: thank you for this patch! > > On Mon, Jun 19, 2023 at 7:36 PM Ziyang Huang <hzyitc@outlook.com> wrote: >> >> Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending >> HS CMD13") set HS clock (52MHz) before switching to HS mode. For this >> freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). >> Then we set rx_clk_phase to 11 or 15 which is out of range and make >> hardware frozen. After we send command request, no irq will be >> interrupted and the mmc driver will keep to wait for request finished, >> even durning rebooting. > I think this is the exact same problem I reported some days ago: [0] > Ulf is questioning whether we properly support 52MHz clocks correctly, > so I think you're onto something! > > So this is an excellent finding! I can confirm that using rx_clk_phase > of 1 makes my Odroid-C1 eMMC work again :-) > >> So let's set a common value - 1 just for initialization. Then let >> meson_mx_sdhc_execute_tuning() to find the accurate value for data >> transfer. > As far as I know unconditionally using value 1 can negatively affect > other devices. > I'm assuming that you're testing on an Odroid-C1 or similar board with > HS200 eMMC: > On those SoC + eMMC combinations we do support. But on other boards > (for example Meson8b EC-100 / Endless Mini) there's no HS200 support > because the eMMC is connected with 3.3V IO lines. So tuning is not > executed there (if I recall correctly). Sorry for the later reply. I'm so busy these day. After checking the code, I found the following flow, so I think tuningshould work for all cards. mmc_start_request() -> __mmc_start_request() -> mmc_retune() -> mmc_execute_tuning() -> host->ops->execute_tuning() And yes, 1 may be not a good choice. I consider 3 values: - 1 - div_val / 2 - div_val - 1 Maybe, (div_val / 2) is a good choice. How do you think? > What do you think about adding a special case for the 51MHz "actual > clock rate" and adding a comment that it was found by manual testing? > For some reason (that I don't understand) Amlogic's vendor driver > maxes out at 47.22MHz (presumably because they limit themselves to > using FCLK_DIV3 as input only - but I don't get why...). With some modifications, I found that the mmc controller of S805 can work with all frequency, not only those mentioned in commit e4bf1b0970ef("mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host"). It seem like that every things work fine. I will use a oscilloscope to confirm the hardware clock is correct when I have time. For this future commit, I don't want to add a special case. > Best regards, > Martin > > > [0] https://lore.kernel.org/linux-amlogic/CAFBinCD0RT0p-jk86W0JuMT3ufohRh1RqWCcM35DKZJpuc10HQ@mail.gmail.com/ > [1] https://lore.kernel.org/linux-amlogic/CAPDyKFpS-UwiaRPMqSpX0mNPrS5p=yJzu3g0=pGyCkWHSYyqWg@mail.gmail.com/
+ Thorsten On Mon, 19 Jun 2023 at 19:36, Ziyang Huang <hzyitc@outlook.com> wrote: > > Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending > HS CMD13") set HS clock (52MHz) before switching to HS mode. For this > freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). > Then we set rx_clk_phase to 11 or 15 which is out of range and make > hardware frozen. After we send command request, no irq will be > interrupted and the mmc driver will keep to wait for request finished, > even durning rebooting. > > So let's set a common value - 1 just for initialization. Then let > meson_mx_sdhc_execute_tuning() to find the accurate value for data > transfer. > > Fixes: e4bf1b0970ef ("mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host") > Signed-off-by: Ziyang Huang <hzyitc@outlook.com> I don't quite understand if this patch is ok for everybody for me to apply? It seems like it solves at least some part of the problems that Martin/Thorsten were looking at too [1], right? Kind regards Uffe [1] https://lore.kernel.org/all/CAFBinCD0RT0p-jk86W0JuMT3ufohRh1RqWCcM35DKZJpuc10HQ@mail.gmail.com/#r > --- > drivers/mmc/host/meson-mx-sdhc-mmc.c | 26 +++----------------------- > 1 file changed, 3 insertions(+), 23 deletions(-) > > diff --git a/drivers/mmc/host/meson-mx-sdhc-mmc.c b/drivers/mmc/host/meson-mx-sdhc-mmc.c > index da85c2f2..a01090a2 100644 > --- a/drivers/mmc/host/meson-mx-sdhc-mmc.c > +++ b/drivers/mmc/host/meson-mx-sdhc-mmc.c > @@ -269,7 +269,6 @@ static int meson_mx_sdhc_enable_clks(struct mmc_host *mmc) > static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) > { > struct meson_mx_sdhc_host *host = mmc_priv(mmc); > - u32 rx_clk_phase; > int ret; > > meson_mx_sdhc_disable_clks(mmc); > @@ -290,31 +289,12 @@ static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) > mmc->actual_clock = clk_get_rate(host->sd_clk); > > /* > - * according to Amlogic the following latching points are > - * selected with empirical values, there is no (known) formula > - * to calculate these. > + * This value is just for initialization. For data transmission, > + * meson_mx_sdhc_execute_tuning() will find a accurate value > */ > - if (mmc->actual_clock > 100000000) { > - rx_clk_phase = 1; > - } else if (mmc->actual_clock > 45000000) { > - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) > - rx_clk_phase = 15; > - else > - rx_clk_phase = 11; > - } else if (mmc->actual_clock >= 25000000) { > - rx_clk_phase = 15; > - } else if (mmc->actual_clock > 5000000) { > - rx_clk_phase = 23; > - } else if (mmc->actual_clock > 1000000) { > - rx_clk_phase = 55; > - } else { > - rx_clk_phase = 1061; > - } > - > regmap_update_bits(host->regmap, MESON_SDHC_CLK2, > MESON_SDHC_CLK2_RX_CLK_PHASE, > - FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, > - rx_clk_phase)); > + FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, 1)); > } else { > mmc->actual_clock = 0; > } > -- > 2.34.1 >
On 14.09.23 16:45, Ulf Hansson wrote: > + Thorsten I recently gave up on this, as it seems nobody cared anymore, but let's give this another try. > On Mon, 19 Jun 2023 at 19:36, Ziyang Huang <hzyitc@outlook.com> wrote: >> >> Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending >> HS CMD13") set HS clock (52MHz) before switching to HS mode. For this >> freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). >> Then we set rx_clk_phase to 11 or 15 which is out of range and make >> hardware frozen. After we send command request, no irq will be >> interrupted and the mmc driver will keep to wait for request finished, >> even durning rebooting. >> >> So let's set a common value - 1 just for initialization. Then let >> meson_mx_sdhc_execute_tuning() to find the accurate value for data >> transfer. >> >> Fixes: e4bf1b0970ef ("mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host") >> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > > I don't quite understand if this patch is ok for everybody for me to apply? > > It seems like it solves at least some part of the problems that > Martin/Thorsten were looking at too [1], right? Martin, could you help clarifying the situation here? It seems Ziyang Huang is busy. I briefly skimmed this thread again and to me it sounded like there was a plan for an improved patch that hasn't shown up yet. Also CCing Brian Norris who according to the bisection from Martin in that "[1]" caused the regression (or am I missing/confusing something here?). > [1] > https://lore.kernel.org/all/CAFBinCD0RT0p-jk86W0JuMT3ufohRh1RqWCcM35DKZJpuc10HQ@mail.gmail.com/#r Ciao, Thorsten >> --- >> drivers/mmc/host/meson-mx-sdhc-mmc.c | 26 +++----------------------- >> 1 file changed, 3 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-mx-sdhc-mmc.c b/drivers/mmc/host/meson-mx-sdhc-mmc.c >> index da85c2f2..a01090a2 100644 >> --- a/drivers/mmc/host/meson-mx-sdhc-mmc.c >> +++ b/drivers/mmc/host/meson-mx-sdhc-mmc.c >> @@ -269,7 +269,6 @@ static int meson_mx_sdhc_enable_clks(struct mmc_host *mmc) >> static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) >> { >> struct meson_mx_sdhc_host *host = mmc_priv(mmc); >> - u32 rx_clk_phase; >> int ret; >> >> meson_mx_sdhc_disable_clks(mmc); >> @@ -290,31 +289,12 @@ static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) >> mmc->actual_clock = clk_get_rate(host->sd_clk); >> >> /* >> - * according to Amlogic the following latching points are >> - * selected with empirical values, there is no (known) formula >> - * to calculate these. >> + * This value is just for initialization. For data transmission, >> + * meson_mx_sdhc_execute_tuning() will find a accurate value >> */ >> - if (mmc->actual_clock > 100000000) { >> - rx_clk_phase = 1; >> - } else if (mmc->actual_clock > 45000000) { >> - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) >> - rx_clk_phase = 15; >> - else >> - rx_clk_phase = 11; >> - } else if (mmc->actual_clock >= 25000000) { >> - rx_clk_phase = 15; >> - } else if (mmc->actual_clock > 5000000) { >> - rx_clk_phase = 23; >> - } else if (mmc->actual_clock > 1000000) { >> - rx_clk_phase = 55; >> - } else { >> - rx_clk_phase = 1061; >> - } >> - >> regmap_update_bits(host->regmap, MESON_SDHC_CLK2, >> MESON_SDHC_CLK2_RX_CLK_PHASE, >> - FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, >> - rx_clk_phase)); >> + FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, 1)); >> } else { >> mmc->actual_clock = 0; >> } >> -- >> 2.34.1 >> >
diff --git a/drivers/mmc/host/meson-mx-sdhc-mmc.c b/drivers/mmc/host/meson-mx-sdhc-mmc.c index da85c2f2..a01090a2 100644 --- a/drivers/mmc/host/meson-mx-sdhc-mmc.c +++ b/drivers/mmc/host/meson-mx-sdhc-mmc.c @@ -269,7 +269,6 @@ static int meson_mx_sdhc_enable_clks(struct mmc_host *mmc) static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) { struct meson_mx_sdhc_host *host = mmc_priv(mmc); - u32 rx_clk_phase; int ret; meson_mx_sdhc_disable_clks(mmc); @@ -290,31 +289,12 @@ static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios) mmc->actual_clock = clk_get_rate(host->sd_clk); /* - * according to Amlogic the following latching points are - * selected with empirical values, there is no (known) formula - * to calculate these. + * This value is just for initialization. For data transmission, + * meson_mx_sdhc_execute_tuning() will find a accurate value */ - if (mmc->actual_clock > 100000000) { - rx_clk_phase = 1; - } else if (mmc->actual_clock > 45000000) { - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) - rx_clk_phase = 15; - else - rx_clk_phase = 11; - } else if (mmc->actual_clock >= 25000000) { - rx_clk_phase = 15; - } else if (mmc->actual_clock > 5000000) { - rx_clk_phase = 23; - } else if (mmc->actual_clock > 1000000) { - rx_clk_phase = 55; - } else { - rx_clk_phase = 1061; - } - regmap_update_bits(host->regmap, MESON_SDHC_CLK2, MESON_SDHC_CLK2_RX_CLK_PHASE, - FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, - rx_clk_phase)); + FIELD_PREP(MESON_SDHC_CLK2_RX_CLK_PHASE, 1)); } else { mmc->actual_clock = 0; }
Commit 4bc31edebde5 ("mmc: core: Set HS clock speed before sending HS CMD13") set HS clock (52MHz) before switching to HS mode. For this freq, FCLK_DIV5 will be selected and div value is 10 (reg value is 9). Then we set rx_clk_phase to 11 or 15 which is out of range and make hardware frozen. After we send command request, no irq will be interrupted and the mmc driver will keep to wait for request finished, even durning rebooting. So let's set a common value - 1 just for initialization. Then let meson_mx_sdhc_execute_tuning() to find the accurate value for data transfer. Fixes: e4bf1b0970ef ("mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host") Signed-off-by: Ziyang Huang <hzyitc@outlook.com> --- drivers/mmc/host/meson-mx-sdhc-mmc.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-)