[PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Jay Fang]

This series introduces a generic implementation to solve the conflict
between the 'cs-gpios' flags and the optional SPI slaves flags. So we
don't need to add two similar quirks separately for DT and ACPI.

Jay Fang (2):
  spi: Correct CS GPIOs polarity when using GPIO descriptors
  Revert "gpio: of: Handle SPI chipselect legacy bindings"

 drivers/gpio/gpiolib-of.c | 51 ++---------------------------------------------
 drivers/spi/spi.c         | 22 ++++++++++++++++++--
 2 files changed, 22 insertions(+), 51 deletions(-)

-- 
2.7.4

[PATCH 1/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Jay Fang]

When using GPIO descriptors for CS GPIOs we need to inspect the child node
to determine if the 'cs-gpios' active flags should have inverted semantics.

Previously, commit 6953c57ab172 ("gpio: of: Handle SPI chipselect legacy
bindings") introduced a special quirk to solve the conflict between the
'cs-gpios' flags and the optional SPI slaves flags. The principle is that
the SPI slave flag will take precedence when we get a conflict. For more
details, see Documentation/devicetree/bindings/spi/spi-controller.yaml.

If following the previous commit, we need to add a new quirk to deal with
this in ACPI. Instead, this patch introduces a generic implementation to
solve this conflict by SPI core.

The 'cs-gpios' active rules are as follows:

1) DT (SPI device have active low chip selects by default):

    device node     | cs-gpio       | CS pin state active
    ================+===============+=====================
    spi-cs-high     | -             | H
    -               | -             | L
    spi-cs-high     | ACTIVE_HIGH   | H
    -               | ACTIVE_HIGH   | L
    spi-cs-high     | ACTIVE_LOW    | H
    -               | ACTIVE_LOW    | L

2) ACPI:

    device node     | cs-gpio       | CS pin state active
    ================+===============+=====================
    PolarityLow     | ACTIVE_LOW    | L
    PolarityLow     | ACTIVE_HIGH   | L
    PolarityHigh    | ACTIVE_LOW    | H
    PolarityHigh    | ACTIVE_HIGH   | H

Signed-off-by: Jay Fang <f.fangjian@huawei.com>
Signed-off-by: Zihao Tang <tangzihao1@hisilicon.com>
---
 drivers/spi/spi.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b08efe8..2c0324d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -33,6 +33,7 @@
 #include <linux/highmem.h>
 #include <linux/idr.h>
 #include <linux/platform_data/x86/apple.h>
+#include <../gpio/gpiolib.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/spi.h>
@@ -604,9 +605,26 @@ int spi_add_device(struct spi_device *spi)
 	}
 
 	/* Descriptors take precedence */
-	if (ctlr->cs_gpiods)
+	if (ctlr->cs_gpiods) {
 		spi->cs_gpiod = ctlr->cs_gpiods[spi->chip_select];
-	else if (ctlr->cs_gpios)
+
+		/* We need to handle the active flag conflict between
+		 * the cs-gpio and the SPI slave. The SPI slave active
+		 * flag will determine the active state of cs-gpio.
+		 */
+		if (spi->mode & SPI_CS_HIGH) {
+			if (test_bit(FLAG_ACTIVE_LOW, &spi->cs_gpiod->flags)) {
+				dev_warn(&spi->dev, "GPIO handle specifies "
+					"active low - ignored\n");
+				clear_bit(FLAG_ACTIVE_LOW, &spi->cs_gpiod->flags);
+			}
+		} else {
+			if (!test_bit(FLAG_ACTIVE_LOW, &spi->cs_gpiod->flags))
+				dev_warn(&spi->dev, "enforce active low on "
+					"chipselect handle\n");
+			set_bit(FLAG_ACTIVE_LOW, &spi->cs_gpiod->flags);
+		}
+	} else if (ctlr->cs_gpios)
 		spi->cs_gpio = ctlr->cs_gpios[spi->chip_select];
 
 	/* Drivers may modify this initial i/o setup, but will
-- 
2.7.4

[PATCH 2/2] Revert "gpio: of: Handle SPI chipselect legacy bindings"

[Jay Fang]

This reverts commit 6953c57ab1721ce57914fc5741d0ce0568756bb0.

The latter introduced a generic implementation to resolve it by SPI core
and is not needed anymore after the previous commit.

Signed-off-by: Jay Fang <f.fangjian@huawei.com>
---
 drivers/gpio/gpiolib-of.c | 51 ++---------------------------------------------
 1 file changed, 2 insertions(+), 49 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index baf0153..b8337f8b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -132,8 +132,7 @@ bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
 
 static void of_gpio_flags_quirks(struct device_node *np,
 				 const char *propname,
-				 enum of_gpio_flags *flags,
-				 int index)
+				 enum of_gpio_flags *flags)
 {
 	/*
 	 * Some GPIO fixed regulator quirks.
@@ -172,52 +171,6 @@ static void of_gpio_flags_quirks(struct device_node *np,
 			of_node_full_name(np));
 	}
 
-	/*
-	 * Legacy handling of SPI active high chip select. If we have a
-	 * property named "cs-gpios" we need to inspect the child node
-	 * to determine if the flags should have inverted semantics.
-	 */
-	if (IS_ENABLED(CONFIG_SPI_MASTER) && !strcmp(propname, "cs-gpios") &&
-	    of_property_read_bool(np, "cs-gpios")) {
-		struct device_node *child;
-		u32 cs;
-		int ret;
-
-		for_each_child_of_node(np, child) {
-			ret = of_property_read_u32(child, "reg", &cs);
-			if (ret)
-				continue;
-			if (cs == index) {
-				/*
-				 * SPI children have active low chip selects
-				 * by default. This can be specified negatively
-				 * by just omitting "spi-cs-high" in the
-				 * device node, or actively by tagging on
-				 * GPIO_ACTIVE_LOW as flag in the device
-				 * tree. If the line is simultaneously
-				 * tagged as active low in the device tree
-				 * and has the "spi-cs-high" set, we get a
-				 * conflict and the "spi-cs-high" flag will
-				 * take precedence.
-				 */
-				if (of_property_read_bool(child, "spi-cs-high")) {
-					if (*flags & OF_GPIO_ACTIVE_LOW) {
-						pr_warn("%s GPIO handle specifies active low - ignored\n",
-							of_node_full_name(child));
-						*flags &= ~OF_GPIO_ACTIVE_LOW;
-					}
-				} else {
-					if (!(*flags & OF_GPIO_ACTIVE_LOW))
-						pr_info("%s enforce active low on chipselect handle\n",
-							of_node_full_name(child));
-					*flags |= OF_GPIO_ACTIVE_LOW;
-				}
-				of_node_put(child);
-				break;
-			}
-		}
-	}
-
 	/* Legacy handling of stmmac's active-low PHY reset line */
 	if (IS_ENABLED(CONFIG_STMMAC_ETH) &&
 	    !strcmp(propname, "snps,reset-gpio") &&
@@ -263,7 +216,7 @@ static struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		goto out;
 
 	if (flags)
-		of_gpio_flags_quirks(np, propname, flags, index);
+		of_gpio_flags_quirks(np, propname, flags);
 
 	pr_debug("%s: parsed '%s' property of node '%pOF[%d]' - status (%d)\n",
 		 __func__, propname, np, index,
-- 
2.7.4

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Jay Fang]

On 2021/5/6 16:14, Andy Shevchenko wrote:
> 
> 
> On Thursday, May 6, 2021, Andy Shevchenko <andy.shevchenko@gmail.com <mailto:andy.shevchenko@gmail.com>> wrote:
> 
> 
> 
>     On Thursday, May 6, 2021, Jay Fang <f.fangjian@huawei.com <mailto:f.fangjian@huawei.com>> wrote:
> 
>         This series introduces a generic implementation to solve the conflict
>         between the 'cs-gpios' flags and the optional SPI slaves flags. So we
>         don't need to add two similar quirks separately for DT and ACPI.
> 
> 
>     NAK. There is a patch against documentation that clarifies polarity of GPIO for ACPI. I have a fix for that to use generic implementation of  CS GPIOs .

Thanks. Has the Fix patch been merged ? Commit Id ?

> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712 <https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712>
> 
> 
> If your ACPI table uses GPIO CS with polarity low (assumes _DSD() is involved), this is a bug. Fix firmware or do a quirk specific for your platform.
> 
>  
> 
>      
> 
>         Jay Fang (2):
>           spi: Correct CS GPIOs polarity when using GPIO descriptors
>           Revert "gpio: of: Handle SPI chipselect legacy bindings"
> 
>          drivers/gpio/gpiolib-of.c | 51 ++---------------------------------------------
>          drivers/spi/spi.c         | 22 ++++++++++++++++++--
>          2 files changed, 22 insertions(+), 51 deletions(-)
> 
>         -- 
>         2.7.4
> 
> 
> 
>     -- 
>     With Best Regards,
>     Andy Shevchenko
> 
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Andy Shevchenko]

On Thu, May 6, 2021 at 12:11 PM Jay Fang <f.fangjian@huawei.com> wrote:
> On 2021/5/6 16:14, Andy Shevchenko wrote:
> > On Thursday, May 6, 2021, Andy Shevchenko <andy.shevchenko@gmail.com <mailto:andy.shevchenko@gmail.com>> wrote:
> >     On Thursday, May 6, 2021, Jay Fang <f.fangjian@huawei.com <mailto:f.fangjian@huawei.com>> wrote:
> >
> >         This series introduces a generic implementation to solve the conflict
> >         between the 'cs-gpios' flags and the optional SPI slaves flags. So we
> >         don't need to add two similar quirks separately for DT and ACPI.

> >     NAK. There is a patch against documentation that clarifies polarity of GPIO for ACPI. I have a fix for that to use generic implementation of  CS GPIOs .
>
> Thanks. Has the Fix patch been merged ? Commit Id ?

Not yet. I'm planning to send it next week (after v5.13-rc1 is out) as a fix.
For your convenience the whole story is available in my publick branch:

https://gitlab.com/andy-shev/next/-/tree/topic/spi/reload

and yes, I have tested it on real hardware.

> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712 <https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712>
> >
> >
> > If your ACPI table uses GPIO CS with polarity low (assumes _DSD() is involved), this is a bug. Fix firmware or do a quirk specific for your platform.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Andy Shevchenko]

On Thu, May 6, 2021 at 12:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, May 6, 2021 at 12:11 PM Jay Fang <f.fangjian@huawei.com> wrote:
> > On 2021/5/6 16:14, Andy Shevchenko wrote:
> > > On Thursday, May 6, 2021, Andy Shevchenko <andy.shevchenko@gmail.com <mailto:andy.shevchenko@gmail.com>> wrote:
> > >     On Thursday, May 6, 2021, Jay Fang <f.fangjian@huawei.com <mailto:f.fangjian@huawei.com>> wrote:
> > >
> > >         This series introduces a generic implementation to solve the conflict
> > >         between the 'cs-gpios' flags and the optional SPI slaves flags. So we
> > >         don't need to add two similar quirks separately for DT and ACPI.
>
> > >     NAK. There is a patch against documentation that clarifies polarity of GPIO for ACPI. I have a fix for that to use generic implementation of  CS GPIOs .
> >
> > Thanks. Has the Fix patch been merged ? Commit Id ?

It's here

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

and now I remember that I didn't dare to make it as a fix due to:
 - recent (non-fix) dependencies
 - no existing driver uses it for ACPI (at least nothing has been reported)

> Not yet. I'm planning to send it next week (after v5.13-rc1 is out) as a fix.
> For your convenience the whole story is available in my publick branch:
>
> https://gitlab.com/andy-shev/next/-/tree/topic/spi/reload
>
> and yes, I have tested it on real hardware.
>
> > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712 <https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=ec3576eac11d66a388b6cba6a7cfb3b45039a712>
> > >
> > >
> > > If your ACPI table uses GPIO CS with polarity low (assumes _DSD() is involved), this is a bug. Fix firmware or do a quirk specific for your platform.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Linus Walleij]

On Thu, May 6, 2021 at 11:45 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> > Thanks. Has the Fix patch been merged ? Commit Id ?
>
> Not yet. I'm planning to send it next week (after v5.13-rc1 is out) as a fix.
> For your convenience the whole story is available in my publick branch:
>
> https://gitlab.com/andy-shev/next/-/tree/topic/spi/reload
>
> and yes, I have tested it on real hardware.

Oh what a nightmare you got into there.

Curious that ACPI has SPI CS as always active high,
but that just underscore that we *really* need to
abstract this out to avoid really complex specialcasing.

Interesting that the PXA2xx XScale lives in Merrifield,
I didn't know. Reminds med of how the Samsung
hardware is alive and well in the recent Apple M1 laptops.

Yours,
Linus Walleij

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

On Thu, May 06, 2021 at 01:06:23PM +0200, Linus Walleij wrote:

> Interesting that the PXA2xx XScale lives in Merrifield,
> I didn't know. Reminds med of how the Samsung
> hardware is alive and well in the recent Apple M1 laptops.

There's a lot of PXA IP blocks in modern Intel SoCs - half the audio
hardware is PXA derived (though mostly connected via DSP rather than
directly to the CPU).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Thu, May 06, 2021 at 02:24:17PM +0300, Andy Shevchenko wrote:
> On Thursday, May 6, 2021, Linus Walleij <linus.walleij@linaro.org> wrote:

> > Curious that ACPI has SPI CS as always active high,

> Here I didn’t get what exactly you are pointing out. GPIOs are active high,
> due to historical reasons. Otherwise SPI CS depends on the actual hardware
> and may be (most of the cases?) active low.

SPI chip selects are almost always active low - the signal is often
written nCS or /CS for that reason.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] spi: Correct CS GPIOs polarity when using GPIO descriptors

[Andy Shevchenko]

On Thu, May 6, 2021 at 2:35 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 06, 2021 at 02:24:17PM +0300, Andy Shevchenko wrote:
> > On Thursday, May 6, 2021, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > > Curious that ACPI has SPI CS as always active high,
>
> > Here I didn’t get what exactly you are pointing out. GPIOs are active high,
> > due to historical reasons. Otherwise SPI CS depends on the actual hardware
> > and may be (most of the cases?) active low.
>
> SPI chip selects are almost always active low - the signal is often
> written nCS or /CS for that reason.

Exactly, and it's not altered with ACPI. That's the whole point of
keeping it active high for _GPIO_ CS case.

-- 
With Best Regards,
Andy Shevchenko