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?
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.c...@nxp.com> написал(а): > >> -----Original Message----- >> From: Alexander Kochetkov <al.koc...@gmail.com> >> Sent: 2023年3月20日 16:03 >> To: h...@denx.de >> Cc: Bough Chen <haibo.c...@nxp.com>; ma...@denx.de; >> u-boot@lists.denx.de; dl-uboot-imx <uboot-...@nxp.com>; >> xypron.g...@gmx.de; Simon Glass <s...@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 <h...@denx.de> написал(а): >>> >>> Hi! >>> >>> On 13.03.23 03:55, Bough Chen wrote: >>>>> -----Original Message----- >>>>> From: Bough Chen <haibo.c...@nxp.com> >>>>> 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 <uboot-...@nxp.com>; >>>>> xypron.g...@gmx.de; Bough Chen <haibo.c...@nxp.com> >>>>> Subject: [PATCH] i2c: correct I2C deblock logic >>>>> >>>>> From: Haibo Chen <haibo.c...@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.c...@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: h...@denx.de >