| Message ID | 20200606093150.32882-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: baytrail: Fix pin being driven low for a while on gpiod_get(..., GPIOD_OUT_HIGH) | expand |
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag
fixing commit: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations").
The bot has tested the following trees: v5.6.16, v5.4.44, v4.19.126, v4.14.183, v4.9.226.
v5.6.16: Build OK!
v5.4.44: Failed to apply! Possible dependencies:
e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output")
v4.19.126: Failed to apply! Possible dependencies:
e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output")
v4.14.183: Failed to apply! Possible dependencies:
e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output")
v4.9.226: Failed to apply! Possible dependencies:
e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
On Sat, Jun 06, 2020 at 11:31:50AM +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. > > This commit also updates the byt_gpio_direction_input() to use direct > register accesses instead of going through pinctrl_gpio_direction_input(), > to keep it consistent with byt_gpio_direction_output(). > > Note for backporting, this commit depends on: > commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once > when setting direct-irq pin to output") > > Cc: stable@vger.kernel.org > Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Sat, Jun 6, 2020 at 11:31 AM Hans de Goede <hdegoede@redhat.com> 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. > > This commit also updates the byt_gpio_direction_input() to use direct > register accesses instead of going through pinctrl_gpio_direction_input(), > to keep it consistent with byt_gpio_direction_output(). > > Note for backporting, this commit depends on: > commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once > when setting direct-irq pin to output") > > Cc: stable@vger.kernel.org > Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") > 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. Looks good to me, I expect this in a pull request from Andy for fixes, alternatively I can apply it directly for fixes if Andy prefers this. Yours, Linus Walleij
On Wed, Jun 10, 2020 at 01:21:37PM +0200, Linus Walleij wrote: > On Sat, Jun 6, 2020 at 11:31 AM Hans de Goede <hdegoede@redhat.com> 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. > > > > This commit also updates the byt_gpio_direction_input() to use direct > > register accesses instead of going through pinctrl_gpio_direction_input(), > > to keep it consistent with byt_gpio_direction_output(). > > > > Note for backporting, this commit depends on: > > commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once > > when setting direct-irq pin to output") > > > > Cc: stable@vger.kernel.org > > Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") > > 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. > > Looks good to me, Thanks! > I expect this in a pull request from Andy for > fixes, alternatively I can apply it directly for fixes if Andy prefers > this. Mika or I will send a PR for v5.8-rcX at some point with fixes material. Though, there are GPIO stuff (PCA953x) which you can handle ;-)
On Mon, Jun 08, 2020 at 01:59:53PM +0300, Mika Westerberg wrote: > On Sat, Jun 06, 2020 at 11:31:50AM +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. > > > > This commit also updates the byt_gpio_direction_input() to use direct > > register accesses instead of going through pinctrl_gpio_direction_input(), > > to keep it consistent with byt_gpio_direction_output(). > > > > Note for backporting, this commit depends on: > > commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once > > when setting direct-irq pin to output") > > > > Cc: stable@vger.kernel.org > > Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> Pushed to my review and testing queue, thanks!
Hi, On 6/15/20 12:03 PM, Andy Shevchenko wrote: > On Mon, Jun 08, 2020 at 01:59:53PM +0300, Mika Westerberg wrote: >> On Sat, Jun 06, 2020 at 11:31:50AM +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. >>> >>> This commit also updates the byt_gpio_direction_input() to use direct >>> register accesses instead of going through pinctrl_gpio_direction_input(), >>> to keep it consistent with byt_gpio_direction_output(). >>> >>> Note for backporting, this commit depends on: >>> commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once >>> when setting direct-irq pin to output") >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Pushed to my review and testing queue, thanks! I'm not seeing it here: https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=review-andy Did you perhaps forgot to push it ? Regards, Hans
On Sat, Jun 20, 2020 at 03:09:20PM +0200, Hans de Goede wrote: > On 6/15/20 12:03 PM, Andy Shevchenko wrote: > > On Mon, Jun 08, 2020 at 01:59:53PM +0300, Mika Westerberg wrote: > > > On Sat, Jun 06, 2020 at 11:31:50AM +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. > > > > > > > > This commit also updates the byt_gpio_direction_input() to use direct > > > > register accesses instead of going through pinctrl_gpio_direction_input(), > > > > to keep it consistent with byt_gpio_direction_output(). > > > > > > > > Note for backporting, this commit depends on: > > > > commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once > > > > when setting direct-irq pin to output") > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Pushed to my review and testing queue, thanks! > > I'm not seeing it here: > > https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=review-andy > > Did you perhaps forgot to push it ? Oh, thanks! Now pushed! I'll send it as a part of PR for v5.8-rc3 to Linus W.
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index 9b821c9cbd16..b033f9d13fb4 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 Bay Trail, 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); @@ -1165,19 +1173,50 @@ static int byt_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) static int byt_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { - return pinctrl_gpio_direction_input(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; + + raw_spin_lock_irqsave(&byt_lock, flags); + + reg = readl(val_reg); + reg &= ~BYT_DIR_MASK; + reg |= BYT_OUTPUT_EN; + writel(reg, val_reg); + + raw_spin_unlock_irqrestore(&byt_lock, flags); + return 0; } +/* + * Note despite the temptation this MUST NOT be converted into a call to + * pinctrl_gpio_direction_output() + byt_gpio_set() that does not work this + * MUST be done as a single BYT_VAL_REG register write. + * See the commit message of the commit adding this comment for details. + */ 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_direct_irq_check(vg, offset); - byt_gpio_set(chip, offset, value); + 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. This commit also updates the byt_gpio_direction_input() to use direct register accesses instead of going through pinctrl_gpio_direction_input(), to keep it consistent with byt_gpio_direction_output(). Note for backporting, this commit depends on: commit e2b74419e5cc ("pinctrl: baytrail: Replace WARN with dev_info_once when setting direct-irq pin to output") Cc: stable@vger.kernel.org Fixes: 86e3ef812fe3 ("pinctrl: baytrail: Update gpio chip operations") 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. --- Changes in v2: - Add fixes tag - Also change byt_gpio_direction_input() to directly making the change itself for consistency - Add a comment above byt_gpio_direction_output() to avoid someone doing a well intended cleanup in the future re-introducing the problem --- drivers/pinctrl/intel/pinctrl-baytrail.c | 67 +++++++++++++++++++----- 1 file changed, 53 insertions(+), 14 deletions(-)