[Intel-gfx] [PATCH] pinctrl: baytrail: Fix pin being driven low for a while on gpiod_get(..., GPIOD_OUT_HIGH)

[Andy Shevchenko]

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 at vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede at 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.

-- 
With Best Regards,
Andy Shevchenko