| Message ID | 20201109031233.25320-1-jh80.chung@samsung.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | mmc: meson_gx_mmc: change a clock phase to stable value | expand |
Hi, On 09/11/2020 04:12, Jaehoon Chung wrote: > Core clock phase value is changed from 180' to 270'. > It's more stable than before. > - Odroidn-N2/C4 : Working fine with 52MHz > - VIM3 : Working fine with 52MHz > > Before this patch, Odroid-C4 doesn't work fine with 52MHz. > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > index 719dd1e5e570..7c60e0566560 100644 > --- a/drivers/mmc/meson_gx_mmc.c > +++ b/drivers/mmc/meson_gx_mmc.c > @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) > } > clk_div = DIV_ROUND_UP(clk, mmc->clock); > > - /* 180 phase core clock */ > - meson_mmc_clk |= CLK_CO_PHASE_180; > - > - /* 180 phase tx clock */ > + /* > + * Clock Phase needs to set a proper value. > + * It can be changed to other value. > + * Because CORE : 270' Phase and TX : 0' Phase are stable, > + * set to them by default. > + */ > + /* Core Clock Phase */ > + meson_mmc_clk |= CLK_CO_PHASE_270; > + > + /* TX Clock Phase */ > meson_mmc_clk |= CLK_TX_PHASE_000; > > /* clock settings */ > The previous values were aligned on the Linux driver, which is functional. How did you test these ? Neil
> From: Neil Armstrong <narmstrong@baylibre.com> > Date: Mon, 9 Nov 2020 14:37:09 +0100 > > Hi, > > On 09/11/2020 04:12, Jaehoon Chung wrote: > > Core clock phase value is changed from 180' to 270'. > > It's more stable than before. > > - Odroidn-N2/C4 : Working fine with 52MHz > > - VIM3 : Working fine with 52MHz > > > > Before this patch, Odroid-C4 doesn't work fine with 52MHz. > > > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > > --- > > drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > > index 719dd1e5e570..7c60e0566560 100644 > > --- a/drivers/mmc/meson_gx_mmc.c > > +++ b/drivers/mmc/meson_gx_mmc.c > > @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) > > } > > clk_div = DIV_ROUND_UP(clk, mmc->clock); > > > > - /* 180 phase core clock */ > > - meson_mmc_clk |= CLK_CO_PHASE_180; > > - > > - /* 180 phase tx clock */ > > + /* > > + * Clock Phase needs to set a proper value. > > + * It can be changed to other value. > > + * Because CORE : 270' Phase and TX : 0' Phase are stable, > > + * set to them by default. > > + */ > > + /* Core Clock Phase */ > > + meson_mmc_clk |= CLK_CO_PHASE_270; > > + > > + /* TX Clock Phase */ > > meson_mmc_clk |= CLK_TX_PHASE_000; > > > > /* clock settings */ > > > > The previous values were aligned on the Linux driver, which is functional. Actually the Linux driver isn't really functional; the 52 MHz high-speed mode doesn't work. But since HS200 does work in Linux, probably nobody noticed this. That said, I'm not confident a single clock phase setting will work across all Amlogic SoCs and across different boards. Maybe we need something in the device tree such that we can control the values on a per-board level.
On 09/11/2020 15:10, Mark Kettenis wrote: >> From: Neil Armstrong <narmstrong@baylibre.com> >> Date: Mon, 9 Nov 2020 14:37:09 +0100 >> >> Hi, >> >> On 09/11/2020 04:12, Jaehoon Chung wrote: >>> Core clock phase value is changed from 180' to 270'. >>> It's more stable than before. >>> - Odroidn-N2/C4 : Working fine with 52MHz >>> - VIM3 : Working fine with 52MHz >>> >>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>> >>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>> --- >>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>> index 719dd1e5e570..7c60e0566560 100644 >>> --- a/drivers/mmc/meson_gx_mmc.c >>> +++ b/drivers/mmc/meson_gx_mmc.c >>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>> } >>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>> >>> - /* 180 phase core clock */ >>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>> - >>> - /* 180 phase tx clock */ >>> + /* >>> + * Clock Phase needs to set a proper value. >>> + * It can be changed to other value. >>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>> + * set to them by default. >>> + */ >>> + /* Core Clock Phase */ >>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>> + >>> + /* TX Clock Phase */ >>> meson_mmc_clk |= CLK_TX_PHASE_000; >>> >>> /* clock settings */ >>> >> >> The previous values were aligned on the Linux driver, which is functional. > > Actually the Linux driver isn't really functional; the 52 MHz > high-speed mode doesn't work. But since HS200 does work in Linux, > probably nobody noticed this. > > That said, I'm not confident a single clock phase setting will work > across all Amlogic SoCs and across different boards. Maybe we need > something in the device tree such that we can control the values on a > per-board level. > So this is specific to SM1 SoCs then, because others families doesn't have such issues at 52MHz. So the Linux must be fixes, including the bindings to introduce a new compatible, then ported to U-Boot. So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this clarified. Neil
Hi Neil, On Mon, 9 Nov 2020 at 19:56, Neil Armstrong <narmstrong@baylibre.com> wrote: > > On 09/11/2020 15:10, Mark Kettenis wrote: > >> From: Neil Armstrong <narmstrong@baylibre.com> > >> Date: Mon, 9 Nov 2020 14:37:09 +0100 > >> > >> Hi, > >> > >> On 09/11/2020 04:12, Jaehoon Chung wrote: > >>> Core clock phase value is changed from 180' to 270'. > >>> It's more stable than before. > >>> - Odroidn-N2/C4 : Working fine with 52MHz > >>> - VIM3 : Working fine with 52MHz > >>> > >>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. > >>> > >>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > >>> --- > >>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- > >>> 1 file changed, 10 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > >>> index 719dd1e5e570..7c60e0566560 100644 > >>> --- a/drivers/mmc/meson_gx_mmc.c > >>> +++ b/drivers/mmc/meson_gx_mmc.c > >>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) > >>> } > >>> clk_div = DIV_ROUND_UP(clk, mmc->clock); > >>> > >>> - /* 180 phase core clock */ > >>> - meson_mmc_clk |= CLK_CO_PHASE_180; > >>> - > >>> - /* 180 phase tx clock */ > >>> + /* > >>> + * Clock Phase needs to set a proper value. > >>> + * It can be changed to other value. > >>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, > >>> + * set to them by default. > >>> + */ > >>> + /* Core Clock Phase */ > >>> + meson_mmc_clk |= CLK_CO_PHASE_270; > >>> + > >>> + /* TX Clock Phase */ > >>> meson_mmc_clk |= CLK_TX_PHASE_000; > >>> > >>> /* clock settings */ > >>> > >> > >> The previous values were aligned on the Linux driver, which is functional. > > > > Actually the Linux driver isn't really functional; the 52 MHz > > high-speed mode doesn't work. But since HS200 does work in Linux, > > probably nobody noticed this. > > > > That said, I'm not confident a single clock phase setting will work > > across all Amlogic SoCs and across different boards. Maybe we need > > something in the device tree such that we can control the values on a > > per-board level. > > > > So this is specific to SM1 SoCs then, because others families doesn't have such issues > at 52MHz. > > So the Linux must be fixes, including the bindings to introduce a new compatible, then > ported to U-Boot. > > So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this > clarified. > > Neil Earlier I had a similar approach for this FIX but somehow it did not get merged. Please add my Tested-by: Anand Moon <linux.amoon@gmail.com> Best Regards -Anand
On 11/9/20 10:37 PM, Neil Armstrong wrote: > Hi, > > On 09/11/2020 04:12, Jaehoon Chung wrote: >> Core clock phase value is changed from 180' to 270'. >> It's more stable than before. >> - Odroidn-N2/C4 : Working fine with 52MHz >> - VIM3 : Working fine with 52MHz >> >> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >> --- >> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >> index 719dd1e5e570..7c60e0566560 100644 >> --- a/drivers/mmc/meson_gx_mmc.c >> +++ b/drivers/mmc/meson_gx_mmc.c >> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >> } >> clk_div = DIV_ROUND_UP(clk, mmc->clock); >> >> - /* 180 phase core clock */ >> - meson_mmc_clk |= CLK_CO_PHASE_180; >> - >> - /* 180 phase tx clock */ >> + /* >> + * Clock Phase needs to set a proper value. >> + * It can be changed to other value. >> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >> + * set to them by default. >> + */ >> + /* Core Clock Phase */ >> + meson_mmc_clk |= CLK_CO_PHASE_270; >> + >> + /* TX Clock Phase */ >> meson_mmc_clk |= CLK_TX_PHASE_000; >> >> /* clock settings */ >> > > The previous values were aligned on the Linux driver, which is functional. > > How did you test these ? Actually, i have tested about all cases on targets what i have. (VIM3/Odroid-N2/Odroid-C4) I also have VIM3L, but i didn't test on VIM3L. (I can test with VIM3L) If check with oscilloscope, it will be a good way to find what's wrong. When i have enabled MMC_DEBUG, it was always returned -5 (IO) error during switching mode. In meson_gx_mmc.c, meson_dm_mmc_send_cmd() is returned to -5. When i have checked status register, CRC error status bit (BIT[10]) is set. It means that clock timing is wrong. In my experiment, my debugging about CRC error is 1) GPIO setting 2) clock value 3) Driver strength 4) clock phase value I assume that 1~3) are correct. So checked PHASE values. I didn't check yet how to set value on Linux driver. Best Regards, Jaehoon Chung > > Neil >
Hi, On 11/9/20 11:10 PM, Mark Kettenis wrote: >> From: Neil Armstrong <narmstrong@baylibre.com> >> Date: Mon, 9 Nov 2020 14:37:09 +0100 >> >> Hi, >> >> On 09/11/2020 04:12, Jaehoon Chung wrote: >>> Core clock phase value is changed from 180' to 270'. >>> It's more stable than before. >>> - Odroidn-N2/C4 : Working fine with 52MHz >>> - VIM3 : Working fine with 52MHz >>> >>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>> >>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>> --- >>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>> index 719dd1e5e570..7c60e0566560 100644 >>> --- a/drivers/mmc/meson_gx_mmc.c >>> +++ b/drivers/mmc/meson_gx_mmc.c >>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>> } >>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>> >>> - /* 180 phase core clock */ >>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>> - >>> - /* 180 phase tx clock */ >>> + /* >>> + * Clock Phase needs to set a proper value. >>> + * It can be changed to other value. >>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>> + * set to them by default. >>> + */ >>> + /* Core Clock Phase */ >>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>> + >>> + /* TX Clock Phase */ >>> meson_mmc_clk |= CLK_TX_PHASE_000; >>> >>> /* clock settings */ >>> >> >> The previous values were aligned on the Linux driver, which is functional. > > Actually the Linux driver isn't really functional; the 52 MHz > high-speed mode doesn't work. But since HS200 does work in Linux, > probably nobody noticed this. Well, i didn't check Linux driver. but i can also check on Linux side. > > That said, I'm not confident a single clock phase setting will work > across all Amlogic SoCs and across different boards. Maybe we need > something in the device tree such that we can control the values on a > per-board level. Agreed. I can't mention that "it's working fine about all Amlogic SoCs". In exynos's case, there are sdr and ddr timing about mmc/sd IP. sdr/ddr timing are trying to get from dt's property, because it's possible that all Exynos SoCs have different values. I think that Amlogic SoCs also needs to apply similar approach. Best Regards, Jaehoon Chung >
On 11/9/20 11:23 PM, Neil Armstrong wrote: > On 09/11/2020 15:10, Mark Kettenis wrote: >>> From: Neil Armstrong <narmstrong@baylibre.com> >>> Date: Mon, 9 Nov 2020 14:37:09 +0100 >>> >>> Hi, >>> >>> On 09/11/2020 04:12, Jaehoon Chung wrote: >>>> Core clock phase value is changed from 180' to 270'. >>>> It's more stable than before. >>>> - Odroidn-N2/C4 : Working fine with 52MHz >>>> - VIM3 : Working fine with 52MHz >>>> >>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>>> >>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>>> --- >>>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>>> index 719dd1e5e570..7c60e0566560 100644 >>>> --- a/drivers/mmc/meson_gx_mmc.c >>>> +++ b/drivers/mmc/meson_gx_mmc.c >>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>>> } >>>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>>> >>>> - /* 180 phase core clock */ >>>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>>> - >>>> - /* 180 phase tx clock */ >>>> + /* >>>> + * Clock Phase needs to set a proper value. >>>> + * It can be changed to other value. >>>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>>> + * set to them by default. >>>> + */ >>>> + /* Core Clock Phase */ >>>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>>> + >>>> + /* TX Clock Phase */ >>>> meson_mmc_clk |= CLK_TX_PHASE_000; >>>> >>>> /* clock settings */ >>>> >>> >>> The previous values were aligned on the Linux driver, which is functional. >> >> Actually the Linux driver isn't really functional; the 52 MHz >> high-speed mode doesn't work. But since HS200 does work in Linux, >> probably nobody noticed this. >> >> That said, I'm not confident a single clock phase setting will work >> across all Amlogic SoCs and across different boards. Maybe we need >> something in the device tree such that we can control the values on a >> per-board level. >> > > So this is specific to SM1 SoCs then, because others families doesn't have such issues > at 52MHz. I don't have much knowledge of SM1 SoCs. But how did its phase value select them on Linux driver? > > So the Linux must be fixes, including the bindings to introduce a new compatible, then > ported to U-Boot. > > So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this > clarified. If you want to limit to 26MHz, I don't have any objection about your opinion. But I wonder how to clarify all. And I also wonder that values what is used on Linux kernel are really right. Best Regards, Jaehoon Chung > > Neil >
Dear Anand, On 11/10/20 4:02 AM, Anand Moon wrote: > Hi Neil, > > On Mon, 9 Nov 2020 at 19:56, Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> On 09/11/2020 15:10, Mark Kettenis wrote: >>>> From: Neil Armstrong <narmstrong@baylibre.com> >>>> Date: Mon, 9 Nov 2020 14:37:09 +0100 >>>> >>>> Hi, >>>> >>>> On 09/11/2020 04:12, Jaehoon Chung wrote: >>>>> Core clock phase value is changed from 180' to 270'. >>>>> It's more stable than before. >>>>> - Odroidn-N2/C4 : Working fine with 52MHz >>>>> - VIM3 : Working fine with 52MHz >>>>> >>>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>>>> >>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>>>> --- >>>>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>>>> index 719dd1e5e570..7c60e0566560 100644 >>>>> --- a/drivers/mmc/meson_gx_mmc.c >>>>> +++ b/drivers/mmc/meson_gx_mmc.c >>>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>>>> } >>>>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>>>> >>>>> - /* 180 phase core clock */ >>>>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>>>> - >>>>> - /* 180 phase tx clock */ >>>>> + /* >>>>> + * Clock Phase needs to set a proper value. >>>>> + * It can be changed to other value. >>>>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>>>> + * set to them by default. >>>>> + */ >>>>> + /* Core Clock Phase */ >>>>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>>>> + >>>>> + /* TX Clock Phase */ >>>>> meson_mmc_clk |= CLK_TX_PHASE_000; >>>>> >>>>> /* clock settings */ >>>>> >>>> >>>> The previous values were aligned on the Linux driver, which is functional. >>> >>> Actually the Linux driver isn't really functional; the 52 MHz >>> high-speed mode doesn't work. But since HS200 does work in Linux, >>> probably nobody noticed this. >>> >>> That said, I'm not confident a single clock phase setting will work >>> across all Amlogic SoCs and across different boards. Maybe we need >>> something in the device tree such that we can control the values on a >>> per-board level. >>> >> >> So this is specific to SM1 SoCs then, because others families doesn't have such issues >> at 52MHz. >> >> So the Linux must be fixes, including the bindings to introduce a new compatible, then >> ported to U-Boot. >> >> So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this >> clarified. >> >> Neil > > Earlier I had a similar approach for this FIX but somehow it did not get merged. > > Please add my > Tested-by: Anand Moon <linux.amoon@gmail.com> Thanks for testing! Best Regards, Jaehoon Chung > > Best Regards > -Anand >
On 09/11/2020 23:19, Jaehoon Chung wrote: > On 11/9/20 11:23 PM, Neil Armstrong wrote: >> On 09/11/2020 15:10, Mark Kettenis wrote: >>>> From: Neil Armstrong <narmstrong@baylibre.com> >>>> Date: Mon, 9 Nov 2020 14:37:09 +0100 >>>> >>>> Hi, >>>> >>>> On 09/11/2020 04:12, Jaehoon Chung wrote: >>>>> Core clock phase value is changed from 180' to 270'. >>>>> It's more stable than before. >>>>> - Odroidn-N2/C4 : Working fine with 52MHz >>>>> - VIM3 : Working fine with 52MHz >>>>> >>>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>>>> >>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>>>> --- >>>>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>>>> index 719dd1e5e570..7c60e0566560 100644 >>>>> --- a/drivers/mmc/meson_gx_mmc.c >>>>> +++ b/drivers/mmc/meson_gx_mmc.c >>>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>>>> } >>>>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>>>> >>>>> - /* 180 phase core clock */ >>>>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>>>> - >>>>> - /* 180 phase tx clock */ >>>>> + /* >>>>> + * Clock Phase needs to set a proper value. >>>>> + * It can be changed to other value. >>>>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>>>> + * set to them by default. >>>>> + */ >>>>> + /* Core Clock Phase */ >>>>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>>>> + >>>>> + /* TX Clock Phase */ >>>>> meson_mmc_clk |= CLK_TX_PHASE_000; >>>>> >>>>> /* clock settings */ >>>>> >>>> >>>> The previous values were aligned on the Linux driver, which is functional. >>> >>> Actually the Linux driver isn't really functional; the 52 MHz >>> high-speed mode doesn't work. But since HS200 does work in Linux, >>> probably nobody noticed this. >>> >>> That said, I'm not confident a single clock phase setting will work >>> across all Amlogic SoCs and across different boards. Maybe we need >>> something in the device tree such that we can control the values on a >>> per-board level. >>> >> >> So this is specific to SM1 SoCs then, because others families doesn't have such issues >> at 52MHz. > > I don't have much knowledge of SM1 SoCs. But how did its phase value select them on Linux driver? > >> >> So the Linux must be fixes, including the bindings to introduce a new compatible, then >> ported to U-Boot. >> >> So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this >> clarified. > If you want to limit to 26MHz, I don't have any objection about your opinion. > But I wonder how to clarify all. And I also wonder that values what is used on Linux kernel are really right. OK, I'm not a MMC & SDCard expert on Amlogic, all the work was done in Linux. I just want to keep a working and stable SDCard & eMMC support for the 7 SoCs Families. Could you rewrite my "[PATCH 2/3] mmc: meson-gx: limit max frequency on SM1 SoCs" by instead changing the clock phase only for SM1 ? I'll then apply it with the 2 other patches and push then for the current development cycle. The driver is functional for all the other SoCs, and I want to keep it stable unless extensively tested. Neil > > Best Regards, > Jaehoon Chung > >> >> Neil >> >
On 11/10/20 5:05 PM, Neil Armstrong wrote: > On 09/11/2020 23:19, Jaehoon Chung wrote: >> On 11/9/20 11:23 PM, Neil Armstrong wrote: >>> On 09/11/2020 15:10, Mark Kettenis wrote: >>>>> From: Neil Armstrong <narmstrong@baylibre.com> >>>>> Date: Mon, 9 Nov 2020 14:37:09 +0100 >>>>> >>>>> Hi, >>>>> >>>>> On 09/11/2020 04:12, Jaehoon Chung wrote: >>>>>> Core clock phase value is changed from 180' to 270'. >>>>>> It's more stable than before. >>>>>> - Odroidn-N2/C4 : Working fine with 52MHz >>>>>> - VIM3 : Working fine with 52MHz >>>>>> >>>>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>>>>> >>>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>>>>> --- >>>>>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>>>>> index 719dd1e5e570..7c60e0566560 100644 >>>>>> --- a/drivers/mmc/meson_gx_mmc.c >>>>>> +++ b/drivers/mmc/meson_gx_mmc.c >>>>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>>>>> } >>>>>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>>>>> >>>>>> - /* 180 phase core clock */ >>>>>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>>>>> - >>>>>> - /* 180 phase tx clock */ >>>>>> + /* >>>>>> + * Clock Phase needs to set a proper value. >>>>>> + * It can be changed to other value. >>>>>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>>>>> + * set to them by default. >>>>>> + */ >>>>>> + /* Core Clock Phase */ >>>>>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>>>>> + >>>>>> + /* TX Clock Phase */ >>>>>> meson_mmc_clk |= CLK_TX_PHASE_000; >>>>>> >>>>>> /* clock settings */ >>>>>> >>>>> >>>>> The previous values were aligned on the Linux driver, which is functional. >>>> >>>> Actually the Linux driver isn't really functional; the 52 MHz >>>> high-speed mode doesn't work. But since HS200 does work in Linux, >>>> probably nobody noticed this. >>>> >>>> That said, I'm not confident a single clock phase setting will work >>>> across all Amlogic SoCs and across different boards. Maybe we need >>>> something in the device tree such that we can control the values on a >>>> per-board level. >>>> >>> >>> So this is specific to SM1 SoCs then, because others families doesn't have such issues >>> at 52MHz. >> >> I don't have much knowledge of SM1 SoCs. But how did its phase value select them on Linux driver? >> >>> >>> So the Linux must be fixes, including the bindings to introduce a new compatible, then >>> ported to U-Boot. >>> >>> So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this >>> clarified. >> If you want to limit to 26MHz, I don't have any objection about your opinion. >> But I wonder how to clarify all. And I also wonder that values what is used on Linux kernel are really right. > > OK, I'm not a MMC & SDCard expert on Amlogic, all the work was done in Linux. > > I just want to keep a working and stable SDCard & eMMC support for the 7 SoCs Families. > > Could you rewrite my "[PATCH 2/3] mmc: meson-gx: limit max frequency on SM1 SoCs" by instead changing the > clock phase only for SM1 ? > I'll then apply it with the 2 other patches and push then for the current development cycle. Ok. I will rewrite with you patch. Is it ok that i send the patch on Tomorrow? > > The driver is functional for all the other SoCs, and I want to keep it stable unless extensively tested. Agreed. If i can test all Amlogic SoCs, I want to do that..But it's impossible to me. In future, i hope that it will be fixed with generic approach. Best Regards, Jaehoon Chung > > Neil > >> >> Best Regards, >> Jaehoon Chung >> >>> >>> Neil >>> >> > >
On 10/11/2020 09:36, Jaehoon Chung wrote: > On 11/10/20 5:05 PM, Neil Armstrong wrote: >> On 09/11/2020 23:19, Jaehoon Chung wrote: >>> On 11/9/20 11:23 PM, Neil Armstrong wrote: >>>> On 09/11/2020 15:10, Mark Kettenis wrote: >>>>>> From: Neil Armstrong <narmstrong@baylibre.com> >>>>>> Date: Mon, 9 Nov 2020 14:37:09 +0100 >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 09/11/2020 04:12, Jaehoon Chung wrote: >>>>>>> Core clock phase value is changed from 180' to 270'. >>>>>>> It's more stable than before. >>>>>>> - Odroidn-N2/C4 : Working fine with 52MHz >>>>>>> - VIM3 : Working fine with 52MHz >>>>>>> >>>>>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz. >>>>>>> >>>>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>>>>>> --- >>>>>>> drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- >>>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>>>>>> index 719dd1e5e570..7c60e0566560 100644 >>>>>>> --- a/drivers/mmc/meson_gx_mmc.c >>>>>>> +++ b/drivers/mmc/meson_gx_mmc.c >>>>>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>>>>>> } >>>>>>> clk_div = DIV_ROUND_UP(clk, mmc->clock); >>>>>>> >>>>>>> - /* 180 phase core clock */ >>>>>>> - meson_mmc_clk |= CLK_CO_PHASE_180; >>>>>>> - >>>>>>> - /* 180 phase tx clock */ >>>>>>> + /* >>>>>>> + * Clock Phase needs to set a proper value. >>>>>>> + * It can be changed to other value. >>>>>>> + * Because CORE : 270' Phase and TX : 0' Phase are stable, >>>>>>> + * set to them by default. >>>>>>> + */ >>>>>>> + /* Core Clock Phase */ >>>>>>> + meson_mmc_clk |= CLK_CO_PHASE_270; >>>>>>> + >>>>>>> + /* TX Clock Phase */ >>>>>>> meson_mmc_clk |= CLK_TX_PHASE_000; >>>>>>> >>>>>>> /* clock settings */ >>>>>>> >>>>>> >>>>>> The previous values were aligned on the Linux driver, which is functional. >>>>> >>>>> Actually the Linux driver isn't really functional; the 52 MHz >>>>> high-speed mode doesn't work. But since HS200 does work in Linux, >>>>> probably nobody noticed this. >>>>> >>>>> That said, I'm not confident a single clock phase setting will work >>>>> across all Amlogic SoCs and across different boards. Maybe we need >>>>> something in the device tree such that we can control the values on a >>>>> per-board level. >>>>> >>>> >>>> So this is specific to SM1 SoCs then, because others families doesn't have such issues >>>> at 52MHz. >>> >>> I don't have much knowledge of SM1 SoCs. But how did its phase value select them on Linux driver? >>> >>>> >>>> So the Linux must be fixes, including the bindings to introduce a new compatible, then >>>> ported to U-Boot. >>>> >>>> So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this >>>> clarified. >>> If you want to limit to 26MHz, I don't have any objection about your opinion. >>> But I wonder how to clarify all. And I also wonder that values what is used on Linux kernel are really right. >> >> OK, I'm not a MMC & SDCard expert on Amlogic, all the work was done in Linux. >> >> I just want to keep a working and stable SDCard & eMMC support for the 7 SoCs Families. >> >> Could you rewrite my "[PATCH 2/3] mmc: meson-gx: limit max frequency on SM1 SoCs" by instead changing the >> clock phase only for SM1 ? >> I'll then apply it with the 2 other patches and push then for the current development cycle. > > Ok. I will rewrite with you patch. Is it ok that i send the patch on Tomorrow? Agreed > >> >> The driver is functional for all the other SoCs, and I want to keep it stable unless extensively tested. > > Agreed. If i can test all Amlogic SoCs, I want to do that..But it's impossible to me. > In future, i hope that it will be fixed with generic approach. Sure, thanks for your work & testing ! Neil > > Best Regards, > Jaehoon Chung > >> >> Neil >> >>> >>> Best Regards, >>> Jaehoon Chung >>> >>>> >>>> Neil >>>> >>> >> >> >
diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c index 719dd1e5e570..7c60e0566560 100644 --- a/drivers/mmc/meson_gx_mmc.c +++ b/drivers/mmc/meson_gx_mmc.c @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc) } clk_div = DIV_ROUND_UP(clk, mmc->clock); - /* 180 phase core clock */ - meson_mmc_clk |= CLK_CO_PHASE_180; - - /* 180 phase tx clock */ + /* + * Clock Phase needs to set a proper value. + * It can be changed to other value. + * Because CORE : 270' Phase and TX : 0' Phase are stable, + * set to them by default. + */ + /* Core Clock Phase */ + meson_mmc_clk |= CLK_CO_PHASE_270; + + /* TX Clock Phase */ meson_mmc_clk |= CLK_TX_PHASE_000; /* clock settings */
Core clock phase value is changed from 180' to 270'. It's more stable than before. - Odroidn-N2/C4 : Working fine with 52MHz - VIM3 : Working fine with 52MHz Before this patch, Odroid-C4 doesn't work fine with 52MHz. Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> --- drivers/mmc/meson_gx_mmc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)