[PATCHv6 1/5] mmc: meson-gx: Fix clk phase tuning for MMC
Anand Moon
linux.amoon at gmail.com
Thu Feb 13 17:41:12 CET 2020
Hi Neil,
On Thu, 13 Feb 2020 at 21:09, Neil Armstrong <narmstrong at baylibre.com> wrote:
>
> On 13/02/2020 12:58, Anand Moon wrote:
> > hi Niel / Jerome,
> >
> > Thanks for your review comments and debug output
> > Sorry for late reply.
> >
> > On Mon, 10 Feb 2020 at 14:33, Neil Armstrong <narmstrong at baylibre.com> wrote:
> >>
> >> On 09/02/2020 18:22, Anand Moon wrote:
> >>> Hi Neil,
> >>>
> >>> Thanks for you review comments.
> >>>
> >>> On Sun, 9 Feb 2020 at 18:38, Neil Armstrong <narmstrong at baylibre.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> Le 09/02/2020 à 12:05, Anand Moon a écrit :
> >>>>> As per mainline line kernel fix the clk tuning phase for mmc,
> >>>>> set Core=180, Tx=0, Rx=0 clk phase for mmc initialization.
> >>>>> As per S905, S905X, AGX and S922X datasheet set the default
> >>>>> values for clk tuning.
> >>>>>
> >>>>> Signed-off-by: Anand Moon <linux.amoon at gmail.com>
> >>>>> ---
> >>>>> Changes from previous
> >>>>> v5 Fix the commit message, configure as per mainline kernel.
> >>>>> drop the RX_DELAY_MASK and TX_DELAY_MASK as they are not used.
> >>>>>
> >>>>> v4 Fix the update mask value using FIELD_PREP macro.
> >>>>>
> >>>>> v3 Fix the initialization of core clk tunning phase as per datasheet.
> >>>>> Fix the commit message.
> >>>>>
> >>>>> v2: Fix the clk phase macro to support PHASE_180
> >>>>> drop the wrong CLK_CORE_PHASE_MASK macro.
> >>>>>
> >>>>> v1: use the mainline kernel tuning for clk tuning.
> >>>>>
> >>>>> Fixed the commmit messages.
> >>>>> Patch v1:
> >>>>> https://patchwork.ozlabs.org/patch/1201208/
> >>>>>
> >>>>> Before these changes.
> >>>>> clock is enabled (380953Hz)
> >>>>> clock is enabled (25000000Hz)
> >>>>> After these changes
> >>>>> clock is enabled (380953Hz)
> >>>>> clock is enabled (25000000Hz)
> >>>>> clock is enabled (52000000Hz)
> >>>>> Test on Odroid N2 and Odroid C2 with eMMC and microSD cards
> >>>>> ---
> >>>>> arch/arm/include/asm/arch-meson/sd_emmc.h | 24 +++++++++++--------
> >>>>> drivers/mmc/meson_gx_mmc.c | 28 +++++++++++++++++++----
> >>>>> 2 files changed, 38 insertions(+), 14 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h
> >>>>> index e3a72c8b66..f4299485dc 100644
> >>>>> --- a/arch/arm/include/asm/arch-meson/sd_emmc.h
> >>>>> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h
> >>>>> @@ -7,6 +7,7 @@
> >>>>> #define __SD_EMMC_H__
> >>>>>
> >>>>> #include <mmc.h>
> >>>>> +#include <linux/bitops.h>
> >>>>>
> >>>>> #define SDIO_PORT_A 0
> >>>>> #define SDIO_PORT_B 1
> >>>>> @@ -19,15 +20,20 @@
> >>>>> #define CLK_MAX_DIV 63
> >>>>> #define CLK_SRC_24M (0 << 6)
> >>>>> #define CLK_SRC_DIV2 (1 << 6)
> >>>>> -#define CLK_CO_PHASE_000 (0 << 8)
> >>>>> -#define CLK_CO_PHASE_090 (1 << 8)
> >>>>> -#define CLK_CO_PHASE_180 (2 << 8)
> >>>>> -#define CLK_CO_PHASE_270 (3 << 8)
> >>>>> -#define CLK_TX_PHASE_000 (0 << 10)
> >>>>> -#define CLK_TX_PHASE_090 (1 << 10)
> >>>>> -#define CLK_TX_PHASE_180 (2 << 10)
> >>>>> -#define CLK_TX_PHASE_270 (3 << 10)
> >>>>> -#define CLK_ALWAYS_ON BIT(24)
> >>>>> +
> >>>>> +#define CRYSTAL_24MHZ 0
> >>>>> +#define CLK_PHASE_0 0
> >>>>> +#define CLK_PHASE_180 2
> >>>>> +
> >>>>> +#define CLK_DIV_MASK GENMASK(5, 0)
> >>>>> +#define CLK_SRC_MASK GENMASK(7, 6)
> >>>>> +#define CLK_CORE_PHASE_MASK GENMASK(9, 8)
> >>>>> +#define CLK_TX_PHASE_MASK GENMASK(11, 10)
> >>>>> +#define CLK_RX_PHASE_MASK GENMASK(13, 12)
> >>>>> +
> >>>>> +#define CLK_V2_ALWAYS_ON BIT(24)
> >>>>> +
> >>>>> +#define CLK_V3_ALWAYS_ON BIT(28)
> >>>>>
> >>>>> #define MESON_SD_EMMC_CFG 0x44
> >>>>> #define CFG_BUS_WIDTH_MASK GENMASK(1, 0)
> >>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c
> >>>>> index 86c1a7164a..b013c7c5fb 100644
> >>>>> --- a/drivers/mmc/meson_gx_mmc.c
> >>>>> +++ b/drivers/mmc/meson_gx_mmc.c
> >>>>> @@ -16,6 +16,10 @@
> >>>>> #include <asm/arch/sd_emmc.h>
> >>>>> #include <linux/log2.h>
> >>>>>
> >>>>> +#include <linux/bitops.h>
> >>>>> +#include <linux/compat.h>
> >>>>> +#include <linux/bitfield.h>
> >>>>> +
> >>>>> static inline void *get_regbase(const struct mmc *mmc)
> >>>>> {
> >>>>> struct meson_mmc_platdata *pdata = mmc->priv;
> >>>>> @@ -51,11 +55,25 @@ 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 */
> >>>>> - meson_mmc_clk |= CLK_TX_PHASE_000;
> >>>>> + /* Clock divider */
> >>>>> + meson_mmc_clk |= CLK_DIV_MASK;
> >>>>
> >>>> This will set the max divider, whatever the value of clk_div, so the following statement:
> >>>> meson_mmc_clk |= clk_div;
> >>>> will have no effect.
> >>>
> >>> As per the datasheet S905 and S922X max divider is 63.
> >>> CLK_DIV_MASK[0-5] Cfg_div: Clock divider
> >>> Frequency = clock source/cfg_div
> >>> Clock off: cfg_div==0, the
> >>> clock is disabled
> >>> Divider bypass: cfg_div==1,
> >>> clock source is used as core clock without divider
> >>> Maximum divider 63
> >>>
> >>> So here is the log of clk_div and clk_freq at my end.
> >>>
> >>> MMC Device 0 not found
> >>> no mmc device at slot 0
> >>> clock is disabled (0Hz)
> >>> clock is enabled (380953Hz)
> >>> ......clk_div : 63
> >>> Card did not respond to voltage select!
> >>> clock is disabled (0Hz)
> >>> clock is enabled (380953Hz)
> >>> ......clk_div : 63
> >>> clock is enabled (25000000Hz)
> >>> ......clk_div : 40
> >>> ......clk_div : 40
> >>> clock is enabled (52000000Hz)
> >>> ......clk_div : 20
> >>> switch to partitions #0, OK
> >>> mmc2(part 0) is current device
> >>> Scanning mmc 2:1...
> >>> Found U-Boot script /boot/boot.scr
> >>
> >>
> >> OK, seems you didn't see the issue.
> >>
> >> With the original code, let's say mmc->clock = 25000000
> >>
> >> clk = SD_EMMC_CLKSRC_DIV2
> >> clk_src = CLK_SRC_DIV2
> >> clk_div = 40
> >>
> >> meson_mmc_clk |= CLK_CO_PHASE_180;
> >> meson_mmc_clk |= CLK_TX_PHASE_000;
> >> meson_mmc_clk |= CLK_SRC_DIV2;
> >> meson_mmc_clk |= 40;
> >>
> >> => meson_mmc_clk = (2 << 8) | (0 << 10) | (1 << 6) | 40
> >> => meson_mmc_clk = 0x268
> >>
> >> With your code :
> >> meson_mmc_clk |= CLK_DIV_MASK;
> >> meson_mmc_clk |= FIELD_PREP(CLK_SRC_MASK, CRYSTAL_24MHZ);
> >> meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
> >> meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
> >> meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
> >> meson_mmc_clk |= CLK_SRC_DIV2;
> >> meson_mmc_clk |= 40;
> >>
> >> => meson_mmc_clk = 0x3f | (0 << 6) | (2 << 8) | (0 << 11) | (0 << 13) | (1 << 6) | 40
> >>
> >> -------------------/\---this---------makes-------this-----useless------------------/\
> >>
> >> => 0x27f
> >>
> >> It sets the clock to 15873015Hz instead of the 25000000Hz requested.
> >
> > I did not observer this at my end, but any way thanks for the input.
> >
> >>
> >> so this code sets max divider whatever mmc->clock value, but keeps the CRYSTAL_24MHZ/CLK_SRC_DIV2 selection.
> >>
> >>>
> >>>>
> >>>>
> >>>>> + /* Clock source : Crystal 24MHz */
> >>>>> + meson_mmc_clk |= FIELD_PREP(CLK_SRC_MASK, CRYSTAL_24MHZ);
> >>>>
> >>>> You set CRYSTAL_24MHZ here, but the src is selected in clk_src and set with:
> >>>> meson_mmc_clk |= clk_src;
> >>>>
> >>>> In conclusion your change forces the 24MHz crystal and max divider whatever the
> >>>> freq asked by the mmc core !
> >>>>
> >>> As per the datasheet S905 and S922X
> >>> Cfg_src: Clock source
> >>> 0: Crystal 24MHz or other frequencies
> >>> selected by clock reset test control register.
> >>> 1: Fix PLL, 1000MHz
> >>> Recommended value: 1
> >>>
> >>> Note: *setting cfg_src value to I i.e; Fix PLL 1000 Mhz *
> >>> some how break the clk_freq tuning setting in my testing,
> >>> see the logs below.
> >>>
> >>> MMC Device 0 not found
> >>> no mmc device at slot 0
> >>> clock is disabled (0Hz)
> >>> clock is enabled (380953Hz)
> >>> ......clk_div : 63
> >>> Card did not respond to voltage select!
> >>> clock is disabled (0Hz)
> >>> clock is enabled (380953Hz)
> >>> ......clk_div : 63
> >>> starting USB...
> >>>
> >>> No emmc will get discovered,
> >>> *This is the real issue I faced for failure of eMMC not getting detected.*
> >>
> >> This is the issue I get on SM1, but when I cap the max freq to 24MHz, it disappears.
> >>
> >> This issue is somewhere else, but I don't know where because the Linux driver behaves
> >> correctly with the same setup.
> >>
> >> My fix was to always use the 24MHz crystal input on non-GX until we find out why, and it still acceptable.
> >
> > I have studied amlogic u-boot and it seen to use 24MHz crystal inputs.
> > I have following fix the issue with select Fix PLL and CRYSTAL24MHz
> > if (mmc->clock > 400000) {
> > /* Clock source : Fix PLL, 1000MHz */
> > meson_mmc_clk |= FIELD_PREP(CLK_SRC_MASK, FIXPLL_10000MHZ);
> > } else {
> > /* Clock source : Crystal 24MHz */
> > meson_mmc_clk |= FIELD_PREP(CLK_SRC_MASK, CRYSTAL_24MHZ);
> > }
>
> This is still wrong and broken. The original code is correct and works fine.
>
> As Jerome said, the underlying issue must be understood, why when using the FIXPLL clock source, it fails ?
>
> The clock setup function is correct, don't try to fix it.
>
> The weirdness is why does it works under linux ? maybe the fixpll clock setup is different ?
>
> Could you dump the emmc and clock registers from linux to see the difference ?
>
> Neil
>
Ok thanks for this Input. I will try to figure our how it's working in
linux kernel first.
-Anand
> >>
> >> Could you post instead :
> >> - your patch 2 introducing the MMC_COMPATIBLE_* as patch 1
> >
> > Ok I will change the order.
> >
> >> - this patch but only forcing 24MHz for MMC_COMPATIBLE_AXG as patch 2
> >
> > I propose the dynamically select 24MHz and Fix PLL, as above code.
> >
> >> - redude patches 3,4 & 5 to only add mmc* aliases into meson-gx-u-boot.dtsi/meson-g12-common-u-boot.dtsi
> >>
> >
> > Ok I will make these changes
> >
> >> If you have a issue, please chat with me on #linux-amlogic on freenode.
> >
> > I could not connect to freenode or register my self to this chat
> > group. sorry about that.
> >
> >>
> >> Thanks,
> >> Neil
> >>
> >
> > -Anand
> >
>
More information about the U-Boot
mailing list