diff mbox series

[PATCHv6,1/5] mmc: meson-gx: Fix clk phase tuning for MMC

Message ID 20200209110557.1996-2-linux.amoon@gmail.com
State New
Headers show
Series Odroid n2 using eMMC would fail to boot up | expand

Commit Message

Anand Moon Feb. 9, 2020, 11:05 a.m. UTC
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(-)

Comments

Neil Armstrong Feb. 9, 2020, 1:08 p.m. UTC | #1
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.


> +	/* 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 !

> +	/* Core clock phase 2:180 */
> +	meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
> +	/* TX clock phase 0:180 */
> +	meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
> +	/* RX clock phase 0:180 */
> +	meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);

These are ok, but it's exactly the same as before with a different style.

> +
> +#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

Why not, not sure about the effect.

>  
>  	/* clock settings */
>  	meson_mmc_clk |= clk_src;
> 

Neil
Anand Moon Feb. 9, 2020, 5:22 p.m. UTC | #2
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

>
>
> > +     /* 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.*

> > +     /* Core clock phase 2:180 */
> > +     meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
> > +     /* TX clock phase 0:180 */
> > +     meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
> > +     /* RX clock phase 0:180 */
> > +     meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
>
> These are ok, but it's exactly the same as before with a different style.
>
> > +
> > +#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
>
> Why not, not sure about the effect.

Ok, I will changes these to  FIELD_PREP for consultancy.

>
> >
> >       /* clock settings */
> >       meson_mmc_clk |= clk_src;
> >
>
> Neil

-Anand
Neil Armstrong Feb. 10, 2020, 9:03 a.m. UTC | #3
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.

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.

Could you post instead :
- your patch 2 introducing the MMC_COMPATIBLE_* as patch 1
- this patch but only forcing 24MHz for MMC_COMPATIBLE_AXG as patch 2
- redude patches 3,4 & 5 to only add mmc* aliases into meson-gx-u-boot.dtsi/meson-g12-common-u-boot.dtsi

If you have a issue, please chat with me on #linux-amlogic on freenode.

Thanks,
Neil

> 
>>> +     /* Core clock phase 2:180 */
>>> +     meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
>>> +     /* TX clock phase 0:180 */
>>> +     meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
>>> +     /* RX clock phase 0:180 */
>>> +     meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
>>
>> These are ok, but it's exactly the same as before with a different style.
>>
>>> +
>>> +#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
>>
>> Why not, not sure about the effect.
> 
> Ok, I will changes these to  FIELD_PREP for consultancy.

Really not sure about the effect of these, please drop these until we find out
if we really need these.

> 
>>
>>>
>>>       /* clock settings */
>>>       meson_mmc_clk |= clk_src;
>>>
>>
>> Neil
> 
> -Anand
>
Jerome Brunet Feb. 10, 2020, 9:35 a.m. UTC | #4
On Sun 09 Feb 2020 at 18:22, Anand Moon <linux.amoon at gmail.com> 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
>
>>
>>
>> > +     /* 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.*

I don't think the reality of the issue reported has been questioned.
What is being questioned is the proposed solution.

The patch you sent forces clock input 0 with the maximum divider
possible. This is an important comment for which an explanation is
missing AFAICT.

1) The doc is not clear but input 0 is not always the oscillator. It is
the MMC composite clock which can set to several different sources,
especially when the ROM code boots from another device, such as SPI
This is the topic of 2 patches I sent recently (not yet merged)

2) Let's assume that input 0 is indeed 24M, your patches forces this
with a 63 divider => 380kHz. With a clock that low, the phase does not
matter much anymore

IOW, you are the only one to report this issue on one specific case, the
cause of the issue is not really identified and the work around would:
* Add statements without effect in the code
* Considerably slow down eMMC and SDCard on all amlogic platforms.
* Provide no explanation about how it actually solves something

This is problematic ...

>
>> > +     /* Core clock phase 2:180 */
>> > +     meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
>> > +     /* TX clock phase 0:180 */
>> > +     meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
>> > +     /* RX clock phase 0:180 */
>> > +     meson_mmc_clk |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
>>
>> These are ok, but it's exactly the same as before with a different style.
>>
>> > +
>> > +#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
>>
>> Why not, not sure about the effect.
>
> Ok, I will changes these to  FIELD_PREP for consultancy.
>
>>
>> >
>> >       /* clock settings */
>> >       meson_mmc_clk |= clk_src;
>> >
>>
>> Neil
>
> -Anand
Anand Moon Feb. 13, 2020, 11:58 a.m. UTC | #5
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);
       }
>
> 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
Anand Moon Feb. 13, 2020, 12:04 p.m. UTC | #6
Hi Jerome / Neil,

On Mon, 10 Feb 2020 at 15:05, Jerome Brunet <jbrunet at baylibre.com> wrote:
>
>
> On Sun 09 Feb 2020 at 18:22, Anand Moon <linux.amoon at gmail.com> 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
> >
> >>
> >>
> >> > +     /* 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.*
>
> I don't think the reality of the issue reported has been questioned.
> What is being questioned is the proposed solution.
>
> The patch you sent forces clock input 0 with the maximum divider
> possible. This is an important comment for which an explanation is
> missing AFAICT.
>
> 1) The doc is not clear but input 0 is not always the oscillator. It is
> the MMC composite clock which can set to several different sources,
> especially when the ROM code boots from another device, such as SPI
> This is the topic of 2 patches I sent recently (not yet merged)
>
> 2) Let's assume that input 0 is indeed 24M, your patches forces this
> with a 63 divider => 380kHz. With a clock that low, the phase does not
> matter much anymore
>
> IOW, you are the only one to report this issue on one specific case, the
> cause of the issue is not really identified and the work around would:
> * Add statements without effect in the code
> * Considerably slow down eMMC and SDCard on all amlogic platforms.
> * Provide no explanation about how it actually solves something
>
> This is problematic ...
>

Ok Thanks for your review comments.
I have tried to test these on all the device that I have with all the
emmc sdcards.
I have almost used the mainline code to resolve this issue.
but my approach to address this was not up to date.

-Anand
Neil Armstrong Feb. 13, 2020, 3:39 p.m. UTC | #7
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

>>
>> 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
>
Anand Moon Feb. 13, 2020, 4:41 p.m. UTC | #8
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
> >
>
diff mbox series

Patch

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;
+	/* Clock source : Crystal 24MHz */
+	meson_mmc_clk |= FIELD_PREP(CLK_SRC_MASK, CRYSTAL_24MHZ);
+	/* Core clock phase 2:180 */
+	meson_mmc_clk |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
+	/* TX clock phase 0:180 */
+	meson_mmc_clk |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
+	/* 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
 
 	/* clock settings */
 	meson_mmc_clk |= clk_src;