| Message ID | 20220724224943.294057-2-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock | expand |
On Mon, Jul 25, 2022 at 12:50 AM Marek Vasut <marex@denx.de> wrote: > Always configure GPIO pins which are used as interrupt source as INPUTs. > In case the default pin configuration is OUTPUT, or the prior stage does > configure the pins as OUTPUT, then Linux will not reconfigure the pin as > INPUT and no interrupts are received. > > Always configure interrupt source GPIO pin as input to fix the above case. > > Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Loic Poulain <loic.poulain@linaro.org> > Cc: Marc Zyngier <maz@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org> > --- > V2: Actually update and clear bits in GDIR register > V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag > V4: No change I understand what you are trying to achieve, and it makes sense. There's is just this one generic GPIO-based driver that makes me a little bit nervous here. Consider: drivers/media/cec/platform/cec-gpio/cec-gpio.c Look what the driver is doing with the gpiod_* operations on it's cec->cec_gpio. A certain GPIO pin is switched back and forth between input and output and in input mode, it is used to generate interrupts as well. Will this still work fine with the MXC driver after this change? At least it will be set to input mode twice, but I suppose that is fine, it's not your fault that the frameworks are orthogonal. Yours, Linus Walleij
On 7/26/22 10:15, Linus Walleij wrote: > On Mon, Jul 25, 2022 at 12:50 AM Marek Vasut <marex@denx.de> wrote: > >> Always configure GPIO pins which are used as interrupt source as INPUTs. >> In case the default pin configuration is OUTPUT, or the prior stage does >> configure the pins as OUTPUT, then Linux will not reconfigure the pin as >> INPUT and no interrupts are received. >> >> Always configure interrupt source GPIO pin as input to fix the above case. >> >> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Loic Poulain <loic.poulain@linaro.org> >> Cc: Marc Zyngier <maz@kernel.org> >> Cc: NXP Linux Team <linux-imx@nxp.com> >> Cc: Peng Fan <peng.fan@nxp.com> >> Cc: Shawn Guo <shawnguo@kernel.org> >> --- >> V2: Actually update and clear bits in GDIR register >> V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag >> V4: No change > > I understand what you are trying to achieve, and it makes sense. > > There's is just this one generic GPIO-based driver that makes me > a little bit nervous here. > > Consider: > drivers/media/cec/platform/cec-gpio/cec-gpio.c > Look what the driver is doing with the gpiod_* operations on it's > cec->cec_gpio. > > A certain GPIO pin is switched back and forth between input and > output and in input mode, it is used to generate interrupts as well. > > Will this still work fine with the MXC driver after this change? > At least it will be set to input mode twice, but I suppose that is > fine, it's not your fault that the frameworks are orthogonal. Ugh. I don't see why it shouldn't work, esp. if the CEC driver controls the direction and the default is input, but I wonder what other corner cases there are.
On 7/26/22 16:42, Marek Vasut wrote: > On 7/26/22 10:15, Linus Walleij wrote: >> On Mon, Jul 25, 2022 at 12:50 AM Marek Vasut <marex@denx.de> wrote: >> >>> Always configure GPIO pins which are used as interrupt source as INPUTs. >>> In case the default pin configuration is OUTPUT, or the prior stage does >>> configure the pins as OUTPUT, then Linux will not reconfigure the pin as >>> INPUT and no interrupts are received. >>> >>> Always configure interrupt source GPIO pin as input to fix the above case. >>> >>> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform") >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> Cc: Linus Walleij <linus.walleij@linaro.org> >>> Cc: Loic Poulain <loic.poulain@linaro.org> >>> Cc: Marc Zyngier <maz@kernel.org> >>> Cc: NXP Linux Team <linux-imx@nxp.com> >>> Cc: Peng Fan <peng.fan@nxp.com> >>> Cc: Shawn Guo <shawnguo@kernel.org> >>> --- >>> V2: Actually update and clear bits in GDIR register >>> V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag >>> V4: No change >> >> I understand what you are trying to achieve, and it makes sense. >> >> There's is just this one generic GPIO-based driver that makes me >> a little bit nervous here. >> >> Consider: >> drivers/media/cec/platform/cec-gpio/cec-gpio.c >> Look what the driver is doing with the gpiod_* operations on it's >> cec->cec_gpio. >> >> A certain GPIO pin is switched back and forth between input and >> output and in input mode, it is used to generate interrupts as well. >> >> Will this still work fine with the MXC driver after this change? >> At least it will be set to input mode twice, but I suppose that is >> fine, it's not your fault that the frameworks are orthogonal. > > Ugh. I don't see why it shouldn't work, esp. if the CEC driver controls the direction and the default is input, but I wonder what other corner cases there are. FYI: you can easily test cec-gpio by adding something along these lines to the dts: cec-gpio { compatible = "cec-gpio"; cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; }; (this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a pull up, then you're OK. It doesn't have to be connected to anything. If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device. If you can run 'cec-ctl --playback -p 1.0.0.0', then it works. Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c where the irq line has to be configured as an output during calibration. See tda998x_cec_calibration(). Regards, Hans
On Mon, Jul 25, 2022 at 12:50 AM Marek Vasut <marex@denx.de> wrote: > Always configure GPIO pins which are used as interrupt source as INPUTs. > In case the default pin configuration is OUTPUT, or the prior stage does > configure the pins as OUTPUT, then Linux will not reconfigure the pin as > INPUT and no interrupts are received. > > Always configure interrupt source GPIO pin as input to fix the above case. > > Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Loic Poulain <loic.poulain@linaro.org> > Cc: Marc Zyngier <maz@kernel.org> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org> It's up to the driver to deal with the final hardware state, and the author knows what he's doing so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 7/26/22 17:13, Hans Verkuil wrote: [...] > FYI: you can easily test cec-gpio by adding something along these lines to the dts: > > cec-gpio { > compatible = "cec-gpio"; > cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; > }; > > (this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a > pull up, then you're OK. It doesn't have to be connected to anything. > > If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device. > > If you can run 'cec-ctl --playback -p 1.0.0.0', then it works. > > Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c > where the irq line has to be configured as an output during calibration. > See tda998x_cec_calibration(). Does this look OK ? $ cec-ctl --playback -p 1.0.0.0 Driver Info: Driver Name : cec-gpio Adapter Name : cec-gpio Capabilities : 0x000000af Physical Address Logical Addresses Transmit Passthrough Monitor All Monitor Pin Driver version : 5.19.0 Available Logical Addresses: 4 Connector Info : None Physical Address : 1.0.0.0 Logical Address Mask : 0x0010 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'Playback' Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 4 (Playback Device 1) Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None $ cec-compliance Driver Info: Driver Name : cec-gpio Adapter Name : cec-gpio Capabilities : 0x000000af Physical Address Logical Addresses Transmit Passthrough Monitor All Monitor Pin Driver version : 5.19.0 Available Logical Addresses: 4 Connector Info : None Physical Address : 1.0.0.0 Logical Address Mask : 0x0010 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'Playback' Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 4 (Playback Device 1) Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None Compliance test for cec-gpio device /dev/cec0: The test results mean the following: OK Supported correctly by the device. OK (Not Supported) Not supported and not mandatory for the device. OK (Presumed) Presumably supported. Manually check to confirm. OK (Unexpected) Supported correctly but is not expected to be supported for this device. OK (Refused) Supported by the device, but was refused. OK (Expected Failure) Failed but this was expected (see -e option). FAIL Failed and was expected to be supported by this device. Find remote devices: Polling: OK FAIL: No remote devices found, exiting.
Hi Marek, On 29/07/2022 05:01, Marek Vasut wrote: > On 7/26/22 17:13, Hans Verkuil wrote: > > [...] > >> FYI: you can easily test cec-gpio by adding something along these lines to the dts: >> >> cec-gpio { >> compatible = "cec-gpio"; >> cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>; >> }; >> >> (this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a >> pull up, then you're OK. It doesn't have to be connected to anything. >> >> If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device. >> >> If you can run 'cec-ctl --playback -p 1.0.0.0', then it works. >> >> Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c >> where the irq line has to be configured as an output during calibration. >> See tda998x_cec_calibration(). > > Does this look OK ? > > $ cec-ctl --playback -p 1.0.0.0 > Driver Info: > Driver Name : cec-gpio > Adapter Name : cec-gpio > Capabilities : 0x000000af > Physical Address > Logical Addresses > Transmit > Passthrough > Monitor All > Monitor Pin > Driver version : 5.19.0 > Available Logical Addresses: 4 > Connector Info : None > Physical Address : 1.0.0.0 > Logical Address Mask : 0x0010 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : 'Playback' > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 4 (Playback Device 1) > Primary Device Type : Playback > Logical Address Type : Playback > All Device Types : Playback > RC TV Profile : None > Device Features : > None > > $ cec-compliance > Driver Info: > Driver Name : cec-gpio > Adapter Name : cec-gpio > Capabilities : 0x000000af > Physical Address > Logical Addresses > Transmit > Passthrough > Monitor All > Monitor Pin > Driver version : 5.19.0 > Available Logical Addresses: 4 > Connector Info : None > Physical Address : 1.0.0.0 > Logical Address Mask : 0x0010 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : 'Playback' > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 4 (Playback Device 1) > Primary Device Type : Playback > Logical Address Type : Playback > All Device Types : Playback > RC TV Profile : None > Device Features : > None > > Compliance test for cec-gpio device /dev/cec0: > > The test results mean the following: > OK Supported correctly by the device. > OK (Not Supported) Not supported and not mandatory for the device. > OK (Presumed) Presumably supported. Manually check to confirm. > OK (Unexpected) Supported correctly but is not expected to be supported for this device. > OK (Refused) Supported by the device, but was refused. > OK (Expected Failure) Failed but this was expected (see -e option). > FAIL Failed and was expected to be supported by this device. > > Find remote devices: > Polling: OK > > FAIL: No remote devices found, exiting. Yes, that looks fine. Regards, Hans
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index 75e7aceecac0a..f4cadbf30c6b8 100644 --- a/drivers/gpio/gpio-mxc.c +++ b/drivers/gpio/gpio-mxc.c @@ -149,7 +149,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type) struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); struct mxc_gpio_port *port = gc->private; unsigned long flags; - u32 bit, val; + u32 bit, val, dir; u32 gpio_idx = d->hwirq; int edge; void __iomem *reg = port->base; @@ -208,6 +208,10 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type) writel(1 << gpio_idx, port->base + GPIO_ISR); + dir = readl(port->base + GPIO_GDIR); + dir &= ~BIT(gpio_idx); + writel(dir, port->base + GPIO_GDIR); + spin_unlock_irqrestore(&port->gc.bgpio_lock, flags); return 0;
Always configure GPIO pins which are used as interrupt source as INPUTs. In case the default pin configuration is OUTPUT, or the prior stage does configure the pins as OUTPUT, then Linux will not reconfigure the pin as INPUT and no interrupts are received. Always configure interrupt source GPIO pin as input to fix the above case. Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Loic Poulain <loic.poulain@linaro.org> Cc: Marc Zyngier <maz@kernel.org> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: Peng Fan <peng.fan@nxp.com> Cc: Shawn Guo <shawnguo@kernel.org> --- V2: Actually update and clear bits in GDIR register V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag V4: No change --- drivers/gpio/gpio-mxc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)