> -----Original Message----- > From: Alexander Kochetkov <al.koc...@gmail.com> > Sent: 2023年3月21日 17:50 > To: Bough Chen <haibo.c...@nxp.com> > Cc: h...@denx.de; 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 > > 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.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 > >