| Message ID | 20200209110557.1996-3-linux.amoon@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Odroid n2 using eMMC would fail to boot up | expand |
Hi, Le 09/02/2020 à 12:05, Anand Moon a écrit : > Use proper compatible string as per the dts so that mmc driver > could be tuned properly. SoC family S905, S905X have common clk > tuning parameters setting, while AGX and G12 have common clk tuning > parameters setting for mmc driver. > > Suggested-by: Neil Armstrong <narmstrong at baylibre.com> > Signed-off-by: Anand Moon <linux.amoon at gmail.com> > --- > No changes. > --- > arch/arm/include/asm/arch-meson/sd_emmc.h | 7 ++++ > drivers/mmc/meson_gx_mmc.c | 46 +++++++++++++++++------ > 2 files changed, 41 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h > index f4299485dc..83142d5d3f 100644 > --- a/arch/arm/include/asm/arch-meson/sd_emmc.h > +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h > @@ -13,6 +13,12 @@ > #define SDIO_PORT_B 1 > #define SDIO_PORT_C 2 > > +enum mmc_compatible { > + MMC_COMPATIBLE_GXBB, > + MMC_COMPATIBLE_GX, > + MMC_COMPATIBLE_AXG, > +}; > + > #define SD_EMMC_CLKSRC_24M 24000000 /* 24 MHz */ > #define SD_EMMC_CLKSRC_DIV2 1000000000 /* 1 GHz */ > > @@ -87,6 +93,7 @@ > struct meson_mmc_platdata { > struct mmc_config cfg; > struct mmc mmc; > + int compat; > void *regbase; > void *w_buf; > }; > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > index b013c7c5fb..1aefe360c4 100644 > --- a/drivers/mmc/meson_gx_mmc.c > +++ b/drivers/mmc/meson_gx_mmc.c > @@ -37,7 +37,8 @@ static inline void meson_write(struct mmc *mmc, uint32_t val, int offset) > writel(val, get_regbase(mmc) + offset); > } > > -static void meson_mmc_config_clock(struct mmc *mmc) > +static void meson_mmc_config_clock(struct mmc *mmc, > + struct meson_mmc_platdata *pdata) > { > uint32_t meson_mmc_clk = 0; > unsigned int clk, clk_src, clk_div; > @@ -66,14 +67,20 @@ static void meson_mmc_config_clock(struct mmc *mmc) > /* RX clock phase 0:180 */ > meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); > > -#ifdef CONFIG_MESON_GX > - /* clk always on */ > - meson_mmc_clk |= CLK_V2_ALWAYS_ON; > -#endif > -#if (defined(CONFIG_MESON_AXG) || defined(CONFIG_MESON_G12A)) > - /* clk always on */ > - meson_mmc_clk |= CLK_V3_ALWAYS_ON; > -#endif > + switch (pdata->compat) { > + case MMC_COMPATIBLE_GXBB: > + case MMC_COMPATIBLE_GX: > + /* clk always on */ > + meson_mmc_clk |= CLK_V2_ALWAYS_ON; > + break; > + case MMC_COMPATIBLE_AXG: > + /* clk always on */ > + meson_mmc_clk |= CLK_V3_ALWAYS_ON; > + break; > + default: > + debug("no compatible supported"); > + break; > + } > > /* clock settings */ > meson_mmc_clk |= clk_src; > @@ -85,9 +92,11 @@ static void meson_mmc_config_clock(struct mmc *mmc) > static int meson_dm_mmc_set_ios(struct udevice *dev) > { > struct mmc *mmc = mmc_get_mmc_dev(dev); > + struct meson_mmc_platdata *pdata = > + (struct meson_mmc_platdata *)dev_get_driver_data(dev); > uint32_t meson_mmc_cfg; > > - meson_mmc_config_clock(mmc); > + meson_mmc_config_clock(mmc, pdata); > > meson_mmc_cfg = meson_read(mmc, MESON_SD_EMMC_CFG); > > @@ -324,9 +333,22 @@ int meson_mmc_bind(struct udevice *dev) > return mmc_bind(dev, &pdata->mmc, &pdata->cfg); > } > > +static const struct meson_mmc_platdata gxbb_data = { > + .compat = MMC_COMPATIBLE_GXBB, > +}; > + > +static const struct meson_mmc_platdata gx_data = { > + .compat = MMC_COMPATIBLE_GX, > +}; > + > +static const struct meson_mmc_platdata axg_data = { > + .compat = MMC_COMPATIBLE_AXG, > +}; > + > static const struct udevice_id meson_mmc_match[] = { > - { .compatible = "amlogic,meson-gx-mmc" }, > - { .compatible = "amlogic,meson-axg-mmc" }, > + { .compatible = "amlogic,meson-gxbb-mmc", .data = (ulong)&gxbb_data }, > + { .compatible = "amlogic,meson-gx-mmc", .data = (ulong)&gx_data }, > + { .compatible = "amlogic,meson-axg-mmc", .data = (ulong)&axg_data }, > { /* sentinel */ } > }; > > It's fine but you should do that before patch 1, and introduce the clk setup directly with the MMC_COMPATIBLE_*. If you move it before, then: Reviewed-by: Neil Armstrong <narmstrong at baylibre.com> Neil
Hi Neil Thanks for your review comments. On Sun, 9 Feb 2020 at 18:31, Neil Armstrong <narmstrong at baylibre.com> wrote: > > Hi, > > Le 09/02/2020 à 12:05, Anand Moon a écrit : > > Use proper compatible string as per the dts so that mmc driver > > could be tuned properly. SoC family S905, S905X have common clk > > tuning parameters setting, while AGX and G12 have common clk tuning > > parameters setting for mmc driver. > > > > Suggested-by: Neil Armstrong <narmstrong at baylibre.com> > > Signed-off-by: Anand Moon <linux.amoon at gmail.com> > > --- > > No changes. > > --- > > arch/arm/include/asm/arch-meson/sd_emmc.h | 7 ++++ > > drivers/mmc/meson_gx_mmc.c | 46 +++++++++++++++++------ > > 2 files changed, 41 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h > > index f4299485dc..83142d5d3f 100644 > > --- a/arch/arm/include/asm/arch-meson/sd_emmc.h > > +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h > > @@ -13,6 +13,12 @@ > > #define SDIO_PORT_B 1 > > #define SDIO_PORT_C 2 > > > > +enum mmc_compatible { > > + MMC_COMPATIBLE_GXBB, > > + MMC_COMPATIBLE_GX, > > + MMC_COMPATIBLE_AXG, > > +}; > > + > > #define SD_EMMC_CLKSRC_24M 24000000 /* 24 MHz */ > > #define SD_EMMC_CLKSRC_DIV2 1000000000 /* 1 GHz */ > > > > @@ -87,6 +93,7 @@ > > struct meson_mmc_platdata { > > struct mmc_config cfg; > > struct mmc mmc; > > + int compat; > > void *regbase; > > void *w_buf; > > }; > > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > > index b013c7c5fb..1aefe360c4 100644 > > --- a/drivers/mmc/meson_gx_mmc.c > > +++ b/drivers/mmc/meson_gx_mmc.c > > @@ -37,7 +37,8 @@ static inline void meson_write(struct mmc *mmc, uint32_t val, int offset) > > writel(val, get_regbase(mmc) + offset); > > } > > > > -static void meson_mmc_config_clock(struct mmc *mmc) > > +static void meson_mmc_config_clock(struct mmc *mmc, > > + struct meson_mmc_platdata *pdata) > > { > > uint32_t meson_mmc_clk = 0; > > unsigned int clk, clk_src, clk_div; > > @@ -66,14 +67,20 @@ static void meson_mmc_config_clock(struct mmc *mmc) > > /* RX clock phase 0:180 */ > > meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); > > > > -#ifdef CONFIG_MESON_GX > > - /* clk always on */ > > - meson_mmc_clk |= CLK_V2_ALWAYS_ON; > > -#endif > > -#if (defined(CONFIG_MESON_AXG) || defined(CONFIG_MESON_G12A)) > > - /* clk always on */ > > - meson_mmc_clk |= CLK_V3_ALWAYS_ON; > > -#endif > > + switch (pdata->compat) { > > + case MMC_COMPATIBLE_GXBB: > > + case MMC_COMPATIBLE_GX: > > + /* clk always on */ > > + meson_mmc_clk |= CLK_V2_ALWAYS_ON; > > + break; > > + case MMC_COMPATIBLE_AXG: > > + /* clk always on */ > > + meson_mmc_clk |= CLK_V3_ALWAYS_ON; > > + break; > > + default: > > + debug("no compatible supported"); > > + break; > > + } > > > > /* clock settings */ > > meson_mmc_clk |= clk_src; > > @@ -85,9 +92,11 @@ static void meson_mmc_config_clock(struct mmc *mmc) > > static int meson_dm_mmc_set_ios(struct udevice *dev) > > { > > struct mmc *mmc = mmc_get_mmc_dev(dev); > > + struct meson_mmc_platdata *pdata = > > + (struct meson_mmc_platdata *)dev_get_driver_data(dev); > > uint32_t meson_mmc_cfg; > > > > - meson_mmc_config_clock(mmc); > > + meson_mmc_config_clock(mmc, pdata); > > > > meson_mmc_cfg = meson_read(mmc, MESON_SD_EMMC_CFG); > > > > @@ -324,9 +333,22 @@ int meson_mmc_bind(struct udevice *dev) > > return mmc_bind(dev, &pdata->mmc, &pdata->cfg); > > } > > > > +static const struct meson_mmc_platdata gxbb_data = { > > + .compat = MMC_COMPATIBLE_GXBB, > > +}; > > + > > +static const struct meson_mmc_platdata gx_data = { > > + .compat = MMC_COMPATIBLE_GX, > > +}; > > + > > +static const struct meson_mmc_platdata axg_data = { > > + .compat = MMC_COMPATIBLE_AXG, > > +}; > > + > > static const struct udevice_id meson_mmc_match[] = { > > - { .compatible = "amlogic,meson-gx-mmc" }, > > - { .compatible = "amlogic,meson-axg-mmc" }, > > + { .compatible = "amlogic,meson-gxbb-mmc", .data = (ulong)&gxbb_data }, > > + { .compatible = "amlogic,meson-gx-mmc", .data = (ulong)&gx_data }, > > + { .compatible = "amlogic,meson-axg-mmc", .data = (ulong)&axg_data }, > > { /* sentinel */ } > > }; > > > > > > > It's fine but you should do that before patch 1, and introduce the clk setup directly with the MMC_COMPATIBLE_*. > Only GXBB and GLX have CLK_ALWAYS_ON(24) bit set and AXG and G12X have CLK_ALWAYS_ON(28) bit set for clk enable rest of the configuration is all most common for all the SoC. let keep this simple as of now. > If you move it before, then: > > Reviewed-by: Neil Armstrong <narmstrong at baylibre.com> > > Neil -Anand
On 09/02/2020 18:23, Anand Moon wrote: > Hi Neil > > Thanks for your review comments. > > On Sun, 9 Feb 2020 at 18:31, Neil Armstrong <narmstrong at baylibre.com> wrote: >> >> Hi, >> >> Le 09/02/2020 à 12:05, Anand Moon a écrit : >>> Use proper compatible string as per the dts so that mmc driver >>> could be tuned properly. SoC family S905, S905X have common clk >>> tuning parameters setting, while AGX and G12 have common clk tuning >>> parameters setting for mmc driver. >>> >>> Suggested-by: Neil Armstrong <narmstrong at baylibre.com> >>> Signed-off-by: Anand Moon <linux.amoon at gmail.com> >>> --- >>> No changes. >>> --- >>> arch/arm/include/asm/arch-meson/sd_emmc.h | 7 ++++ >>> drivers/mmc/meson_gx_mmc.c | 46 +++++++++++++++++------ >>> 2 files changed, 41 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h >>> index f4299485dc..83142d5d3f 100644 >>> --- a/arch/arm/include/asm/arch-meson/sd_emmc.h >>> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h >>> @@ -13,6 +13,12 @@ >>> #define SDIO_PORT_B 1 >>> #define SDIO_PORT_C 2 >>> >>> +enum mmc_compatible { >>> + MMC_COMPATIBLE_GXBB, >>> + MMC_COMPATIBLE_GX, >>> + MMC_COMPATIBLE_AXG, >>> +}; >>> + >>> #define SD_EMMC_CLKSRC_24M 24000000 /* 24 MHz */ >>> #define SD_EMMC_CLKSRC_DIV2 1000000000 /* 1 GHz */ >>> >>> @@ -87,6 +93,7 @@ >>> struct meson_mmc_platdata { >>> struct mmc_config cfg; >>> struct mmc mmc; >>> + int compat; >>> void *regbase; >>> void *w_buf; >>> }; >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>> index b013c7c5fb..1aefe360c4 100644 >>> --- a/drivers/mmc/meson_gx_mmc.c >>> +++ b/drivers/mmc/meson_gx_mmc.c >>> @@ -37,7 +37,8 @@ static inline void meson_write(struct mmc *mmc, uint32_t val, int offset) >>> writel(val, get_regbase(mmc) + offset); >>> } >>> >>> -static void meson_mmc_config_clock(struct mmc *mmc) >>> +static void meson_mmc_config_clock(struct mmc *mmc, >>> + struct meson_mmc_platdata *pdata) >>> { >>> uint32_t meson_mmc_clk = 0; >>> unsigned int clk, clk_src, clk_div; >>> @@ -66,14 +67,20 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>> /* RX clock phase 0:180 */ >>> meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); >>> >>> -#ifdef CONFIG_MESON_GX >>> - /* clk always on */ >>> - meson_mmc_clk |= CLK_V2_ALWAYS_ON; >>> -#endif >>> -#if (defined(CONFIG_MESON_AXG) || defined(CONFIG_MESON_G12A)) >>> - /* clk always on */ >>> - meson_mmc_clk |= CLK_V3_ALWAYS_ON; >>> -#endif >>> + switch (pdata->compat) { >>> + case MMC_COMPATIBLE_GXBB: >>> + case MMC_COMPATIBLE_GX: >>> + /* clk always on */ >>> + meson_mmc_clk |= CLK_V2_ALWAYS_ON; >>> + break; >>> + case MMC_COMPATIBLE_AXG: >>> + /* clk always on */ >>> + meson_mmc_clk |= CLK_V3_ALWAYS_ON; >>> + break; >>> + default: >>> + debug("no compatible supported"); >>> + break; >>> + } >>> >>> /* clock settings */ >>> meson_mmc_clk |= clk_src; >>> @@ -85,9 +92,11 @@ static void meson_mmc_config_clock(struct mmc *mmc) >>> static int meson_dm_mmc_set_ios(struct udevice *dev) >>> { >>> struct mmc *mmc = mmc_get_mmc_dev(dev); >>> + struct meson_mmc_platdata *pdata = >>> + (struct meson_mmc_platdata *)dev_get_driver_data(dev); >>> uint32_t meson_mmc_cfg; >>> >>> - meson_mmc_config_clock(mmc); >>> + meson_mmc_config_clock(mmc, pdata); >>> >>> meson_mmc_cfg = meson_read(mmc, MESON_SD_EMMC_CFG); >>> >>> @@ -324,9 +333,22 @@ int meson_mmc_bind(struct udevice *dev) >>> return mmc_bind(dev, &pdata->mmc, &pdata->cfg); >>> } >>> >>> +static const struct meson_mmc_platdata gxbb_data = { >>> + .compat = MMC_COMPATIBLE_GXBB, >>> +}; >>> + >>> +static const struct meson_mmc_platdata gx_data = { >>> + .compat = MMC_COMPATIBLE_GX, >>> +}; >>> + >>> +static const struct meson_mmc_platdata axg_data = { >>> + .compat = MMC_COMPATIBLE_AXG, >>> +}; >>> +a in >>> static const struct udevice_id meson_mmc_match[] = { >>> - { .compatible = "amlogic,meson-gx-mmc" }, >>> - { .compatible = "amlogic,meson-axg-mmc" }, >>> + { .compatible = "amlogic,meson-gxbb-mmc", .data = (ulong)&gxbb_data }, >>> + { .compatible = "amlogic,meson-gx-mmc", .data = (ulong)&gx_data }, >>> + { .compatible = "amlogic,meson-axg-mmc", .data = (ulong)&axg_data }, >>> { /* sentinel */ } >>> }; >>> >>> >> >> >> It's fine but you should do that before patch 1, and introduce the clk setup directly with the MMC_COMPATIBLE_*. >> > > Only GXBB and GLX have CLK_ALWAYS_ON(24) bit set > and AXG and G12X have CLK_ALWAYS_ON(28) bit set > for clk enable rest of the configuration is all most common for all the SoC. > let keep this simple as of now. It's not the point, just add the "struct meson_mmc_platdata" and .data in meson_mmc_match _before_ patch 1, to avoid adding hideous #ifdef/#elif/#endif in the meantime. Neil > > >> If you move it before, then: >> >> Reviewed-by: Neil Armstrong <narmstrong at baylibre.com> >> >> Neil > > -Anand >
diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h index f4299485dc..83142d5d3f 100644 --- a/arch/arm/include/asm/arch-meson/sd_emmc.h +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h @@ -13,6 +13,12 @@ #define SDIO_PORT_B 1 #define SDIO_PORT_C 2 +enum mmc_compatible { + MMC_COMPATIBLE_GXBB, + MMC_COMPATIBLE_GX, + MMC_COMPATIBLE_AXG, +}; + #define SD_EMMC_CLKSRC_24M 24000000 /* 24 MHz */ #define SD_EMMC_CLKSRC_DIV2 1000000000 /* 1 GHz */ @@ -87,6 +93,7 @@ struct meson_mmc_platdata { struct mmc_config cfg; struct mmc mmc; + int compat; void *regbase; void *w_buf; }; diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c index b013c7c5fb..1aefe360c4 100644 --- a/drivers/mmc/meson_gx_mmc.c +++ b/drivers/mmc/meson_gx_mmc.c @@ -37,7 +37,8 @@ static inline void meson_write(struct mmc *mmc, uint32_t val, int offset) writel(val, get_regbase(mmc) + offset); } -static void meson_mmc_config_clock(struct mmc *mmc) +static void meson_mmc_config_clock(struct mmc *mmc, + struct meson_mmc_platdata *pdata) { uint32_t meson_mmc_clk = 0; unsigned int clk, clk_src, clk_div; @@ -66,14 +67,20 @@ static void meson_mmc_config_clock(struct mmc *mmc) /* RX clock phase 0:180 */ meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); -#ifdef CONFIG_MESON_GX - /* clk always on */ - meson_mmc_clk |= CLK_V2_ALWAYS_ON; -#endif -#if (defined(CONFIG_MESON_AXG) || defined(CONFIG_MESON_G12A)) - /* clk always on */ - meson_mmc_clk |= CLK_V3_ALWAYS_ON; -#endif + switch (pdata->compat) { + case MMC_COMPATIBLE_GXBB: + case MMC_COMPATIBLE_GX: + /* clk always on */ + meson_mmc_clk |= CLK_V2_ALWAYS_ON; + break; + case MMC_COMPATIBLE_AXG: + /* clk always on */ + meson_mmc_clk |= CLK_V3_ALWAYS_ON; + break; + default: + debug("no compatible supported"); + break; + } /* clock settings */ meson_mmc_clk |= clk_src; @@ -85,9 +92,11 @@ static void meson_mmc_config_clock(struct mmc *mmc) static int meson_dm_mmc_set_ios(struct udevice *dev) { struct mmc *mmc = mmc_get_mmc_dev(dev); + struct meson_mmc_platdata *pdata = + (struct meson_mmc_platdata *)dev_get_driver_data(dev); uint32_t meson_mmc_cfg; - meson_mmc_config_clock(mmc); + meson_mmc_config_clock(mmc, pdata); meson_mmc_cfg = meson_read(mmc, MESON_SD_EMMC_CFG); @@ -324,9 +333,22 @@ int meson_mmc_bind(struct udevice *dev) return mmc_bind(dev, &pdata->mmc, &pdata->cfg); } +static const struct meson_mmc_platdata gxbb_data = { + .compat = MMC_COMPATIBLE_GXBB, +}; + +static const struct meson_mmc_platdata gx_data = { + .compat = MMC_COMPATIBLE_GX, +}; + +static const struct meson_mmc_platdata axg_data = { + .compat = MMC_COMPATIBLE_AXG, +}; + static const struct udevice_id meson_mmc_match[] = { - { .compatible = "amlogic,meson-gx-mmc" }, - { .compatible = "amlogic,meson-axg-mmc" }, + { .compatible = "amlogic,meson-gxbb-mmc", .data = (ulong)&gxbb_data }, + { .compatible = "amlogic,meson-gx-mmc", .data = (ulong)&gx_data }, + { .compatible = "amlogic,meson-axg-mmc", .data = (ulong)&axg_data }, { /* sentinel */ } };
Use proper compatible string as per the dts so that mmc driver could be tuned properly. SoC family S905, S905X have common clk tuning parameters setting, while AGX and G12 have common clk tuning parameters setting for mmc driver. Suggested-by: Neil Armstrong <narmstrong at baylibre.com> Signed-off-by: Anand Moon <linux.amoon at gmail.com> --- No changes. --- arch/arm/include/asm/arch-meson/sd_emmc.h | 7 ++++ drivers/mmc/meson_gx_mmc.c | 46 +++++++++++++++++------ 2 files changed, 41 insertions(+), 12 deletions(-)