| Message ID | 20200602122130.45630-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: baytrail: Fix pin being driven low for a while on gpiod_get(..., GPIOD_OUT_HIGH) | expand |
On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: > The pins on the Bay Trail SoC have separate input-buffer and output-buffer > enable bits and a read of the level bit of the value register will always > return the value from the input-buffer. > > The BIOS of a device may configure a pin in output-only mode, only enabling > the output buffer, and write 1 to the level bit to drive the pin high. > This 1 written to the level bit will be stored inside the data-latch of the > output buffer. > > But a subsequent read of the value register will return 0 for the level bit > because the input-buffer is disabled. This causes a read-modify-write as > done by byt_gpio_set_direction() to write 0 to the level bit, driving the > pin low! > > Before this commit byt_gpio_direction_output() relied on > pinctrl_gpio_direction_output() to set the direction, followed by a call > to byt_gpio_set() to apply the selected value. This causes the pin to > go low between the pinctrl_gpio_direction_output() and byt_gpio_set() > calls. > > Change byt_gpio_direction_output() to directly make the register > modifications itself instead. Replacing the 2 subsequent writes to the > value register with a single write. > > Note that the pinctrl code does not keep track internally of the direction, > so not going through pinctrl_gpio_direction_output() is not an issue. > > This issue was noticed on a Trekstor SurfTab Twin 10.1. When the panel is > already on at boot (no external monitor connected), then the i915 driver > does a gpiod_get(..., GPIOD_OUT_HIGH) for the panel-enable GPIO. The > temporarily going low of that GPIO was causing the panel to reset itself > after which it would not show an image until it was turned off and back on > again (until a full modeset was done on it). This commit fixes this. No Fixes tag? > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> ... > +static void byt_gpio_direct_irq_check(struct intel_pinctrl *vg, > + unsigned int offset) > +{ > + void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); > + > + /* > + * Before making any direction modifications, do a check if gpio is set > + * for direct IRQ. On baytrail, setting GPIO to output does not make Since we change this, perhaps 'IRQ. On baytrail' -> 'IRQ. On Baytrail' (one space and capital 'B'). > + * sense, so let's at least inform the caller before they shoot > + * themselves in the foot. > + */ > + if (readl(conf_reg) & BYT_DIRECT_IRQ_EN) > + dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output"); > +} ... > static int byt_gpio_direction_output(struct gpio_chip *chip, > unsigned int offset, int value) > { > - int ret = pinctrl_gpio_direction_output(chip->base + offset); > + struct intel_pinctrl *vg = gpiochip_get_data(chip); > + void __iomem *val_reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); > + unsigned long flags; > + u32 reg; > > - if (ret) > - return ret; > + raw_spin_lock_irqsave(&byt_lock, flags); > > - byt_gpio_set(chip, offset, value); > + byt_gpio_direct_irq_check(vg, offset); > > + reg = readl(val_reg); > + reg &= ~BYT_DIR_MASK; > + if (value) > + reg |= BYT_LEVEL; > + else > + reg &= ~BYT_LEVEL; > + > + writel(reg, val_reg); > + > + raw_spin_unlock_irqrestore(&byt_lock, flags); > return 0; > } Wouldn't be simple below fix the issue? @@ -1171,14 +1171,10 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) static int byt_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value) { - int ret = pinctrl_gpio_direction_output(chip->base + offset); - - if (ret) - return ret; - + /* Set value first to avoid a glitch */ byt_gpio_set(chip, offset, value); - return 0; + return pinctrl_gpio_direction_output(chip->base + offset); } P.S. It's mangled, sorry.
On Tue, Jun 02, 2020 at 06:23:17PM +0300, Andy Shevchenko wrote: > On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: ... > Wouldn't be simple below fix the issue? > > @@ -1171,14 +1171,10 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) > static int byt_gpio_direction_output(struct gpio_chip *chip, > unsigned int offset, int value) > { > - int ret = pinctrl_gpio_direction_output(chip->base + offset); > - > - if (ret) > - return ret; > - > + /* Set value first to avoid a glitch */ > byt_gpio_set(chip, offset, value); > > - return 0; > + return pinctrl_gpio_direction_output(chip->base + offset); > } > > > P.S. It's mangled, sorry. Cherrytrail does this way, btw, 549e783f6a1.
Hi, On 6/2/20 5:23 PM, Andy Shevchenko wrote: > On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: >> The pins on the Bay Trail SoC have separate input-buffer and output-buffer >> enable bits and a read of the level bit of the value register will always >> return the value from the input-buffer. >> >> The BIOS of a device may configure a pin in output-only mode, only enabling >> the output buffer, and write 1 to the level bit to drive the pin high. >> This 1 written to the level bit will be stored inside the data-latch of the >> output buffer. >> >> But a subsequent read of the value register will return 0 for the level bit >> because the input-buffer is disabled. This causes a read-modify-write as >> done by byt_gpio_set_direction() to write 0 to the level bit, driving the >> pin low! >> >> Before this commit byt_gpio_direction_output() relied on >> pinctrl_gpio_direction_output() to set the direction, followed by a call >> to byt_gpio_set() to apply the selected value. This causes the pin to >> go low between the pinctrl_gpio_direction_output() and byt_gpio_set() >> calls. >> >> Change byt_gpio_direction_output() to directly make the register >> modifications itself instead. Replacing the 2 subsequent writes to the >> value register with a single write. >> >> Note that the pinctrl code does not keep track internally of the direction, >> so not going through pinctrl_gpio_direction_output() is not an issue. >> >> This issue was noticed on a Trekstor SurfTab Twin 10.1. When the panel is >> already on at boot (no external monitor connected), then the i915 driver >> does a gpiod_get(..., GPIOD_OUT_HIGH) for the panel-enable GPIO. The >> temporarily going low of that GPIO was causing the panel to reset itself >> after which it would not show an image until it was turned off and back on >> again (until a full modeset was done on it). This commit fixes this. > > No Fixes tag? It is sort of hard to pin the introduction of this down to a single commit. If I were to guess, I guess the commit introducing the driver? >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > ... > >> +static void byt_gpio_direct_irq_check(struct intel_pinctrl *vg, >> + unsigned int offset) >> +{ >> + void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); >> + >> + /* >> + * Before making any direction modifications, do a check if gpio is set > >> + * for direct IRQ. On baytrail, setting GPIO to output does not make > > Since we change this, perhaps > > 'IRQ. On baytrail' -> 'IRQ. On Baytrail' (one space and capital 'B'). Sure, not sure if that is worth respinning the patch for though, either way let me know. >> + * sense, so let's at least inform the caller before they shoot >> + * themselves in the foot. >> + */ >> + if (readl(conf_reg) & BYT_DIRECT_IRQ_EN) >> + dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output"); >> +} > > ... > >> static int byt_gpio_direction_output(struct gpio_chip *chip, >> unsigned int offset, int value) >> { >> - int ret = pinctrl_gpio_direction_output(chip->base + offset); >> + struct intel_pinctrl *vg = gpiochip_get_data(chip); >> + void __iomem *val_reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); >> + unsigned long flags; >> + u32 reg; >> >> - if (ret) >> - return ret; >> + raw_spin_lock_irqsave(&byt_lock, flags); >> >> - byt_gpio_set(chip, offset, value); >> + byt_gpio_direct_irq_check(vg, offset); >> >> + reg = readl(val_reg); >> + reg &= ~BYT_DIR_MASK; >> + if (value) >> + reg |= BYT_LEVEL; >> + else >> + reg &= ~BYT_LEVEL; >> + >> + writel(reg, val_reg); >> + >> + raw_spin_unlock_irqrestore(&byt_lock, flags); >> return 0; >> } > > Wouldn't be simple below fix the issue? > > @@ -1171,14 +1171,10 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) > static int byt_gpio_direction_output(struct gpio_chip *chip, > unsigned int offset, int value) > { > - int ret = pinctrl_gpio_direction_output(chip->base + offset); > - > - if (ret) > - return ret; > - > + /* Set value first to avoid a glitch */ > byt_gpio_set(chip, offset, value); > > - return 0; > + return pinctrl_gpio_direction_output(chip->base + offset); > } No that will not help the pin is already high, but any reads of the register will return the BYT_LEVEL bit as being low, so the read-write-modify done when setting the direction reads BYT_LEVEL as 0 and writes it back as such. So your proposal would actually make the problem much worse (and more obvious) if we do the byt_gpio_set() first then for pins which have there input-buffer initially disabled, the value passed to byt_gpio_direction_output will be completely ignored and they will always end up as being driven low. Regards, Hans
On Fri, Jun 05, 2020 at 04:33:47PM +0200, Hans de Goede wrote: > On 6/2/20 5:23 PM, Andy Shevchenko wrote: > > On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: > > > The pins on the Bay Trail SoC have separate input-buffer and output-buffer > > > enable bits and a read of the level bit of the value register will always > > > return the value from the input-buffer. > > > > > > The BIOS of a device may configure a pin in output-only mode, only enabling > > > the output buffer, and write 1 to the level bit to drive the pin high. > > > This 1 written to the level bit will be stored inside the data-latch of the > > > output buffer. > > > > > > But a subsequent read of the value register will return 0 for the level bit > > > because the input-buffer is disabled. This causes a read-modify-write as > > > done by byt_gpio_set_direction() to write 0 to the level bit, driving the > > > pin low! > > > > > > Before this commit byt_gpio_direction_output() relied on > > > pinctrl_gpio_direction_output() to set the direction, followed by a call > > > to byt_gpio_set() to apply the selected value. This causes the pin to > > > go low between the pinctrl_gpio_direction_output() and byt_gpio_set() > > > calls. > > > > > > Change byt_gpio_direction_output() to directly make the register > > > modifications itself instead. Replacing the 2 subsequent writes to the > > > value register with a single write. > > > > > > Note that the pinctrl code does not keep track internally of the direction, > > > so not going through pinctrl_gpio_direction_output() is not an issue. > > > > > > This issue was noticed on a Trekstor SurfTab Twin 10.1. When the panel is > > > already on at boot (no external monitor connected), then the i915 driver > > > does a gpiod_get(..., GPIOD_OUT_HIGH) for the panel-enable GPIO. The > > > temporarily going low of that GPIO was causing the panel to reset itself > > > after which it would not show an image until it was turned off and back on > > > again (until a full modeset was done on it). This commit fixes this. > > > > No Fixes tag? > > It is sort of hard to pin the introduction of this down to a single > commit. If I were to guess, I guess the commit introducing the driver? Why not? Good guess to me (but I think rather the one which converts GPIO driver to pin control). ... > > > + /* > > > + * Before making any direction modifications, do a check if gpio is set > > > > > + * for direct IRQ. On baytrail, setting GPIO to output does not make > > > > Since we change this, perhaps > > > > 'IRQ. On baytrail' -> 'IRQ. On Baytrail' (one space and capital 'B'). > > Sure, not sure if that is worth respinning the patch for though, > either way let me know. I think makes sense to respin. We still have time. > > > + * sense, so let's at least inform the caller before they shoot > > > + * themselves in the foot. > > > + */ ... > > Wouldn't be simple below fix the issue? > No that will not help the pin is already high, but any reads > of the register will return the BYT_LEVEL bit as being low, so > the read-write-modify done when setting the direction reads BYT_LEVEL > as 0 and writes it back as such. So, if I read documentation correctly, there is no means to read back current output value if input is disabled. Alas, quite a bad design of hardware. And on top of that likely nobody has tested that on non-Windows platform. > So your proposal would actually make the problem much worse (and more > obvious) if we do the byt_gpio_set() first then for pins which have > there input-buffer initially disabled, the value passed to > byt_gpio_direction_output will be completely ignored and they will > always end up as being driven low. What I proposed is not gonna work AFAIU documentation. Btw, can we for sake of consistency update direction_input() as well?
Hi, On 6/5/20 7:09 PM, Andy Shevchenko wrote: > On Fri, Jun 05, 2020 at 04:33:47PM +0200, Hans de Goede wrote: >> On 6/2/20 5:23 PM, Andy Shevchenko wrote: >>> On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: >>>> The pins on the Bay Trail SoC have separate input-buffer and output-buffer >>>> enable bits and a read of the level bit of the value register will always >>>> return the value from the input-buffer. >>>> >>>> The BIOS of a device may configure a pin in output-only mode, only enabling >>>> the output buffer, and write 1 to the level bit to drive the pin high. >>>> This 1 written to the level bit will be stored inside the data-latch of the >>>> output buffer. >>>> >>>> But a subsequent read of the value register will return 0 for the level bit >>>> because the input-buffer is disabled. This causes a read-modify-write as >>>> done by byt_gpio_set_direction() to write 0 to the level bit, driving the >>>> pin low! >>>> >>>> Before this commit byt_gpio_direction_output() relied on >>>> pinctrl_gpio_direction_output() to set the direction, followed by a call >>>> to byt_gpio_set() to apply the selected value. This causes the pin to >>>> go low between the pinctrl_gpio_direction_output() and byt_gpio_set() >>>> calls. >>>> >>>> Change byt_gpio_direction_output() to directly make the register >>>> modifications itself instead. Replacing the 2 subsequent writes to the >>>> value register with a single write. >>>> >>>> Note that the pinctrl code does not keep track internally of the direction, >>>> so not going through pinctrl_gpio_direction_output() is not an issue. >>>> >>>> This issue was noticed on a Trekstor SurfTab Twin 10.1. When the panel is >>>> already on at boot (no external monitor connected), then the i915 driver >>>> does a gpiod_get(..., GPIOD_OUT_HIGH) for the panel-enable GPIO. The >>>> temporarily going low of that GPIO was causing the panel to reset itself >>>> after which it would not show an image until it was turned off and back on >>>> again (until a full modeset was done on it). This commit fixes this. >>> >>> No Fixes tag? >> >> It is sort of hard to pin the introduction of this down to a single >> commit. If I were to guess, I guess the commit introducing the driver? > > Why not? Good guess to me (but I think rather the one which converts GPIO > driver to pin control). I will check and add a fixes tag for v2. > ... > >>>> + /* >>>> + * Before making any direction modifications, do a check if gpio is set >>> >>>> + * for direct IRQ. On baytrail, setting GPIO to output does not make >>> >>> Since we change this, perhaps >>> >>> 'IRQ. On baytrail' -> 'IRQ. On Baytrail' (one space and capital 'B'). >> >> Sure, not sure if that is worth respinning the patch for though, >> either way let me know. > > I think makes sense to respin. We still have time. I wasn't talking about timing, more just that it creates extra work (for me) and if that was just for the capital 'B' thingie it would not be worth the extra work IMHO, but since we need a v2 for the fixes tag anyways I'll fix this as well. >>>> + * sense, so let's at least inform the caller before they shoot >>>> + * themselves in the foot. >>>> + */ > > ... > >>> Wouldn't be simple below fix the issue? > >> No that will not help the pin is already high, but any reads >> of the register will return the BYT_LEVEL bit as being low, so >> the read-write-modify done when setting the direction reads BYT_LEVEL >> as 0 and writes it back as such. > > So, if I read documentation correctly, there is no means to read back current > output value if input is disabled. Alas, quite a bad design of hardware. > And on top of that likely nobody has tested that on non-Windows platform. > >> So your proposal would actually make the problem much worse (and more >> obvious) if we do the byt_gpio_set() first then for pins which have >> there input-buffer initially disabled, the value passed to >> byt_gpio_direction_output will be completely ignored and they will >> always end up as being driven low. > > What I proposed is not gonna work AFAIU documentation. > > Btw, can we for sake of consistency update direction_input() as well? Sure, the change for that will be quite small, so shall I out it in this patch, or do you want a second patch for that? Regards, Hans
On Fri, Jun 05, 2020 at 07:31:35PM +0200, Hans de Goede wrote: > On 6/5/20 7:09 PM, Andy Shevchenko wrote: > > On Fri, Jun 05, 2020 at 04:33:47PM +0200, Hans de Goede wrote: > > > On 6/2/20 5:23 PM, Andy Shevchenko wrote: > > > > On Tue, Jun 02, 2020 at 02:21:30PM +0200, Hans de Goede wrote: ... > > > Sure, not sure if that is worth respinning the patch for though, > > > either way let me know. > > > > I think makes sense to respin. We still have time. > > I wasn't talking about timing, more just that it creates extra > work (for me) and if that was just for the capital 'B' thingie it > would not be worth the extra work IMHO, but since we need a v2 for > the fixes tag anyways I'll fix this as well. I got your point, no problem, I would fix myself, if it is only the comment to address. ... > > Btw, can we for sake of consistency update direction_input() as well? > > Sure, the change for that will be quite small, so shall I out it > in this patch, or do you want a second patch for that? I think we can do in one.
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index 9b821c9cbd16..83be13b83eb5 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -800,6 +800,21 @@ static void byt_gpio_disable_free(struct pinctrl_dev *pctl_dev, pm_runtime_put(vg->dev); } +static void byt_gpio_direct_irq_check(struct intel_pinctrl *vg, + unsigned int offset) +{ + void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); + + /* + * Before making any direction modifications, do a check if gpio is set + * for direct IRQ. On baytrail, setting GPIO to output does not make + * sense, so let's at least inform the caller before they shoot + * themselves in the foot. + */ + if (readl(conf_reg) & BYT_DIRECT_IRQ_EN) + dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output"); +} + static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev, struct pinctrl_gpio_range *range, unsigned int offset, @@ -807,7 +822,6 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev, { struct intel_pinctrl *vg = pinctrl_dev_get_drvdata(pctl_dev); void __iomem *val_reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); - void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); unsigned long flags; u32 value; @@ -817,14 +831,8 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev, value &= ~BYT_DIR_MASK; if (input) value |= BYT_OUTPUT_EN; - else if (readl(conf_reg) & BYT_DIRECT_IRQ_EN) - /* - * Before making any direction modifications, do a check if gpio - * is set for direct IRQ. On baytrail, setting GPIO to output - * does not make sense, so let's at least inform the caller before - * they shoot themselves in the foot. - */ - dev_info_once(vg->dev, "Potential Error: Setting GPIO with direct_irq_en to output"); + else + byt_gpio_direct_irq_check(vg, offset); writel(value, val_reg); @@ -1171,13 +1179,25 @@ static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) static int byt_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value) { - int ret = pinctrl_gpio_direction_output(chip->base + offset); + struct intel_pinctrl *vg = gpiochip_get_data(chip); + void __iomem *val_reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); + unsigned long flags; + u32 reg; - if (ret) - return ret; + raw_spin_lock_irqsave(&byt_lock, flags); - byt_gpio_set(chip, offset, value); + byt_gpio_direct_irq_check(vg, offset); + reg = readl(val_reg); + reg &= ~BYT_DIR_MASK; + if (value) + reg |= BYT_LEVEL; + else + reg &= ~BYT_LEVEL; + + writel(reg, val_reg); + + raw_spin_unlock_irqrestore(&byt_lock, flags); return 0; }
The pins on the Bay Trail SoC have separate input-buffer and output-buffer enable bits and a read of the level bit of the value register will always return the value from the input-buffer. The BIOS of a device may configure a pin in output-only mode, only enabling the output buffer, and write 1 to the level bit to drive the pin high. This 1 written to the level bit will be stored inside the data-latch of the output buffer. But a subsequent read of the value register will return 0 for the level bit because the input-buffer is disabled. This causes a read-modify-write as done by byt_gpio_set_direction() to write 0 to the level bit, driving the pin low! Before this commit byt_gpio_direction_output() relied on pinctrl_gpio_direction_output() to set the direction, followed by a call to byt_gpio_set() to apply the selected value. This causes the pin to go low between the pinctrl_gpio_direction_output() and byt_gpio_set() calls. Change byt_gpio_direction_output() to directly make the register modifications itself instead. Replacing the 2 subsequent writes to the value register with a single write. Note that the pinctrl code does not keep track internally of the direction, so not going through pinctrl_gpio_direction_output() is not an issue. This issue was noticed on a Trekstor SurfTab Twin 10.1. When the panel is already on at boot (no external monitor connected), then the i915 driver does a gpiod_get(..., GPIOD_OUT_HIGH) for the panel-enable GPIO. The temporarily going low of that GPIO was causing the panel to reset itself after which it would not show an image until it was turned off and back on again (until a full modeset was done on it). This commit fixes this. Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note the factoring out of the direct IRQ mode warning is deliberately not split into a separate patch to make backporting this easier. --- drivers/pinctrl/intel/pinctrl-baytrail.c | 46 +++++++++++++++++------- 1 file changed, 33 insertions(+), 13 deletions(-)