| Message ID | 424d37007b7e298cf9bca5ac38d45a723e0976ee.1620301297.git.xhao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] spi: fix client driver can't register success when use GPIO as CS | expand |
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
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
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.
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
在 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
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
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 :)
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
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")
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.
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
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.
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.
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
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(-)