Re: [PATCH 4/4] gpio: mpc8xxx: Preserve pre-init state of outputs

[Sinan Akman <sinan@writeme.com>]

Hi

On 2024-12-15 10:18, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer"<j.ne@posteo.net>
>
> The mpc8xxx_gpio driver contains a workaround for certain chips
> where the previously written state of outputs cannot be read back
> from the GPIO data (GPDAT) register (MPC8572/MPC8536). This workaround
> consists of tracking the state of GPDAT in a "shadow register" (i.e. a
> software variable). The shadow register is initialized to zero.
>
> This results in a problem w.r.t. outputs that are configured to a
> high (1) state before U-Boot runs, but not touched by U-Boot itself:
> Due to the zero-initialization, these GPIOs end up being set to zero,
> the first time that any other output is set.
>
> To avoid such issues initialize the GPDAT shadow register to the value
> previously held by any outputs, if possible. On MPC8572/MPC8536 this
> should make no difference,

  Did you test this on a MPC83xx board ?

> i.e. the shadow register should be
> initialized to zero on these chips.
>
> Signed-off-by: J. Neuschäfer<j.ne@posteo.net>
> ---
>   drivers/gpio/mpc8xxx_gpio.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c
> index e9bd38f162c10c76460bc63f93c03a711cb4d38e..709d04017d154ecae1e8d16d7c35ced521157c4d 100644
> --- a/drivers/gpio/mpc8xxx_gpio.c
> +++ b/drivers/gpio/mpc8xxx_gpio.c
> @@ -204,7 +204,17 @@ static int mpc8xxx_gpio_plat_to_priv(struct udevice *dev)
>   		return -ENOMEM;
>
>   	priv->gpio_count = plat->ngpios;
> -	priv->dat_shadow = 0;
> +
> +	/*
> +	 * On platforms that do support reading back output values, we want to
> +	 * try preserving them, so that we don't accidentally set unrelated
> +	 * GPIOs to zero in mpc8xxx_gpio_set_value.
> +	 */
> +	if (priv->little_endian)
> +		priv->dat_shadow = in_le32(&priv->base->gpdat) & in_le32(&priv->base->gpdir);
> +	else
> +		priv->dat_shadow = in_be32(&priv->base->gpdat) & in_be32(&priv->base->gpdir);
> +
>
>   	priv->type = driver_data;
>
>