MXC I2C recover/idle_bus does not work if CONFIG_DM_I2C is configured
Heiko,
When an I2C transaction fails because a previous transaction (by the kernel) was aborted halfway the MXC I2C driver tries to recover from this by calling i2c_idle_bus (if CONFIG_DM_I2C is configured). i2c_idle_bus is defined in drivers/i2c/mxc_i2c.c
As part of the recover procedure i2c_idle_bus (tries) to change the direction of I2C GPIO pins from output to input using the function dm_gpio_set_dir_flags. However dm_gpio_set_dir_flags only sets flags without clearing any previously set flags, see statement "flags |= desc->flags" in dm_gpio_set_dir_flags. Because it is not allowed to set both GPIOD_IS_IN and GPIOD_IS_OUT (see function check_dir_flags) only the dm_gpio_set_dir_flags(GPIO_D_IS_OUT) calls are successful, the dm_gpio_set_dir_flags(GPIO_D_IS_IN) calls fail and the GPIO pin is never set as an input. This causes that i2c_idle_bus finds that clearing the bus is unsuccessful and returns an error.
Although i2c_idle_bus returns an error the i2c recover procedure itself is successful and the next I2C transfer will be successful. This requires that the I2C application to do a retry. This will work but is not the intention of the I2C driver.
I want to fix this problem. However it looks like that no dm_gpio API exists to change the direction of an GPIO pin while this is required to successful recover the i2c bus and detect that the recovery is successful. To fix this issue I see 2 possibilities:
1. Use the old fashioned APIs gpio_direction_input and gpio_direction_output 2. Add a new dm_gpio API dm_gpio_clear_and_set_dir_flags that allows to clear all existing set flags before setting the new flag(s)
I prefer option 2 but I like to hear the opinion of the developer that designed the dm_gpio interface.
Kind regards,
Kees Trommel.
--- This communication contains confidential information. If you are not the intended recipient please return this email to the sender and delete it from your records.
Diese Nachricht enthaelt vertrauliche Informationen. Sollten Sie nicht der beabsichtigte Empfaenger dieser E-mail sein, senden Sie bitte diese an den Absender zurueck und loeschen Sie die E-mail aus Ihrem System.
Hello Kees,
added Simon to cc...
On 08.04.21 14:20, Trommel, Kees (Contractor) wrote:
Heiko,
When an I2C transaction fails because a previous transaction (by the kernel) was aborted halfway the MXC I2C driver tries to recover from this by calling i2c_idle_bus (if CONFIG_DM_I2C is configured). i2c_idle_bus is defined in drivers/i2c/mxc_i2c.c
Ok, so imx based hardware.
As part of the recover procedure i2c_idle_bus (tries) to change the direction of I2C GPIO pins from output to input using the function dm_gpio_set_dir_flags. However dm_gpio_set_dir_flags only sets flags without clearing any previously set flags, see statement "flags |= desc->flags" in dm_gpio_set_dir_flags. Because it is not allowed to set both GPIOD_IS_IN and GPIOD_IS_OUT (see function check_dir_flags) only the dm_gpio_set_dir_flags(GPIO_D_IS_OUT) calls are successful, the dm_gpio_set_dir_flags(GPIO_D_IS_IN) calls fail and the GPIO pin is never set as an input. This causes that i2c_idle_bus finds that clearing the bus is unsuccessful and returns an error.
Ok, sounds like a bug to me.
Although i2c_idle_bus returns an error the i2c recover procedure itself is successful and the next I2C transfer will be successful. This requires that the I2C application to do a retry. This will work but is not the intention of the I2C driver.
I want to fix this problem. However it looks like that no dm_gpio API exists to change the direction of an GPIO pin while this is required to successful recover the i2c bus and detect that the recovery is successful. To fix this issue I see 2 possibilities:
- Use the old fashioned APIs gpio_direction_input and gpio_direction_output
- Add a new dm_gpio API dm_gpio_clear_and_set_dir_flags that allows to clear all existing set flags before setting the new flag(s)
Hmm.. I see, we only "or" the flags in dm_gpio_set_dir_flags() ... and than check if there are conflicts ... May we rework dm_gpio_set_dir_flags() so we prevent the conflicts we check in check_dir_flags() ?
Assumption is, the caller of dm_gpio_set_dir_flags() knows what he do...
@Simon: What do you think?
I prefer option 2 but I like to hear the opinion of the developer that designed the dm_gpio interface.
I did not designed the gpio interface!
bye, Heiko
Kind regards,
Kees Trommel.
This communication contains confidential information. If you are not the intended recipient please return this email to the sender and delete it from your records.
Diese Nachricht enthaelt vertrauliche Informationen. Sollten Sie nicht der beabsichtigte Empfaenger dieser E-mail sein, senden Sie bitte diese an den Absender zurueck und loeschen Sie die E-mail aus Ihrem System.
Simon,
I found this issue in 2020.10. I just checked the master version and found that the "flags |= desc->flags" statement in dm_gpio_set_dir_flags has been removed. So it looks like that this issue is already solved.
Kees.
-----Original Message----- From: Heiko Schocher hs@denx.de Sent: 09 April 2021 06:40 To: Trommel, Kees (Contractor) kees.trommel.contractor@draeger.com Cc: u-boot@lists.denx.de; Simon Glass sjg@chromium.org Subject: Re: MXC I2C recover/idle_bus does not work if CONFIG_DM_I2C is configured
Hello Kees,
added Simon to cc...
On 08.04.21 14:20, Trommel, Kees (Contractor) wrote:
Heiko,
When an I2C transaction fails because a previous transaction (by the kernel) was aborted halfway the MXC I2C driver tries to recover from this by calling i2c_idle_bus (if CONFIG_DM_I2C is configured). i2c_idle_bus is defined in drivers/i2c/mxc_i2c.c
Ok, so imx based hardware.
As part of the recover procedure i2c_idle_bus (tries) to change the direction of I2C GPIO pins from output to input using the function dm_gpio_set_dir_flags. However dm_gpio_set_dir_flags only sets flags without clearing any previously set flags, see statement "flags |= desc->flags" in dm_gpio_set_dir_flags. Because it is not allowed to set both GPIOD_IS_IN and GPIOD_IS_OUT (see function check_dir_flags) only the dm_gpio_set_dir_flags(GPIO_D_IS_OUT) calls are successful, the dm_gpio_set_dir_flags(GPIO_D_IS_IN) calls fail and the GPIO pin is never set as an input. This causes that i2c_idle_bus finds that clearing the bus is unsuccessful and returns an error.
Ok, sounds like a bug to me.
Although i2c_idle_bus returns an error the i2c recover procedure itself is successful and the next I2C transfer will be successful. This requires that the I2C application to do a retry. This will work but is not the intention of the I2C driver.
I want to fix this problem. However it looks like that no dm_gpio API exists to change the direction of an GPIO pin while this is required to successful recover the i2c bus and detect that the recovery is successful. To fix this issue I see 2 possibilities:
- Use the old fashioned APIs gpio_direction_input and
gpio_direction_output 2. Add a new dm_gpio API dm_gpio_clear_and_set_dir_flags that allows to clear all existing set flags before setting the new flag(s)
Hmm.. I see, we only "or" the flags in dm_gpio_set_dir_flags() ... and than check if there are conflicts ... May we rework dm_gpio_set_dir_flags() so we prevent the conflicts we check in check_dir_flags() ?
Assumption is, the caller of dm_gpio_set_dir_flags() knows what he do...
@Simon: What do you think?
I prefer option 2 but I like to hear the opinion of the developer that designed the dm_gpio interface.
I did not designed the gpio interface!
bye, Heiko
Kind regards,
Kees Trommel.
This communication contains confidential information. If you are not the intended recipient please return this email to the sender and delete it from your records.
Diese Nachricht enthaelt vertrauliche Informationen. Sollten Sie nicht der beabsichtigte Empfaenger dieser E-mail sein, senden Sie bitte diese an den Absender zurueck und loeschen Sie die E-mail aus Ihrem System.
Hi Kees,
On Fri, 9 Apr 2021 at 08:00, Trommel, Kees (Contractor) kees.trommel.contractor@draeger.com wrote:
Simon,
I found this issue in 2020.10. I just checked the master version and found that the "flags |= desc->flags" statement in dm_gpio_set_dir_flags has been removed. So it looks like that this issue is already solved.
Yes I believe so.
- Simon
Kees.
-----Original Message----- From: Heiko Schocher hs@denx.de Sent: 09 April 2021 06:40 To: Trommel, Kees (Contractor) kees.trommel.contractor@draeger.com Cc: u-boot@lists.denx.de; Simon Glass sjg@chromium.org Subject: Re: MXC I2C recover/idle_bus does not work if CONFIG_DM_I2C is configured
Hello Kees,
added Simon to cc...
On 08.04.21 14:20, Trommel, Kees (Contractor) wrote:
Heiko,
When an I2C transaction fails because a previous transaction (by the kernel) was aborted halfway the MXC I2C driver tries to recover from this by calling i2c_idle_bus (if CONFIG_DM_I2C is configured). i2c_idle_bus is defined in drivers/i2c/mxc_i2c.c
Ok, so imx based hardware.
As part of the recover procedure i2c_idle_bus (tries) to change the direction of I2C GPIO pins from output to input using the function dm_gpio_set_dir_flags. However dm_gpio_set_dir_flags only sets flags without clearing any previously set flags, see statement "flags |= desc->flags" in dm_gpio_set_dir_flags. Because it is not allowed to set both GPIOD_IS_IN and GPIOD_IS_OUT (see function check_dir_flags) only the dm_gpio_set_dir_flags(GPIO_D_IS_OUT) calls are successful, the dm_gpio_set_dir_flags(GPIO_D_IS_IN) calls fail and the GPIO pin is never set as an input. This causes that i2c_idle_bus finds that clearing the bus is unsuccessful and returns an error.
Ok, sounds like a bug to me.
Although i2c_idle_bus returns an error the i2c recover procedure itself is successful and the next I2C transfer will be successful. This requires that the I2C application to do a retry. This will work but is not the intention of the I2C driver.
I want to fix this problem. However it looks like that no dm_gpio API exists to change the direction of an GPIO pin while this is required to successful recover the i2c bus and detect that the recovery is successful. To fix this issue I see 2 possibilities:
- Use the old fashioned APIs gpio_direction_input and
gpio_direction_output 2. Add a new dm_gpio API dm_gpio_clear_and_set_dir_flags that allows to clear all existing set flags before setting the new flag(s)
Hmm.. I see, we only "or" the flags in dm_gpio_set_dir_flags() ... and than check if there are conflicts ... May we rework dm_gpio_set_dir_flags() so we prevent the conflicts we check in check_dir_flags() ?
Assumption is, the caller of dm_gpio_set_dir_flags() knows what he do...
@Simon: What do you think?
I prefer option 2 but I like to hear the opinion of the developer that designed the dm_gpio interface.
I did not designed the gpio interface!
bye, Heiko
Kind regards,
Kees Trommel.
This communication contains confidential information. If you are not the intended recipient please return this email to the sender and delete it from your records.
Diese Nachricht enthaelt vertrauliche Informationen. Sollten Sie nicht der beabsichtigte Empfaenger dieser E-mail sein, senden Sie bitte diese an den Absender zurueck und loeschen Sie die E-mail aus Ihrem System.
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
participants (3)
-
Heiko Schocher -
Simon Glass -
Trommel, Kees (Contractor)