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