RE: [PATCH] i2c: correct I2C deblock logic
> -Original Message-
> From: Bough Chen
> Sent: 2023年2月10日 17:27
> To: h...@denx.de; al.koc...@gmail.com; ma...@denx.de
> Cc: u-boot@lists.denx.de; dl-uboot-imx ;
> xypron.g...@gmx.de; Bough Chen
> Subject: [PATCH] i2c: correct I2C deblock logic
>
> From: Haibo Chen
>
> Current code use dm_gpio_get_value() to get SDA and SCL value, and the value
> depends on the flag GPIOD_ACTIVE_LOW. When toggle SCL to wait slave
> release SDA, the SDA are config as GPIOD_IS_IN, and whether contain the
> GPIOD_ACTIVE_LOW depends on the DTS setting. Usually, for I2C GPIO, we use
> GPIOD_ACTIVE_LOW flag in DTS, so if the SDA is in low level, then
> dm_gpio_get_value() will return 1, current code logic will stop the SCL toggle
> wrongly, cause the I2C deblock not work as expect.
>
> This patch force set the GPIOD_ACTIVE_LOW for both GPIOD_IS_IN and
> GPIOD_IS_OUT, and make the return value of i2c_gpio_get_pin() eaqual to the
> physical voltage logic level.
>
Hi,
Any comments for this patch, just a reminder in case you miss it.
Best Regards
Haibo Chen
> Fixes: aa54192d4a87 ("dm: i2c: implement gpio-based I2C deblock")
> Signed-off-by: Haibo Chen
> ---
> drivers/i2c/i2c-uclass.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c index
> 8d9a89ed89..4dc707c659 100644
> --- a/drivers/i2c/i2c-uclass.c
> +++ b/drivers/i2c/i2c-uclass.c
> @@ -505,7 +505,8 @@ uint i2c_get_chip_addr_offset_mask(struct udevice
> *dev) static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> if (bit)
> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
> +GPIOD_ACTIVE_LOW);
> else
> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> GPIOD_ACTIVE_LOW |
> @@ -514,7 +515,7 @@ static void i2c_gpio_set_pin(struct gpio_desc *pin, int
> bit)
>
> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
> - return dm_gpio_get_value(pin);
> + return !dm_gpio_get_value(pin);
> }
>
> int i2c_deblock_gpio_loop(struct gpio_desc *sda_pin,
> --
> 2.34.1
RE: [PATCH 1/3] dm: adc: add iMX93 ADC support
> -Original Message-
> From: Luca Ellero
> Sent: 2023年3月13日 21:06
> To: u-boot@lists.denx.de; sba...@denx.de; feste...@gmail.com; dl-uboot-imx
> ; luca.ell...@brickedbrain.com; Ye Li ;
> Peng Fan ; Bough Chen
> Cc: Luca Ellero
> Subject: [PATCH 1/3] dm: adc: add iMX93 ADC support
>
> This commit adds driver for iMX93 ADC.
>
> The driver is implemented using driver model and provides ADC uclass's methods
> for ADC single channel operations:
> - adc_start_channel()
> - adc_channel_data()
> - adc_stop()
>
> ADC features:
> - channels: 4
> - resolution: 12-bit
>
> Signed-off-by: Luca Ellero
> ---
> drivers/adc/Kconfig | 8 ++
> drivers/adc/Makefile| 1 +
> drivers/adc/imx93-adc.c | 284
>
> 3 files changed, 293 insertions(+)
> create mode 100644 drivers/adc/imx93-adc.c
>
> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index
> e719c38bb3..4336732dee 100644
> --- a/drivers/adc/Kconfig
> +++ b/drivers/adc/Kconfig
> @@ -63,3 +63,11 @@ config STM32_ADC
> - core driver to deal with common resources
> - child driver to deal with individual ADC resources (declare ADC
> device and associated channels, start/stop conversions)
> +
> +config ADC_IMX93
> + bool "Enable NXP IMX93 ADC driver"
> + help
> + This enables basic driver for NXP IMX93 ADC.
> + It provides:
> + - 4 analog input channels
> + - 12-bit resolution
> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile index
> c1387f3a34..5336c82097 100644
> --- a/drivers/adc/Makefile
> +++ b/drivers/adc/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_ADC_SANDBOX) += sandbox.o
> obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o
> obj-$(CONFIG_SARADC_MESON) += meson-saradc.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o stm32-adc-core.o
> +obj-$(CONFIG_ADC_IMX93) += imx93-adc.o
> diff --git a/drivers/adc/imx93-adc.c b/drivers/adc/imx93-adc.c new file mode
> 100644 index 00..c5f0268eca
> --- /dev/null
> +++ b/drivers/adc/imx93-adc.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2023 ASEM Srl
> + * Author: Luca Ellero
> + *
> + * Originally based on NXP linux-imx kernel v5.15
> +drivers/iio/adc/imx93_adc.c */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IMX93_ADC_MCR0x00
> +#define IMX93_ADC_MSR0x04
> +#define IMX93_ADC_ISR0x10
> +#define IMX93_ADC_IMR0x20
> +#define IMX93_ADC_CIMR0 0x24
> +#define IMX93_ADC_CTR0 0x94
> +#define IMX93_ADC_NCMR0 0xA4
> +#define IMX93_ADC_PCDR0 0x100
> +#define IMX93_ADC_PCDR1 0x104
> +#define IMX93_ADC_PCDR2 0x108
> +#define IMX93_ADC_PCDR3 0x10c
> +#define IMX93_ADC_PCDR4 0x110
> +#define IMX93_ADC_PCDR5 0x114
> +#define IMX93_ADC_PCDR6 0x118
> +#define IMX93_ADC_PCDR7 0x11c
> +#define IMX93_ADC_CALSTAT0x39C
> +
> +#define IMX93_ADC_MCR_MODE_MASK BIT(29)
> +#define IMX93_ADC_MCR_NSTART_MASKBIT(24)
> +#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> +#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> +
> +#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> +#define IMX93_ADC_MSR_CALBUSY_MASK BIT(29)
> +#define IMX93_ADC_MSR_ADCSTATUS_MASK GENMASK(2, 0)
> +
> +#define IMX93_ADC_ISR_EOC_MASK BIT(1)
> +
> +#define IMX93_ADC_IMR_EOC_MASK BIT(1)
> +#define IMX93_ADC_IMR_ECH_MASK BIT(0)
> +
> +#define IMX93_ADC_PCDR_CDATA_MASKGENMASK(11, 0)
> +
> +#define IDLE 0
> +#define POWER_DOWN 1
> +#define WAIT_STATE 2
> +#define BUSY_IN_CALIBRATION 3
> +#define SAMPLE 4
> +#define CONVERSION 6
> +
> +#define IMX93_ADC_MAX_CHANNEL3
> +#define IMX93_ADC_DAT_MASK 0xfff
> +#define IMX93_ADC_TIMEOUT10
> +
> +struct imx93_adc_priv {
> + int active_channel;
> + void __iomem *regs;
> +};
> +
> +static int imx93_adc_power_down(struct imx93_adc_priv *adc) {
> + u32 mcr, msr;
> + int ret;
> +
> + mcr = readl(adc->regs + IMX93_ADC_MCR);
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_PWDN_MASK, 1);
>
RE: [PATCH 1/3] dm: adc: add iMX93 ADC support
> -Original Message-
> From: Luca Ellero
> Sent: 2023年3月15日 21:36
> To: u-boot@lists.denx.de; sba...@denx.de; feste...@gmail.com; dl-uboot-imx
> ; luca.ell...@brickedbrain.com; Ye Li ;
> Peng Fan ; Bough Chen
> Cc: Luca Ellero
> Subject: [PATCH 1/3] dm: adc: add iMX93 ADC support
>
> This commit adds driver for iMX93 ADC.
>
> The driver is implemented using driver model and provides ADC uclass's methods
> for ADC single channel operations:
> - adc_start_channel()
> - adc_channel_data()
> - adc_stop()
>
> ADC features:
> - channels: 4
> - resolution: 12-bit
>
> Signed-off-by: Luca Ellero
> ---
> drivers/adc/Kconfig | 8 ++
> drivers/adc/Makefile| 1 +
> drivers/adc/imx93-adc.c | 286
>
> 3 files changed, 295 insertions(+)
> create mode 100644 drivers/adc/imx93-adc.c
>
> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index
> e719c38bb3..4336732dee 100644
> --- a/drivers/adc/Kconfig
> +++ b/drivers/adc/Kconfig
> @@ -63,3 +63,11 @@ config STM32_ADC
> - core driver to deal with common resources
> - child driver to deal with individual ADC resources (declare ADC
> device and associated channels, start/stop conversions)
> +
> +config ADC_IMX93
> + bool "Enable NXP IMX93 ADC driver"
> + help
> + This enables basic driver for NXP IMX93 ADC.
> + It provides:
> + - 4 analog input channels
> + - 12-bit resolution
> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile index
> c1387f3a34..5336c82097 100644
> --- a/drivers/adc/Makefile
> +++ b/drivers/adc/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_ADC_SANDBOX) += sandbox.o
> obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o
> obj-$(CONFIG_SARADC_MESON) += meson-saradc.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o stm32-adc-core.o
> +obj-$(CONFIG_ADC_IMX93) += imx93-adc.o
> diff --git a/drivers/adc/imx93-adc.c b/drivers/adc/imx93-adc.c new file mode
> 100644 index 00..b3bbea6c4e
> --- /dev/null
> +++ b/drivers/adc/imx93-adc.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2023 ASEM Srl
> + * Author: Luca Ellero
> + *
> + * Originally based on NXP linux-imx kernel v5.15
> +drivers/iio/adc/imx93_adc.c */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IMX93_ADC_MCR0x00
> +#define IMX93_ADC_MSR0x04
> +#define IMX93_ADC_ISR0x10
> +#define IMX93_ADC_IMR0x20
> +#define IMX93_ADC_CIMR0 0x24
> +#define IMX93_ADC_CTR0 0x94
> +#define IMX93_ADC_NCMR0 0xA4
> +#define IMX93_ADC_PCDR0 0x100
> +#define IMX93_ADC_PCDR1 0x104
> +#define IMX93_ADC_PCDR2 0x108
> +#define IMX93_ADC_PCDR3 0x10c
> +#define IMX93_ADC_PCDR4 0x110
> +#define IMX93_ADC_PCDR5 0x114
> +#define IMX93_ADC_PCDR6 0x118
> +#define IMX93_ADC_PCDR7 0x11c
> +#define IMX93_ADC_CALSTAT0x39C
> +
> +#define IMX93_ADC_MCR_MODE_MASK BIT(29)
> +#define IMX93_ADC_MCR_NSTART_MASKBIT(24)
> +#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> +#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> +
> +#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> +#define IMX93_ADC_MSR_CALBUSY_MASK BIT(29)
> +#define IMX93_ADC_MSR_ADCSTATUS_MASK GENMASK(2, 0)
> +
> +#define IMX93_ADC_ISR_EOC_MASK BIT(1)
> +
> +#define IMX93_ADC_IMR_EOC_MASK BIT(1)
> +#define IMX93_ADC_IMR_ECH_MASK BIT(0)
> +
> +#define IMX93_ADC_PCDR_CDATA_MASKGENMASK(11, 0)
> +
> +#define IDLE 0
> +#define POWER_DOWN 1
> +#define WAIT_STATE 2
> +#define BUSY_IN_CALIBRATION 3
> +#define SAMPLE 4
> +#define CONVERSION 6
> +
> +#define IMX93_ADC_MAX_CHANNEL3
> +#define IMX93_ADC_DAT_MASK 0xfff
> +#define IMX93_ADC_TIMEOUT10
> +
> +struct imx93_adc_priv {
> + int active_channel;
> + void __iomem *regs;
> +};
> +
> +static int imx93_adc_power_down(struct imx93_adc_priv *adc) {
> + u32 mcr, msr;
> + int ret;
> +
> + mcr = readl(adc->regs + IMX93_ADC_MCR);
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_PWDN_MASK, 1);
>
RE: [PATCH] i2c: correct I2C deblock logic
> -Original Message-
> From: Alexander Kochetkov
> Sent: 2023年3月20日 16:03
> To: h...@denx.de
> Cc: Bough Chen ; ma...@denx.de;
> u-boot@lists.denx.de; dl-uboot-imx ;
> xypron.g...@gmx.de; Simon Glass
> Subject: Re: [PATCH] i2c: correct I2C deblock logic
>
> Hello!
>
> The patch doesn’t add new functionality to the code.
> May be it makes code more readable. But in later case the patch description
> should be corrected and Fixes tag removed.
Hi Alexander,
This patch not only make the code more readable, but also really fix a bug.
In our current code, we will call i2c_gpio_set_pin(scl_pin, 1) and then
i2c_gpio_set_pin(scl_pin, 0).
After the i2c_gpio_set_pin(scl_pin, 0), the flags contain GPIOD_ACTIVE_LOW.
Though for SDA, it will first call i2c_gpio_set_pin(sda_pin, 0) then
i2c_gpio_set_pin(sda_pin, 1), but
i2c_gpio_set_pin-> dm_gpio_set_dir_flags-> dm_gpio_clrset_flags, it only clear
GPIOD_MASK_DIR, this has no impact with GPIOD_ACTIVE_LOW. So this
GPIOD_ACTIVE_LOW keep in flags.
Then call i2c_gpio_get_pin by using dm_gpio_get_value(pin) will get the wrong
data, let the function i2c_deblock_gpio_loop always return -EREMOTEIO
Best Regards
Haibo Chen
>
> The flag GPIOD_ACTIVE_LOW affects return value dm_gpio_get_value().
> And return value doesn’t depends on the DTS settings. It depends on the flag
> passed to dm_gpio_set_dir_flags(). Usually the code parses DTS and provide the
> correct flag to the dm_gpio_set_dir_flags().
>
> So the patch adds flag GPIOD_ACTIVE_LOW to the dm_gpio_set_dir_flags() and
> as expected this negates the return value of dm_gpio_get_value(). So in order
> to
> keep the code correct the patch also fixes adds negotiation to
> dm_gpio_get_value().
>
> Alexander.
>
>
>
> > 20 марта 2023 г., в 10:44, Heiko Schocher написал(а):
> >
> > Hi!
> >
> > On 13.03.23 03:55, Bough Chen wrote:
> >>> -Original Message-
> >>> From: Bough Chen
> >>> Sent: 2023年2月10日 17:27
> >>> To: h...@denx.de; al.koc...@gmail.com; ma...@denx.de
> >>> Cc: u-boot@lists.denx.de; dl-uboot-imx ;
> >>> xypron.g...@gmx.de; Bough Chen
> >>> Subject: [PATCH] i2c: correct I2C deblock logic
> >>>
> >>> From: Haibo Chen
> >>>
> >>> Current code use dm_gpio_get_value() to get SDA and SCL value, and
> >>> the value depends on the flag GPIOD_ACTIVE_LOW. When toggle SCL to
> >>> wait slave release SDA, the SDA are config as GPIOD_IS_IN, and
> >>> whether contain the GPIOD_ACTIVE_LOW depends on the DTS setting.
> >>> Usually, for I2C GPIO, we use GPIOD_ACTIVE_LOW flag in DTS, so if
> >>> the SDA is in low level, then
> >>> dm_gpio_get_value() will return 1, current code logic will stop the
> >>> SCL toggle wrongly, cause the I2C deblock not work as expect.
> >>>
> >>> This patch force set the GPIOD_ACTIVE_LOW for both GPIOD_IS_IN and
> >>> GPIOD_IS_OUT, and make the return value of i2c_gpio_get_pin() eaqual
> >>> to the physical voltage logic level.
> >>>
> >>
> >> Hi,
> >>
> >> Any comments for this patch, just a reminder in case you miss it.
> >
> > Indeed, I missed this patch, sorry!
> > Your explanation sounds reasonable to me, but I wonder if nobody
> > tapped into this problem... it would be good to have some test reports
> > from others! Also added Simon, as he did a lot of work in this space.
> >
> > Thanks!
> >
> > bye,
> > Heiko
> >>
> >> Best Regards
> >> Haibo Chen
> >>
> >>> Fixes: aa54192d4a87 ("dm: i2c: implement gpio-based I2C deblock")
> >>> Signed-off-by: Haibo Chen
> >>> ---
> >>> drivers/i2c/i2c-uclass.c | 5 +++--
> >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> >>> index
> >>> 8d9a89ed89..4dc707c659 100644
> >>> --- a/drivers/i2c/i2c-uclass.c
> >>> +++ b/drivers/i2c/i2c-uclass.c
> >>> @@ -505,7 +505,8 @@ uint i2c_get_chip_addr_offset_mask(struct
> >>> udevice
> >>> *dev) static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> >>> if (bit)
> >>> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >>> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
> >>> +GPIOD_ACTIVE_LOW);
> >>> else
> >>> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> >>> GPIOD_ACTIVE_LOW |
> >>> @@ -514,7 +515,7 @@ static void i2c_gpio_set_pin(struct gpio_desc
> >>> *pin, int
> >>> bit)
> >>>
> >>> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
> >>> - return dm_gpio_get_value(pin);
> >>> + return !dm_gpio_get_value(pin);
> >>> }
> >>>
> >>> int i2c_deblock_gpio_loop(struct gpio_desc *sda_pin,
> >>> --
> >>> 2.34.1
> >>
> >
> > --
> > DENX Software Engineering GmbH, Managing Director: Erika Unter
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de
RE: [PATCH] i2c: correct I2C deblock logic
> -Original Message- > From: Alexander Kochetkov > Sent: 2023年3月21日 17:50 > To: Bough Chen > Cc: h...@denx.de; ma...@denx.de; u-boot@lists.denx.de; dl-uboot-imx > ; xypron.g...@gmx.de; Simon Glass > > Subject: Re: [PATCH] i2c: correct I2C deblock logic > > Hi Haibo. > > Nice catch! Agree with you patch. But may be fix problem in general? > I suggest to add GPIOD_ACTIVE_LOW to the GPIOD_MASK_DIR mask to fix > problems 1, 2, 4. > Not sure if that logically correct. What do you think? Hi Alexander I will have a try, and test on my side. Best Regards Haibo Chen > > I grepped the code for ‘dm_gpio_set_dir_flags’ and found another places > affected by the bug. > > 1. u-boot/drivers/spi/mxc_spi.c: > ret = dm_gpio_set_dir_flags(&mxcs->cs_gpios[i], > GPIOD_IS_OUT | > GPIOD_ACTIVE_LOW); > > Here GPIOD_ACTIVE_LOW is passed to dm_gpio_set_dir_flags() and filtered out > by dm_gpio_set_dir_flags(). > GPIOD_ACTIVE_LOW can be added to GPIOD_MASK_DIR mask to fix that. > > 2. u-boot/drivers/spi/mvebu_a3700_spi.c: > ret = dm_gpio_set_dir_flags(&plat->cs_gpios[i], > GPIOD_IS_OUT | > GPIOD_ACTIVE_LOW); Same as 1. > > 3. u-boot/drivers/phy/allwinner/phy-sun4i-usb.c: > ret = dm_gpio_set_dir_flags(&phy->gpio_id_det, > GPIOD_IS_IN | > GPIOD_PULL_UP); > > Here GPIOD_PULL_UP is passed to dm_gpio_set_dir_flags() and filtered out by > dm_gpio_set_dir_flags(). > Looks, like we have to introduce dm_gpio_set_pull_flags() and use it together > with dm_gpio_set_dir_flags(). > > 4. u-boot/drivers/i2c/i2c-uclass.c > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | > > GPIOD_ACTIVE_LOW | > > GPIOD_IS_OUT_ACTIVE); Same as 1. Your patch cover that bug. > > > > > > 21 марта 2023 г., в 11:37, Bough Chen > написал(а): > > > >> -Original Message- > >> From: Alexander Kochetkov > >> Sent: 2023年3月20日 16:03 > >> To: h...@denx.de > >> Cc: Bough Chen ; ma...@denx.de; > >> u-boot@lists.denx.de; dl-uboot-imx ; > >> xypron.g...@gmx.de; Simon Glass > >> Subject: Re: [PATCH] i2c: correct I2C deblock logic > >> > >> Hello! > >> > >> The patch doesn’t add new functionality to the code. > >> May be it makes code more readable. But in later case the patch > >> description should be corrected and Fixes tag removed. > > > > Hi Alexander, > > > > This patch not only make the code more readable, but also really fix a bug. > > In our current code, we will call i2c_gpio_set_pin(scl_pin, 1) and then > i2c_gpio_set_pin(scl_pin, 0). > > After the i2c_gpio_set_pin(scl_pin, 0), the flags contain GPIOD_ACTIVE_LOW. > > Though for SDA, it will first call i2c_gpio_set_pin(sda_pin, 0) then > > i2c_gpio_set_pin(sda_pin, 1), but i2c_gpio_set_pin-> dm_gpio_set_dir_flags-> > dm_gpio_clrset_flags, it only clear GPIOD_MASK_DIR, this has no impact with > GPIOD_ACTIVE_LOW. So this GPIOD_ACTIVE_LOW keep in flags. > > Then call i2c_gpio_get_pin by using dm_gpio_get_value(pin) will get > > the wrong data, let the function i2c_deblock_gpio_loop always return > > -EREMOTEIO > > > > Best Regards > > Haibo Chen > > > >> > >> The flag GPIOD_ACTIVE_LOW affects return value dm_gpio_get_value(). > >> And return value doesn’t depends on the DTS settings. It depends on > >> the flag passed to dm_gpio_set_dir_flags(). Usually the code parses > >> DTS and provide the correct flag to the dm_gpio_set_dir_flags(). > >> > >> So the patch adds flag GPIOD_ACTIVE_LOW to the > >> dm_gpio_set_dir_flags() and as expected this negates the return value > >> of dm_gpio_get_value(). So in order to keep the code correct the > >> patch also fixes adds negotiation to dm_gpio_get_value(). > >> > >> Alexander. > >> > >> > >> > >>> 20 марта 2023 г., в 10:44, Heiko Schocher написал(а): > >>> > >>> Hi! > >>> > >>> On 13.03.23 03:55, Bough Chen wrote: > >>>>> -Original Message- > >>>>> From: Bough Chen > >>>>> Sent: 2023年2月10日 17:27 > >>>>> To: h...@denx.de; al.koc...@gmail.com; ma...@denx.de > >>>>> Cc: u-boot@lists.denx.de; dl-uboot-imx ; > >>>>> xypron.g...@gmx.de; Bough Chen > >>>>> Subject: [PATCH] i2c: correct I2C deblock logic > >>>>> > >>>>>
RE: [PATCH 1/3] dm: adc: add iMX93 ADC support
> -Original Message-
> From: Luca Ellero
> Sent: 2023年3月22日 0:01
> To: u-boot@lists.denx.de; sba...@denx.de; feste...@gmail.com; dl-uboot-imx
> ; luca.ell...@brickedbrain.com; Ye Li ;
> Peng Fan ; Bough Chen
> Cc: Luca Ellero
> Subject: [PATCH 1/3] dm: adc: add iMX93 ADC support
>
> This commit adds driver for iMX93 ADC.
>
> The driver is implemented using driver model and provides ADC uclass's methods
> for ADC single channel operations:
> - adc_start_channel()
> - adc_channel_data()
> - adc_stop()
>
> ADC features:
> - channels: 4
> - resolution: 12-bit
>
> Signed-off-by: Luca Ellero
For this patch set:
Reviewed-by: Haibo Chen
Best Regards
Haibo Chen
> ---
> drivers/adc/Kconfig | 8 ++
> drivers/adc/Makefile| 1 +
> drivers/adc/imx93-adc.c | 290
>
> 3 files changed, 299 insertions(+)
> create mode 100644 drivers/adc/imx93-adc.c
>
> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index
> e719c38bb3..4336732dee 100644
> --- a/drivers/adc/Kconfig
> +++ b/drivers/adc/Kconfig
> @@ -63,3 +63,11 @@ config STM32_ADC
> - core driver to deal with common resources
> - child driver to deal with individual ADC resources (declare ADC
> device and associated channels, start/stop conversions)
> +
> +config ADC_IMX93
> + bool "Enable NXP IMX93 ADC driver"
> + help
> + This enables basic driver for NXP IMX93 ADC.
> + It provides:
> + - 4 analog input channels
> + - 12-bit resolution
> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile index
> c1387f3a34..5336c82097 100644
> --- a/drivers/adc/Makefile
> +++ b/drivers/adc/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_ADC_SANDBOX) += sandbox.o
> obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o
> obj-$(CONFIG_SARADC_MESON) += meson-saradc.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o stm32-adc-core.o
> +obj-$(CONFIG_ADC_IMX93) += imx93-adc.o
> diff --git a/drivers/adc/imx93-adc.c b/drivers/adc/imx93-adc.c new file mode
> 100644 index 00..41d04e0426
> --- /dev/null
> +++ b/drivers/adc/imx93-adc.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2023 ASEM Srl
> + * Author: Luca Ellero
> + *
> + * Originally based on NXP linux-imx kernel v5.15
> +drivers/iio/adc/imx93_adc.c */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IMX93_ADC_MCR0x00
> +#define IMX93_ADC_MSR0x04
> +#define IMX93_ADC_ISR0x10
> +#define IMX93_ADC_IMR0x20
> +#define IMX93_ADC_CIMR0 0x24
> +#define IMX93_ADC_CTR0 0x94
> +#define IMX93_ADC_NCMR0 0xA4
> +#define IMX93_ADC_PCDR0 0x100
> +#define IMX93_ADC_PCDR1 0x104
> +#define IMX93_ADC_PCDR2 0x108
> +#define IMX93_ADC_PCDR3 0x10c
> +#define IMX93_ADC_PCDR4 0x110
> +#define IMX93_ADC_PCDR5 0x114
> +#define IMX93_ADC_PCDR6 0x118
> +#define IMX93_ADC_PCDR7 0x11c
> +#define IMX93_ADC_CALSTAT0x39C
> +
> +#define IMX93_ADC_MCR_MODE_MASK BIT(29)
> +#define IMX93_ADC_MCR_NSTART_MASKBIT(24)
> +#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> +#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> +
> +#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> +#define IMX93_ADC_MSR_CALBUSY_MASK BIT(29)
> +#define IMX93_ADC_MSR_ADCSTATUS_MASK GENMASK(2, 0)
> +
> +#define IMX93_ADC_ISR_EOC_MASK BIT(1)
> +
> +#define IMX93_ADC_IMR_EOC_MASK BIT(1)
> +#define IMX93_ADC_IMR_ECH_MASK BIT(0)
> +
> +#define IMX93_ADC_PCDR_CDATA_MASKGENMASK(11, 0)
> +
> +#define IDLE 0
> +#define POWER_DOWN 1
> +#define WAIT_STATE 2
> +#define BUSY_IN_CALIBRATION 3
> +#define SAMPLE 4
> +#define CONVERSION 6
> +
> +#define IMX93_ADC_MAX_CHANNEL3
> +#define IMX93_ADC_DAT_MASK 0xfff
> +#define IMX93_ADC_TIMEOUT10
> +
> +struct imx93_adc_priv {
> + int active_channel;
> + void __iomem *regs;
> + struct clk ipg_clk;
> +};
> +
> +static void imx93_adc_power_down(struct imx93_adc_priv *adc) {
> + u32 mcr, msr;
> + int ret;
>
RE: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR
> -Original Message-
> From: Patrick DELAUNAY
> Sent: 2023年3月23日 3:11
> To: Bough Chen ; al.koc...@gmail.com; h...@denx.de;
> s...@chromium.org; and...@aj.id.au; patrice.chot...@foss.st.com;
> sam...@sholland.org; ma...@denx.de
> Cc: dl-uboot-imx ; u-boot@lists.denx.de
> Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR
>
> Hi
>
> On 3/22/23 12:26, haibo.c...@nxp.com wrote:
> > From: Haibo Chen
> >
> > dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
> > But there are cases like i2c_deblock_gpio_loop() will do like this:
> >
> > -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
> > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> >GPIOD_ACTIVE_LOW |
> >GPIOD_IS_OUT_ACTIVE);
> >
> > -then config GPIO input
> > dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >
> > -then get the GPIO input value:
> > dm_gpio_get_value(pin);
> >
> > When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
> > since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
> > flags, make the value from dm_gpio_get_value() not logic correct.
> >
> > So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > include/asm-generic/gpio.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index dd0bdf2315..903b237aac 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -131,7 +131,7 @@ struct gpio_desc {
> >
> > /* Flags for updating the above */
> > #define GPIOD_MASK_DIR(GPIOD_IS_OUT | GPIOD_IS_IN | \
> > - GPIOD_IS_OUT_ACTIVE)
> > +GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
> > #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN |
> GPIOD_OPEN_SOURCE)
> > #define GPIOD_MASK_PULL (GPIOD_PULL_UP |
> GPIOD_PULL_DOWN)
> >
>
>
> I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by
> device tree in the GPIO uclass:
>
> because the modified GPIOD_MASK_DIR is used in other location
>
>
> normally GPIOD_ACTIVE_LOW is saved in desc->flags
>
> it is the "desciptor flags" and must be not cleary by normal API
>
>
> see gpio_xlate_offs_flags() => gpio_flags_xlate()
>
>
>
> For example in gpio_request_tail(), in the line:
>
> /* Keep any direction flags provided by the devicetree */ ret =
> dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With
> your patch the descriptor flags is cleared / so DT information is lost.
> For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect.
> and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is
> normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class =>
> dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can
> change the caller ?
> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) { if (bit)
> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); else dm_gpio_set_dir_flags(pin,
> GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } =>
>
> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> if (bit)
> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> else
> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT); }
Here, for i2c-deblock, when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), the
software actually want to config the gpio at output, and config the output to
low level.
This means in dts, need to config the i2c gpio as GPIO_ACTIVE_HIGH, then when
call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), it finally config the value to
0, which means low level.
But from user point, we usually take the i2c gpio as GPIO_ACTIVE_LOW, seems a
bit conflict.
Any thoughts? Or just use my first patch?
Best Regards
Haibo Chen
>
> The output value is the same => GPIOD_ACTIVE_LOW and
> GPIOD_IS_OUT_ACTIVE not active but you don't need to modify
> GPIOD_ACTIVE_LOW outside the GPIO uclass.
> Patrick
RE: [PATCH] i2c: correct I2C deblock logic
> -Original Message-
> From: Alexander Kochetkov
> Sent: 2023年3月23日 17:34
> To: Bough Chen
> Cc: h...@denx.de; ma...@denx.de; u-boot@lists.denx.de; dl-uboot-imx
> ; xypron.g...@gmx.de
> Subject: Re: [PATCH] i2c: correct I2C deblock logic
>
> Or even simpler. Like your original patch. If we take into accout Patrik’s
> comment from another message:
>
> > but if I assume that GPIO_ACTIVE_HIGH is NOT activated in DT
>
> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
>if (bit) {
>dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>} else {
>dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
>}
> }
>
> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
> return !dm_gpio_get_value(pin);
> }
If use this code, must make sure dts use GPIO_ACTIVE_LOW, otherwise
dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE) will config gpio
output high, this is not what we want.
And here use !dm_gpio_get_value(pin), this already hit use GPIO_ACTIVE_LOW,
why not directly use GPIOD_ACTIVE_LOW
when call dm_gpio_set_dir_flags, this make code more readable, and make sure
the code logic can work and do not depends
on dts setting.
Best Regards
Haibo Chen
>
>
> > 23 марта 2023 г., в 11:43, Alexander Kochetkov
> написал(а):
> >
> > Hello Haibo Chen!
> >
> > Setting GPIOD_ACTIVE_LOW has no effect. It filtered out by
> dm_gpio_set_dir_flags().
> >>
> >>
> >> if (bit)
> >> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
> >> +GPIOD_ACTIVE_LOW);
> >
> > Here in original code GPIOD_ACTIVE_LOW has not effect.
> >
> > else
> >dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> >
> GPIOD_ACTIVE_LOW |
> >
> GPIOD_IS_OUT_ACTIVE);
> >
> >
> > This code actually fix problem with your DTS-settings:
> >> + return !dm_gpio_get_value(pin);
> >
> >
> > Can you debug and check using oscilloscope the code like this:
> >
> > static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> >ulong flags;
> >ulong active;
> >
> >if (bit) {
> >dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >} else {
> >dm_gpio_get_flags(pin, &flags);
> >active = (flags & GPIOD_ACTIVE_LOW) ? GPIOD_IS_OUT_ACTIVE :
> 0;
> >dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | active);
> >}
> > }
> >
> > static int i2c_gpio_get_pin(struct gpio_desc *pin) {
> > ulong flags;
> > int value;
> >
> > value = dm_gpio_get_value(pin);
> > return (flags & GPIOD_ACTIVE_LOW) ? !value : value; }
> >
> >
> >> 10 февр. 2023 г., в 12:27, haibo.c...@nxp.com написал(а):
> >>
> >> From: Haibo Chen
> >>
> >> Current code use dm_gpio_get_value() to get SDA and SCL value, and
> >> the value depends on the flag GPIOD_ACTIVE_LOW. When toggle SCL to
> >> wait slave release SDA, the SDA are config as GPIOD_IS_IN, and
> >> whether contain the GPIOD_ACTIVE_LOW depends on the DTS setting.
> >> Usually, for I2C GPIO, we use GPIOD_ACTIVE_LOW flag in DTS, so if the
> >> SDA is in low level, then
> >> dm_gpio_get_value() will return 1, current code logic will stop the
> >> SCL toggle wrongly, cause the I2C deblock not work as expect.
> >>
> >> This patch force set the GPIOD_ACTIVE_LOW for both GPIOD_IS_IN and
> >> GPIOD_IS_OUT, and make the return value of i2c_gpio_get_pin() eaqual
> >> to the physical voltage logic level.
> >>
> >> Fixes: aa54192d4a87 ("dm: i2c: implement gpio-based I2C deblock")
> >> Signed-off-by: Haibo Chen
> >> ---
> >> drivers/i2c/i2c-uclass.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> >> index 8d9a89ed89..4dc707c659 100644
> >> --- a/drivers/i2c/i2c-uclass.c
> >> +++ b/drivers/i2c/i2c-uclass.c
> >> @@ -505,7 +505,8 @@ uint i2c_get_chip_addr_offset_mask(struct udevice
> >> *dev) static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> >> if (bit)
> >> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
> >> + GPIOD_ACTIVE_LOW);
> >> else
> >> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | @@
> >> -514,7 +515,7 @@ static void i2c_gpio_set_pin(struct gpio_desc *pin,
> >> int bit)
> >>
> >> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
> >> - return dm_gpio_get_value(pin);
> >> + return !dm_gpio_get_value(pin);
> >> }
> >>
> >> int i2c_deblock_gpio_loop(struct gpio_desc *sda_pin,
> >> --
> >> 2.34.1
> >>
> >
RE: [PATCH 1/2] dm: adc: add iMX93 ADC support
> -Original Message-
> From: Luca Ellero
> Sent: 2023年7月5日 20:56
> To: u-boot@lists.denx.de; sba...@denx.de; feste...@gmail.com; dl-uboot-imx
> ; luca.ell...@brickedbrain.com; Ye Li ;
> Peng Fan ; Bough Chen
> Cc: Luca Ellero
> Subject: [PATCH 1/2] dm: adc: add iMX93 ADC support
>
> This commit adds driver for iMX93 ADC.
>
> The driver is implemented using driver model and provides ADC uclass's methods
> for ADC single channel operations:
> - adc_start_channel()
> - adc_channel_data()
> - adc_stop()
>
> ADC features:
> - channels: 4
> - resolution: 12-bit
>
> Signed-off-by: Luca Ellero
Reviewed-by: Haibo Chen
Best Regards
Haibo Chen
> ---
> drivers/adc/Kconfig | 8 ++
> drivers/adc/Makefile| 1 +
> drivers/adc/imx93-adc.c | 290
>
> 3 files changed, 299 insertions(+)
> create mode 100644 drivers/adc/imx93-adc.c
>
> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index
> e719c38bb3..4336732dee 100644
> --- a/drivers/adc/Kconfig
> +++ b/drivers/adc/Kconfig
> @@ -63,3 +63,11 @@ config STM32_ADC
> - core driver to deal with common resources
> - child driver to deal with individual ADC resources (declare ADC
> device and associated channels, start/stop conversions)
> +
> +config ADC_IMX93
> + bool "Enable NXP IMX93 ADC driver"
> + help
> + This enables basic driver for NXP IMX93 ADC.
> + It provides:
> + - 4 analog input channels
> + - 12-bit resolution
> diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile index
> c1387f3a34..5336c82097 100644
> --- a/drivers/adc/Makefile
> +++ b/drivers/adc/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_ADC_SANDBOX) += sandbox.o
> obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o
> obj-$(CONFIG_SARADC_MESON) += meson-saradc.o
> obj-$(CONFIG_STM32_ADC) += stm32-adc.o stm32-adc-core.o
> +obj-$(CONFIG_ADC_IMX93) += imx93-adc.o
> diff --git a/drivers/adc/imx93-adc.c b/drivers/adc/imx93-adc.c new file mode
> 100644 index 00..41d04e0426
> --- /dev/null
> +++ b/drivers/adc/imx93-adc.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2023 ASEM Srl
> + * Author: Luca Ellero
> + *
> + * Originally based on NXP linux-imx kernel v5.15
> +drivers/iio/adc/imx93_adc.c */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IMX93_ADC_MCR0x00
> +#define IMX93_ADC_MSR0x04
> +#define IMX93_ADC_ISR0x10
> +#define IMX93_ADC_IMR0x20
> +#define IMX93_ADC_CIMR0 0x24
> +#define IMX93_ADC_CTR0 0x94
> +#define IMX93_ADC_NCMR0 0xA4
> +#define IMX93_ADC_PCDR0 0x100
> +#define IMX93_ADC_PCDR1 0x104
> +#define IMX93_ADC_PCDR2 0x108
> +#define IMX93_ADC_PCDR3 0x10c
> +#define IMX93_ADC_PCDR4 0x110
> +#define IMX93_ADC_PCDR5 0x114
> +#define IMX93_ADC_PCDR6 0x118
> +#define IMX93_ADC_PCDR7 0x11c
> +#define IMX93_ADC_CALSTAT0x39C
> +
> +#define IMX93_ADC_MCR_MODE_MASK BIT(29)
> +#define IMX93_ADC_MCR_NSTART_MASKBIT(24)
> +#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> +#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> +
> +#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> +#define IMX93_ADC_MSR_CALBUSY_MASK BIT(29)
> +#define IMX93_ADC_MSR_ADCSTATUS_MASK GENMASK(2, 0)
> +
> +#define IMX93_ADC_ISR_EOC_MASK BIT(1)
> +
> +#define IMX93_ADC_IMR_EOC_MASK BIT(1)
> +#define IMX93_ADC_IMR_ECH_MASK BIT(0)
> +
> +#define IMX93_ADC_PCDR_CDATA_MASKGENMASK(11, 0)
> +
> +#define IDLE 0
> +#define POWER_DOWN 1
> +#define WAIT_STATE 2
> +#define BUSY_IN_CALIBRATION 3
> +#define SAMPLE 4
> +#define CONVERSION 6
> +
> +#define IMX93_ADC_MAX_CHANNEL3
> +#define IMX93_ADC_DAT_MASK 0xfff
> +#define IMX93_ADC_TIMEOUT10
> +
> +struct imx93_adc_priv {
> + int active_channel;
> + void __iomem *regs;
> + struct clk ipg_clk;
> +};
> +
> +static void imx93_adc_power_down(struct imx93_adc_priv *adc) {
> + u32 mcr, msr;
> + int ret;
> +
> + mcr = re
RE: [PATCH 1/1] dm: adc: imx93-adc depends on ADC (fix boot)
> -Original Message-
> From: Sébastien Szymanski
> Sent: 2023年7月24日 23:18
> To: u-boot@lists.denx.de
> Cc: Bough Chen ; Luca Ellero ;
> Sébastien Szymanski
> Subject: [PATCH 1/1] dm: adc: imx93-adc depends on ADC (fix boot)
>
> The i.MX93 11x11 EVK fails to boot with following error:
>
> Model: NXP i.MX93 11X11 EVK board
> DRAM: 2 GiB
> Error binding driver 'imx93-adc': -96
> Some drivers failed to bind
> Error binding driver 'simple_bus': -96
> Some drivers failed to bind
> Error binding driver 'simple_bus': -96
> Some drivers failed to bind
> initcall sequence fffb8f28 failed at call 8021e0d4 (err=-96)
> ### ERROR ### Please RESET the board ###
>
> That's because since commit e7ff54d96303 ("imx93_evk: defconfig: add adc
> support") CONFIG_ADC_IMX93 is enabled but CONFIG_ADC is not.
> Fix this by making CONFIG_ADC_IMX93 depend on CONFIG_ADC.
>
> Signed-off-by: Sébastien Szymanski
Reviewed-by: Haibo Chen
> ---
> drivers/adc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index
> 4336732dee56..a01d73846b74 100644
> --- a/drivers/adc/Kconfig
> +++ b/drivers/adc/Kconfig
> @@ -66,6 +66,7 @@ config STM32_ADC
>
> config ADC_IMX93
> bool "Enable NXP IMX93 ADC driver"
> + depends on ADC
> help
> This enables basic driver for NXP IMX93 ADC.
> It provides:
> --
> 2.41.0
RE: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"
> -Original Message-
> From: Fabio Estevam [mailto:feste...@gmail.com]
> Sent: 2021年6月8日 4:40
> To: Peng Fan
> Cc: Bough Chen ; u-boot@lists.denx.de;
> thar...@gateworks.com; Fabio Estevam
> Subject: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output"
>
> This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.
>
> Since this commit a imx6qdl-pico board boots extremely slowly in both SPL
> as well as U-Boot proper.
>
> Fix this regression by reverting the offending commit for now.
Hi Fabio,
Force clock on did help us fix some issue, like voltage switch for UHS card.
According to your commit log, seems this patch affect the booting time, do
you mean
the API readx_poll_timeout() cost a lot time? Can you show us the detail
info about
booting time effected by this patch?
Regards,
Haibo Chen
>
> Signed-off-by: Fabio Estevam
> ---
> Changes since v1:
> - Include the author of the offending commit on Cc.
>
> drivers/mmc/fsl_esdhc_imx.c | 29 -
> include/fsl_esdhc_imx.h | 2 --
> 2 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index a4675838e58a..b49d5ede2711 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -653,10 +653,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv,
> struct mmc *mmc, uint clock)
> clk = (pre_div << 8) | (div << 4);
>
> #ifdef CONFIG_FSL_USDHC
> - esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> - ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> - if (ret)
> - pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n");
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN);
> #else
> esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif @@ -668,7
> +665,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc
> *mmc, uint clock)
> pr_warn("fsl_esdhc_imx: Internal clock never
stabilised.\n");
>
> #ifdef CONFIG_FSL_USDHC
> - esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN |
> +VENDORSPEC_CKEN);
> #else
> esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> #endif @@ -723,14 +720,8 @@ static void esdhc_set_strobe_dll(struct mmc
> *mmc)
> struct fsl_esdhc_priv *priv = dev_get_priv(mmc->dev);
> struct fsl_esdhc *regs = priv->esdhc_regs;
> u32 val;
> - u32 tmp;
> - int ret;
>
> if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) {
> - esdhc_clrbits32(®s->vendorspec,
> VENDORSPEC_FRC_SDCLK_ON);
> - ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
tmp
> & PRSSTAT_SDOFF, 100);
> - if (ret)
> - pr_warn("fsl_esdhc_imx: Internal clock never gate
off.\n");
> esdhc_write32(®s->strobe_dllctrl,
> ESDHC_STROBE_DLL_CTRL_RESET);
>
> /*
> @@ -748,7 +739,6 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
> pr_warn("HS400 strobe DLL status REF not lock!\n");
> if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK))
> pr_warn("HS400 strobe DLL status SLV not lock!\n");
> - esdhc_setbits32(®s->vendorspec,
> VENDORSPEC_FRC_SDCLK_ON);
> }
> }
>
> @@ -980,18 +970,14 @@ static int esdhc_set_ios_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc) #ifdef MMC_SUPPORTS_TUNING
> if (mmc->clk_disable) {
> #ifdef CONFIG_FSL_USDHC
> - u32 tmp;
> -
> - esdhc_clrbits32(®s->vendorspec,
> VENDORSPEC_FRC_SDCLK_ON);
> - ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp,
tmp
> & PRSSTAT_SDOFF, 100);
> - if (ret)
> - pr_warn("fsl_esdhc_imx: Internal clock never gate
off.\n");
> + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN);
> #else
> esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif
> } else {
> #ifdef CONFIG_FSL_USDHC
> - esdhc_setbits32(®s->vendorspec,
> VENDORSPEC_FRC_SDCLK_ON);
> + esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN |
> + VENDORSPEC_CKEN);
> #else
> esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> #endif @@ -1067,7 +1053,7 @@ static int esdhc_init_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc) #ifndef CONFIG_FSL_USDHC
>
RE: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"
> -Original Message-
> From: Jaehoon Chung [mailto:jh80.ch...@samsung.com]
> Sent: 2021年6月9日 7:35
> To: Fabio Estevam
> Cc: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de; thar...@gateworks.com
> Subject: Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output"
>
> Hi Fabio,
>
> On 6/9/21 6:47 AM, Fabio Estevam wrote:
> > Hi Jaehoon,
> >
> > On Mon, Jun 7, 2021 at 11:50 PM Jaehoon Chung
> wrote:
> >
> >> Is your target success to boot finally after 20sec?
> >
> > Yes, it boots fine after 20s.
> >
> >> I didn't have target to use fsl_esdhc_imx driver.
> >>
> >> If booted after 20sec, how about below code?
> >>
> >> #ifdef CONFIG_FSL_ESDHC_IMX_TIMEOUT
> >> #define FSL_ESDHC_IMX_TIMEOUT
> CONFIG_FSL_ESDHC_IMX_TIMEOUT
> >> #else
> >> #define FSL_ESDHC_IMX_TIMEOUT 0
> >> #endif
> >>
> >>
> >> ret = readx_poll_timeout(..., FSL_ESDHC_IMX_TIMEOUT); if
> >> (FSL_ESDHC_IMX_TIMEOUT && ret)
> >> pr_warn("...");
> >
> > Thanks for the suggestion, but it didn't work.
>
> If i find a board to use fs_esdhc_imx driver, I will check more.
> I hope that Bough will suggest better solution to satisfy both case.
Give me a few days, I'm busy these days, will do when I have time.
Best Regards
Bough
>
> Best Regards,
> Jaehoon Chung
>
> >
smime.p7s
Description: S/MIME cryptographic signature
RE: IMX8MM SD UHS support
Hi Tim and Andrey
I can reproduce this issue on my side, after revert my patch which you point
out, SD can work on SDR104 mode.
Till now, I do not know why wait for data0 status will cause this issue. Give
me more time, I will try to dig that out.
Again, thanks for report this issue.
Haibo
> -Original Message-
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> Sent: 2021年1月20日 4:48
> To: thar...@gateworks.com
> Cc: Bough Chen ; Adam Ford
> ; Fabio Estevam ; Peng Fan
> ; u-boot ; Stefano Babic
> ; Jaehoon Chung
> Subject: RE: IMX8MM SD UHS support
>
> Hello Tim,
>
> > -Original Message-
> > From: Tim Harvey
> > Sent: Tuesday, January 19, 2021 6:32 PM
> > To: ZHIZHIKIN Andrey
> > Cc: haibo.c...@nxp.com; Adam Ford ; Fabio
> Estevam
> > ; Peng Fan ; u-boot > b...@lists.denx.de>; Stefano Babic ; Jaehoon Chung
> >
> > Subject: Re: IMX8MM SD UHS support
> >
> > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey
> > wrote:
> > >
> > > Hello Tim,
> > >
> > > Sorry it took me quite some time to get this sorted out, but I
> > > believe I was able
> > to identify an offending commit that is preventing the USDHC to switch
> > to higher speed modes.
> > >
> > > It is in fact b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0()
> > > support"),
> > reverting it makes SD Card to properly report capabilities and switch
> > to high speed modes.
> > >
> > > Can you try to revert this on your end to see if the SD Card would
> > > start to
> > operate in high speed mode?
> > >
> > > I'm still investigating why this addition of wait_dat0() caused
> > > this, I believe this
> > is due to the fact that the same wait is already performed while
> > voltage switch to handle the errata, thus this addition wait might
> erroneously timeout.
> > >
> > > ++ Haibo Chen
> > >
> > > Haibo,
> > >
> > > Can you please explain the purpose of adding wait_dat0() Introduced
> > > with
> > commit b5874b552f? It is not clear from the commit message what was
> > the purpose of adding it. Have you tested the USDHC switch to higher
> > modes with this change?
> >
> > Andrey,
> >
> > Reverting b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0() support")
> > does not resolve the issue. That function waits for a specified
> > timeout for the card to assert DAT[0] either high or low depending on
> > arg and is used to check for command busy or failure. The patch in
> > question adds that function so that the common mmc code can utilize
> > it. If the function does not exist in the host controller driver any
> > call to mmc_wait_dat0 returns -ENOSYS so it must be there to support
> > UHS anyway.
>
> Perhaps, this is due to the fact that the same wait cycle is already executed
> during the invocation of mmc_send_cmd above the
> mmc_wait_dat0() in drivers/mmc/mmc.c.
>
> mmc_send_cmd calls esdhc_send_cmd_common in
> drivers/mmc/fsl_esdhc_imx.c, which already has a wait loop implemented to
> cover the "Workaround for ESDHC errata ENGcm03648"
> (as seen from the comment), which might explain why consecutive wait fails
> to return within timeout value.
>
> >
> > What is not clear to me is why the card would hold DAT[0] low on the
> > voltage switch indicating a failure as it does not in the Linux driver
> > which appears to me to be doing the same thing.
>
> This behavior might be explained by the implementation I traced above.
>
> > Again, if we ignore the return code UHS works fine and I'm not at all
> > clear why you wouldn't run into this issue:
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> > a6394bc..5ea3cd2 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -579,10 +579,12 @@ static int mmc_switch_voltage(struct mmc
> *mmc,
> > int signal_voltage)
> > * dat[0:3] low. Wait for at least 1 ms according to spec
> > */
> > err = mmc_wait_dat0(mmc, 1, 1000);
> > +#if 0 // hack: not clear why card always holds DAT[0] high here on
> > fsl_esdhc_imx
> > if (err == -ENOSYS)
> > udelay(1000);
> > else if (err)
> > return -ETIMEDOUT;
> > +#endif
> >
> > return 0;
> > }
>
> This is expected. When the wait_dat0 callback is undefined in dm_mmc_ops
> structure - it defaults to return -ENOSYS, which effectively just inhibit a
> dela
RE: IMX8MM SD UHS support
Hi Tim and Andrey,
I find the root cause, it is because during the 1.8v voltage switch process, SD
clock do not gate off/ on according to the SD spec.
For i.MX usdhc, if want to gate off the sd card clock, need to clear the bit 8
of VEND_SPEC, and set this bit to gate on the sd card clock.
For the bit[14:11] of VEND_SPEC, our IP do not implement the function, just
mark as reserved bits.
So for current logic, we'll see the second mmc_wait_dat0() in
mmc_switch_voltage() always return timeout.
Let me double confirm with our IC team, and will send a formal fix patch
tomorrow.
Haibo
> -Original Message-
> From: Bough Chen
> Sent: 2021年1月22日 20:42
> To: 'ZHIZHIKIN Andrey' ;
> thar...@gateworks.com
> Cc: Adam Ford ; Fabio Estevam
> ; Peng Fan ; u-boot
> ; Stefano Babic ; Jaehoon Chung
>
> Subject: RE: IMX8MM SD UHS support
>
> Hi Tim and Andrey
>
> I can reproduce this issue on my side, after revert my patch which you point
> out,
> SD can work on SDR104 mode.
> Till now, I do not know why wait for data0 status will cause this issue. Give
> me
> more time, I will try to dig that out.
>
> Again, thanks for report this issue.
>
> Haibo
>
> > -Original Message-
> > From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> > Sent: 2021年1月20日 4:48
> > To: thar...@gateworks.com
> > Cc: Bough Chen ; Adam Ford
> ;
> > Fabio Estevam ; Peng Fan ;
> > u-boot ; Stefano Babic ; Jaehoon
> > Chung
> > Subject: RE: IMX8MM SD UHS support
> >
> > Hello Tim,
> >
> > > -Original Message-
> > > From: Tim Harvey
> > > Sent: Tuesday, January 19, 2021 6:32 PM
> > > To: ZHIZHIKIN Andrey
> > > Cc: haibo.c...@nxp.com; Adam Ford ; Fabio
> > Estevam
> > > ; Peng Fan ; u-boot > > b...@lists.denx.de>; Stefano Babic ; Jaehoon Chung
> > >
> > > Subject: Re: IMX8MM SD UHS support
> > >
> > > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey
> > > wrote:
> > > >
> > > > Hello Tim,
> > > >
> > > > Sorry it took me quite some time to get this sorted out, but I
> > > > believe I was able
> > > to identify an offending commit that is preventing the USDHC to
> > > switch to higher speed modes.
> > > >
> > > > It is in fact b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0()
> > > > support"),
> > > reverting it makes SD Card to properly report capabilities and
> > > switch to high speed modes.
> > > >
> > > > Can you try to revert this on your end to see if the SD Card would
> > > > start to
> > > operate in high speed mode?
> > > >
> > > > I'm still investigating why this addition of wait_dat0() caused
> > > > this, I believe this
> > > is due to the fact that the same wait is already performed while
> > > voltage switch to handle the errata, thus this addition wait might
> > erroneously timeout.
> > > >
> > > > ++ Haibo Chen
> > > >
> > > > Haibo,
> > > >
> > > > Can you please explain the purpose of adding wait_dat0()
> > > > Introduced with
> > > commit b5874b552f? It is not clear from the commit message what was
> > > the purpose of adding it. Have you tested the USDHC switch to higher
> > > modes with this change?
> > >
> > > Andrey,
> > >
> > > Reverting b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0() support")
> > > does not resolve the issue. That function waits for a specified
> > > timeout for the card to assert DAT[0] either high or low depending
> > > on arg and is used to check for command busy or failure. The patch
> > > in question adds that function so that the common mmc code can
> > > utilize it. If the function does not exist in the host controller
> > > driver any call to mmc_wait_dat0 returns -ENOSYS so it must be there
> > > to support UHS anyway.
> >
> > Perhaps, this is due to the fact that the same wait cycle is already
> > executed during the invocation of mmc_send_cmd above the
> > mmc_wait_dat0() in drivers/mmc/mmc.c.
> >
> > mmc_send_cmd calls esdhc_send_cmd_common in
> > drivers/mmc/fsl_esdhc_imx.c, which already has a wait loop implemented
> > to cover the "Workaround for ESDHC errata ENGcm03648"
> > (as seen from the comment), which might explain why consecutive wait
> > fails to return within timeout value.
> >
> > >
> > > What
RE: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
> -Original Message-
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> Sent: 2021年2月1日 17:52
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de
> Cc: dl-uboot-imx ; thar...@gateworks.com
> Subject: RE: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON
> to control card clock output
>
> Hello Haibo,
>
> > -Original Message-
> > From: haibo.c...@nxp.com
> > Sent: Wednesday, January 27, 2021 11:47 AM
> > To: peng@nxp.com; u-boot@lists.denx.de
> > Cc: haibo.c...@nxp.com; uboot-...@nxp.com; thar...@gateworks.com;
> > ZHIZHIKIN Andrey
> > Subject: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to
> > control card clock output
> >
> > This email is not from Hexagon’s Office 365 instance. Please be
> > careful while clicking links, opening attachments, or replying to this
> > email.
> >
> >
> > From: Haibo Chen
> >
> > For FSL_USDHC, it do not implement
> VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
> > these are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate
> > on/off the card clock output.
> >
> > After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0()
> > support"), we meet SD3.0 card can't work at UHS mode,
> > mmc_switch_voltage() fail because the second mmc_wait_dat0 return
> > -ETIMEDOUT. According to SD spec, during voltage switch, need to gate
> > off/on the card clock. If not set the FRC_SDCLK_ON, after CMD11,
> > hardware will gate off the card clock automatically, so card do not
> > detect the clock off/on behavior, so will draw the data0 line low until next
> command.
> >
> > Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support")
>
> This patch does not fix the switch of uSDHC to 1v8...
>
> I've applied it locally on the imx8mmevk, and had following log during the
> boot
> when tried to query SD Card info:
> ---
> U-Boot SPL 2021.01-01004-gb852007333 (Feb 01 2021 - 09:45:42 +0100)
> Normal Boot
> WDT: Started with servicing (60s timeout)
> Trying to boot from MMC1
> NOTICE: BL31: v2.2(release):rel_imx_5.4.70_2.3.0-0-gf1d7187f2
> NOTICE: BL31: Built : 22:29:05, Jan 17 2021
>
>
> U-Boot 2021.01-01004-gb852007333 (Feb 01 2021 - 09:45:42 +0100)
>
> CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz
> Reset cause: POR
> Model: FSL i.MX8MM EVK board
> DRAM: 2 GiB
> WDT: Started with servicing (60s timeout)
> MMC: FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... Run CMD11 1.8V switch Card did not
> respond to voltage select! : -110
This do not align with my test result. Can you help identify which function
first return the timeout on your side?
Or can you try a different SD card?
Best Regards
Haibo
> *** Warning - No block device, using default environment
>
> In:serial
> Out: serial
> Err: serial
> Net: eth0: ethernet@30be
> Hit any key to stop autoboot: 0
> u-boot=> mmc dev 1
> Card did not respond to voltage select! : -110 u-boot=> mmc info MMC Device
> 0 not found no mmc device at slot 0 u-boot=> mmc dev 2 switch to partitions
> #0,
> OK mmc2(part 0) is current device u-boot=> mmc info
> Device: FSL_SDHC
> Manufacturer ID: 45
> OEM: 100
> Name: DG401
> Bus Speed: 2
> Mode: HS400ES (200MHz)
> Rd Block Len: 512
> MMC version 5.1
> High Capacity: Yes
> Capacity: 14.7 GiB
> Bus Width: 8-bit DDR
> Erase Group Size: 512 KiB
> HC WP Group Size: 8 MiB
> User Capacity: 14.7 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
> ---
>
> Note, that eMMC is not affected and is operating in HS400ES mode without any
> issues.
>
> Reverting patch b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support")
> with this patch applied in tree does switch the SD Card to high-speed mode,
> following log is produced:
> ---
> U-Boot SPL 2021.01-01005-gb8aeb689a2 (Feb 01 2021 - 10:38:01 +0100)
> Normal Boot
> WDT: Started with servicing (60s timeout)
> Trying to boot from MMC1
> NOTICE: BL31: v2.2(release):rel_imx_5.4.70_2.3.0-0-gf1d7187f2
> NOTICE: BL31: Built : 22:29:05, Jan 17 2021
>
>
> U-Boot 2021.01-01005-gb8aeb689a2 (Feb 01 2021 - 10:38:01 +0100)
>
> CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz
> Reset cause: POR
> Model: FSL i.MX8MM EVK board
> DRAM: 2 GiB
> WDT: Started with servicing (60s timeout)
> MMC: FSL_SDHC: 1, FSL_SDHC: 2
> Loading Env
RE: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
> -Original Message-
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> Sent: 2021年2月1日 19:41
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de
> Cc: dl-uboot-imx ; thar...@gateworks.com
> Subject: RE: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON
> to control card clock output
>
> Hello Haibo,
>
> > -----Original Message-
> > From: Bough Chen
> > Sent: Monday, February 1, 2021 11:10 AM
> > To: ZHIZHIKIN Andrey ; Peng Fan
> > ; u-boot@lists.denx.de
> > Cc: dl-uboot-imx ; thar...@gateworks.com
> > Subject: RE: [PATCH] mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON
> > to control card clock output
> >
> > > -Original Message-
> > > From: ZHIZHIKIN Andrey
> > > [mailto:andrey.zhizhi...@leica-geosystems.com]
> > > Sent: 2021年2月1日 17:52
> > > To: Bough Chen ; Peng Fan ;
> > > u-boot@lists.denx.de
> > > Cc: dl-uboot-imx ; thar...@gateworks.com
> > > Subject: RE: [PATCH] mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON
> > > to control card clock output
> > >
> > > Hello Haibo,
> > >
> > > > -Original Message-
> > > > From: haibo.c...@nxp.com
> > > > Sent: Wednesday, January 27, 2021 11:47 AM
> > > > To: peng@nxp.com; u-boot@lists.denx.de
> > > > Cc: haibo.c...@nxp.com; uboot-...@nxp.com;
> thar...@gateworks.com;
> > > > ZHIZHIKIN Andrey
> > > > Subject: [PATCH] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON
> > > > to control card clock output
> > > >
> > > > From: Haibo Chen
> > > >
> > > > For FSL_USDHC, it do not implement
> > > VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN,
> > > > these are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to
> > > > gate on/off the card clock output.
> > > >
> > > > After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0()
> > > > support"), we meet SD3.0 card can't work at UHS mode,
> > > > mmc_switch_voltage() fail because the second mmc_wait_dat0 return
> > > > -ETIMEDOUT. According to SD spec, during voltage switch, need to
> > > > gate off/on the card clock. If not set the FRC_SDCLK_ON, after
> > > > CMD11, hardware will gate off the card clock automatically, so
> > > > card do not detect the clock off/on behavior, so will draw the
> > > > data0 line low until next
> > > command.
> > > >
> > > > Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0()
> > > > support")
> > >
> > > This patch does not fix the switch of uSDHC to 1v8...
> > >
> > > I've applied it locally on the imx8mmevk, and had following log
> > > during the boot when tried to query SD Card info:
> > > ---
> > > U-Boot SPL 2021.01-01004-gb852007333 (Feb 01 2021 - 09:45:42 +0100)
> > > Normal Boot
> > > WDT: Started with servicing (60s timeout)
> > > Trying to boot from MMC1
> > > NOTICE: BL31: v2.2(release):rel_imx_5.4.70_2.3.0-0-gf1d7187f2
> > > NOTICE: BL31: Built : 22:29:05, Jan 17 2021
> > >
> > >
> > > U-Boot 2021.01-01004-gb852007333 (Feb 01 2021 - 09:45:42 +0100)
> > >
> > > CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz
> > > Reset cause: POR
> > > Model: FSL i.MX8MM EVK board
> > > DRAM: 2 GiB
> > > WDT: Started with servicing (60s timeout)
> > > MMC: FSL_SDHC: 1, FSL_SDHC: 2
> > > Loading Environment from MMC... Run CMD11 1.8V switch Card did not
> > > respond to voltage select! : -110
> >
> > This do not align with my test result. Can you help identify which
> > function first return the timeout on your side?
>
> I would have a look at the exact function, but it seems to me that it would be
> wait_dat0() since if I revert the patch introducing it - high speed mode
> switch is
> not timing out.
>
> > Or can you try a different SD card?
>
> It is rather strange, it seems like it is dependent on the SD Card used.
>
> So far, I've tried 3 SD Cards I had on hands, one of which being operable:
> == Working:
> "Transcend 32GB"
> Manufacturer ID: 74
> OEM: 4a60
> Name: USDU1
>
> == Failed:
> 1. (Kingston 32GB)
> Manufacturer ID: 41
> OEM: 3432
> Name: SD32G
>
> 2. (Intenso 32 GB)
> Manufacturer ID: 9f
> OEM: 5449
> Name: 0
>
Hi Andrey,
With this patch, can you also add the fo
RE: [EXT] Re: [PATCH 4/4] imx8mq_evk: Enable the USB3.0 host port
> -Original Message-
> From: Ye Li
> Sent: 2021年2月27日 14:05
> To: feste...@gmail.com; Bough Chen
> Cc: Peng Fan ; u-boot@lists.denx.de; dl-uboot-imx
> ; sba...@denx.de
> Subject: Re: [EXT] Re: [PATCH 4/4] imx8mq_evk: Enable the USB3.0 host port
>
> Hi Fabio,
>
> On Thu, 2021-02-25 at 10:49 -0300, Fabio Estevam wrote:
> > Caution: EXT Email
> >
> > Hi Ye Li,
> >
> > On Thu, Feb 25, 2021 at 10:34 AM Ye Li wrote:
> >
> > >
> > > Sure, I have tested it on 8mq evk. I can reproduce the two issues
> > > you met.
> > > The first issue is caused by the ALIGN. The implementation of
> > > standard ALIGN requires the aligned size to be power of 2. But the
> > > ALIGN in imx8mimage does not have this requirement. So below result
> > > is wrong by using the standard ALIGN. Your fix should be OK for this
> > > issue.
> > Good, could you please reply to my ALIGN macro patch with your
> > Tested-by tag then?
> >
> Replied it.
>
> > >
> > > For the second issue, I did not debug into it. But our vendor tree
> > > also uses off-on-delay-us in both u-boot and kernel. So it is likely
> > > caused by other change.
> > Considering we are already at 2021.04-rc2, I think it would be safer
> > to go with my patch that removes off-on-delay-us.
> >
> > What do you think?
> >
> > Thanks
> My debug shows the issue is triggered by below commit:
>
> commit 9098682200e6cca4b776638a51200dafa16f50fb
> Author: Haibo Chen
> Date: Tue Sep 22 18:11:43 2020 +0800
>
> mmc: fsl_esdhc_imx: remove the 1ms delay before sending command
>
> This 1ms delay before sending command already exist from the beginning
> of the fsl_esdhc driver added in year 2008. Now this driver has been
> split for two files: fsl_esdhc.c and fsl_esdhc_imx.c.
> fsl_esdhc_imx.c
> only for i.MX series. i.MX series esdhc/usdhc do not need this 1ms delay
> before sending any command. So remove this 1ms, this will save a lot
> time if handling a large mmc data.
>
> Signed-off-by: Haibo Chen
>
>
> The first "go idle" command in mmc_get_op_cond seems not put SD card to
> idle status, but if adding a delay before it (like 1ms delay), then everything
> works. This commit removed 1ms delay in sending command, so the issue is
> triggered. The root cause might be "startup-delay-us"
> needed for this regulator to reach a threshold voltage for SD working.
> Below change also can fix the issue.
>
> --- a/arch/arm/dts/imx8mq-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-evk-u-boot.dtsi
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: (GPL-2.0 OR MIT)
>
> ®_usdhc2_vmmc {
> + startup-delay-us = <1000>;
> u-boot,off-on-delay-us = <2>;
> };
>
>
> @Haibo, Could you help looking into the issue. What's your opinion to add the
> startup-delay-us or revert your commit?
>
Hi Fabio,
I co-debug with Ye, and find the issue is also related with clock
enable/disable. For current logic on imx usdhc, hardware automatically gate off
the card clock when idle.
So before the first command "go idle", there is no clock on the clock line,
which not align with the sd spec.
Refer to SD3.0 spec 6.4.1 Power UP
The host shall supply power to the card so that the voltage is reached to
Vdd_min within 250ms and
start to supply at least 74 SD clocks to the SD card with keeping CMD line to
high. In case of SPI
mode, CS shall be held to high during 74 clock cycles
if we give the card the correct clock rate before the first "go idle" command,
this issue gone.
Please try to apply the patch I send on 2021/1/27 [PATCH] mmc: fsl_esdhc_imx:
use VENDORSPEC_FRC_SDCLK_ON to control card clock output
> Best regards,
> Ye Li
smime.p7s
Description: S/MIME cryptographic signature
RE: [PATCH] Revert "mmc: fsl_esdhc_imx: remove the 1ms delay before sending command"
> -Original Message-
> From: Fabio Estevam [mailto:feste...@gmail.com]
> Sent: 2021年2月27日 21:54
> To: Peng Fan
> Cc: u-boot@lists.denx.de; loru...@gmail.com;
> andrey.zhizhi...@leica-geosystems.com; Ye Li ; Bough Chen
> ; sba...@denx.de; tr...@konsulko.com; Fabio Estevam
>
> Subject: [PATCH] Revert "mmc: fsl_esdhc_imx: remove the 1ms delay before
> sending command"
>
> Removing the 1ms delay before sending command causes a regression on
> imx8mq-evk where the SD card cannot be accessed.
>
> Since this 1ms delay has been present since the driver introduction in
2008,
> keep it to avoid regressions.
>
> This reverts commit 9098682200e6cca4b776638a51200dafa16f50fb.
>
Hi Fabio,
This 1ms delay exist in esdhc_send_cmd_common, which means every time we
send a command, will delay 1ms, which will involve a huge delay overall for
the whole u-boot cycle.
For the issue you meet, I already find the root cause, and already reply in
the mail loop: [EXT] Re: [PATCH 4/4] imx8mq_evk: Enable the USB3.0 host port
So I think no need to involve this 1ms back again.
Best regards
Haibo Chen
> Reported-by: Ye Li
> Signed-off-by: Fabio Estevam
> ---
> drivers/mmc/fsl_esdhc_imx.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index
> e0e132698e30..a0a5c8960563 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -463,6 +463,13 @@ static int esdhc_send_cmd_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc,
> while (esdhc_read32(®s->prsstat) & PRSSTAT_DLA)
> ;
>
> + /* Wait at least 8 SD clock cycles before the next command */
> + /*
> + * Note: This is way more than 8 cycles, but 1ms seems to
> + * resolve timing issues with some cards
> + */
> + udelay(1000);
> +
> /* Set up for a data transfer if we have one */
> if (data) {
> err = esdhc_setup_data(priv, mmc, data);
> --
> 2.25.1
smime.p7s
Description: S/MIME cryptographic signature
RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
> -Original Message-
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> Sent: 2021年3月3日 19:00
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de; sba...@denx.de
> Cc: dl-uboot-imx ; thar...@gateworks.com;
> feste...@gmail.com; Ye Li
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
>
>
>
> > -Original Message-
> > From: haibo.c...@nxp.com
> > Sent: Wednesday, March 3, 2021 10:06 AM
> > To: peng@nxp.com; u-boot@lists.denx.de; sba...@denx.de
> > Cc: haibo.c...@nxp.com; uboot-...@nxp.com; thar...@gateworks.com;
> > ZHIZHIKIN Andrey ;
> > feste...@gmail.com; ye...@nxp.com
> > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > From: Haibo Chen
> >
> > Common code already handle the voltage switch sequence based on spec,
> > so remove the redundant voltage switch code.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > drivers/mmc/fsl_esdhc_imx.c | 10 +-
> > 1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > index
> > af36558b3c..a199cf3df6 100644
> > --- a/drivers/mmc/fsl_esdhc_imx.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > fsl_esdhc_priv *priv, struct mmc *mmc,
> > goto out;
> > }
> >
> > - /* Switch voltage to 1.8V if CMD11 succeeded */
> > - if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > - esdhc_setbits32(®s->vendorspec,
> ESDHC_VENDORSPEC_VSELECT);
> > -
> > - printf("Run CMD11 1.8V switch\n");
> > - /* Sleep for 5 ms - max time for card to switch to 1.8V */
> > - udelay(5000);
> > - }
> > -
> > /* Workaround for ESDHC errata ENGcm03648 */
> > if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > int timeout = 5;
> > @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
> > }
> > #endif
> > esdhc_setbits32(®s->vendorspec,
> > ESDHC_VENDORSPEC_VSELECT);
> > + mdelay(5);
>
> Why is this delay introduced here? It is not clear from the commit message
> whether and why it is required here.
>
> If this is kept from the removed block - maybe it is better to move the
> corresponding comment here as well.
Hi Andrev,
Thanks for your careful review!
Without this 5ms delay, with patch1 and remove the upper redundant cmd11
related code,
mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return
timeout. Seems for usdhc,
gate off clock for 10ms is not enough. If total delay 15ms, then the second
mmc_wait_dat0() can return normal.
So I add 5ms delay here. Yes, I should add a comment for this 5ms in the code.
You can also do the test on your side.
Best Regards
Haibo
>
> > if (esdhc_read32(®s->vendorspec) &
> ESDHC_VENDORSPEC_VSELECT)
> > return 0;
> >
> > --
> > 2.17.1
>
> -- andrey
smime.p7s
Description: S/MIME cryptographic signature
RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
> -Original Message-
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> Sent: 2021年3月3日 20:50
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de; sba...@denx.de
> Cc: dl-uboot-imx ; thar...@gateworks.com;
> feste...@gmail.com; Ye Li
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
>
> Hello Haibo,
>
> > -Original Message-
> > From: Bough Chen
> > Sent: Wednesday, March 3, 2021 12:27 PM
> > To: ZHIZHIKIN Andrey ; Peng Fan
> > ; u-boot@lists.denx.de; sba...@denx.de
> > Cc: dl-uboot-imx ; thar...@gateworks.com;
> > feste...@gmail.com; Ye Li
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > > -Original Message-
> > > From: ZHIZHIKIN Andrey
> > > [mailto:andrey.zhizhi...@leica-geosystems.com]
> > > Sent: 2021年3月3日 19:00
> > > To: Bough Chen ; Peng Fan ;
> > > u-boot@lists.denx.de; sba...@denx.de
> > > Cc: dl-uboot-imx ; thar...@gateworks.com;
> > > feste...@gmail.com; Ye Li
> > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: haibo.c...@nxp.com
> > > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > > To: peng@nxp.com; u-boot@lists.denx.de; sba...@denx.de
> > > > Cc: haibo.c...@nxp.com; uboot-...@nxp.com;
> thar...@gateworks.com;
> > > > ZHIZHIKIN Andrey ;
> > > > feste...@gmail.com; ye...@nxp.com
> > > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > related code.
> > > >
> > > > From: Haibo Chen
> > > >
> > > > Common code already handle the voltage switch sequence based on
> > > > spec, so remove the redundant voltage switch code.
> > > >
> > > > Signed-off-by: Haibo Chen
> > > > ---
> > > > drivers/mmc/fsl_esdhc_imx.c | 10 +-
> > > > 1 file changed, 1 insertion(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > > b/drivers/mmc/fsl_esdhc_imx.c index
> > > > af36558b3c..a199cf3df6 100644
> > > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > > > goto out;
> > > > }
> > > >
> > > > - /* Switch voltage to 1.8V if CMD11 succeeded */
> > > > - if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > > - esdhc_setbits32(®s->vendorspec,
> > > ESDHC_VENDORSPEC_VSELECT);
> > > > -
> > > > - printf("Run CMD11 1.8V switch\n");
> > > > - /* Sleep for 5 ms - max time for card to switch to 1.8V
> */
> > > > - udelay(5000);
> > > > - }
> > > > -
> > > > /* Workaround for ESDHC errata ENGcm03648 */
> > > > if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > > > int timeout = 5; @@ -839,6 +830,7 @@ static
> > > > int esdhc_set_voltage(struct mmc *mmc)
> > > > }
> > > > #endif
> > > > esdhc_setbits32(®s->vendorspec,
> > > > ESDHC_VENDORSPEC_VSELECT);
> > > > + mdelay(5);
> > >
> > > Why is this delay introduced here? It is not clear from the commit
> > > message whether and why it is required here.
> > >
> > > If this is kept from the removed block - maybe it is better to move
> > > the corresponding comment here as well.
> >
> > Hi Andrev,
> >
> > Thanks for your careful review!
>
> Thanks for the patch series on the first place! This allows to switch uSDHC
> into
> high-speed modes, which is quite valuable.
>
> > Without this 5ms delay, with patch1 and remove the upper redundant
> > cmd11 related code,
> > mmc_switch_voltage() will fail, due to the second mmc_wait_dat0()
> > return timeout. Seems for usdhc, gate off clock for 10ms is not
> > enough. If total delay 15ms, then the second
> > mmc_wait_dat0() can return normal.
> > So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
> code.
>
> Exactly this i
RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
> -Original Message-
> From: Bough Chen
> Sent: 2021年3月8日 17:50
> To: ZHIZHIKIN Andrey ; Peng Fan
> ; u-boot@lists.denx.de; sba...@denx.de
> Cc: dl-uboot-imx ; thar...@gateworks.com;
> feste...@gmail.com; Ye Li
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
>
>
> > -Original Message-
> > From: ZHIZHIKIN Andrey [mailto:andrey.zhizhi...@leica-geosystems.com]
> > Sent: 2021年3月3日 20:50
> > To: Bough Chen ; Peng Fan ;
> > u-boot@lists.denx.de; sba...@denx.de
> > Cc: dl-uboot-imx ; thar...@gateworks.com;
> > feste...@gmail.com; Ye Li
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > Hello Haibo,
> >
> > > -Original Message-
> > > From: Bough Chen
> > > Sent: Wednesday, March 3, 2021 12:27 PM
> > > To: ZHIZHIKIN Andrey ; Peng Fan
> > > ; u-boot@lists.denx.de; sba...@denx.de
> > > Cc: dl-uboot-imx ; thar...@gateworks.com;
> > > feste...@gmail.com; Ye Li
> > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > > > -Original Message-
> > > > From: ZHIZHIKIN Andrey
> > > > [mailto:andrey.zhizhi...@leica-geosystems.com]
> > > > Sent: 2021年3月3日 19:00
> > > > To: Bough Chen ; Peng Fan ;
> > > > u-boot@lists.denx.de; sba...@denx.de
> > > > Cc: dl-uboot-imx ; thar...@gateworks.com;
> > > > feste...@gmail.com; Ye Li
> > > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > related code.
> > > >
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: haibo.c...@nxp.com
> > > > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > > > To: peng@nxp.com; u-boot@lists.denx.de; sba...@denx.de
> > > > > Cc: haibo.c...@nxp.com; uboot-...@nxp.com;
> > thar...@gateworks.com;
> > > > > ZHIZHIKIN Andrey ;
> > > > > feste...@gmail.com; ye...@nxp.com
> > > > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > > related code.
> > > > >
> > > > > From: Haibo Chen
> > > > >
> > > > > Common code already handle the voltage switch sequence based on
> > > > > spec, so remove the redundant voltage switch code.
> > > > >
> > > > > Signed-off-by: Haibo Chen
> > > > > ---
> > > > > drivers/mmc/fsl_esdhc_imx.c | 10 +-
> > > > > 1 file changed, 1 insertion(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > > > b/drivers/mmc/fsl_esdhc_imx.c index
> > > > > af36558b3c..a199cf3df6 100644
> > > > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > > > > goto out;
> > > > > }
> > > > >
> > > > > - /* Switch voltage to 1.8V if CMD11 succeeded */
> > > > > - if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > > > - esdhc_setbits32(®s->vendorspec,
> > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > -
> > > > > - printf("Run CMD11 1.8V switch\n");
> > > > > - /* Sleep for 5 ms - max time for card to switch to
> 1.8V
> > */
> > > > > - udelay(5000);
> > > > > - }
> > > > > -
> > > > > /* Workaround for ESDHC errata ENGcm03648 */
> > > > > if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > > > > int timeout = 5; @@ -839,6 +830,7 @@ static
> > > > > int esdhc_set_voltage(struct mmc *mmc)
> > > > > }
> > > > > #endif
> > > > > esdhc_setbits32(®s->vendorspec,
> > > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > + mdelay(5);
> > > >
> > > > Why is this delay introduced here? It is not clear from the commit
> > > > message whether and why it is required here.
> > > >
> > > > If this is kept from the removed
RE: [PATCH] mmc: fsl_esdhc_imx: replace all readl/writel to esdhc_read32/esdhc_write32
> -Original Message-
> From: Stefano Babic [mailto:sba...@denx.de]
> Sent: 2020年9月18日 17:19
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de
> Cc: dl-uboot-imx
> Subject: Re: [PATCH] mmc: fsl_esdhc_imx: replace all readl/writel to
> esdhc_read32/esdhc_write32
>
> Hi Haibo,
>
> On 01.09.20 09:34, haibo.c...@nxp.com wrote:
> > From: Haibo Chen
> >
> > Currently, readl/writel and esdhc_read32/esdhc_write32 are used. To
> > align the usage, change to only use esdhc_read32/esdhc_write32.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > drivers/mmc/fsl_esdhc_imx.c | 64
> > ++---
> > 1 file changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > index 0c866b168f..a0a0903ae4 100644
> > --- a/drivers/mmc/fsl_esdhc_imx.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -729,7 +729,7 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
> > u32 val;
> >
> > if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) {
> > - writel(ESDHC_STROBE_DLL_CTRL_RESET, ®s->strobe_dllctrl);
> > + esdhc_write32(ESDHC_STROBE_DLL_CTRL_RESET,
> ®s->strobe_dllctrl);
> >
> > /*
> > * enable strobe dll ctrl and adjust the delay target @@ -738,10
> > +738,10 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
> > val = ESDHC_STROBE_DLL_CTRL_ENABLE |
> > (priv->strobe_dll_delay_target <<
> > ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
> > - writel(val, ®s->strobe_dllctrl);
> > + esdhc_write32(val, ®s->strobe_dllctrl);
> > /* wait 1us to make sure strobe dll status register stable */
> > mdelay(1);
> > - val = readl(®s->strobe_dllstat);
> > + val = esdhc_read32(®s->strobe_dllstat);
> > if (!(val & ESDHC_STROBE_DLL_STS_REF_LOCK))
> > pr_warn("HS400 strobe DLL status REF not lock!\n");
> > if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK)) @@ -755,18
> +755,18 @@
> > static int esdhc_set_timing(struct mmc *mmc)
> > struct fsl_esdhc *regs = priv->esdhc_regs;
> > u32 mixctrl;
> >
> > - mixctrl = readl(®s->mixctrl);
> > + mixctrl = esdhc_read32(®s->mixctrl);
> > mixctrl &= ~(MIX_CTRL_DDREN | MIX_CTRL_HS400_EN);
> >
> > switch (mmc->selected_mode) {
> > case MMC_LEGACY:
> > esdhc_reset_tuning(mmc);
> > - writel(mixctrl, ®s->mixctrl);
> > + esdhc_write32(mixctrl, ®s->mixctrl);
> > break;
> > case MMC_HS_400:
> > case MMC_HS_400_ES:
> > mixctrl |= MIX_CTRL_DDREN | MIX_CTRL_HS400_EN;
> > - writel(mixctrl, ®s->mixctrl);
> > + esdhc_write32(mixctrl, ®s->mixctrl);
> > esdhc_set_strobe_dll(mmc);
> > break;
> > case MMC_HS:
> > @@ -777,12 +777,12 @@ static int esdhc_set_timing(struct mmc *mmc)
> > case UHS_SDR25:
> > case UHS_SDR50:
> > case UHS_SDR104:
> > - writel(mixctrl, ®s->mixctrl);
> > + esdhc_write32(mixctrl, ®s->mixctrl);
> > break;
> > case UHS_DDR50:
> > case MMC_DDR_52:
> > mixctrl |= MIX_CTRL_DDREN;
> > - writel(mixctrl, ®s->mixctrl);
> > + esdhc_write32(mixctrl, ®s->mixctrl);
> > break;
> > default:
> > printf("Not supported %d\n", mmc->selected_mode); @@ -862,8
> +862,8
> > @@ static int fsl_esdhc_execute_tuning(struct udevice *dev, uint32_t
> opcode)
> > struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> > struct fsl_esdhc *regs = priv->esdhc_regs;
> > struct mmc *mmc = &plat->mmc;
> > - u32 irqstaten = readl(®s->irqstaten);
> > - u32 irqsigen = readl(®s->irqsigen);
> > + u32 irqstaten = esdhc_read32(®s->irqstaten);
> > + u32 irqsigen = esdhc_read32(®s->irqsigen);
> > int i, ret = -ETIMEDOUT;
> > u32 val, mixctrl;
> >
> > @@ -873,25 +873,25 @@ static int fsl_esdhc_execute_tuning(struct
> > udevice *dev, uint32_t opcode)
> >
> > /* This is readw/writew SDHCI_HOST_CONTROL2 when tuning */
> > if (priv->flags & ESDHC_FLAG_STD_TUNING) {
> > - val = readl(®s->autoc12err);
> > - mixctrl = readl(®s->mixctrl);
> >
RE: [PATCH] mmc: fsl_esdhc: Fix 'Internal clock never stabilised.' error
+Yangbo
> -Original Message-
> From: Peng Fan (OSS)
> Sent: 2022年5月9日 13:37
> To: Pali Rohár ; Peng Fan ; Priyanka Jain
> ; Jaehoon Chung ; Sinan
> Akman ; Bough Chen
> Cc: u-boot@lists.denx.de
> Subject: Re: [PATCH] mmc: fsl_esdhc: Fix 'Internal clock never stabilised.'
> error
>
> +Haibo
>
> On 2022/4/30 2:27, Pali Rohár wrote:
> > Only newer eSDHC controllers set PRSSTAT_SDSTB flag. So do not wait
> > until flag PRSSTAT_SDSTB is set on old pre-2.2 controllers. Instead
> > sleep for fixed amount of time like it was before commit 6f883e501b65
> ("mmc:
> > fsl_esdhc: Add emmc hs200 support").
> >
> > This change fixes error 'Internal clock never stabilised.' which is
> > printed on P2020 board at every access to SD card.
> >
> > Fixes: 6f883e501b65 ("mmc: fsl_esdhc: Add emmc hs200 support")
> > Signed-off-by: Pali Rohár
> > ---
> > drivers/mmc/fsl_esdhc.c | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> > fdf2cc290e06..3b3587bd8d72 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -503,6 +503,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv,
> > struct
> mmc *mmc, uint clock)
> > u32 time_out;
> > u32 value;
> > uint clk;
> > + u32 hostver;
> >
> > if (clock < mmc->cfg->f_min)
> > clock = mmc->cfg->f_min;
> > @@ -543,6 +544,14 @@ static void set_sysctl(struct fsl_esdhc_priv
> > *priv, struct mmc *mmc, uint clock)
> >
> > esdhc_clrsetbits32(®s->sysctl, SYSCTL_CLOCK_MASK, clk);
> >
> > + /* Only newer eSDHC controllers set PRSSTAT_SDSTB flag */
> > + hostver = esdhc_read32(&priv->esdhc_regs->hostver);
> > + if (HOSTVER_VENDOR(hostver) <= VENDOR_V_22) {
> > + udelay(1);
> > + esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> > + return;
> > + }
> > +
> > time_out = 20;
> > value = PRSSTAT_SDSTB;
> > while (!(esdhc_read32(®s->prsstat) & value)) { @@ -562,6 +571,7
> > @@ static void esdhc_clock_control(struct fsl_esdhc_priv *priv, bool enable)
> > struct fsl_esdhc *regs = priv->esdhc_regs;
> > u32 value;
> > u32 time_out;
> > + u32 hostver;
> >
> > value = esdhc_read32(®s->sysctl);
> >
> > @@ -572,6 +582,13 @@ static void esdhc_clock_control(struct
> > fsl_esdhc_priv *priv, bool enable)
> >
> > esdhc_write32(®s->sysctl, value);
> >
> > + /* Only newer eSDHC controllers set PRSSTAT_SDSTB flag */
> > + hostver = esdhc_read32(&priv->esdhc_regs->hostver);
> > + if (HOSTVER_VENDOR(hostver) <= VENDOR_V_22) {
> > + udelay(1);
> > + return;
> > + }
> > +
> > time_out = 20;
> > value = PRSSTAT_SDSTB;
> > while (!(esdhc_read32(®s->prsstat) & value)) {
> >
RE: [PATCH 3/3] mmc: fsl_esdhc_imx: correct the actual card clock
> -Original Message- > From: Tim Harvey > Sent: 2022年7月23日 10:23 > To: Bough Chen > Cc: Peng Fan ; Jaehoon Chung > ; Fabio Estevam ; Sean > Anderson ; u-boot ; Marek > Vasut ; Adam Ford ; Andrey Zhizhikin > ; dl-uboot-imx > Subject: Re: [PATCH 3/3] mmc: fsl_esdhc_imx: correct the actual card clock > > On Fri, Feb 11, 2022 at 3:48 AM wrote: > > > > From: Haibo Chen > > > > The original code logic can not show the correct card clock, and also > > has one risk when the div is 0. Because there is div -=1 before. > > > > So move the operation before div -=1, and also involve ddr_pre_div to > > get the correct value. > > > > Signed-off-by: Haibo Chen > > --- > > drivers/mmc/fsl_esdhc_imx.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c > > index 0be7cae1e5..0ea7b0b50c 100644 > > --- a/drivers/mmc/fsl_esdhc_imx.c > > +++ b/drivers/mmc/fsl_esdhc_imx.c > > @@ -609,6 +609,8 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, > > struct > mmc *mmc, uint clock) > > while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16) > > div++; > > > > + mmc->clock = sdhc_clk / pre_div / div / ddr_pre_div; > > + > > pre_div >>= 1; > > div -= 1; > > > > @@ -630,7 +632,6 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, > > struct > mmc *mmc, uint clock) > > else > > esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | > > SYSCTL_CKEN); > > > > - mmc->clock = sdhc_clk / pre_div / div; > > priv->clock = clock; > > } > > > > -- > > 2.17.1 > > > > Haibo, > > I found that this particular patch keeps an imx8mm-venice-gw7901 board that > has a viking vwsdinbdg4 eMMC from booting to Linux. While u-boot appears to > work ok, as soon as I load a kernel (from emmc or even > network) and boot to it I hang at 'starting kernel' even with early debug > turned > on. > > u-boot=> mmc list > FSL_SDHC: 0 > FSL_SDHC: 1 > FSL_SDHC: 2 (eMMC) > u-boot=> mmc dev 2 > switch to partitions #0, OK > mmc2(part 0) is current device > u-boot=> mmc info > Device: FSL_SDHC > Manufacturer ID: 45 > OEM: 0 > Name: DG4008 > Bus Speed: 2 > Mode: HS400ES (200MHz) > Rd Block Len: 512 > MMC version 5.1 > High Capacity: Yes > Capacity: 7.3 GiB > Bus Width: 8-bit DDR > Erase Group Size: 512 KiB > HC WP Group Size: 8 MiB > User Capacity: 7.3 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 > > I have other boards with a Micron MTFC8GAKAJCN non HS400ES that don't have > any issue so it appears to be something to do with HS400ES support and I find > if I > disable CONFIG_MMC_HS400_ES_SUPPORT or revert this patch the issue goes > away. > > Any idea what might be going on here? Hi Tim, I think this should be related with the HS400ES downgrade miss. Before boot linux, uboot will do some cleanup, remove all devices. Call mmc_blk_remove-> mmc_deinit-> mmc_select_mode_and_width-> mmc_set_card_speed(mmc, MMC_HS, true); Please make sure in your environment, finally call mmc_set_card_speed(mmc, MMC_HS, true); which means the HS400ES mode first downgrade to HS mode, then switch to legacy mode. Otherwise, emmc mode switch may meet issue. Some emmc may hang in somewhere, we did meet that before. To debug this, you can enable the debug info, and see the detail steps of mmc related. Not sure in your side, you miss some config which cause this issue. For why this is related with the clock related patch, I think the wrong clock config happed workaround it. Best Regards Haibo Chen > > Best Regards, > > Tim
[U-Boot] [PATCH] mmc: correct the HS400 initialization process
After the commit b9a2a0e2e9c0 ("mmc: Add support for downgrading
HS200/HS400 to HS mode"), it add a parameter in mmc_set_card_speed()
which indicates that the HS200/HS400 to HS downgrade is happening.
During the HS400 initialization, first select to HS200, and config
the related clock rate, then downgrade to HS mode. So here also need
to config the downgrade value to be true, make sure in the function
mmc_set_card_speed(), after switch to HS mode, first config the
clock rate, then read the EXT_CSD. Otherwise read EXT_CSD in HS mode
at wrong clock rate, e.g. 200MHz, may lead to uncertain result.
Test on i.MX8QM MEK board, some Micron eMMC will stuck in transfer
mode in this case, and USDHC will never get data transfer complete
status, cause the uboot hang.
Signed-off-by: Haibo Chen
---
drivers/mmc/mmc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 1c1527cc74..55d3560b30 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1892,8 +1892,7 @@ static int mmc_select_hs400(struct mmc *mmc)
}
/* Set back to HS */
- mmc_set_card_speed(mmc, MMC_HS, false);
- mmc_set_clock(mmc, mmc_mode2freq(mmc, MMC_HS), false);
+ mmc_set_card_speed(mmc, MMC_HS, true);
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
EXT_CSD_BUS_WIDTH_8 | EXT_CSD_DDR_FLAG);
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: correct the HS400 initialization process
> -Original Message-
> From: Marek Vasut [mailto:marek.va...@gmail.com]
> Sent: 2019年3月18日 19:53
> To: BOUGH CHEN ; jh80.ch...@samsung.com;
> tr...@konsulko.com
> Cc: marek.vasut+rene...@gmail.com; Ye Li ; dl-uboot-imx
> ; u-boot@lists.denx.de
> Subject: Re: [PATCH] mmc: correct the HS400 initialization process
>
> On 3/18/19 9:23 AM, BOUGH CHEN wrote:
> > After the commit b9a2a0e2e9c0 ("mmc: Add support for downgrading
> > HS200/HS400 to HS mode"), it add a parameter in mmc_set_card_speed()
> > which indicates that the HS200/HS400 to HS downgrade is happening.
> >
> > During the HS400 initialization, first select to HS200, and config the
> > related clock rate, then downgrade to HS mode. So here also need to
> > config the downgrade value to be true, make sure in the function
> > mmc_set_card_speed(), after switch to HS mode, first config the clock
> > rate, then read the EXT_CSD. Otherwise read EXT_CSD in HS mode at
> > wrong clock rate, e.g. 200MHz, may lead to uncertain result.
>
> I think the fix is right, but the reasoning for it is not.
>
> If you call mmc_set_card_speed() with hsdowngrade=true, it will result in
> calling __mmc_switch() with send_status=false , which in turn means that
> after issuing the MMC_CMD_SWITCH command, the code won't poll the card
> using MMC_CMD_SEND_STATUS, but just wait a bit and then switch the bus
> properties. This is indeed required when switching bus properties.
> And that's what I think was making your card unstable.
>
> Is that the case ?
Sorry for the tardy reply, I miss the email last week.
No, if without this patch, even I add the dealy after the switch command, eMMC
still stuck after the CMD8.
The main issue is that, in mmc_set_card_speed, if the bus mode the HS mode, it
will send CMD8 to get the EXT_CSD, but the clock is still 200MHz at that time.
It is wrong that EMMC output data in HS mode at 200MHz.
Best Regards
Haibo Chen
>
> > Test on i.MX8QM MEK board, some Micron eMMC will stuck in transfer
> > mode in this case, and USDHC will never get data transfer complete
> > status, cause the uboot hang.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > drivers/mmc/mmc.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> > 1c1527cc74..55d3560b30 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -1892,8 +1892,7 @@ static int mmc_select_hs400(struct mmc *mmc)
> > }
> >
> > /* Set back to HS */
> > - mmc_set_card_speed(mmc, MMC_HS, false);
> > - mmc_set_clock(mmc, mmc_mode2freq(mmc, MMC_HS), false);
> > + mmc_set_card_speed(mmc, MMC_HS, true);
> >
> > err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
> EXT_CSD_BUS_WIDTH,
> > EXT_CSD_BUS_WIDTH_8 | EXT_CSD_DDR_FLAG);
> >
>
>
> --
> Best regards,
> Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] mmc: correct the HS400 initialization process
After the commit b9a2a0e2e9c0 ("mmc: Add support for downgrading
HS200/HS400 to HS mode"), it add a parameter in mmc_set_card_speed()
which indicates that the HS200/HS400 to HS downgrade is happening.
During the HS400 initialization, first select to HS200, and config
the related clock rate, then downgrade to HS mode. So here also need
to config the downgrade value to be true for two reasons. First,
make sure in the function mmc_set_card_speed(), after switch to HS
mode, first config the clock rate, then read the EXT_CSD, avoid
receiving data of EXT_CSD in HS mode at 200MHz. Second, after issue
the MMC_CMD_SWITCH command, it need to wait a bit then switch bus
properties.
Test on i.MX8QM MEK board, some Micron eMMC will stuck in transfer
mode in this case, and USDHC will never get data transfer complete
status, cause the uboot hang.
Signed-off-by: Haibo Chen
---
drivers/mmc/mmc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 89b255daf4..456c1b4cc9 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1892,8 +1892,7 @@ static int mmc_select_hs400(struct mmc *mmc)
}
/* Set back to HS */
- mmc_set_card_speed(mmc, MMC_HS, false);
- mmc_set_clock(mmc, mmc_mode2freq(mmc, MMC_HS), false);
+ mmc_set_card_speed(mmc, MMC_HS, true);
err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
EXT_CSD_BUS_WIDTH_8 | EXT_CSD_DDR_FLAG);
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue"
> -Original Message- > From: Lukasz Majewski [mailto:lu...@denx.de] > Sent: 2019年5月8日 19:38 > To: Peng Fan > Cc: u-boot@lists.denx.de; Tom Rini ; Marcel Ziswiler > ; Fabio Estevam ; > dl-uboot-imx ; sba...@denx.de; Stefan Agner > ; BOUGH CHEN ; Ye Li > > Subject: Re: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode clock > setting issue" > > Hi Peng, > > > > Subject: Re: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode > > > clock setting issue" > > > > > > On Wed, 8 May 2019 08:19:45 +0200 > > > Lukasz Majewski wrote: > > > > > > > Hi Peng, > > > > > > > > > Hi Lukasz, > > > > > > > > > > > Subject: [PATCH] Revert "mmc: fsl_esdhc: fix sd/mmc ddr mode > > > > > > clock setting issue" > > > > > > > > > > > > This reverts commit 72a89e0da5ac6a4ab929b15a2b656f04f50767f6, > > > > > > which causes the imx53 HSC to hang as the eMMC is not working > > > > > > properly anymore. > > > > > > > > > > > > The exact error message: > > > > > > MMC write: dev # 0, block # 2, count 927 ... mmc write failed > > > > > > 0 blocks written: ERROR > > > > > > > > > > > > imx53 is not using the DDR mode. > > > > > > > > > > > > Debugging of pre_div and div generation showed that those > > > > > > values are generated in a way, which is not matching the ones > > > > > > from working setup. > > > > > > > > > > > > As the original patch was performing code refactoring, let's > > > > > > revert this change, so all imx53 boards would work again. > > > > > > > > > > Could you share what is the clock value for your board? > > > > > > > > Sure, no problem: > > > > > > > > Working setup: > > > > -- > > > > > > > > MMC: > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > FSL_SDHC: 0 > > > > Loading Environment from MMC... > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 1 div: 1 set_sysctl: clk: 272 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 1 div: 0 set_sysctl: clk: 256 > > > > > > > > > > > > > > > > > > > > Broken: > > > > --- > > > > MMC: > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > FSL_SDHC: 0 > > > > Loading Environment from MMC... > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 8 div: 12 set_sysctl: clk: 2240 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 0 div: 3 set_sysctl: clk: 48 > > > > > > > > set_sysctl: pre_div = 2 mmc->ddr: 0 sdhc_clk: 8000 > > > > pre_div: 0 div: 1 set_sysctl: clk: 16 > > > > > > > > > > > > (Please also find attached patch to reproduce debug output). > > > > > > > > > > And maybe the most important question - why it was necessary to > > > refactor this code? > > > > > > Parts responsible for calculating pre_div and div seems not related > > > to ddr problem (one spot issue is div <= 16 , but in the original > > > code it was div < 16)? > > > > > > This patch is mostly to support HS400 DDR_EN mode, But after I check > > more, seems it is not a right fix, sorry for this regression. > > > > It does almost the same calculation as before code expect > > writel(readl(®s->mixctrl) | MIX_CTRL_DDREN, ®s->mixctrl); > > > > and one condition break others. &
RE: IMX8MM SD UHS support
> -Original Message-
> From: Adam Ford [mailto:aford...@gmail.com]
> Sent: 2022年1月12日 7:32
> To: Bough Chen
> Cc: ZHIZHIKIN Andrey ;
> thar...@gateworks.com; Fabio Estevam ; Peng Fan
> ; u-boot ; Stefano Babic
> ; Jaehoon Chung
> Subject: Re: IMX8MM SD UHS support
>
> On Mon, Jan 25, 2021 at 4:43 AM Bough Chen wrote:
> >
> > Hi Tim and Andrey,
> >
> > I find the root cause, it is because during the 1.8v voltage switch
> > process, SD
> clock do not gate off/ on according to the SD spec.
> > For i.MX usdhc, if want to gate off the sd card clock, need to clear the
> > bit 8 of
> VEND_SPEC, and set this bit to gate on the sd card clock.
> > For the bit[14:11] of VEND_SPEC, our IP do not implement the function, just
> mark as reserved bits.
> > So for current logic, we'll see the second mmc_wait_dat0() in
> mmc_switch_voltage() always return timeout.
> >
> > Let me double confirm with our IC team, and will send a formal fix patch
> tomorrow.
>
> Haibo,
>
> Sorry to resurrect an old thread, but it's been almost a year since we last
> discussed this, and from what I can tell, the MMC switching to
> SDR104 is still broken.
>
> I was hoping you might have a suggestion as to how to move forward.
>
HI Adam,
The original fix patch is reverted few month ago.
Please refer to commit f132aab403271 " Revert "mmc: fsl_esdhc_imx: use
VENDORSPEC_FRC_SDCLK_ON to control card clock output""
According to my test, I find for some mmc card, we can't keep the clock always
on, especially when mmc internal status get stuck, seems must need the clock
gate off, then the card can recover back. Otherwise, the card can't response
any command. I meet this issue on one eMMC card, after meet switch error, must
gate off and gate on the clock rate, then this emmc Card can recover back. I
think this would be the reason of why this patched is reverted.
I'm working on this issue these days, will try to send another patch to fix all
these issue.
Best Regards
Haibo Chen
> >
> >
> > Haibo
> >
> > > -Original Message-
> > > From: Bough Chen
> > > Sent: 2021年1月22日 20:42
> > > To: 'ZHIZHIKIN Andrey' ;
> > > thar...@gateworks.com
> > > Cc: Adam Ford ; Fabio Estevam
> > > ; Peng Fan ; u-boot
> > > ; Stefano Babic ; Jaehoon
> > > Chung
> > > Subject: RE: IMX8MM SD UHS support
> > >
> > > Hi Tim and Andrey
> > >
> > > I can reproduce this issue on my side, after revert my patch which
> > > you point out, SD can work on SDR104 mode.
> > > Till now, I do not know why wait for data0 status will cause this
> > > issue. Give me more time, I will try to dig that out.
> > >
> > > Again, thanks for report this issue.
> > >
> > > Haibo
> > >
> > > > -Original Message-
> > > > From: ZHIZHIKIN Andrey
> > > > [mailto:andrey.zhizhi...@leica-geosystems.com]
> > > > Sent: 2021年1月20日 4:48
> > > > To: thar...@gateworks.com
> > > > Cc: Bough Chen ; Adam Ford
> > > ;
> > > > Fabio Estevam ; Peng Fan ;
> > > > u-boot ; Stefano Babic ;
> > > > Jaehoon Chung
> > > > Subject: RE: IMX8MM SD UHS support
> > > >
> > > > Hello Tim,
> > > >
> > > > > -Original Message-
> > > > > From: Tim Harvey
> > > > > Sent: Tuesday, January 19, 2021 6:32 PM
> > > > > To: ZHIZHIKIN Andrey
> > > > > Cc: haibo.c...@nxp.com; Adam Ford ; Fabio
> > > > Estevam
> > > > > ; Peng Fan ; u-boot > > > > b...@lists.denx.de>; Stefano Babic ; Jaehoon
> > > > > Chung
> > > > > Subject: Re: IMX8MM SD UHS support
> > > > >
> > > > > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey
> > > > > wrote:
> > > > > >
> > > > > > Hello Tim,
> > > > > >
> > > > > > Sorry it took me quite some time to get this sorted out, but I
> > > > > > believe I was able
> > > > > to identify an offending commit that is preventing the USDHC to
> > > > > switch to higher speed modes.
> > > > > >
> > > > > > It is in fact b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0()
> > > > > > support"),
> > > > > reverting it makes SD Card to properly report capabilities and
> > > > > switch to high speed modes.
&g
RE: [PATCH 1/2] mmc: fsl_esdhc_imx: Fix fsl_esdhc_wait_dat0
> -Original Message- > From: Adam Ford [mailto:aford...@gmail.com] > Sent: 2022年1月12日 21:54 > To: u-boot@lists.denx.de > Cc: Peng Fan ; Bough Chen ; > andrey.zhizhi...@leica-geosystems.com; thar...@gateworks.com; > este...@gmail.com; sba...@denx.de; af...@beaconembedded.com; Adam > Ford > Subject: [PATCH 1/2] mmc: fsl_esdhc_imx: Fix fsl_esdhc_wait_dat0 > > According to Haibo Chen [1] - the current implementation mmc_wait_dat0, the > second mmc_wait_dat0() in mmc_switch_voltage() always return timeout. > This causes UHS cards to not properly initialize to their highest rate, and > default back for high-speed mode. > > When reviewing [1] and comparing it to the linux driver, it appears that this > function can be accomplished by turning off the clock, and waiting for the > clock-standby bit to become active. > > [1] - > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.de > nx.de%2Fpipermail%2Fu-boot%2F2021-January%2F438644.html&data=04 > %7C01%7Chaibo.chen%40nxp.com%7C5de948ddd6fb41a9ab2c08d9d5d304d9 > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63777592456049525 > 1%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL > CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=QVXYNWR3t2N9oCr > BZSnoJVquwWdJuvo0hmuP%2FZhZifc%3D&reserved=0 > > Reported-by: Tim Harvey > Signed-off-by: Adam Ford > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index > 4c06361bee..e5814232a2 100644 > --- a/drivers/mmc/fsl_esdhc_imx.c > +++ b/drivers/mmc/fsl_esdhc_imx.c > @@ -1684,9 +1684,15 @@ static int fsl_esdhc_wait_dat0(struct udevice *dev, > int state, > struct fsl_esdhc_priv *priv = dev_get_priv(dev); > struct fsl_esdhc *regs = priv->esdhc_regs; > > + /* > + * Clear the clock-enable and wait for the bit indicating it > + * is in standby. > + */ > + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN); Hi Adam, For the usdhc on i.MX, the bit VENDORSPEC_CKEN is not implement by IP, instead, we use the bit 8 of register 0xc0. By the way, for the wait_dat0 function, it not only cover the IO voltage switch process, other place also need this function. So I think this change is not correct. Best Regards Haibo Chen > ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, > - !!(tmp & PRSSTAT_DAT0) == !!state, > + (tmp & PRSSTAT_SDSTB), > timeout_us); > + > return ret; > } > > -- > 2.32.0 smime.p7s Description: S/MIME cryptographic signature
RE: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
> -Original Message-
> From: Jaehoon Chung [mailto:jh80.ch...@samsung.com]
> Sent: 2020年11月3日 5:52
> To: Bough Chen ; Peng Fan ;
> u-boot@lists.denx.de
> Cc: dl-uboot-imx
> Subject: Re: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
>
> On 11/2/20 8:17 PM, haibo.c...@nxp.com wrote:
> > From: Haibo Chen
> >
> > Add wait_dat0() support, upper layer will use this callback.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > drivers/mmc/fsl_esdhc_imx.c | 23 +++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > index 22040c67a8..dc6a6006fa 100644
> > --- a/drivers/mmc/fsl_esdhc_imx.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -1646,6 +1646,28 @@ static int
> fsl_esdhc_set_enhanced_strobe(struct
> > udevice *dev) } #endif
> >
> > +static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> > + int timeout_us)
> > +{
> > + int ret = -ETIMEDOUT;
> > + bool dat0_high;
> > + bool target_dat0_high = !!state;
> > + struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> > + struct fsl_esdhc *regs = priv->esdhc_regs;
> > +
> > + timeout_us = DIV_ROUND_UP(timeout_us, 10); /* check every 10 us. */
> > + while (timeout_us--) {
> > + dat0_high = !!(esdhc_read32(®s->prsstat) & PRSSTAT_DAT0);
> > + if (dat0_high == target_dat0_high) {
> > + ret = 0;
> > + break;
> > + }
> > + udelay(10);
>
> Fix indent.
Yes, will fix.
> And can't use wait_for_bit_xx()?
Just to align with the mmc_wait_dat0() defined in mmc-uclass.c, named
fsl_esdhc_wait_dat0 should be more readable.
Best Regards
Haibo Chen
>
> Best Regards,
> Jaehoon Chung
>
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static const struct dm_mmc_ops fsl_esdhc_ops = {
> > .get_cd = fsl_esdhc_get_cd,
> > .send_cmd = fsl_esdhc_send_cmd,
> > @@ -1656,6 +1678,7 @@ static const struct dm_mmc_ops fsl_esdhc_ops
> = {
> > #if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
> > .set_enhanced_strobe = fsl_esdhc_set_enhanced_strobe, #endif
> > + .wait_dat0 = fsl_esdhc_wait_dat0,
> > };
> > #endif
> >
> >
RE: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
> -Original Message-
> From: Peng Fan
> Sent: 2020年11月3日 16:17
> To: Jaehoon Chung ; Bough Chen
> ; u-boot@lists.denx.de
> Cc: dl-uboot-imx
> Subject: RE: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
>
> > Subject: Re: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
> >
> > On 11/2/20 8:17 PM, haibo.c...@nxp.com wrote:
> > > From: Haibo Chen
> > >
> > > Add wait_dat0() support, upper layer will use this callback.
> > >
> > > Signed-off-by: Haibo Chen
> > > ---
> > > drivers/mmc/fsl_esdhc_imx.c | 23 +++
> > > 1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > b/drivers/mmc/fsl_esdhc_imx.c index 22040c67a8..dc6a6006fa 100644
> > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > @@ -1646,6 +1646,28 @@ static int
> > > fsl_esdhc_set_enhanced_strobe(struct
> > > udevice *dev) } #endif
> > >
> > > +static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> > > + int timeout_us)
> > > +{
> > > + int ret = -ETIMEDOUT;
> > > + bool dat0_high;
> > > + bool target_dat0_high = !!state;
> > > + struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> > > + struct fsl_esdhc *regs = priv->esdhc_regs;
> > > +
> > > + timeout_us = DIV_ROUND_UP(timeout_us, 10); /* check every 10
> us. */
> > > + while (timeout_us--) {
> > > + dat0_high = !!(esdhc_read32(®s->prsstat) &
> PRSSTAT_DAT0);
> > > + if (dat0_high == target_dat0_high) {
> > > + ret = 0;
> > > + break;
> > > + }
> > > + udelay(10);
> >
> > Fix indent.
> > And can't use wait_for_bit_xx()?
>
> +1, read_poll_timeout or similar should be used here.
Do not use read_poll_timeout here is because the parameter 'state' can be 0 or
1.
if use read_poll_timeout, should like this:
read_poll_timeout(esdhc_read32, ®s->prsstat, tmp, !!(tmp & PRSSTAT_DAT0) ==
!!state, 10, timeout_us);
I think the condition is a little bit complicated, not that readable, but if
you prefer this, I can do that.
Best Regards
Haibo Chen
>
> Thanks,
> Peng.
>
> >
> > Best Regards,
> > Jaehoon Chung
> >
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static const struct dm_mmc_ops fsl_esdhc_ops = {
> > > .get_cd = fsl_esdhc_get_cd,
> > > .send_cmd = fsl_esdhc_send_cmd,
> > > @@ -1656,6 +1678,7 @@ static const struct dm_mmc_ops
> fsl_esdhc_ops =
> > {
> > > #if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
> > > .set_enhanced_strobe = fsl_esdhc_set_enhanced_strobe, #endif
> > > + .wait_dat0 = fsl_esdhc_wait_dat0,
> > > };
> > > #endif
> > >
> > >
RE: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"
> -Original Message-
> From: Marek Vasut [mailto:ma...@denx.de]
> Sent: 2022年1月29日 1:37
> To: Jaehoon Chung ; u-boot@lists.denx.de
> Cc: sba...@denx.de; Bough Chen ; Igor Opaniuk
> ; Peng Fan
> Subject: Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"
>
> On 1/28/22 08:30, Jaehoon Chung wrote:
> > Hi Marek,
>
> Hi,
>
> > On 1/28/22 12:40, Marek Vasut wrote:
> >> This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.
> >>
> >> It seems the iMX8MM SDHC controller always reports DAT0 line status
> >> as zero after voltage switch at the end of mmc_switch_voltage(), even
> >> if it is supposed to be high and scope confirms the DAT0 is high.
> >> Reverting this patch makes SDR104 work on iMX8MM, however, it is not
> >> clear why the DAT0 status is not correctly reported by the
> >> controller.
> >
> > I didn't have an imx8mm board, so I wonder that other boards which is using
> fsl_esdhci_imx driver have same problem.
>
> I have multiple 8mm, just not the 8mm-evk.
>
> > I'm not sure...I remember that some controller doesn't support a busy
> > signal.
> > In kernel driver, also doesn't report correct status?
>
> Kernel doesn't use this DAT0 readback, that's why it works in kernel.
>
> U-Boot has been augmented with this DAT0 readback some time ago, I just
> noticed it on an 8mm board recently that it doesn't really work. There is
> also no
> errata.
>
> I think it would be good to have NXP check this.
Hi,
The issue you meet should be related to one patch that was revert recently,
refer to
Commit f132aab40327 " Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON
to control card clock output""
The issue you meet seems the same as what I mentioned in my original commit
log, I paste in the end.
Revert this patch because we did find this patch has some side effect, like
enlarge the whole boot up time about 20s.
If you will, you can add back this patch on your side, and test it.
I will send one new patch this week, to fix issue and avoid side effect.
Best Regards
Haibo Chen
commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2
Author: Haibo Chen
Date: Wed Mar 3 17:05:46 2021 +0800
mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these
are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the
card clock output.
After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"),
we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because
the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during
voltage switch, need to gate off/on the card clock. If not set the
FRC_SDCLK_ON,
after CMD11, hardware will gate off the card clock automatically, so card do
not detect the clock off/on behavior, so will draw the data0 line low until
next command.
Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support")
Tested-by: Tim Harvey
Signed-off-by: Haibo Chen
smime.p7s
Description: S/MIME cryptographic signature
RE: [PATCH 1/3] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON when necessary
Hi Fabio,
Can you help test these 3 patches on imx6qdl-pico board or imx7s board on your
side to double check whether these patches enlarge the total boot time?
Best Regards
Bough Chen
> -Original Message-
> From: Marek Vasut [mailto:ma...@denx.de]
> Sent: 2022年2月14日 5:53
> To: Bough Chen ; Peng Fan ;
> jh80.ch...@samsung.com; feste...@gmail.com; sean.ander...@seco.com;
> u-boot@lists.denx.de; aford...@gmail.com; thar...@gateworks.com;
> andrey.zhizhi...@leica-geosystems.com
> Cc: dl-uboot-imx
> Subject: Re: [PATCH 1/3] mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON when necessary
>
> On 2/11/22 12:16, haibo.c...@nxp.com wrote:
>
> Hi,
>
> [...]
>
> > @@ -897,6 +900,11 @@ static int fsl_esdhc_execute_tuning(struct udevice
> *dev, uint32_t opcode)
> >
> > esdhc_stop_tuning(mmc);
> >
> > + /* change to default setting, let host control the card clock */
> > + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> > + if (readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100))
>
> Please propagate the error in both cases:
>
> ret = readx_poll..();
> if (ret)
>dev_warn(...);
>
> return ret;
>
> > + pr_warn("fsl_esdhc_imx: card clock not gate off as expect.\n");
>
> btw. s@gate@gated@ , s@expect@expected@ (past tense)
>
> With those small details fixed:
>
> Reviewed-by: Marek Vasut
smime.p7s
Description: S/MIME cryptographic signature
RE: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC is ready
> -Original Message-
> From: Peng Fan
> Sent: 2020年5月19日 11:38
> To: BOUGH CHEN ; u-boot@lists.denx.de
> Cc: dl-uboot-imx
> Subject: RE: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the
> eMMC is ready
>
> > Subject: [PATCH] mmc: retry CMD1 in mmc_send_op_cond() until the eMMC
> > is ready
> >
> > From: Haibo Chen
> >
> > According to eMMC specification v5.1 section 6.4.3, we should issue
> > CMD1 repeatedly in the idle state until the eMMC is ready even if
> > mmc_send_op_cond() send CMD1 with argument = 0. Otherwise some
> eMMC
> > devices seems to enter the inactive mode after
> > mmc_complete_op_cond() issued CMD0 when the eMMC device is busy. This
> > patch also align with Linux 5.7.
> >
> > Signed-off-by: Haibo Chen
> > ---
> > drivers/mmc/mmc.c | 41 +
> > 1 file changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> > 523c055967..509549756b 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -664,21 +664,46 @@ static int mmc_send_op_cond_iter(struct mmc
> > *mmc, int use_arg)
> >
> > static int mmc_send_op_cond(struct mmc *mmc) {
> > + struct mmc_cmd cmd;
> > int err, i;
> > + int timeout = 1000;
> > + ulong start;
> >
> > /* Some cards seem to need this */
> > mmc_go_idle(mmc);
> >
> > - /* Asking to the card its capabilities */
> > - for (i = 0; i < 2; i++) {
> > - err = mmc_send_op_cond_iter(mmc, i != 0);
> > + cmd.cmdidx = MMC_CMD_SEND_OP_COND;
> > + cmd.resp_type = MMC_RSP_R3;
> > + cmd.cmdarg = 0;
> > +
> > + start = get_timer(0);
> > + /*
> > +* According to eMMC specification v5.1 section 6.4.3, we
> > +* should issue CMD1 repeatedly in the idle state until
> > +* the eMMC is ready. Otherwise some eMMC devices seem to enter
> > +* the inactive mode after mmc_complete_op_cond() issued CMD0
> > when
> > +* the eMMC device is busy.
> > +*/
>
> Could we just extend the previous for loop to fix the issue?
> Your piece code duplicate much of the code and changed the original behavior.
>
> > + while (1) {
> > + err = mmc_send_cmd(mmc, &cmd, NULL);
> > if (err)
> > return err;
> > -
> > - /* exit if not busy (flag seems to be inverted) */
> > - if (mmc->ocr & OCR_BUSY)
> > - break;
> > + if (mmc_host_is_spi(mmc)) {
> > + if (!(cmd.response[0] & (1 << 0)))
> > + break;
> > + } else {
> > + mmc->ocr = cmd.response[0];
> > + /* exit if not busy (flag seems to be inverted) */
> > + if (mmc->ocr & OCR_BUSY)
> > + break;
> > + }
> > + if (get_timer(start) > timeout)
> > + return -EOPNOTSUPP;
>
> TIMEOUT, please.
>
> > + udelay(100);
>
> This will change the original behavior by adding a delay.
>
> > + if (!mmc_host_is_spi(mmc))
> > + cmd.cmdarg = cmd.response[0] | (1 << 30);
>
> Do we need "1 << 30" ?
>
> > }
> > +
> > mmc->op_cond_pending = 1;
> > return 0;
> > }
> > @@ -691,7 +716,7 @@ static int mmc_complete_op_cond(struct mmc
> > *mmc)
> > int err;
> >
> > mmc->op_cond_pending = 0;
> > - if (!(mmc->ocr & OCR_BUSY)) {
> > + if (mmc->ocr & OCR_BUSY) {
>
> When card not go out busy, it means card not finish power up seq. Why drop !?
>
> > /* Some cards seem to need this */
> > mmc_go_idle(mmc);
> >
> > --
>
> Would the following patch help?
>
Hi Peng,
Thanks for your carefully review and suggestion, most eMMC chip can work normal
on the original code, only some Toshiba eMMC need more time to get out of busy
after cmd1.
The patch I send out just refer to the Linux driver code, and pass on the
customer side.
Currently I'm not sure whether the bit 30 of the argument for cmd1 is necessary
for this eMMC, I will get the reworked board which soldered this Toshiba eMMC
in a few days, then I can test on my hand.
Will test this then and give you response.
Best Regards
Haibo Chen
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> 523c055967..d3a0538
