[PATCHv6 2/5] mmc: meson-gx: Use proper compatible string as per the dts

[Neil Armstrong]

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
>