Hi Jerome / Neil, On Mon, 10 Feb 2020 at 15:05, Jerome Brunet <[email protected]> wrote: > > > On Sun 09 Feb 2020 at 18:22, Anand Moon <[email protected]> wrote: > > > Hi Neil, > > > > Thanks for you review comments. > > > > On Sun, 9 Feb 2020 at 18:38, Neil Armstrong <[email protected]> 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 <[email protected]> > >> > --- > >> > 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
