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