spi: Support high CS when using descriptors

Message ID 20190110121503.25436-1-linus.walleij@linaro.org
State Superseded
Headers show
Series
  • spi: Support high CS when using descriptors
Related show

Commit Message

Linus Walleij Jan. 10, 2019, 12:15 p.m.
All controllers using GPIO descriptors can by definition
support high CS connections, so just enforce this when
registering an SPI controller.

This fixes a regression where controllers were missing
SPI_CS_HIGH, the drivers would fail like this:

spi spi0.0: setup: unsupported mode bits 4
cdns-spi fd0b0000.spi: can't setup spi0.0, status -22

This is because as using descriptors moves the CS inversion
logic over to gpiolib, all such controllers are registered
with CS active high.

Cc: Janek Kotas <jank@cadence.com>
Reported-by: Janek Kotas <jank@cadence.com>
Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Janek, it'd be great if you could test this patch and
confirm that it makes the DW driver tick again.
---
 drivers/spi/spi.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.19.2

Comments

Janek Kotas Jan. 10, 2019, 12:46 p.m. | #1
> On 10 Jan 2019, at 13:15, Linus Walleij <linus.walleij@linaro.org> wrote:

> 

> Janek, it'd be great if you could test this patch and

> confirm that it makes the DW driver tick again.

> 


Hi Linus,

The setup function works now and the Cadence driver is initialized:
at25 spi0.0: 1 KByte at25 eeprom, pagesize 16

However, the chip select signal is inverted.
The moment the SPI HW is initialized it goes low.
It is driven high during SPI transmissions.

That means EEPROM accesses, I use for testing, don’t work correctly:
Testing SPI /sys/bus/spi/devices/spi0.0/eeprom
TESTING: spi: eeprom1 write PASSED
TESTING: spi: eeprom1 read FAILED

Regards,
Janek
Linus Walleij Jan. 10, 2019, 2:25 p.m. | #2
On Thu, Jan 10, 2019 at 1:46 PM Janek Kotas <jank@cadence.com> wrote:
> > On 10 Jan 2019, at 13:15, Linus Walleij <linus.walleij@linaro.org> wrote:

> >

> > Janek, it'd be great if you could test this patch and

> > confirm that it makes the DW driver tick again.

> >

>

> Hi Linus,

>

> The setup function works now and the Cadence driver is initialized:

> at25 spi0.0: 1 KByte at25 eeprom, pagesize 16

>

> However, the chip select signal is inverted.

> The moment the SPI HW is initialized it goes low.

> It is driven high during SPI transmissions.


Ouch! Which device tree file is this?

My intention was that of_gpio_flags_quirks() in
drivers/gpio/gpiolib-of.c would activate and you would
get a print like this in dmesg:

of_gpio_flags_quirks: enforce active low on chipselect handle

I take it that this is not working, so I need to try to reproduce.

But I'm on to it!

Yours,
Linus Walleij
Janek Kotas Jan. 10, 2019, 3:51 p.m. | #3
Hi Linus,

> On 10 Jan 2019, at 15:25, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Jan 10, 2019 at 1:46 PM Janek Kotas <jank@cadence.com> wrote:

>>> On 10 Jan 2019, at 13:15, Linus Walleij <linus.walleij@linaro.org> wrote:

>>> 

>>> Janek, it'd be great if you could test this patch and

>>> confirm that it makes the DW driver tick again.

>>> 

>> 

>> Hi Linus,

>> 

>> The setup function works now and the Cadence driver is initialized:

>> at25 spi0.0: 1 KByte at25 eeprom, pagesize 16

>> 

>> However, the chip select signal is inverted.

>> The moment the SPI HW is initialized it goes low.

>> It is driven high during SPI transmissions.

> 

> Ouch! Which device tree file is this?


I’m using a custom one for out test system.
Here’s the SPI part:

spi0: spi@fd0b0000 {
    compatible = "cdns,spi-r1p6";
    reg = <0x0 0xfd0b0000 0x0 0x1000>;
    clock-names = "ref_clk", "pclk";
    clocks = <&sysclock>, <&sysclock>;
    interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
    interrupt-parent = <&gic>;
    num-cs = <4>;
    is-decoded-cs = <0>;
    #address-cells = <1>;
    #size-cells = <0>;

    eeprom@0 {
        compatible = "at,at25";
        reg = <0>;
        spi-max-frequency = <10000000>;
        /* spi-cpha; */
        /* spi-cpol; */
        pagesize = <16>;
        size = <1024>;
        address-width = <16>;
    };
};



> My intention was that of_gpio_flags_quirks() in

> drivers/gpio/gpiolib-of.c would activate and you would

> get a print like this in dmesg:

> 

> of_gpio_flags_quirks: enforce active low on chipselect handle


I haven’t seen that in my logs.

> But I'm on to it!

> 

> Yours,

> Linus Walleij


Regards,
Janek
Linus Walleij Jan. 10, 2019, 7:38 p.m. | #4
On Thu, Jan 10, 2019 at 4:52 PM Janek Kotas <jank@cadence.com> wrote:

> >> However, the chip select signal is inverted.

> >> The moment the SPI HW is initialized it goes low.

> >> It is driven high during SPI transmissions.

> >

> > Ouch! Which device tree file is this?

>

> I’m using a custom one for out test system.

> Here’s the SPI part:

>

> spi0: spi@fd0b0000 {

>     compatible = "cdns,spi-r1p6";

>     reg = <0x0 0xfd0b0000 0x0 0x1000>;

>     clock-names = "ref_clk", "pclk";

>     clocks = <&sysclock>, <&sysclock>;

>     interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;

>     interrupt-parent = <&gic>;

>     num-cs = <4>;

>     is-decoded-cs = <0>;

>     #address-cells = <1>;

>     #size-cells = <0>;

>

>     eeprom@0 {

>         compatible = "at,at25";

>         reg = <0>;

>         spi-max-frequency = <10000000>;

>         /* spi-cpha; */

>         /* spi-cpol; */

>         pagesize = <16>;

>         size = <1024>;

>         address-width = <16>;

>     };

> };


Ah! You're using native chip selects!

And now I enforce CS as high, and that will not just
affect the GPIO CS but also the native CS.
How sloppy of me.

I will send a separate patch for that that you can
test toghether with the first patch (which I think
is fine in itself).

> > My intention was that of_gpio_flags_quirks() in

> > drivers/gpio/gpiolib-of.c would activate and you would

> > get a print like this in dmesg:

> >

> > of_gpio_flags_quirks: enforce active low on chipselect handle

>

> I haven’t seen that in my logs.


Not strange as you're not using GPIOs.

Yours,
Linus Walleij

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 06b9139664a3..31696f2fc8d5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2336,6 +2336,11 @@  int spi_register_controller(struct spi_controller *ctlr)
 			status = spi_get_gpio_descs(ctlr);
 			if (status)
 				return status;
+			/*
+			 * A controller using GPIO descriptors always
+			 * supports SPI_CS_HIGH if need be.
+			 */
+			ctlr->mode_bits |= SPI_CS_HIGH;
 		} else {
 			/* Legacy code path for GPIOs from DT */
 			status = of_spi_register_master(ctlr);