spi: add driver for the SiFive SPI controller
Subsystems (4)
RFC PATCH 1/1 spi: add driver for the SiFive SPI controller
From: Palmer Dabbelt <[email protected]>
Add driver for the SiFive SPI controller
on the HiFive Unleashed board.
--- .../devicetree/bindings/spi/spi-sifive.txt | 29 ++ drivers/spi/Kconfig | 6 + drivers/spi/Makefile | 1 + drivers/spi/spi-sifive.c | 442 ++++++++++++++++++ 4 files changed, 478 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-sifive.txt create mode 100644 drivers/spi/spi-sifive.c
Hi all,
I know the discussions about the sifive devicetree compatible strings haven't come to a conclusion, so I'm sending this as an RFC to get some feedback on the rest of the code.
Compared to the original[1] I've done the following:
- Update register names and bit fields to the ones in the FU540-C000 documentation.
- Change the optional devicetree property from "sifive,buffer-size" to "sifive,fifo-depth". The string "fifo-depth" seems to have more hits in the existing devicetree bindings and is IMHO a little more descriptive.
- Change the optional devicetree property from "sifive,bits-per-word" to "sifive,max-bits-per-word". For a long time I wondered why the SPI word size was a property on the controller and not the device. This change makes the meaning a little more clear.
- Be honest about only supporting 8bit SPI words in the driver. Without SPI_LSB_FIRST and bits_per_word < 8 we need some code to shift the bits in each byte which is not yet there.
- Program the csdef, csid and sckmode registers from prepare_message rather than transfer_one. This way we only do it once pr. message rather than every transfer.
- Drop the irq field from driver data. With devm_request_irq we don't need to remember it after requesting the irq.
- Drop hz = t->speed_hz ? t->speed_hz : device->max_speed_hz; The SPI framework handles this for us, so we can just always use t->speed_hz.
- Fix most checkpatch warnings.
[1]: https://github.com/riscv/riscv-linux/commit/801805694740ad0895e21d10b8f124d138beefbb
/Emil
Documentation/devicetree/bindings/spi/spi-sifive.txt new file +29 -0
@@ -0,0 +1,29 @@ +SiFive SPI controller Device Tree Bindings +------------------------------------------ + +Required properties: +- compatible : Should be "sifive,spi0" +- reg : Physical base address and size of SPI registers map + A second (optional) range can indicate memory mapped flash +- interrupts : Must contain one entry +- interrupt-parent : Must be core interrupt controller +- clocks : Must reference the frequency given to the controller +- #address-cells : Must be '1', indicating which CS to use +- #size-cells : Must be '0' + +Optional properties: +- sifive,fifo-depth : Depth of hardware queues; defaults to 8 +- sifive,max-bits-per-word : Maximum bits per word; defaults to 8 + +Example: + spi: spi@10040000 { + compatible = "sifive,spi0"; + reg = <0x0 0x10040000 0x0 0x1000 0x0 0x20000000 0x0 0x10000000>; + interrupt-parent = <&plic>; + interrupts = <51>; + clocks = <&tlclk>; + #address-cells = <1>; + #size-cells = <0>; + sifive,fifo-depth = <8>; + sifive,max-bits-per-word = <8>; + };
drivers/spi/Kconfig +6 -0
@@ -615,6 +615,12 @@ config SPI_SH_HSPI help SPI driver for SuperH HSPI blocks. +config SPI_SIFIVE + tristate "SiFive SPI controller" + depends on HAS_IOMEM + help + This exposes the SPI controller IP from SiFive. + config SPI_SIRF tristate "CSR SiRFprimaII SPI controller" depends on SIRF_DMA
drivers/spi/Makefile +1 -0
@@ -90,6 +90,7 @@ obj-$(CONFIG_SPI_SH) += spi-sh.o obj-$(CONFIG_SPI_SH_HSPI) += spi-sh-hspi.o obj-$(CONFIG_SPI_SH_MSIOF) += spi-sh-msiof.o obj-$(CONFIG_SPI_SH_SCI) += spi-sh-sci.o +obj-$(CONFIG_SPI_SIFIVE) += spi-sifive.o obj-$(CONFIG_SPI_SIRF) += spi-sirf.o obj-$(CONFIG_SPI_SLAVE_MT27XX) += spi-slave-mt27xx.o obj-$(CONFIG_SPI_SPRD) += spi-sprd.o
drivers/spi/spi-sifive.c new file +442 -0
442 lines not on this page · raw message
Signature · 1 line
2.19.1
Mark Brown
maintainerQuoted 3 lines · Emil Renner Berthing
I know the discussions about the sifive devicetree compatible strings haven't come to a conclusion, so I'm sending this as an RFC to get some feedback on the rest of the code.
I've not seen any of these discussions or earlier versions of this driver so I've no idea what's going on here :(
Documentation/devicetree/bindings/spi/spi-sifive.txt line 17
Quoted 4 lines
Documentation/devicetree/bindings/spi/spi-sifive.txt new file +4
-0+Optional properties: +- sifive,fifo-depth : Depth of hardware queues; defaults to 8 +- sifive,max-bits-per-word : Maximum bits per word; defaults to 8 +
If the hardware isn't fixed yet making these enumerable from the hardware would be good...
drivers/spi/spi-sifive.c line 4
Quoted 5 lines
drivers/spi/spi-sifive.c new file +4
-0@@ -0,0 +1,442 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* SiFive SPI controller driver (master mode only) +*
Please make the entire comment a C++ one to make this look more intentinal.
drivers/spi/spi-sifive.c line 23
Quoted 4 lines
drivers/spi/spi-sifive.c new file +4
-0+/* for consistency we need this symbol */ +#ifdef REG_FMT +#undef REG_FMT +#endif
We do? For consistency with what?
drivers/spi/spi-sifive.c line 122
Quoted 6 lines
drivers/spi/spi-sifive.c new file +5
-0+static void sifive_spi_init (struct sifive_spi *spi) +{ +/* Set CS/SCK Delays and Inactive Time to defaults */ + +/* Exit specialized memory-mapped SPI flash mode */
...or not?
drivers/spi/spi-sifive.c line 166
Quoted 6 lines
drivers/spi/spi-sifive.c new file +6
-0+/* Set frame format */ + cr =FMT_LEN (t->bits_per_word); +switch (mode) { +case SPI_NBITS_QUAD : + cr |=FMT_PROTO_QUAD ; +break ;
Some namespacing on the driver #defines would be a bit safer against the possibility of collision with future changes in headers.
drivers/spi/spi-sifive.c line 208
Quoted 6 lines
drivers/spi/spi-sifive.c new file +6
-0+static void sifive_spi_wait (struct sifive_spi *spi,u32 bit,int poll) +{ +if (poll) { +u32 cr; +do cr =sifive_spi_read (spi,REG_IP ); +while (!(cr & bit));
Please add some braces, indentation or something to make it more clear that the read is part of a do/while loop - right now it's not immediately obvious that this is correct.
drivers/spi/spi-sifive.c line 255
Quoted 8 lines
drivers/spi/spi-sifive.c new file +8
-0+static int sifive_spi_transfer_one (struct spi_master *master, +struct spi_device *device,struct spi_transfer *t) +{ +struct sifive_spi *spi =spi_master_get_devdata (master); +int poll =sifive_spi_prep_transfer (spi, device, t); + +sifive_spi_execute (spi, t, poll); +
Why not just inline the execute function here? It's the only caller AFAICT.
drivers/spi/spi-sifive.c line 265
Quoted 7 lines
drivers/spi/spi-sifive.c new file +7
-0+static void sifive_spi_set_cs (struct spi_device *device,bool is_high) +{ +struct sifive_spi *spi =spi_master_get_devdata (device->master); + +/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */ +if (device->mode &SPI_CS_HIGH ) + is_high = !is_high;
spi_set_cs() will handle CS_HIGH for you.
drivers/spi/spi-sifive.c line 373
Quoted 1 line
drivers/spi/spi-sifive.c new file +1
-0+ master->bits_per_word_mask =SPI_BPW_MASK (8 );
I thought the device supported other bits per word values?
drivers/spi/spi-sifive.c line 382
Quoted 4 lines
drivers/spi/spi-sifive.c new file +4
-0+/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers. +* Probably it should rely on the SPI core auto mapping instead. +*/ + pdev->dev.dma_mask =NULL ;
If this is a problem please fix it in the MMC core, don't bodge it like this.
drivers/spi/spi-sifive.c line 428
Quoted 5 lines
drivers/spi/spi-sifive.c new file +5
-0+static const struct of_device_id sifive_spi_of_match[] = { + { .compatible ="sifive,spi0" , }, + {} +}; +MODULE_DEVICE_TABLE (of, sifive_spi_of_match);
spi0 is a *weird* compatible name.
Hi Mark,
Quoted 8 lines · Mark Brown
Quoted 3 lines · Emil Renner Berthing
I know the discussions about the sifive devicetree compatible strings haven't come to a conclusion, so I'm sending this as an RFC to get some feedback on the rest of the code.
I've not seen any of these discussions or earlier versions of this driver so I've no idea what's going on here :(
No, sorry. This has been discussed on linux-riscv for other drivers like the uart. See my last answer.
Quoted 7 lines
Documentation/devicetree/bindings/spi/spi-sifive.txt line 17
Quoted 4 lines
Documentation/devicetree/bindings/spi/spi-sifive.txt new file +4
-0+Optional properties: +- sifive,fifo-depth : Depth of hardware queues; defaults to 8 +- sifive,max-bits-per-word : Maximum bits per word; defaults to 8 +If the hardware isn't fixed yet making these enumerable from the hardware would be good...
Agreed, but unfortunately this is already in the FU540-C000 chip on the HiFive Unleashed board sold by SiFive.
Quoted 8 lines
drivers/spi/spi-sifive.c line 4
Quoted 5 lines
drivers/spi/spi-sifive.c new file +4
-0@@ -0,0 +1,442 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* SiFive SPI controller driver (master mode only) +* Please make the entire comment a C++ one to make this look more intentinal.
Will do.
Quoted 6 lines
drivers/spi/spi-sifive.c line 23
Quoted 4 lines
drivers/spi/spi-sifive.c new file +4
-0+/* for consistency we need this symbol */ +#ifdef REG_FMT +#undef REG_FMT +#endif We do? For consistency with what?
Below all the register offsets are defined as
REG_<register name>. This is is a pattern I
copied from other drivers, but here we have a
register called "fmt" - hence REG_FMT.
If you have a better pattern that doesn't clash
with REG_FMT please let me know.
Quoted 8 lines
drivers/spi/spi-sifive.c line 122
Quoted 6 lines
drivers/spi/spi-sifive.c new file +5
-0+static void sifive_spi_init (struct sifive_spi *spi) +{ +/* Set CS/SCK Delays and Inactive Time to defaults */ + +/* Exit specialized memory-mapped SPI flash mode */ ...or not?
Right. Will add that or just delete the comment.
Quoted 9 lines
drivers/spi/spi-sifive.c line 166
Quoted 6 lines
drivers/spi/spi-sifive.c new file +6
-0+/* Set frame format */ + cr =FMT_LEN (t->bits_per_word); +switch (mode) { +case SPI_NBITS_QUAD : + cr |=FMT_PROTO_QUAD ; +break ;Some namespacing on the driver #defines would be a bit safer against the possibility of collision with future changes in headers.
Right.
Quoted 10 lines
drivers/spi/spi-sifive.c line 208
Quoted 6 lines
drivers/spi/spi-sifive.c new file +6
-0+static void sifive_spi_wait (struct sifive_spi *spi,u32 bit,int poll) +{ +if (poll) { +u32 cr; +do cr =sifive_spi_read (spi,REG_IP ); +while (!(cr & bit));Please add some braces, indentation or something to make it more clear that the read is part of a do/while loop - right now it's not immediately obvious that this is correct.
Good point. Will do.
Quoted 11 lines
drivers/spi/spi-sifive.c line 255
Quoted 8 lines
drivers/spi/spi-sifive.c new file +8
-0+static int sifive_spi_transfer_one (struct spi_master *master, +struct spi_device *device,struct spi_transfer *t) +{ +struct sifive_spi *spi =spi_master_get_devdata (master); +int poll =sifive_spi_prep_transfer (spi, device, t); + +sifive_spi_execute (spi, t, poll); +Why not just inline the execute function here? It's the only caller AFAICT.
Yeah, it is. Will do.
Quoted 10 lines
drivers/spi/spi-sifive.c line 265
Quoted 7 lines
drivers/spi/spi-sifive.c new file +7
-0+static void sifive_spi_set_cs (struct spi_device *device,bool is_high) +{ +struct sifive_spi *spi =spi_master_get_devdata (device->master); + +/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */ +if (device->mode &SPI_CS_HIGH ) + is_high = !is_high;
spi_set_cs()will handleCS_HIGHfor you.
Quoted 3 lines
Quoted 1 line
+
master->bits_per_word_mask=SPI_BPW_MASK(8);I thought the device supported other bits per word values?
It does, but the driver doesn't yet. When bits per word is < 8 we need to shift the bits in each byte to be "left-aligned" (unless SPI_LSB_FIRST is set).
Quoted 7 lines
drivers/spi/spi-sifive.c line 382
Quoted 4 lines
drivers/spi/spi-sifive.c new file +4
-0+/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers. +* Probably it should rely on the SPI core auto mapping instead. +*/ + pdev->dev.dma_mask =NULL ;If this is a problem please fix it in the MMC core, don't bodge it like this.
Gotcha. Will remove this.
Quoted 7 lines
drivers/spi/spi-sifive.c line 428
Quoted 5 lines
drivers/spi/spi-sifive.c new file +5
-0+static const struct of_device_id sifive_spi_of_match[] = { + { .compatible ="sifive,spi0" , }, + {} +}; +MODULE_DEVICE_TABLE (of, sifive_spi_of_match);spi0 is a *weird* compatible name.
Exactly. Hence the discussion about the compatible strings. Once this discussion comes to a conclusion I'll update this.
Thank you for the review!
/Emil
Mark Brown
maintainerQuoted 17 lines · Emil Renner Berthing
Quoted 8 lines · Mark Brown
Quoted 3 lines · Emil Renner Berthing
I know the discussions about the sifive devicetree compatible strings haven't come to a conclusion, so I'm sending this as an RFC to get some feedback on the rest of the code.
I've not seen any of these discussions or earlier versions of this driver so I've no idea what's going on here :(
No, sorry. This has been discussed on linux-riscv for other drivers like the uart. See my last answer.
Quoted 3 lines
Quoted 3 lines
+Optional properties:
+- sifive,fifo-depth : Depth of hardware queues; defaults to 8 +- sifive,max-bits-per-word : Maximum bits per word; defaults to 8
Quoted 5 lines
Quoted 2 lines
If the hardware isn't fixed yet making these enumerable from the hardware would be good...
Agreed, but unfortunately this is already in the FU540-C000 chip on the HiFive Unleashed board sold by SiFive.
Pick an unused register you can read safely and define that value to the default :)
Quoted 7 lines
Quoted 6 lines
drivers/spi/spi-sifive.c line 23
Quoted 4 lines
drivers/spi/spi-sifive.c new file +4
-0+/* for consistency we need this symbol */ +#ifdef REG_FMT +#undef REG_FMT +#endif We do? For consistency with what?
Quoted 6 lines
Below all the register offsets are defined as
REG_<register name>. This is is a pattern I
copied from other drivers, but here we have a
register called "fmt" - henceREG_FMT.
If you have a better pattern that doesn't clash
withREG_FMTplease let me know.
You shouldn't be using such generic names for your internal identifiers, add a prefix to everything.
Hi Emil,
I am working full time on Linux device drivers for SiFive. I have some bandwidth to work on this SPI driver. Do you mind if I take over this SPI driver from here on?
Thanks,
Yash Shah
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Yash,
Quoted 3 lines · Yash Shah
I am working full time on Linux device drivers for SiFive. I have some bandwidth to work on this SPI driver. Do you mind if I take over this SPI driver from here on?
No, that's fine. You can pick it up from here:
https://github.com/esmil/linux/commit/870b04e85662a02ee1f6333e1d037c774ed4350e
That's with all of Mark's comments fixed, but the bindings doc probably needs to be updated and split into a separate patch.
/Emil
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv