-----Original Message----- From: Alexander Kochetkov al.kochet@gmail.com Sent: 2023年3月23日 17:34 To: Bough Chen haibo.chen@nxp.com Cc: hs@denx.de; marex@denx.de; u-boot@lists.denx.de; dl-uboot-imx uboot-imx@nxp.com; xypron.glpk@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 al.kochet@gmail.com
написал(а):
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.chen@nxp.com написал(а):
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.
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);
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