| Message ID | 20201110075055.25318-1-jh80.chung@samsung.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | mmc: meson_gx_mmc: control ddr_mode bit | expand |
On 10/11/2020 08:50, Jaehoon Chung wrote: > EMMC_CFG register has a cfg_ddr bit(BIT[2]). > It needs to set when mmc is running to ddr mode. > Otherwise, its bit should be cleared. > CFG_DDR[2] - 1: DDR mode, 0: SDR mode > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + > drivers/mmc/meson_gx_mmc.c | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h > index 1e9f8cf498b4..c2f77c7308ec 100644 > --- a/arch/arm/include/asm/arch-meson/sd_emmc.h > +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h > @@ -38,6 +38,7 @@ > #define CFG_BUS_WIDTH_1 0 > #define CFG_BUS_WIDTH_4 1 > #define CFG_BUS_WIDTH_8 2 > +#define CFG_DDR_MODE BIT(2) > #define CFG_BL_LEN_MASK GENMASK(7, 4) > #define CFG_BL_LEN_SHIFT 4 > #define CFG_BL_LEN_512 (9 << 4) > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > index 7c60e0566560..6fcf6c2ced27 100644 > --- a/drivers/mmc/meson_gx_mmc.c > +++ b/drivers/mmc/meson_gx_mmc.c > @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) > else > return -EINVAL; > > + if (mmc->ddr_mode) > + meson_mmc_cfg |= CFG_DDR_MODE; > + else > + meson_mmc_cfg &= ~CFG_DDR_MODE; > + > /* 512 bytes block length */ > meson_mmc_cfg &= ~CFG_BL_LEN_MASK; > meson_mmc_cfg |= CFG_BL_LEN_512; > Interesting, how did it work without this bit ? This driver seems to really be in a bad state... Neil
On 11/10/20 5:15 PM, Neil Armstrong wrote: > On 10/11/2020 08:50, Jaehoon Chung wrote: >> EMMC_CFG register has a cfg_ddr bit(BIT[2]). >> It needs to set when mmc is running to ddr mode. >> Otherwise, its bit should be cleared. >> CFG_DDR[2] - 1: DDR mode, 0: SDR mode >> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >> --- >> arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + >> drivers/mmc/meson_gx_mmc.c | 5 +++++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h >> index 1e9f8cf498b4..c2f77c7308ec 100644 >> --- a/arch/arm/include/asm/arch-meson/sd_emmc.h >> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h >> @@ -38,6 +38,7 @@ >> #define CFG_BUS_WIDTH_1 0 >> #define CFG_BUS_WIDTH_4 1 >> #define CFG_BUS_WIDTH_8 2 >> +#define CFG_DDR_MODE BIT(2) >> #define CFG_BL_LEN_MASK GENMASK(7, 4) >> #define CFG_BL_LEN_SHIFT 4 >> #define CFG_BL_LEN_512 (9 << 4) >> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >> index 7c60e0566560..6fcf6c2ced27 100644 >> --- a/drivers/mmc/meson_gx_mmc.c >> +++ b/drivers/mmc/meson_gx_mmc.c >> @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) >> else >> return -EINVAL; >> >> + if (mmc->ddr_mode) >> + meson_mmc_cfg |= CFG_DDR_MODE; >> + else >> + meson_mmc_cfg &= ~CFG_DDR_MODE; >> + >> /* 512 bytes block length */ >> meson_mmc_cfg &= ~CFG_BL_LEN_MASK; >> meson_mmc_cfg |= CFG_BL_LEN_512; >> > > Interesting, how did it work without this bit ? Without this bit, it's only work to Single Data rate. Linux driver is also controlling this bit. As you know, Amlogic SoCs supports DDR mode. If this bit is not set, this driver can't use DDR mode. When DDR mode is running without this bit, CRC error will be returned (In meson_gx_mmc's case, IO error is returned.) After applied more patch, it's working DDR mode. Device: mmc@ffe07000 Manufacturer ID: 15 OEM: 100 Name: BJTD4 Bus Speed: 52000000 Mode: MMC DDR52 (52MHz) Rd Block Len: 512 MMC version 5.1 High Capacity: Yes Capacity: 29.1 GiB Bus Width: 8-bit DDR Erase Group Size: 512 KiB HC WP Group Size: 8 MiB User Capacity: 29.1 GiB WRREL Boot Capacity: 4 MiB ENH RPMB Capacity: 4 MiB ENH Boot area 0 is not write protected Boot area 1 is not write protected > > This driver seems to really be in a bad state... Right. This driver doesn't use any dt's property. It also needs to add mmc_of_parse() to use kernel property. I will do that. Best Regards, Jaehoon Chung > > Neil > >
> From: Neil Armstrong <narmstrong@baylibre.com> > Date: Tue, 10 Nov 2020 09:15:14 +0100 > > On 10/11/2020 08:50, Jaehoon Chung wrote: > > EMMC_CFG register has a cfg_ddr bit(BIT[2]). > > It needs to set when mmc is running to ddr mode. > > Otherwise, its bit should be cleared. > > CFG_DDR[2] - 1: DDR mode, 0: SDR mode > > > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > > --- > > arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + > > drivers/mmc/meson_gx_mmc.c | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h > > index 1e9f8cf498b4..c2f77c7308ec 100644 > > --- a/arch/arm/include/asm/arch-meson/sd_emmc.h > > +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h > > @@ -38,6 +38,7 @@ > > #define CFG_BUS_WIDTH_1 0 > > #define CFG_BUS_WIDTH_4 1 > > #define CFG_BUS_WIDTH_8 2 > > +#define CFG_DDR_MODE BIT(2) > > #define CFG_BL_LEN_MASK GENMASK(7, 4) > > #define CFG_BL_LEN_SHIFT 4 > > #define CFG_BL_LEN_512 (9 << 4) > > diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > > index 7c60e0566560..6fcf6c2ced27 100644 > > --- a/drivers/mmc/meson_gx_mmc.c > > +++ b/drivers/mmc/meson_gx_mmc.c > > @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) > > else > > return -EINVAL; > > > > + if (mmc->ddr_mode) > > + meson_mmc_cfg |= CFG_DDR_MODE; > > + else > > + meson_mmc_cfg &= ~CFG_DDR_MODE; > > + > > /* 512 bytes block length */ > > meson_mmc_cfg &= ~CFG_BL_LEN_MASK; > > meson_mmc_cfg |= CFG_BL_LEN_512; > > > > Interesting, how did it work without this bit ? Well, in meson_mmc_probe() we have: cfg->host_caps = MMC_MODE_8BIT | MMC_MODE_4BIT | MMC_MODE_HS_52MHz | MMC_MODE_HS; So the driver doesn't advertise DDR support.
On 11/10/20 5:50 PM, Mark Kettenis wrote: >> From: Neil Armstrong <narmstrong@baylibre.com> >> Date: Tue, 10 Nov 2020 09:15:14 +0100 >> >> On 10/11/2020 08:50, Jaehoon Chung wrote: >>> EMMC_CFG register has a cfg_ddr bit(BIT[2]). >>> It needs to set when mmc is running to ddr mode. >>> Otherwise, its bit should be cleared. >>> CFG_DDR[2] - 1: DDR mode, 0: SDR mode >>> >>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >>> --- >>> arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + >>> drivers/mmc/meson_gx_mmc.c | 5 +++++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h >>> index 1e9f8cf498b4..c2f77c7308ec 100644 >>> --- a/arch/arm/include/asm/arch-meson/sd_emmc.h >>> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h >>> @@ -38,6 +38,7 @@ >>> #define CFG_BUS_WIDTH_1 0 >>> #define CFG_BUS_WIDTH_4 1 >>> #define CFG_BUS_WIDTH_8 2 >>> +#define CFG_DDR_MODE BIT(2) >>> #define CFG_BL_LEN_MASK GENMASK(7, 4) >>> #define CFG_BL_LEN_SHIFT 4 >>> #define CFG_BL_LEN_512 (9 << 4) >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >>> index 7c60e0566560..6fcf6c2ced27 100644 >>> --- a/drivers/mmc/meson_gx_mmc.c >>> +++ b/drivers/mmc/meson_gx_mmc.c >>> @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) >>> else >>> return -EINVAL; >>> >>> + if (mmc->ddr_mode) >>> + meson_mmc_cfg |= CFG_DDR_MODE; >>> + else >>> + meson_mmc_cfg &= ~CFG_DDR_MODE; >>> + >>> /* 512 bytes block length */ >>> meson_mmc_cfg &= ~CFG_BL_LEN_MASK; >>> meson_mmc_cfg |= CFG_BL_LEN_512; >>> >> >> Interesting, how did it work without this bit ? > > Well, in meson_mmc_probe() we have: > > cfg->host_caps = MMC_MODE_8BIT | MMC_MODE_4BIT | > MMC_MODE_HS_52MHz | MMC_MODE_HS; > > So the driver doesn't advertise DDR support. Right. I added some patch in my local for testing. Actually, those is wrong. If want to follow linux driver, it needs to remove. Instead, it has to use mmc_of_parse() for dt's property. I will send patch after more test. Best Regards, Jaehoon Chung >
> From: Jaehoon Chung <jh80.chung@samsung.com> > Date: Tue, 10 Nov 2020 18:02:02 +0900 > > On 11/10/20 5:50 PM, Mark Kettenis wrote: > >> From: Neil Armstrong <narmstrong@baylibre.com> > >> Date: Tue, 10 Nov 2020 09:15:14 +0100 > >> > >> On 10/11/2020 08:50, Jaehoon Chung wrote: > >>> EMMC_CFG register has a cfg_ddr bit(BIT[2]). > >>> It needs to set when mmc is running to ddr mode. > >>> Otherwise, its bit should be cleared. > >>> CFG_DDR[2] - 1: DDR mode, 0: SDR mode > >>> > >>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > >>> --- > >>> arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + > >>> drivers/mmc/meson_gx_mmc.c | 5 +++++ > >>> 2 files changed, 6 insertions(+) > >>> > >>> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h > >>> index 1e9f8cf498b4..c2f77c7308ec 100644 > >>> --- a/arch/arm/include/asm/arch-meson/sd_emmc.h > >>> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h > >>> @@ -38,6 +38,7 @@ > >>> #define CFG_BUS_WIDTH_1 0 > >>> #define CFG_BUS_WIDTH_4 1 > >>> #define CFG_BUS_WIDTH_8 2 > >>> +#define CFG_DDR_MODE BIT(2) > >>> #define CFG_BL_LEN_MASK GENMASK(7, 4) > >>> #define CFG_BL_LEN_SHIFT 4 > >>> #define CFG_BL_LEN_512 (9 << 4) > >>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c > >>> index 7c60e0566560..6fcf6c2ced27 100644 > >>> --- a/drivers/mmc/meson_gx_mmc.c > >>> +++ b/drivers/mmc/meson_gx_mmc.c > >>> @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) > >>> else > >>> return -EINVAL; > >>> > >>> + if (mmc->ddr_mode) > >>> + meson_mmc_cfg |= CFG_DDR_MODE; > >>> + else > >>> + meson_mmc_cfg &= ~CFG_DDR_MODE; > >>> + > >>> /* 512 bytes block length */ > >>> meson_mmc_cfg &= ~CFG_BL_LEN_MASK; > >>> meson_mmc_cfg |= CFG_BL_LEN_512; > >>> > >> > >> Interesting, how did it work without this bit ? > > > > Well, in meson_mmc_probe() we have: > > > > cfg->host_caps = MMC_MODE_8BIT | MMC_MODE_4BIT | > > MMC_MODE_HS_52MHz | MMC_MODE_HS; > > > > So the driver doesn't advertise DDR support. > > Right. I added some patch in my local for testing. > > Actually, those is wrong. If want to follow linux driver, it needs to remove. > Instead, it has to use mmc_of_parse() for dt's property. > > I will send patch after more test. Note that you also need to double the clock frequency for DDR modes. This isn't done automatically like on other SoCs.
diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h b/arch/arm/include/asm/arch-meson/sd_emmc.h index 1e9f8cf498b4..c2f77c7308ec 100644 --- a/arch/arm/include/asm/arch-meson/sd_emmc.h +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h @@ -38,6 +38,7 @@ #define CFG_BUS_WIDTH_1 0 #define CFG_BUS_WIDTH_4 1 #define CFG_BUS_WIDTH_8 2 +#define CFG_DDR_MODE BIT(2) #define CFG_BL_LEN_MASK GENMASK(7, 4) #define CFG_BL_LEN_SHIFT 4 #define CFG_BL_LEN_512 (9 << 4) diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c index 7c60e0566560..6fcf6c2ced27 100644 --- a/drivers/mmc/meson_gx_mmc.c +++ b/drivers/mmc/meson_gx_mmc.c @@ -90,6 +90,11 @@ static int meson_dm_mmc_set_ios(struct udevice *dev) else return -EINVAL; + if (mmc->ddr_mode) + meson_mmc_cfg |= CFG_DDR_MODE; + else + meson_mmc_cfg &= ~CFG_DDR_MODE; + /* 512 bytes block length */ meson_mmc_cfg &= ~CFG_BL_LEN_MASK; meson_mmc_cfg |= CFG_BL_LEN_512;
EMMC_CFG register has a cfg_ddr bit(BIT[2]). It needs to set when mmc is running to ddr mode. Otherwise, its bit should be cleared. CFG_DDR[2] - 1: DDR mode, 0: SDR mode Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> --- arch/arm/include/asm/arch-meson/sd_emmc.h | 1 + drivers/mmc/meson_gx_mmc.c | 5 +++++ 2 files changed, 6 insertions(+)