[PATCH] spi: mpc8xxx_spi.c: fix cs activate/deactivate

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] spi: mpc8xxx_spi.c: fix cs activate/deactivate

Rasmus Villemoes
Somewhere between v2020.04 and v2020.07 the mpc8xxx_spi driver broke,
I'm guessing due to this hunk

@@ -559,6 +560,8 @@ int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
        if (ret)
                return ret;

+       /* combine the requested flags (for IN/OUT) and the descriptor flags */
+       flags |= desc->flags;
        ret = _dm_gpio_set_dir_flags(desc, flags);

from commit 695e5fd5469a ("gpio: update dir_flags management"). But
the blame is mostly on the driver itself which seems rather confused:
The chip select gpios are requested with GPIOD_ACTIVE_LOW, but then in
each activate/deactivate, dm_gpio_set_dir_flags() is called with
merely GPIOD_IS_OUT, and then the driver call set_value(0) for
activate.

That used to work, but with the above hunk, the ACTIVE_LOW setting
from the request becomes persistent, so the gpio driver ends up being
asked to set the value to 1 in mpc8xxx_spi_cs_activate().

So drop the dm_gpio_set_dir_flags() calls in the activate/deactivate
functions, and use a value of 1 to mean "logically enabled".

Ideally, I think we should also drop the GPIOD_ACTIVE_LOW from the
request and make it up to the list of gpio cs in DT to indicate
whether that CS is enabled when driven low (as is of course usually
the case), but that requires changing
arch/powerpc/dts/gdsys/gazerbeam-base.dtsi among others, and I don't
have that hardware to test on. I have, however, tested our
own (mpc8309-based) hardware with this change, and I have also tested
that removing the GPIOD_ACTIVE_LOW from the request and updating our
DT as

-                       gpios = <&spisel 0 0>;
+                       gpios = <&spisel 0 GPIO_ACTIVE_LOW>;

still works.

Signed-off-by: Rasmus Villemoes <[hidden email]>
---
 drivers/spi/mpc8xxx_spi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 811b5d44fb..ec39c12b3d 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -109,8 +109,7 @@ static void mpc8xxx_spi_cs_activate(struct udevice *dev)
  struct mpc8xxx_priv *priv = dev_get_priv(dev->parent);
  struct dm_spi_slave_platdata *platdata = dev_get_parent_platdata(dev);
 
- dm_gpio_set_dir_flags(&priv->gpios[platdata->cs], GPIOD_IS_OUT);
- dm_gpio_set_value(&priv->gpios[platdata->cs], 0);
+ dm_gpio_set_value(&priv->gpios[platdata->cs], 1);
 }
 
 static void mpc8xxx_spi_cs_deactivate(struct udevice *dev)
@@ -118,8 +117,7 @@ static void mpc8xxx_spi_cs_deactivate(struct udevice *dev)
  struct mpc8xxx_priv *priv = dev_get_priv(dev->parent);
  struct dm_spi_slave_platdata *platdata = dev_get_parent_platdata(dev);
 
- dm_gpio_set_dir_flags(&priv->gpios[platdata->cs], GPIOD_IS_OUT);
- dm_gpio_set_value(&priv->gpios[platdata->cs], 1);
+ dm_gpio_set_value(&priv->gpios[platdata->cs], 0);
 }
 
 static int mpc8xxx_spi_xfer(struct udevice *dev, uint bitlen,
--
2.23.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] spi: mpc8xxx_spi.c: fix cs activate/deactivate

Tom Rini-4
On Fri, Sep 18, 2020 at 04:26:06PM +0200, Rasmus Villemoes wrote:

> Somewhere between v2020.04 and v2020.07 the mpc8xxx_spi driver broke,
> I'm guessing due to this hunk
>
> @@ -559,6 +560,8 @@ int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
>         if (ret)
>                 return ret;
>
> +       /* combine the requested flags (for IN/OUT) and the descriptor flags */
> +       flags |= desc->flags;
>         ret = _dm_gpio_set_dir_flags(desc, flags);
>
> from commit 695e5fd5469a ("gpio: update dir_flags management"). But
> the blame is mostly on the driver itself which seems rather confused:
> The chip select gpios are requested with GPIOD_ACTIVE_LOW, but then in
> each activate/deactivate, dm_gpio_set_dir_flags() is called with
> merely GPIOD_IS_OUT, and then the driver call set_value(0) for
> activate.
>
> That used to work, but with the above hunk, the ACTIVE_LOW setting
> from the request becomes persistent, so the gpio driver ends up being
> asked to set the value to 1 in mpc8xxx_spi_cs_activate().
>
> So drop the dm_gpio_set_dir_flags() calls in the activate/deactivate
> functions, and use a value of 1 to mean "logically enabled".
>
> Ideally, I think we should also drop the GPIOD_ACTIVE_LOW from the
> request and make it up to the list of gpio cs in DT to indicate
> whether that CS is enabled when driven low (as is of course usually
> the case), but that requires changing
> arch/powerpc/dts/gdsys/gazerbeam-base.dtsi among others, and I don't
> have that hardware to test on. I have, however, tested our
> own (mpc8309-based) hardware with this change, and I have also tested
> that removing the GPIOD_ACTIVE_LOW from the request and updating our
> DT as
>
> -                       gpios = <&spisel 0 0>;
> +                       gpios = <&spisel 0 GPIO_ACTIVE_LOW>;
>
> still works.
>
> Signed-off-by: Rasmus Villemoes <[hidden email]>
Applied to u-boot/master, thanks!

--
Tom

signature.asc (673 bytes) Download Attachment