| Message ID | 20181210142135.23960-1-vladimir_zapolskiy@mentor.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: rcar: select General Output Register to set output states | expand |
On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > R-Car GPIO controller provides two interfaces to set GPIO line output > signal state, and for a particular GPIO line the selected interface is > determined by OUTDTSEL bit value. > > At the moment the driver supports only one of two interfaces, namely > OUTDT General Output Register is used to control the output signal. > > While this selection is the default one on reset, it is not explicitly > configured on probe, thus it might be possible that kernel and userspace > consumers of a GPIO won't be able to set the wanted GPIO output signal. > > Below is a simple test case to reproduce the described problem and > verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL > configuration from a bootloader: > > u-boot > mw.l 0xe6055440 0x3000 1 > ... > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger > > Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> Patch applied for devel as "non-urgent fix" as I can't see any immediate regressions. Thanks for your thorough investigation of the issue! Yours, Linus Walleij
Hi Vladimir, (increasing review priority due to Linus applying the patch) On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > R-Car GPIO controller provides two interfaces to set GPIO line output > signal state, and for a particular GPIO line the selected interface is > determined by OUTDTSEL bit value. > > At the moment the driver supports only one of two interfaces, namely > OUTDT General Output Register is used to control the output signal. > > While this selection is the default one on reset, it is not explicitly > configured on probe, thus it might be possible that kernel and userspace > consumers of a GPIO won't be able to set the wanted GPIO output signal. > > Below is a simple test case to reproduce the described problem and > verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL > configuration from a bootloader: > > u-boot > mw.l 0xe6055440 0x3000 1 > ... > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger > > Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> Thanks for your patch! > The proposed change could be seen as an invitation for a more interesting > discussion about a necessity to add a pretty trivial support of the second > interface, for instance by selecting between OUTDT and OUTDTH/OUTDTL on > basis of read-only value of OUTDTSEL register, or, simply by switching > the driver to use the second interface only, because it does not require > an additional gpio_rcar_read() call, theoretically it should give noticeably > faster rate of bitbanging. > > For reference the problem with the original interface comes from an inability > to set GPIO output signals from an RTOS, which runs in parallel to Linux and > wants to control some GPIOs on its own, usage of OUTDTH/OUTDTL excludes > a race in concurrent read/write register operations. > > As a note in my opinion the selection of OUTDT vs. OUTDTH/OUTDTL should > NOT be done in DTS, extension to 3-cell values for GPIO consumers seems > unreasonable. Indeed, this is pure software configuration, not hardware description, so it does not belong in DT. > Below is the list of helpful tips for change reviewers, comments are welcome: > * I didn't manage to find H1 or M1A User's Manuals to confirm that > OUTDTSEL register and the second OUTDTH/OUTDTL interface is present > on the GPIO controllers found on R-Car Gen1 SoCs, Unfortunately R-Car M1A and H1 do not have the OUTDTSEL nor OUTDTH/OUTDL registers. So your patch may break them. > * Fixes tag here is pretty weak, nevertheless I suppose it is a fix in fact, IMHO the SHA1 is not appropriate, as commit 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") added support for R-Car Gen1 only, while the OUTDT* registers appeared in R-Car Gen2. > * gpio_rcar_suspend()/gpio_rcar_resume() don't respect OUTDTSEL/OUTDTH/OUTDTL > values, if there is a reason to dump/restore registers, it might be good > to include them to the list also, Given resume calls gpio_rcar_direction_{in,out}put(), at least OUTDTSEL will be restored for output. Is that sufficient, or should it be restored for input, too? > * alternatively it might be possible to replace the original interface with > OUTDTH/OUTDTL one, it will be a nice valid fix also. Unfortunately that is not supported by all SoCs supported by the driver. Gr{oetje,eeting}s, Geert
Hi Linus, On Sun, Dec 16, 2018 at 1:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> wrote: > > R-Car GPIO controller provides two interfaces to set GPIO line output > > signal state, and for a particular GPIO line the selected interface is > > determined by OUTDTSEL bit value. > > > > At the moment the driver supports only one of two interfaces, namely > > OUTDT General Output Register is used to control the output signal. > > > > While this selection is the default one on reset, it is not explicitly > > configured on probe, thus it might be possible that kernel and userspace > > consumers of a GPIO won't be able to set the wanted GPIO output signal. > > > > Below is a simple test case to reproduce the described problem and > > verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL > > configuration from a bootloader: > > > > u-boot > mw.l 0xe6055440 0x3000 1 > > ... > > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger > > userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger > > > > Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > Patch applied for devel as "non-urgent fix" as I can't see any > immediate regressions. Unfortunately the driver now writes unconditionally to a register that does not exist on R-Car Gen1. Hence please revert the patch. Thanks! Gr{oetje,eeting}s, Geert
Hi Linus, Geert, On 12/16/2018 12:04 PM, Geert Uytterhoeven wrote: > Hi Linus, > > On Sun, Dec 16, 2018 at 1:19 AM Linus Walleij <linus.walleij@linaro.org> wrote: >> On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy >> <vladimir_zapolskiy@mentor.com> wrote: >>> R-Car GPIO controller provides two interfaces to set GPIO line output >>> signal state, and for a particular GPIO line the selected interface is >>> determined by OUTDTSEL bit value. >>> >>> At the moment the driver supports only one of two interfaces, namely >>> OUTDT General Output Register is used to control the output signal. >>> >>> While this selection is the default one on reset, it is not explicitly >>> configured on probe, thus it might be possible that kernel and userspace >>> consumers of a GPIO won't be able to set the wanted GPIO output signal. >>> >>> Below is a simple test case to reproduce the described problem and >>> verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL >>> configuration from a bootloader: >>> >>> u-boot > mw.l 0xe6055440 0x3000 1 >>> ... >>> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger >>> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger >>> >>> Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") >>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> >> Patch applied for devel as "non-urgent fix" as I can't see any >> immediate regressions. > > Unfortunately the driver now writes unconditionally to a register that does > not exist on R-Car Gen1. Hence please revert the patch. yes, for sure revert the change then. I'll prepare v2 which does not touch R-Car Gen1 platforms. -- Best wishes, Vladimir
On Sun, Dec 16, 2018 at 11:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") > > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > > > Patch applied for devel as "non-urgent fix" as I can't see any > > immediate regressions. > > Unfortunately the driver now writes unconditionally to a register that does > not exist on R-Car Gen1. Hence please revert the patch. Ooopsidaisy, I pulled it out! Yours, Linus Walleij
Hi Geert, On 12/16/2018 12:03 PM, Geert Uytterhoeven wrote: > Hi Vladimir, > > (increasing review priority due to Linus applying the patch) > > On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> wrote: >> R-Car GPIO controller provides two interfaces to set GPIO line output >> signal state, and for a particular GPIO line the selected interface is >> determined by OUTDTSEL bit value. >> >> At the moment the driver supports only one of two interfaces, namely >> OUTDT General Output Register is used to control the output signal. >> >> While this selection is the default one on reset, it is not explicitly >> configured on probe, thus it might be possible that kernel and userspace >> consumers of a GPIO won't be able to set the wanted GPIO output signal. >> >> Below is a simple test case to reproduce the described problem and >> verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL >> configuration from a bootloader: >> >> u-boot > mw.l 0xe6055440 0x3000 1 >> ... >> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger >> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger >> >> Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > Thanks for your patch! > >> The proposed change could be seen as an invitation for a more interesting >> discussion about a necessity to add a pretty trivial support of the second >> interface, for instance by selecting between OUTDT and OUTDTH/OUTDTL on >> basis of read-only value of OUTDTSEL register, or, simply by switching >> the driver to use the second interface only, because it does not require >> an additional gpio_rcar_read() call, theoretically it should give noticeably >> faster rate of bitbanging. >> >> For reference the problem with the original interface comes from an inability >> to set GPIO output signals from an RTOS, which runs in parallel to Linux and >> wants to control some GPIOs on its own, usage of OUTDTH/OUTDTL excludes >> a race in concurrent read/write register operations. >> >> As a note in my opinion the selection of OUTDT vs. OUTDTH/OUTDTL should >> NOT be done in DTS, extension to 3-cell values for GPIO consumers seems >> unreasonable. > > Indeed, this is pure software configuration, not hardware description, so it > does not belong in DT. > >> Below is the list of helpful tips for change reviewers, comments are welcome: >> * I didn't manage to find H1 or M1A User's Manuals to confirm that >> OUTDTSEL register and the second OUTDTH/OUTDTL interface is present >> on the GPIO controllers found on R-Car Gen1 SoCs, > > Unfortunately R-Car M1A and H1 do not have the OUTDTSEL nor OUTDTH/OUTDL > registers. So your patch may break them. FWIW I've managed to find only R01UH0573EJ0100 Rev.1.00 R-Car D1 User’s Manual (see Merlot evaluation board), and it describes OUTDTSEL/OUTDTH/OUTDTL and BOTHEDGE registers, thus a GPIO controller on this R-Car Gen1 SoC looks similar to GPIO controllers on R-Car Gen2/Gen3 SoCs. Thank you for clarification about R-Car M1A and H1 GPIO controllers, my v2 is in progress. >> * Fixes tag here is pretty weak, nevertheless I suppose it is a fix in fact, > > IMHO the SHA1 is not appropriate, as commit 119f5e448d32c ("gpio: Renesas > R-Car GPIO driver V3") added support for R-Car Gen1 only, while the OUTDT* > registers appeared in R-Car Gen2. > >> * gpio_rcar_suspend()/gpio_rcar_resume() don't respect OUTDTSEL/OUTDTH/OUTDTL >> values, if there is a reason to dump/restore registers, it might be good >> to include them to the list also, > > Given resume calls gpio_rcar_direction_{in,out}put(), at least OUTDTSEL > will be restored for output. Is that sufficient, or should it be restored for > input, too? Hmm, I was reflecting on necessity to save/restore OUTDTSEL value as a whole independently of per line gpiochip_line_is_valid() value, but let's omit it. I'm still influenced by a use-case of competing access to a GPIO controller from two OSes, there might be an overlapping with Linux PM routines in the driver. As a side note I'm not convinced that gpiochip_line_is_valid() and gpiochip->valid_mask usage in the driver is justified, unless it is agreed that 'gpio-reserved-ranges' property is really supposed to describe "holes" in GPIO controllers. The property found in r8a77470.dtsi (RZ/G1C) looks like a kludge instead of making a proper assignment of 'gpio-ranges' property: - gpio-ranges = <&pfc 0 96 30>; - gpio-reserved-ranges = <17 10>; + gpio-ranges = <&pfc 0 96 17>, <&pfc 27 123 3>; The change above is untested and I have no access to RZ/G1C manual, it is shared just to demonstrate an alternative idea of describing holes. >> * alternatively it might be possible to replace the original interface with >> OUTDTH/OUTDTL one, it will be a nice valid fix also. > > Unfortunately that is not supported by all SoCs supported by the driver. Would it be seen as beneficial to add support of a likely better interface for modern SoCs? The associated complexity in the driver won't be drastic. -- Best wishes, Vladimir
Hi Vladimir, On Tue, Dec 18, 2018 at 1:58 PM Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > On 12/16/2018 12:03 PM, Geert Uytterhoeven wrote: > > On Mon, Dec 10, 2018 at 3:22 PM Vladimir Zapolskiy > > <vladimir_zapolskiy@mentor.com> wrote: > >> R-Car GPIO controller provides two interfaces to set GPIO line output > >> signal state, and for a particular GPIO line the selected interface is > >> determined by OUTDTSEL bit value. > >> > >> At the moment the driver supports only one of two interfaces, namely > >> OUTDT General Output Register is used to control the output signal. > >> > >> While this selection is the default one on reset, it is not explicitly > >> configured on probe, thus it might be possible that kernel and userspace > >> consumers of a GPIO won't be able to set the wanted GPIO output signal. > >> > >> Below is a simple test case to reproduce the described problem and > >> verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL > >> configuration from a bootloader: > >> > >> u-boot > mw.l 0xe6055440 0x3000 1 > >> ... > >> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger > >> userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger > >> > >> Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > > > > Thanks for your patch! > > > >> The proposed change could be seen as an invitation for a more interesting > >> discussion about a necessity to add a pretty trivial support of the second > >> interface, for instance by selecting between OUTDT and OUTDTH/OUTDTL on > >> basis of read-only value of OUTDTSEL register, or, simply by switching > >> the driver to use the second interface only, because it does not require > >> an additional gpio_rcar_read() call, theoretically it should give noticeably > >> faster rate of bitbanging. > >> > >> For reference the problem with the original interface comes from an inability > >> to set GPIO output signals from an RTOS, which runs in parallel to Linux and > >> wants to control some GPIOs on its own, usage of OUTDTH/OUTDTL excludes > >> a race in concurrent read/write register operations. > >> > >> As a note in my opinion the selection of OUTDT vs. OUTDTH/OUTDTL should > >> NOT be done in DTS, extension to 3-cell values for GPIO consumers seems > >> unreasonable. > > > > Indeed, this is pure software configuration, not hardware description, so it > > does not belong in DT. > > > >> Below is the list of helpful tips for change reviewers, comments are welcome: > >> * I didn't manage to find H1 or M1A User's Manuals to confirm that > >> OUTDTSEL register and the second OUTDTH/OUTDTL interface is present > >> on the GPIO controllers found on R-Car Gen1 SoCs, > > > > Unfortunately R-Car M1A and H1 do not have the OUTDTSEL nor OUTDTH/OUTDL > > registers. So your patch may break them. > > FWIW I've managed to find only R01UH0573EJ0100 Rev.1.00 R-Car D1 User’s I don't have that one... > Manual (see Merlot evaluation board), and it describes OUTDTSEL/OUTDTH/OUTDTL Never heard of the Merlot board... > and BOTHEDGE registers, thus a GPIO controller on this R-Car Gen1 SoC looks > similar to GPIO controllers on R-Car Gen2/Gen3 SoCs. I'm not that surprised R-Car D1 contains R-Car Gen2 DNA: it's newer than R-Car H1/M1, and usually listed together with R-Car Gen2 SoCs. > >> * Fixes tag here is pretty weak, nevertheless I suppose it is a fix in fact, > > > > IMHO the SHA1 is not appropriate, as commit 119f5e448d32c ("gpio: Renesas > > R-Car GPIO driver V3") added support for R-Car Gen1 only, while the OUTDT* > > registers appeared in R-Car Gen2. > > > >> * gpio_rcar_suspend()/gpio_rcar_resume() don't respect OUTDTSEL/OUTDTH/OUTDTL > >> values, if there is a reason to dump/restore registers, it might be good > >> to include them to the list also, > > > > Given resume calls gpio_rcar_direction_{in,out}put(), at least OUTDTSEL > > will be restored for output. Is that sufficient, or should it be restored for > > input, too? > > Hmm, I was reflecting on necessity to save/restore OUTDTSEL value as a whole > independently of per line gpiochip_line_is_valid() value, but let's omit it. > > I'm still influenced by a use-case of competing access to a GPIO controller > from two OSes, there might be an overlapping with Linux PM routines in > the driver. PM is the module clock? If the second OS runs on the RT core, and uses the _R_MSTPCRn registers, everything should work fine. > As a side note I'm not convinced that gpiochip_line_is_valid() and > gpiochip->valid_mask usage in the driver is justified, unless it is agreed > that 'gpio-reserved-ranges' property is really supposed to describe "holes" > in GPIO controllers. The property found in r8a77470.dtsi (RZ/G1C) looks like > a kludge instead of making a proper assignment of 'gpio-ranges' property: > > - gpio-ranges = <&pfc 0 96 30>; > - gpio-reserved-ranges = <17 10>; > + gpio-ranges = <&pfc 0 96 17>, <&pfc 27 123 3>; > > The change above is untested and I have no access to RZ/G1C manual, it is > shared just to demonstrate an alternative idea of describing holes. FWIW, the RZ/G1C "User's Manual: Hardware" can be download from www.renesas.com. > >> * alternatively it might be possible to replace the original interface with > >> OUTDTH/OUTDTL one, it will be a nice valid fix also. > > > > Unfortunately that is not supported by all SoCs supported by the driver. > > Would it be seen as beneficial to add support of a likely better interface > for modern SoCs? The associated complexity in the driver won't be drastic. Sure. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index 8802b59bfec7..d6ada162b167 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -63,6 +63,7 @@ struct gpio_rcar_priv { #define POSNEG 0x20 /* Positive/Negative Logic Select Register */ #define EDGLEVEL 0x24 /* Edge/level Select Register */ #define FILONOFF 0x28 /* Chattering Prevention On/Off Register */ +#define OUTDTSEL 0x40 /* Output Data Select Register */ #define BOTHEDGE 0x4c /* One Edge/Both Edge Select Register */ #define RCAR_MAX_GPIO_PER_BANK 32 @@ -243,6 +244,10 @@ static void gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip, /* Select Input Mode or Output Mode in INOUTSEL */ gpio_rcar_modify_bit(p, INOUTSEL, gpio, output); + /* Select General Output Register to output data in OUTDTSEL */ + if (output) + gpio_rcar_modify_bit(p, OUTDTSEL, gpio, false); + spin_unlock_irqrestore(&p->lock, flags); }
R-Car GPIO controller provides two interfaces to set GPIO line output signal state, and for a particular GPIO line the selected interface is determined by OUTDTSEL bit value. At the moment the driver supports only one of two interfaces, namely OUTDT General Output Register is used to control the output signal. While this selection is the default one on reset, it is not explicitly configured on probe, thus it might be possible that kernel and userspace consumers of a GPIO won't be able to set the wanted GPIO output signal. Below is a simple test case to reproduce the described problem and verify this fix in the kernel on H3 ULCB by setting non-default OUTDTSEL configuration from a bootloader: u-boot > mw.l 0xe6055440 0x3000 1 ... userspace > echo -n default-on > /sys/devices/platform/leds/leds/led5/trigger userspace > echo -n default-on > /sys/devices/platform/leds/leds/led6/trigger Fixes: 119f5e448d32c ("gpio: Renesas R-Car GPIO driver V3") Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> --- The proposed change could be seen as an invitation for a more interesting discussion about a necessity to add a pretty trivial support of the second interface, for instance by selecting between OUTDT and OUTDTH/OUTDTL on basis of read-only value of OUTDTSEL register, or, simply by switching the driver to use the second interface only, because it does not require an additional gpio_rcar_read() call, theoretically it should give noticeably faster rate of bitbanging. For reference the problem with the original interface comes from an inability to set GPIO output signals from an RTOS, which runs in parallel to Linux and wants to control some GPIOs on its own, usage of OUTDTH/OUTDTL excludes a race in concurrent read/write register operations. As a note in my opinion the selection of OUTDT vs. OUTDTH/OUTDTL should NOT be done in DTS, extension to 3-cell values for GPIO consumers seems unreasonable. Below is the list of helpful tips for change reviewers, comments are welcome: * I didn't manage to find H1 or M1A User's Manuals to confirm that OUTDTSEL register and the second OUTDTH/OUTDTL interface is present on the GPIO controllers found on R-Car Gen1 SoCs, * Fixes tag here is pretty weak, nevertheless I suppose it is a fix in fact, * gpio_rcar_suspend()/gpio_rcar_resume() don't respect OUTDTSEL/OUTDTH/OUTDTL values, if there is a reason to dump/restore registers, it might be good to include them to the list also, * alternatively it might be possible to replace the original interface with OUTDTH/OUTDTL one, it will be a nice valid fix also. drivers/gpio/gpio-rcar.c | 5 +++++ 1 file changed, 5 insertions(+)