| Message ID | 20190110121503.25436-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | spi: Support high CS when using descriptors | expand |
> 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
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
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
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
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);
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(+)