[PATCHv6 1/5] mmc: meson-gx: Fix clk phase tuning for MMC
Neil Armstrong
narmstrong at baylibre.com
Mon Feb 10 10:03:41 CET 2020
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
>
More information about the U-Boot
mailing list