[PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Xin Hao]

When i was testing the TPM2 device, I found that the driver
always failed to register which used SPI bus and GPIO as CS
signal, i found that the reason for the error was that CS could
not be set correctly, so there fixed it.

Fixes: 766c6b63aa044e ("spi: fix client driver breakages when using
GPIO descriptors")
Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 drivers/spi/spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b08efe88ccd6..d4342909c1c8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -826,7 +826,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
 			if (spi->cs_gpiod)
 				/* polarity handled by gpiolib */
 				gpiod_set_value_cansleep(spi->cs_gpiod,
-							 enable1);
+							 !enable);
 			else
 				/*
 				 * invert the enable line, as active low is
-- 
2.31.0

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Linus Walleij]

On Thu, May 6, 2021 at 1:45 PM Xin Hao <bier@b-x3vxmd6m-2058.local> wrote:

> From: Xin Hao <xhao@linux.alibaba.com>
>
> When i was testing the TPM2 device, I found that the driver
> always failed to register which used SPI bus and GPIO as CS
> signal, i found that the reason for the error was that CS could
> not be set correctly, so there fixed it.
>
> Fixes: 766c6b63aa044e ("spi: fix client driver breakages when using
> GPIO descriptors")
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>

(...)
>                                 /* polarity handled by gpiolib */
>                                 gpiod_set_value_cansleep(spi->cs_gpiod,
> -                                                        enable1);
> +                                                        !enable);

We have been over this code a lot of times, can you
help us to investigate the root cause here and check
how the interrupts are provided on this platform.

TPM2 makes me think that this is an Intel platform
and maybe ACPI of some kind so you need to run
it by Andy, who is working on some related fixes.

Yours,
Linus Walleij

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Andy Shevchenko]

On Thu, May 06, 2021 at 02:30:01PM +0200, Linus Walleij wrote:
> On Thu, May 6, 2021 at 1:45 PM Xin Hao <bier@b-x3vxmd6m-2058.local> wrote:
> > From: Xin Hao <xhao@linux.alibaba.com>
> >
> > When i was testing the TPM2 device, I found that the driver
> > always failed to register which used SPI bus and GPIO as CS
> > signal, i found that the reason for the error was that CS could
> > not be set correctly, so there fixed it.
> >
> > Fixes: 766c6b63aa044e ("spi: fix client driver breakages when using
> > GPIO descriptors")
> > Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> 
> (...)
> >                                 /* polarity handled by gpiolib */
> >                                 gpiod_set_value_cansleep(spi->cs_gpiod,
> > -                                                        enable1);
> > +                                                        !enable);
> 
> We have been over this code a lot of times, can you
> help us to investigate the root cause here and check
> how the interrupts are provided on this platform.
> 
> TPM2 makes me think that this is an Intel platform
> and maybe ACPI of some kind so you need to run
> it by Andy, who is working on some related fixes.

The above is exactly what my quirk [1] for ACPI does in case the controller
gets GPIOs from the ACPI.

[1]: https://gitlab.com/andy-shev/next/-/commit/5ccbdbb4787d871722f361d77c5f3cb806811c48

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Xin Hao]

Hi, Andy:

     Yes, i got gpio from ACPI, i have a question why not we can't keep 
same with the gpio get from dt, i think it is a bug,

it should be fixed.

     BTW, my platform is arm64,not intel.

-- 
Best Regards!
Xin Hao

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Linus Walleij]

On Fri, May 7, 2021 at 9:11 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> It can’t be done due to differences of the expectations from them.
> Your patch breaks OF as far as I understand. Linus?

It looks to me like it would break OF.

commit 766c6b63aa044e84b045803b40b14754d69a2a1d
"spi: fix client driver breakages when using GPIO descriptors"
passes enable1 to gpiod_set_value_cansleep() expecting
gpiolib to handle polarity.

If this should be changed, Sven van Asbroeck really needs
to look at it first.

But I think Andy's approach is the best.

Yours,
Linus Walleij

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[xhao]

在 2021/5/8 上午5:33, Linus Walleij 写道:
> On Fri, May 7, 2021 at 9:11 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
>> It can’t be done due to differences of the expectations from them.
>> Your patch breaks OF as far as I understand. Linus?
> It looks to me like it would break OF.
>
> commit 766c6b63aa044e84b045803b40b14754d69a2a1d
> "spi: fix client driver breakages when using GPIO descriptors"
> passes enable1 to gpiod_set_value_cansleep() expecting
> gpiolib to handle polarity.
>
> If this should be changed, Sven van Asbroeck really needs
> to look at it first.
>
> But I think Andy's approach is the best.

Ok, agree, i check the relative patches, They do respond to different 
situations，

Andy，When do you send out your patch to fix this problem？

>
> Yours,
> Linus Walleij

-- 
Best Regards!
Xin Hao

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

Mike, Linus, Andy, XinHao,

On Fri, May 7, 2021 at 5:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> But I think Andy's approach is the best.

I too like Andy's approach. It would fix XinHao's use case (acpi) and
not break OF.

But, we have to be careful not to put work-around on top of
work-around, and complicate the code too much. Sometimes when logic
gets hard to follow, there's an opportunity to refactor and simplify.
Perhaps if we put our heads together here, we can find that
opportunity? Let's try?

For reasons explained below, the gpiod OF code moves the SPI
chip-select inverting logic from the SPI mode flags to the gpiods. If
I understand XinHao/Andy's problem correctly, this breaks ACPI
chip-selects, because the ACPI gpio code has no way to know that it's
part of a SPI bus, and apply quirks. Does that sound right so far?

If we were able to store the polarity in the SPI mode flag, then we
could refactor very elegantly:

1. Simplify Linus's OF gpio quirks, so:
- they print warnings in case of polarity conflicts
- but no longer change the gpiod polarity (i.e. they just keep what
was specified in OF)

2. Drive the gpiod chip select using the raw value, ignoring the
built-in polarity, which treats it the same as a gpio_xxx. Nice!

in driver/spi/spi.c:
+       /*
+        * invert the enable line, as active low is
+        * default for SPI.
+        */
        if (spi->cs_gpiod)
-               /* polarity handled by gpiolib */
-               gpiod_set_value_cansleep(spi->cs_gpiod,
-                                        enable1);
+               gpiod_set_raw_value_cansleep(spi->cs_gpiod, !enable);
        else
-               /*
-                * invert the enable line, as active low is
-                * default for SPI.
-                */
                gpio_set_value_cansleep(spi->cs_gpio, !enable);

Andy/XinHao, is it correct that this will fix the ACPI case?
Example: enable ACPI CS when SPI_CS_HIGH:
        enable = true
        mode & SPI_CS_HIGH => invert enable => false
        gpiod_set_raw_value_cansleep(!enable => true)
        ACPI gpiod: always active high
        chip select goes high.

Now we get to the tricky bit. Storing the polarity in the SPI mode
breaks a lot of OF spi client drivers. Why? Hardware designers love to
put chip-selects behind inverting gates. This is quite common in case
a voltage domain shift is needed - a single transistor will work, but
is inverting. So depending on the hardware topology (OF), sometimes
client device X has SPI_CS_HIGH set, sometimes it doesn't.

That would all be fine, but... a common pattern in SPI client drivers is this:

drivers/net/phy/spi_ks8995.c:
        spi->mode = SPI_MODE_0;
        spi->bits_per_word = 8;
        err = spi_setup(spi);

In its zeal to specify the correct mode, the driver "plows" right over
the SPI_CS_HIGH mode flag. That'll break stuff.

If there was some way to prevent this from happening, we could make
our code a lot simpler. So I'd like to reach out to Mike Brown to hear
his opinion.

In case of a SPI bus described by OF or ACPI, the mode flags have
already been filled out, so there should be no need for the
initialization in the driver? Could we perhaps replace the pattern
with the following code?

        spi->mode = spi->mode ? : SPI_MODE_0;
        spi->bits_per_word = 8;
        err = spi_setup(spi);

I am not sure in which cases the spi->mode has not been filled out
yet. I live mostly in the OF world, so I'm a bit myopic here.

Even if Mike Brown agrees to change the pattern, it still means lots
of changes to spi client drivers, all over the place. So in terms of
stability, Andy's solution might be preferable.

Looking forward to hearing your opinions,
Sven

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

On Fri, May 7, 2021 at 10:36 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Mike, Linus, Andy, XinHao,

Oops, s/Mike Brown/Mark Brown/. It was getting a bit late :)

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

Hi Andy,

On Sat, May 8, 2021 at 3:38 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>>
>> 2. Drive the gpiod chip select using the raw value, ignoring the
>> built-in polarity, which treats it the same as a gpio_xxx. Nice!
>>
>
> Looks nice (if for now don’t take into account the zillion of drivers to be changed), but IIRC last time discussions about this piece of code, the problem also in DT itself, you may not break boards with already cooked DTs. If you are sure about this, go ahead!

Yes, you're absolutely right, the zillion of drivers to be changed is
a serious problem. I'm definitely not trying to sweep that under the
carpet...

The rule table seems to indicate that the gpio's second devicetree
flag is ignored when it's used as a SPI gpio. So changing where the
polarity is stored, should not break DT? It may have repercussions
elsewhere though, not sure.

(I hope the formatting will come out ok here. If not, use the link below)
      device node     | cs-gpio       | CS pin state active | Note
      ================+===============+=====================+=====
      spi-cs-high     | -             | H                   |
      -               | -             | L                   |
      spi-cs-high     | ACTIVE_HIGH   | H                   |
      -               | ACTIVE_HIGH   | L                   | 1
      spi-cs-high     | ACTIVE_LOW    | H                   | 2
      -               | ACTIVE_LOW    | L                   |


https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.12#n54

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

On Sat, May 8, 2021 at 7:46 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>It may have repercussions
> elsewhere though, not sure.

We can try to identify the fixes generated to deal with the changes
introduced in 766c6b63aa04, and revert them:
$ git log | grep 766c6b63aa04
$ git log | grep SPI_CS_HIGH
$ git log -p | grep SPI_CS_HIGH

This brings up:
7a2da5d7960a6 ("spi: fsl: Fix driver breakage when SPI_CS_HIGH is not
set in spi->mode")

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Andy Shevchenko]

On Sat, May 08, 2021 at 07:46:13AM -0400, Sven Van Asbroeck wrote:
> On Sat, May 8, 2021 at 3:38 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >> 2. Drive the gpiod chip select using the raw value, ignoring the
> >> built-in polarity, which treats it the same as a gpio_xxx. Nice!
> >>
> >
> > Looks nice (if for now don’t take into account the zillion of drivers to be changed), but IIRC last time discussions about this piece of code, the problem also in DT itself, you may not break boards with already cooked DTs. If you are sure about this, go ahead!
> 
> Yes, you're absolutely right, the zillion of drivers to be changed is
> a serious problem. I'm definitely not trying to sweep that under the
> carpet...
> 
> The rule table seems to indicate that the gpio's second devicetree
> flag is ignored when it's used as a SPI gpio. So changing where the
> polarity is stored, should not break DT? It may have repercussions
> elsewhere though, not sure.
> 
> (I hope the formatting will come out ok here. If not, use the link below)
>       device node     | cs-gpio       | CS pin state active | Note
>       ================+===============+=====================+=====
>       spi-cs-high     | -             | H                   |
>       -               | -             | L                   |
>       spi-cs-high     | ACTIVE_HIGH   | H                   |
>       -               | ACTIVE_HIGH   | L                   | 1
>       spi-cs-high     | ACTIVE_LOW    | H                   | 2
>       -               | ACTIVE_LOW    | L                   |
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.12#n54

This table is incompatible with ACPI. So we can't unify them until each of them
will play by the same rules. Can't say it won't happen, but it's far from that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

Hi Andy,

On Mon, May 10, 2021 at 7:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> >
> >       device node     | cs-gpio       | CS pin state active | Note
> >       ================+===============+=====================+=====
> >       spi-cs-high     | -             | H                   |
> >       -               | -             | L                   |
> >       spi-cs-high     | ACTIVE_HIGH   | H                   |
> >       -               | ACTIVE_HIGH   | L                   | 1
> >       spi-cs-high     | ACTIVE_LOW    | H                   | 2
> >       -               | ACTIVE_LOW    | L                   |
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.12#n54
>
> This table is incompatible with ACPI. So we can't unify them until each of them
> will play by the same rules. Can't say it won't happen, but it's far from that.

Linus Wallej has added some gpiod OF quirks that checks if the gpio is
used as a chip-select, and if so forces the gpiod polarity to
implement the inversion:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-of.c?h=v5.13-rc1#n175

If as suggested above, we disable that OF quirk and use the polarity
flag from the SPI mode flags instead, and ignore the built-in gpiod
polarity, the OF table boils down to:

device node    |  CS pin active state
=====================================
-              |  L
spi-cs-high    |  H

which is exactly the same as the ACPI case:
SPI mode flag  |  CS pin active state
=====================================
-              | L
SPI_CS_HIGH    | H

Your github commit says:
> in ACPI case the default polarity
> is active high and can't be altered

So if ACPI gpiods are always active-high then unification can happen
here, correct?

But if I have misunderstood the ACPI case, and ACPI gpiod chip-selects
can have any polarity, then I agree that unification cannot happen.
Like I said earlier, I live mostly in OF-land, so my apologies if I
have not fully grasped the ACPI case.

Sven

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Andy Shevchenko]

On Mon, May 10, 2021 at 4:56 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> On Mon, May 10, 2021 at 7:36 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > >
> > >       device node     | cs-gpio       | CS pin state active | Note
> > >       ================+===============+=====================+=====
> > >       spi-cs-high     | -             | H                   |
> > >       -               | -             | L                   |
> > >       spi-cs-high     | ACTIVE_HIGH   | H                   |
> > >       -               | ACTIVE_HIGH   | L                   | 1
> > >       spi-cs-high     | ACTIVE_LOW    | H                   | 2
> > >       -               | ACTIVE_LOW    | L                   |
> > >
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.12#n54
> >
> > This table is incompatible with ACPI. So we can't unify them until each of them
> > will play by the same rules. Can't say it won't happen, but it's far from that.
>
> Linus Wallej has added some gpiod OF quirks that checks if the gpio is
> used as a chip-select, and if so forces the gpiod polarity to
> implement the inversion:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-of.c?h=v5.13-rc1#n175
>
> If as suggested above, we disable that OF quirk and use the polarity
> flag from the SPI mode flags instead, and ignore the built-in gpiod
> polarity, the OF table boils down to:
>
> device node    |  CS pin active state
> =====================================
> -              |  L
> spi-cs-high    |  H
>
> which is exactly the same as the ACPI case:
> SPI mode flag  |  CS pin active state
> =====================================
> -              | L
> SPI_CS_HIGH    | H
>
> Your github commit says:
> > in ACPI case the default polarity
> > is active high and can't be altered

Right. This is the correct statement.

> So if ACPI gpiods are always active-high then unification can happen
> here, correct?

Probably. I really won't dive into OF rabbit hole, if you think it
will work, go for it!

For now I guess my patch is necessary to have. I don't think we may
delay its distribution while developing a better solution, do you
agree on this?

> But if I have misunderstood the ACPI case, and ACPI gpiod chip-selects
> can have any polarity, then I agree that unification cannot happen.
> Like I said earlier, I live mostly in OF-land, so my apologies if I
> have not fully grasped the ACPI case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] spi: fix client driver can't register success when use GPIO as CS

[Sven Van Asbroeck]

Hi Andy,

On Mon, May 10, 2021 at 10:02 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> For now I guess my patch is necessary to have. I don't think we may
> delay its distribution while developing a better solution, do you
> agree on this?

Agreed 100%. Fixing your/XinHao's immediate issue is the important thing here.

From what I can see, your patch won't break OF, so I'm good with it. I
can review / test the rebased patch on an OF system if you like. All
subject to Mark Brown's agreement, of course.