[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.
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,
-----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.
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
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
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.
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
-----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
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.chen@nxp.com написал(а):
-----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);
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@denx.de
-----Original Message----- From: Alexander Kochetkov al.kochet@gmail.com Sent: 2023年3月21日 17:50 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; Simon Glass sjg@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.
- 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.
- 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.
- 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().
- 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@nxp.com
написал(а):
-----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);
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@denx.de
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
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); }
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
-----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
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.
Patrik told that if we don’t have GPIO_ACTIVE_LOW in the DTS, this is the case where hardware has some sort of signal inversion inside. May be there is no such hardware now. When we set gpio output active, it became high inside hardware, hardware set i2c line low. If you think that previous version of my today’s patch is more correct, try it.
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.
Patric told, that we must not use GPIOD_ACTIVE_LOW in client code. So you must drop it. From any patch you send. GPIOD_ACTIVE_LOW is not applied to gpio flags when you call dm_gpio_set_dir_flags(). It is configured from DTS. You think it is applied, but actually not. It’s your finding. Here your words:
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.
23 марта 2023 г., в 13:41, Bough Chen haibo.chen@nxp.com написал(а):
-----Original Message----- From: Alexander Kochetkov <al.kochet@gmail.com mailto:al.kochet@gmail.com> Sent: 2023年3月23日 17:34 To: Bough Chen <haibo.chen@nxp.com mailto:haibo.chen@nxp.com> Cc: hs@denx.de mailto:hs@denx.de; marex@denx.de mailto:marex@denx.de; u-boot@lists.denx.de mailto:u-boot@lists.denx.de; dl-uboot-imx <uboot-imx@nxp.com mailto:uboot-imx@nxp.com>; xypron.glpk@gmx.de mailto: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); }
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
participants (4)
-
Alexander Kochetkov -
Bough Chen -
haibo.chen＠nxp.com
-
Heiko Schocher