* [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR @ 2023-03-22 11:26 haibo.chen 2023-03-22 11:30 ` Alexander Kochetkov 2023-03-22 19:10 ` Patrick DELAUNAY 0 siblings, 2 replies; 5+ messages in thread From: haibo.chen @ 2023-03-22 11:26 UTC (permalink / raw) To: al.kochet, hs, patrick.delaunay, sjg, andrew, patrice.chotard, samuel, marex Cc: uboot-imx, u-boot, haibo.chen From: Haibo Chen <haibo.chen@nxp.com> dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags. But there are cases like i2c_deblock_gpio_loop() will do like this: -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); -then config GPIO input dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); -then get the GPIO input value: dm_gpio_get_value(pin); When config the GPIO input, only set GPIOD_IS_IN, but unfortunately since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in flags, make the value from dm_gpio_get_value() not logic correct. So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue. Signed-off-by: Haibo Chen <haibo.chen@nxp.com> --- include/asm-generic/gpio.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index dd0bdf2315..903b237aac 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -131,7 +131,7 @@ struct gpio_desc { /* Flags for updating the above */ #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ - GPIOD_IS_OUT_ACTIVE) + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW) #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE) #define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN) -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR 2023-03-22 11:26 [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR haibo.chen @ 2023-03-22 11:30 ` Alexander Kochetkov 2023-03-22 19:10 ` Patrick DELAUNAY 1 sibling, 0 replies; 5+ messages in thread From: Alexander Kochetkov @ 2023-03-22 11:30 UTC (permalink / raw) To: Bough Chen Cc: hs, patrick.delaunay, Simon Glass, andrew, patrice.chotard, samuel, marex, dl-uboot-imx, u-boot Reviewed-by: Alexander Kochetkov <al.kochet@gmail.com> > 22 марта 2023 г., в 14:26, haibo.chen@nxp.com написал(а): > > From: Haibo Chen <haibo.chen@nxp.com> > > dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags. > But there are cases like i2c_deblock_gpio_loop() will do like this: > > -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | > GPIOD_ACTIVE_LOW | > GPIOD_IS_OUT_ACTIVE); > > -then config GPIO input > dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); > > -then get the GPIO input value: > dm_gpio_get_value(pin); > > When config the GPIO input, only set GPIOD_IS_IN, but unfortunately > since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in > flags, make the value from dm_gpio_get_value() not logic correct. > > So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > include/asm-generic/gpio.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > index dd0bdf2315..903b237aac 100644 > --- a/include/asm-generic/gpio.h > +++ b/include/asm-generic/gpio.h > @@ -131,7 +131,7 @@ struct gpio_desc { > > /* Flags for updating the above */ > #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ > - GPIOD_IS_OUT_ACTIVE) > + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW) > #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE) > #define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN) > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR 2023-03-22 11:26 [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR haibo.chen 2023-03-22 11:30 ` Alexander Kochetkov @ 2023-03-22 19:10 ` Patrick DELAUNAY 2023-03-23 8:17 ` Bough Chen 1 sibling, 1 reply; 5+ messages in thread From: Patrick DELAUNAY @ 2023-03-22 19:10 UTC (permalink / raw) To: haibo.chen, al.kochet, hs, sjg, andrew, patrice.chotard, samuel, marex Cc: uboot-imx, u-boot Hi On 3/22/23 12:26, haibo.chen@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags. > But there are cases like i2c_deblock_gpio_loop() will do like this: > > -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | > GPIOD_ACTIVE_LOW | > GPIOD_IS_OUT_ACTIVE); > > -then config GPIO input > dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); > > -then get the GPIO input value: > dm_gpio_get_value(pin); > > When config the GPIO input, only set GPIOD_IS_IN, but unfortunately > since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in > flags, make the value from dm_gpio_get_value() not logic correct. > > So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > include/asm-generic/gpio.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > index dd0bdf2315..903b237aac 100644 > --- a/include/asm-generic/gpio.h > +++ b/include/asm-generic/gpio.h > @@ -131,7 +131,7 @@ struct gpio_desc { > > /* Flags for updating the above */ > #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ > - GPIOD_IS_OUT_ACTIVE) > + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW) > #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE) > #define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN) > I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by device tree in the GPIO uclass: because the modified GPIOD_MASK_DIR is used in other location.... normally GPIOD_ACTIVE_LOW is saved in desc->flags it is the "desciptor flags" and must be not cleary by normal API see gpio_xlate_offs_flags() => gpio_flags_xlate() For example in gpio_request_tail(), in the line: /* Keep any direction flags provided by the devicetree */ ret = dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With your patch the descriptor flags is cleared / so DT information is lost. For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect. and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class => dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can change the caller ? 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_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } => 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); } The output value is the same => GPIOD_ACTIVE_LOW and GPIOD_IS_OUT_ACTIVE not active but you don't need to modify GPIOD_ACTIVE_LOW outside the GPIO uclass. Patrick ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR 2023-03-22 19:10 ` Patrick DELAUNAY @ 2023-03-23 8:17 ` Bough Chen 2023-03-23 9:05 ` Patrick DELAUNAY 0 siblings, 1 reply; 5+ messages in thread From: Bough Chen @ 2023-03-23 8:17 UTC (permalink / raw) To: Patrick DELAUNAY, al.kochet, hs, sjg, andrew, patrice.chotard, samuel, marex Cc: dl-uboot-imx, u-boot > -----Original Message----- > From: Patrick DELAUNAY <patrick.delaunay@foss.st.com> > Sent: 2023年3月23日 3:11 > To: Bough Chen <haibo.chen@nxp.com>; al.kochet@gmail.com; hs@denx.de; > sjg@chromium.org; andrew@aj.id.au; patrice.chotard@foss.st.com; > samuel@sholland.org; marex@denx.de > Cc: dl-uboot-imx <uboot-imx@nxp.com>; u-boot@lists.denx.de > Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR > > Hi > > On 3/22/23 12:26, haibo.chen@nxp.com wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags. > > But there are cases like i2c_deblock_gpio_loop() will do like this: > > > > -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW > > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | > > GPIOD_ACTIVE_LOW | > > GPIOD_IS_OUT_ACTIVE); > > > > -then config GPIO input > > dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); > > > > -then get the GPIO input value: > > dm_gpio_get_value(pin); > > > > When config the GPIO input, only set GPIOD_IS_IN, but unfortunately > > since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in > > flags, make the value from dm_gpio_get_value() not logic correct. > > > > So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > --- > > include/asm-generic/gpio.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > > index dd0bdf2315..903b237aac 100644 > > --- a/include/asm-generic/gpio.h > > +++ b/include/asm-generic/gpio.h > > @@ -131,7 +131,7 @@ struct gpio_desc { > > > > /* Flags for updating the above */ > > #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ > > - GPIOD_IS_OUT_ACTIVE) > > + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW) > > #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | > GPIOD_OPEN_SOURCE) > > #define GPIOD_MASK_PULL (GPIOD_PULL_UP | > GPIOD_PULL_DOWN) > > > > > I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by > device tree in the GPIO uclass: > > because the modified GPIOD_MASK_DIR is used in other location.... > > > normally GPIOD_ACTIVE_LOW is saved in desc->flags > > it is the "desciptor flags" and must be not cleary by normal API > > > see gpio_xlate_offs_flags() => gpio_flags_xlate() > > > > For example in gpio_request_tail(), in the line: > > /* Keep any direction flags provided by the devicetree */ ret = > dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With > your patch the descriptor flags is cleared / so DT information is lost. > For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect. > and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is > normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class => > dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can > change the caller ? > 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_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } => > > 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); } Here, for i2c-deblock, when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), the software actually want to config the gpio at output, and config the output to low level. This means in dts, need to config the i2c gpio as GPIO_ACTIVE_HIGH, then when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), it finally config the value to 0, which means low level. But from user point, we usually take the i2c gpio as GPIO_ACTIVE_LOW, seems a bit conflict. Any thoughts? Or just use my first patch? Best Regards Haibo Chen > > The output value is the same => GPIOD_ACTIVE_LOW and > GPIOD_IS_OUT_ACTIVE not active but you don't need to modify > GPIOD_ACTIVE_LOW outside the GPIO uclass. > Patrick ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR 2023-03-23 8:17 ` Bough Chen @ 2023-03-23 9:05 ` Patrick DELAUNAY 0 siblings, 0 replies; 5+ messages in thread From: Patrick DELAUNAY @ 2023-03-23 9:05 UTC (permalink / raw) To: Bough Chen, al.kochet, hs, sjg, andrew, patrice.chotard, samuel, marex Cc: dl-uboot-imx, u-boot Hi, On 3/23/23 09:17, Bough Chen wrote: >> -----Original Message----- >> From: Patrick DELAUNAY <patrick.delaunay@foss.st.com> >> Sent: 2023年3月23日 3:11 >> To: Bough Chen <haibo.chen@nxp.com>; al.kochet@gmail.com; hs@denx.de; >> sjg@chromium.org; andrew@aj.id.au; patrice.chotard@foss.st.com; >> samuel@sholland.org; marex@denx.de >> Cc: dl-uboot-imx <uboot-imx@nxp.com>; u-boot@lists.denx.de >> Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR >> >> Hi >> >> On 3/22/23 12:26, haibo.chen@nxp.com wrote: >>> From: Haibo Chen <haibo.chen@nxp.com> >>> >>> dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags. >>> But there are cases like i2c_deblock_gpio_loop() will do like this: >>> >>> -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW >>> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | >>> GPIOD_ACTIVE_LOW | >>> GPIOD_IS_OUT_ACTIVE); >>> >>> -then config GPIO input >>> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); >>> >>> -then get the GPIO input value: >>> dm_gpio_get_value(pin); >>> >>> When config the GPIO input, only set GPIOD_IS_IN, but unfortunately >>> since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in >>> flags, make the value from dm_gpio_get_value() not logic correct. >>> >>> So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue. >>> >>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> >>> --- >>> include/asm-generic/gpio.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h >>> index dd0bdf2315..903b237aac 100644 >>> --- a/include/asm-generic/gpio.h >>> +++ b/include/asm-generic/gpio.h >>> @@ -131,7 +131,7 @@ struct gpio_desc { >>> >>> /* Flags for updating the above */ >>> #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \ >>> - GPIOD_IS_OUT_ACTIVE) >>> + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW) >>> #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | >> GPIOD_OPEN_SOURCE) >>> #define GPIOD_MASK_PULL (GPIOD_PULL_UP | >> GPIOD_PULL_DOWN) >> >> I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by >> device tree in the GPIO uclass: >> >> because the modified GPIOD_MASK_DIR is used in other location.... >> >> >> normally GPIOD_ACTIVE_LOW is saved in desc->flags >> >> it is the "desciptor flags" and must be not cleary by normal API >> >> >> see gpio_xlate_offs_flags() => gpio_flags_xlate() >> >> >> >> For example in gpio_request_tail(), in the line: >> >> /* Keep any direction flags provided by the devicetree */ ret = >> dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With >> your patch the descriptor flags is cleared / so DT information is lost. >> For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect. >> and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is >> normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class => >> dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can >> change the caller ? >> 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_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } => >> >> 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); } > Here, for i2c-deblock, when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), the software actually want to config the gpio at output, and config the output to low level. > This means in dts, need to config the i2c gpio as GPIO_ACTIVE_HIGH, then when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), it finally config the value to 0, which means low level. > > But from user point, we usually take the i2c gpio as GPIO_ACTIVE_LOW, seems a bit conflict. > > Any thoughts? Or just use my first patch? I am lost (I am not dig in I2C GPIO part) but if I assume that GPIO_ACTIVE_HIGH is NOT activated in DT (because the GPIO line is directly connected to the I2C device) Then GPIO line = HIGH => GPIO value is 1 for uclass (input or output) => dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT); if you want to have output LOW => dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE); if you want to have output HIGH You can select what it is expected.... in I2C input => GPIO input selection output => ouput value selected by bit for example i2c_gpio_set_pin(struct gpio_desc *pin, int input, int bit) { if (input) dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); else if (bit) dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE); else dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT); } if the GPIO is inverted in DT (that means some inverse logic exist on hardware), with GPIO_ACTIVE_LOW => the result is inverted as expected for INPUT and OUTPUT if the logic is always inverted => you need to change the caller (request 1 instead 0) For me the SW side in U-boot should be not take care of GPIO_ACTIVE_LOW, except the GPIO uclass. for me your patch is not acceptable it solves the I2C GPIO issue but break ALL the other users of GPIO uclass. Regards Patrick ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-23 9:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-22 11:26 [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR haibo.chen 2023-03-22 11:30 ` Alexander Kochetkov 2023-03-22 19:10 ` Patrick DELAUNAY 2023-03-23 8:17 ` Bough Chen 2023-03-23 9:05 ` Patrick DELAUNAY
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.