[PATCH] i2c: correct I2C deblock logic
Bough Chen
haibo.chen at nxp.com
Tue Mar 21 12:00:11 CET 2023
> -----Original Message-----
> From: Alexander Kochetkov <al.kochet at gmail.com>
> Sent: 2023年3月21日 17:50
> To: Bough Chen <haibo.chen at nxp.com>
> Cc: hs at denx.de; marex at denx.de; u-boot at lists.denx.de; dl-uboot-imx
> <uboot-imx at nxp.com>; xypron.glpk at gmx.de; Simon Glass
> <sjg at chromium.org>
> 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 <haibo.chen at nxp.com>
> написал(а):
> >
> >> -----Original Message-----
> >> From: Alexander Kochetkov <al.kochet at gmail.com>
> >> Sent: 2023年3月20日 16:03
> >> To: hs at denx.de
> >> Cc: Bough Chen <haibo.chen at nxp.com>; marex at denx.de;
> >> u-boot at lists.denx.de; dl-uboot-imx <uboot-imx at nxp.com>;
> >> xypron.glpk at gmx.de; Simon Glass <sjg at chromium.org>
> >> 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 <hs at denx.de> написал(а):
> >>>
> >>> Hi!
> >>>
> >>> On 13.03.23 03:55, Bough Chen wrote:
> >>>>> -----Original Message-----
> >>>>> From: Bough Chen <haibo.chen at nxp.com>
> >>>>> Sent: 2023年2月10日 17:27
> >>>>> To: hs at denx.de; al.kochet at gmail.com; marex at denx.de
> >>>>> Cc: u-boot at lists.denx.de; dl-uboot-imx <uboot-imx at nxp.com>;
> >>>>> xypron.glpk at gmx.de; Bough Chen <haibo.chen at nxp.com>
> >>>>> Subject: [PATCH] i2c: correct I2C deblock logic
> >>>>>
> >>>>> From: Haibo Chen <haibo.chen at nxp.com>
> >>>>>
> >>>>> 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 <haibo.chen at nxp.com>
> >>>>> ---
> >>>>> 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:
> hs at denx.de
> >
More information about the U-Boot
mailing list