spi: Add FSI-attached SPI controller driver
Subsystems (2)
Eddie James
maintainerPATCH v2 1/1 spi: Add FSI-attached SPI controller driver
There exists a set of SPI controllers on some POWER processors that may be accessed through the FSI bus. Add a driver to traverse the FSI CFAM engine that can access and drive the SPI controllers. This driver would typically be used by a baseboard management controller (BMC).
The SPI controllers operate by means of programming a sequencing engine which automatically manages the usual SPI protocol buses. The driver programs each transfer into the sequencer as various operations specifying the slave chip and shifting data in and out on the lines.
---
Changes since v1:
- Add comments
- Switch to upper/lower_32_bits rather than manually shifting
- Use min rather than manual conditional
- Switch to dev_dbg for reset log
- Add a timeout and abort for multiple resets for the transfer init procedure
- Add MODULE_DEVICE_TABLE
- Misc minor clean up items
MAINTAINERS | 6 + drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi-fsi.c | 542 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 556 insertions(+) create mode 100644 drivers/spi/spi-fsi.c
MAINTAINERS +6 -0
@@ -6862,6 +6862,12 @@ S: Maintained F: drivers/i2c/busses/i2c-fsi.c F: Documentation/devicetree/bindings/i2c/i2c-fsi.txt +FSI-ATTACHED SPI DRIVER +M: Eddie James <[email protected]> +L: [email protected] +S: Maintained +F: drivers/spi/spi-fsi.c + FSNOTIFY: FILESYSTEM NOTIFICATION INFRASTRUCTURE M: Jan Kara <[email protected]> R: Amir Goldstein <[email protected]>
drivers/spi/Kconfig +7 -0
@@ -264,6 +264,13 @@ config SPI_FALCON has only been tested with m25p80 type chips. The hardware has no support for other types of SPI peripherals. +config SPI_FSI + tristate "FSI SPI driver" + depends on FSI + help + This enables support for the driver for FSI bus attached SPI + controllers. + config SPI_FSL_LPSPI tristate "Freescale i.MX LPSPI controller" depends on ARCH_MXC || COMPILE_TEST
drivers/spi/Makefile +1 -0
@@ -40,6 +40,7 @@ spi-dw-midpci-objs := spi-dw-pci.o spi-dw-mid.o obj-$(CONFIG_SPI_EFM32) += spi-efm32.o obj-$(CONFIG_SPI_EP93XX) += spi-ep93xx.o obj-$(CONFIG_SPI_FALCON) += spi-falcon.o +obj-$(CONFIG_SPI_FSI) += spi-fsi.o obj-$(CONFIG_SPI_FSL_CPM) += spi-fsl-cpm.o obj-$(CONFIG_SPI_FSL_DSPI) += spi-fsl-dspi.o obj-$(CONFIG_SPI_FSL_LIB) += spi-fsl-lib.o
drivers/spi/spi-fsi.c new file +542 -0
542 lines not on this page · raw message
Signature · 1 line
1.8.3.1
Quoted 10 lines · Eddie James
There exists a set of SPI controllers on some POWER processors that may be accessed through the FSI bus. Add a driver to traverse the FSI CFAM engine that can access and drive the SPI controllers. This driver would typically be used by a baseboard management controller (BMC).
The SPI controllers operate by means of programming a sequencing engine which automatically manages the usual SPI protocol buses. The driver programs each transfer into the sequencer as various operations specifying the slave chip and shifting data in and out on the lines.
Some comments left unsettled in v1, thus applicable here.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Eddie James
There exists a set of SPI controllers on some POWER processors that may be accessed through the FSI bus. Add a driver to traverse the FSI CFAM engine that can access and drive the SPI controllers. This driver would typically be used by a baseboard management controller (BMC).
The SPI controllers operate by means of programming a sequencing engine which automatically manages the usual SPI protocol buses. The driver programs each transfer into the sequencer as various operations specifying the slave chip and shifting data in and out on the lines.
Some comments left unsettled in v1, thus applicable here.
I think I addressed all of Andy's comments on v1 now, if Andy agreed about leaving the byte shifting as-is in fsi_spi_data_in/out.
Thanks,
Eddie
Quoted 1 line
Hi Eddie,
Some comments below. For the most part it looks good.
Review
Quoted 3 lines · Eddie James
(quoted diff) +3
-0+FSI-ATTACHED SPI DRIVER +M: Eddie James <[email protected]> +L: [email protected]
Add linux-fsi-uLR06cmDAlY/[email protected] too
Quoted 8 lines
drivers/spi/spi-fsi.c new file +7
-0+S: Maintained +F: drivers/spi/spi-fsi.c +static int fsi_spi_write_reg (struct fsi_spi *ctx,u32 offset,u64 value) +{ +int rc; +__be32 cmd_be; +__be32 data_be;
drivers/spi/spi-fsi.c line 149
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ +dev_dbg (ctx->dev,"Write %02x[%016llx].\n" , offset, value); + + data_be =cpu_to_be32 (upper_32_bits (value)); + rc =fsi_device_write (ctx->fsi,FSI2SPI_DATA0 , &data_be, +sizeof (data_be)); +if (rc) +return rc; + + data_be =cpu_to_be32 (lower_32_bits (value));
The lower_32_bits cast is redundant (but harmless).
drivers/spi/spi-fsi.c line 155
Quoted 6 lines
drivers/spi/spi-fsi.c new file +6
-0+ rc =fsi_device_write (ctx->fsi,FSI2SPI_DATA1 , &data_be, +sizeof (data_be)); +if (rc) +return rc; + + cmd_be =cpu_to_be32 ((offset + ctx->base) |FSI2SPI_CMD_WRITE );
offset + ctx->base must be less than 2 ^ 31 otherwise the top bit of the address collides with the write command. Should we introduce a check for that?
Quoted 17 lines
drivers/spi/spi-fsi.c new file +17
-0+ rc =fsi_device_write (ctx->fsi,FSI2SPI_CMD , &cmd_be,sizeof (cmd_be)); +if (rc) +return rc; + +return fsi_spi_check_status (ctx); +} + +static int fsi_spi_data_in (u64 in,u8 *rx,int len) +{ +int i; +int num_bytes =min (len,8 ); + +for (i =0 ; i < num_bytes; ++i) + rx[i] = (u8)(in >> (8 * ((num_bytes -1 ) - i))); + +return num_bytes; +}
drivers/spi/spi-fsi.c line 182
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ +static int fsi_spi_data_out (u64 *out,const u8 *tx,int len) +{ +int i; +int num_bytes =min (len,8 ); + + *out =0ULL ; + +for (i =0 ; i < num_bytes; ++i) + *out |= (u64 )tx[i] << (8 * (8 - (i +1 )));
Did this work with non-8 byte transfers? I think the second 8 should be num_bytes.
The loop requires careful reading to check. I wonder if we could do this instead, which eliminates a lot duplicated loads and stores and is easier to read:
uint8_t *outp = (uint8_t *)out;
for (i = 0; i < num_bytes; ++i) {
outp[num_bytes - (i + 1)] = tx[i];
}Quoted 15 lines
drivers/spi/spi-fsi.c new file +15
-0+ +return num_bytes; +} + +static int fsi_spi_reset (struct fsi_spi *ctx) +{ +int rc; + +dev_dbg (ctx->dev,"Resetting SPI controller.\n" ); + + rc =fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , +SPI_FSI_CLOCK_CFG_RESET1 ); +if (rc) +return rc; +
drivers/spi/spi-fsi.c line 207
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+return fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , +SPI_FSI_CLOCK_CFG_RESET2 ); +} + +static int fsi_spi_sequence_add (struct fsi_spi_sequence *seq,u8 val) +{ + seq->data |= (u64 )val << seq->bit; + seq->bit -=8 ; + +return ((64 - seq->bit) /8 ) -2 ;
I have no idea what this is doing. Add a comment?
Quoted 9 lines
drivers/spi/spi-fsi.c new file +8
-0+} + +static int fsi_spi_transfer_init (struct fsi_spi *ctx) +{ +int rc; +bool reset = false; +unsigned long end; +u64 seq_state;
drivers/spi/spi-fsi.c line 342
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+u64 clock_cfg =0ULL ; +u64 status =0ULL ; +u64 wanted_clock_cfg =SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_SCK_NO_DEL | +FIELD_PREP (SPI_FSI_CLOCK_CFG_SCK_DIV ,4 ); + + end = jiffies +msecs_to_jiffies (SPI_FSI_INIT_TIMEOUT_MS ); + do { + if (time_after(jiffies, end)) + return -ETIMEDOUT;
How tightly does this loop spin?
Should there be a delay inside of it?
Quoted 11 lines
drivers/spi/spi-fsi.c new file +11
-0+ + rc =fsi_spi_read_reg (ctx,SPI_FSI_STATUS , &status); +if (rc) +return rc; + +if (status & (SPI_FSI_STATUS_ANY_ERROR | +SPI_FSI_STATUS_TDR_FULL | +SPI_FSI_STATUS_RDR_FULL )) { +if (reset) +return -EIO ; +
drivers/spi/spi-fsi.c line 363
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ rc =fsi_spi_reset (ctx); +if (rc) +return rc; + + reset = true; +continue ; + } + + seq_state = status &SPI_FSI_STATUS_SEQ_STATE ; + }while (seq_state && (seq_state !=SPI_FSI_STATUS_SEQ_STATE_IDLE ));
../drivers/spi/spi-fsi.c: In function ‘fsi_spi_transfer_one_message’:../drivers/spi/spi-fsi.c:363:11: warning: ‘seq_state’ may be used uninitialized in this function [-Wmaybe-uninitialized]
363 | } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
| ^~~~~~~~~Quoted 20 lines
drivers/spi/spi-fsi.c new file +19
-0+ + rc =fsi_spi_read_reg (ctx,SPI_FSI_CLOCK_CFG , &clock_cfg); +if (rc) +return rc; + +if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE | +SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_MODE | +SPI_FSI_CLOCK_CFG_SCK_RECV_DEL | +SPI_FSI_CLOCK_CFG_SCK_DIV )) != wanted_clock_cfg) + rc =fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , + wanted_clock_cfg); + +return rc; +} +static int fsi_spi_probe (struct device *dev) +{ +int rc; +u32 root_ctrl_8;
drivers/spi/spi-fsi.c
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+struct device_node *np; +int num_controllers_registered =0 ; +struct fsi_device *fsi =to_fsi_dev (dev); + +/* +* Check the SPI mux before attempting to probe. If the mux isn't set +* then the SPI controllers can't access their slave devices. +*/ + rc =fsi_slave_read (fsi->slave,FSI_MBOX_ROOT_CTRL_8 , &root_ctrl_8, +sizeof (root_ctrl_8));
Is it correct to stop probing if the mux is not set?
I assume it changes at runtime depending on the state of the host. This could mean the driver probes correctly, but fails to work (if the mux was set at BMC boot, but then changes).
Should it instead block reads/writes when the mux is in the incorrect state?
Quoted 4 lines
drivers/spi/spi-fsi.c new file +4
-0+if (rc) +return rc; + +if (!root_ctrl_8) {
drivers/spi/spi-fsi.c line 493
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+dev_dbg (dev,"SPI mux not set, aborting probe.\n" ); +return -ENODEV ; + } + +for_each_available_child_of_node (dev->of_node, np) { +u32 base; +struct fsi_spi *ctx; +struct spi_controller *ctlr; + +if (of_property_read_u32 (np,"reg" , &base))
It looks like this has a device tree binding. Can you add a document describing it too?
Quoted 29 lines
drivers/spi/spi-fsi.c new file +29
-0+continue ; + + ctlr =spi_alloc_master (dev,sizeof (*ctx)); +if (!ctlr) +break ; + + ctlr->dev.of_node = np; + ctlr->num_chipselect =of_get_available_child_count (np) ?:1 ; + ctlr->flags =SPI_CONTROLLER_HALF_DUPLEX ; + ctlr->max_transfer_size = fsi_spi_max_transfer_size; + ctlr->transfer_one_message = fsi_spi_transfer_one_message; + + ctx =spi_controller_get_devdata (ctlr); + ctx->dev = &ctlr->dev; + ctx->fsi = fsi; + ctx->base = base +SPI_FSI_BASE ; + + rc =devm_spi_register_controller (dev, ctlr); +if (rc) +spi_controller_put (ctlr); +else + num_controllers_registered++; + } + +if (!num_controllers_registered) +return -ENODEV ; + +return 0 ; +}
Quoted 1 line · Joel Stanley
On Mon, 3 Feb 2020 at 22:30, Eddie James <[email protected]> wrote:
...
Quoted 28 lines
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+static int fsi_spi_data_in (u64 in,u8 *rx,int len) +{ +int i; +int num_bytes =min (len,8 ); + +for (i =0 ; i < num_bytes; ++i) + rx[i] = (u8)(in >> (8 * ((num_bytes -1 ) - i))); + +return num_bytes; +}drivers/spi/spi-fsi.c line 182
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ +static int fsi_spi_data_out (u64 *out,const u8 *tx,int len) +{ +int i; +int num_bytes =min (len,8 ); + + *out =0ULL ; + +for (i =0 ; i < num_bytes; ++i) + *out |= (u64 )tx[i] << (8 * (8 - (i +1 )));Did this work with non-8 byte transfers? I think the second 8 should be
num_bytes.The loop requires careful reading to check. I wonder if we could do this instead, which eliminates a lot duplicated loads and stores and is easier to read:
Quoted 5 lines
uint8_t *outp = (uint8_t *)out;
for (i = 0; i < num_bytes; ++i) {
outp[num_bytes - (i + 1)] = tx[i];
}Have you had a chance to read my review of this? What do you think about put_unaligned*()/get_unaligned*() instead of above?
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+ return num_bytes; +}
...
Quoted 46 lines
Quoted 6 lines
drivers/spi/spi-fsi.c new file +6
-0+static int fsi_spi_transfer_init (struct fsi_spi *ctx) +{ +int rc; +bool reset = false; +unsigned long end; +u64 seq_state;drivers/spi/spi-fsi.c line 342
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+u64 clock_cfg =0ULL ; +u64 status =0ULL ; +u64 wanted_clock_cfg =SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_SCK_NO_DEL | +FIELD_PREP (SPI_FSI_CLOCK_CFG_SCK_DIV ,4 ); + + end = jiffies +msecs_to_jiffies (SPI_FSI_INIT_TIMEOUT_MS ); + do { + if (time_after(jiffies, end)) + return -ETIMEDOUT;How tightly does this loop spin?
Should there be a delay inside of it?
Quoted 11 lines
drivers/spi/spi-fsi.c new file +11
-0+ + rc =fsi_spi_read_reg (ctx,SPI_FSI_STATUS , &status); +if (rc) +return rc; + +if (status & (SPI_FSI_STATUS_ANY_ERROR | +SPI_FSI_STATUS_TDR_FULL | +SPI_FSI_STATUS_RDR_FULL )) { +if (reset) +return -EIO ; +drivers/spi/spi-fsi.c line 363
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ rc =fsi_spi_reset (ctx); +if (rc) +return rc; + + reset = true; +continue ; + } + + seq_state = status &SPI_FSI_STATUS_SEQ_STATE ; + }while (seq_state && (seq_state !=SPI_FSI_STATUS_SEQ_STATE_IDLE ));
../drivers/spi/spi-fsi.c: In function ‘fsi_spi_transfer_one_message’:../drivers/spi/spi-fsi.c:363:11: warning: ‘seq_state’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Quoted 2 lines
363 | } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
| ^~~~~~~~~It's bogus warning, though, I think, easy to fix by reshuffling loop body.
Quoted 14 lines
Quoted 14 lines
drivers/spi/spi-fsi.c new file +14
-0+ rc =fsi_spi_read_reg (ctx,SPI_FSI_CLOCK_CFG , &clock_cfg); +if (rc) +return rc; + +if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE | +SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_MODE | +SPI_FSI_CLOCK_CFG_SCK_RECV_DEL | +SPI_FSI_CLOCK_CFG_SCK_DIV )) != wanted_clock_cfg) + rc =fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , + wanted_clock_cfg); + +return rc; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Eddie James
maintainerQuoted 36 lines · Joel Stanley
Hi Eddie,
Some comments below. For the most part it looks good.
Review
Quoted 3 lines · Eddie James
(quoted diff) +3
-0+FSI-ATTACHED SPI DRIVER +M: Eddie James <[email protected]> +L: [email protected]Add linux-fsi-uLR06cmDAlY/[email protected] too
Quoted 7 lines
drivers/spi/spi-fsi.c new file +7
-0+S: Maintained +F: drivers/spi/spi-fsi.c +static int fsi_spi_write_reg (struct fsi_spi *ctx,u32 offset,u64 value) +{ +int rc; +__be32 cmd_be; +__be32 data_be;drivers/spi/spi-fsi.c line 149
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ +dev_dbg (ctx->dev,"Write %02x[%016llx].\n" , offset, value); + + data_be =cpu_to_be32 (upper_32_bits (value)); + rc =fsi_device_write (ctx->fsi,FSI2SPI_DATA0 , &data_be, +sizeof (data_be)); +if (rc) +return rc; + + data_be =cpu_to_be32 (lower_32_bits (value));The
lower_32_bitscast is redundant (but harmless).drivers/spi/spi-fsi.c line 155
Quoted 6 lines
drivers/spi/spi-fsi.c new file +6
-0+ rc =fsi_device_write (ctx->fsi,FSI2SPI_DATA1 , &data_be, +sizeof (data_be)); +if (rc) +return rc; + + cmd_be =cpu_to_be32 ((offset + ctx->base) |FSI2SPI_CMD_WRITE );
Quoted 3 lines
offset +
ctx->basemust be less than 2 ^ 31 otherwise the top bit of the address collides with the write command. Should we introduce a check for that?
Sure, good idea.
Quoted 22 lines
Quoted 22 lines
drivers/spi/spi-fsi.c new file +21
-0+ rc =fsi_device_write (ctx->fsi,FSI2SPI_CMD , &cmd_be,sizeof (cmd_be)); +if (rc) +return rc; + +return fsi_spi_check_status (ctx); +} + +static int fsi_spi_data_in (u64 in,u8 *rx,int len) +{ +int i; +int num_bytes =min (len,8 ); + +for (i =0 ; i < num_bytes; ++i) + rx[i] = (u8)(in >> (8 * ((num_bytes -1 ) - i))); + +return num_bytes; +} + +static int fsi_spi_data_out (u64 *out,const u8 *tx,int len) +{ +int i;
Quoted 8 lines
drivers/spi/spi-fsi.c line 182
Quoted 6 lines
drivers/spi/spi-fsi.c new file +6
-0+int num_bytes =min (len,8 ); + + *out =0ULL ; + +for (i =0 ; i < num_bytes; ++i) + *out |= (u64 )tx[i] << (8 * (8 - (i +1 )));Did this work with non-8 byte transfers? I think the second 8 should be
num_bytes.
No it should be 8. From the spec, "The value written to this register has to be left aligned." So to send 0x5a (1 byte) should write 0x5a00000000000000. I've tested this with 1-7 byte writes and obtained the correct result.
Quoted 10 lines
The loop requires careful reading to check. I wonder if we could do this instead, which eliminates a lot duplicated loads and stores and is easier to read:
uint8_t *outp = (uint8_t *)out; for (i = 0; i < num_bytes; ++i) { outp[num_bytes - (i + 1)] = tx[i]; }
Yes that should also work (using 8 instead of num_bytes in the assignment).
Quoted 19 lines
Quoted 19 lines
drivers/spi/spi-fsi.c new file +18
-0+ +return num_bytes; +} + +static int fsi_spi_reset (struct fsi_spi *ctx) +{ +int rc; + +dev_dbg (ctx->dev,"Resetting SPI controller.\n" ); + + rc =fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , +SPI_FSI_CLOCK_CFG_RESET1 ); +if (rc) +return rc; + +return fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , +SPI_FSI_CLOCK_CFG_RESET2 ); +}
Quoted 8 lines
drivers/spi/spi-fsi.c line 207
Quoted 7 lines
drivers/spi/spi-fsi.c new file +7
-0+ +static int fsi_spi_sequence_add (struct fsi_spi_sequence *seq,u8 val) +{ + seq->data |= (u64 )val << seq->bit; + seq->bit -=8 ; + +return ((64 - seq->bit) /8 ) -2 ;I have no idea what this is doing. Add a comment?
Sure. It's adding the next command to the sequence register and incrementing it's counter.
Quoted 21 lines
Quoted 9 lines
drivers/spi/spi-fsi.c new file +8
-0+} + +static int fsi_spi_transfer_init (struct fsi_spi *ctx) +{ +int rc; +bool reset = false; +unsigned long end; +u64 seq_state;drivers/spi/spi-fsi.c line 342
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+u64 clock_cfg =0ULL ; +u64 status =0ULL ; +u64 wanted_clock_cfg =SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_SCK_NO_DEL | +FIELD_PREP (SPI_FSI_CLOCK_CFG_SCK_DIV ,4 ); + + end = jiffies +msecs_to_jiffies (SPI_FSI_INIT_TIMEOUT_MS ); + do { + if (time_after(jiffies, end)) + return -ETIMEDOUT;How tightly does this loop spin?
Quoted 1 line
Should there be a delay inside of it?
Well, very tightly but I haven't seen it ever do more than a couple of iterations. And the timeout is 1 second. Not sure if we need a delay?
Quoted 25 lines
Quoted 12 lines
drivers/spi/spi-fsi.c new file +11
-0+ + rc =fsi_spi_read_reg (ctx,SPI_FSI_STATUS , &status); +if (rc) +return rc; + +if (status & (SPI_FSI_STATUS_ANY_ERROR | +SPI_FSI_STATUS_TDR_FULL | +SPI_FSI_STATUS_RDR_FULL )) { +if (reset) +return -EIO ; +drivers/spi/spi-fsi.c line 363
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+ rc =fsi_spi_reset (ctx); +if (rc) +return rc; + + reset = true; +continue ; + } + + seq_state = status &SPI_FSI_STATUS_SEQ_STATE ; + }while (seq_state && (seq_state !=SPI_FSI_STATUS_SEQ_STATE_IDLE ));
../drivers/spi/spi-fsi.c: In function ‘fsi_spi_transfer_one_message’:../drivers/spi/spi-fsi.c:363:11: warning: ‘seq_state’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Quoted 3 lines
363 | } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE));
| ^~~~~~~~~Strange that I don't see this when I compile. I'll move the seq_state assignment up.
Quoted 31 lines
Quoted 19 lines
drivers/spi/spi-fsi.c new file +19
-0+ + rc =fsi_spi_read_reg (ctx,SPI_FSI_CLOCK_CFG , &clock_cfg); +if (rc) +return rc; + +if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE | +SPI_FSI_CLOCK_CFG_ECC_DISABLE | +SPI_FSI_CLOCK_CFG_MODE | +SPI_FSI_CLOCK_CFG_SCK_RECV_DEL | +SPI_FSI_CLOCK_CFG_SCK_DIV )) != wanted_clock_cfg) + rc =fsi_spi_write_reg (ctx,SPI_FSI_CLOCK_CFG , + wanted_clock_cfg); + +return rc; +} +static int fsi_spi_probe (struct device *dev) +{ +int rc; +u32 root_ctrl_8;drivers/spi/spi-fsi.c
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+struct device_node *np; +int num_controllers_registered =0 ; +struct fsi_device *fsi =to_fsi_dev (dev); + +/* +* Check the SPI mux before attempting to probe. If the mux isn't set +* then the SPI controllers can't access their slave devices. +*/ + rc =fsi_slave_read (fsi->slave,FSI_MBOX_ROOT_CTRL_8 , &root_ctrl_8, +sizeof (root_ctrl_8));Is it correct to stop probing if the mux is not set?
Quoted 3 lines
I assume it changes at runtime depending on the state of the host. This could mean the driver probes correctly, but fails to work (if the mux was set at BMC boot, but then changes).
Right, it could change as the host switches the mux. I'm not sure what the correct option here is.
Quoted 2 lines
Should it instead block reads/writes when the mux is in the incorrect state?
I see a couple of problems with this.
1) you'd have to check the state of the mux each time you try an operation?
2) how do slave devices (eeproms, etc) get probed up? They'd fail at first since operations are blocked, but then your master is probed with no easy way to probe the slave?
Quoted 17 lines
Quoted 5 lines
drivers/spi/spi-fsi.c new file +4
-0+if (rc) +return rc; + +if (!root_ctrl_8) {drivers/spi/spi-fsi.c line 493
Quoted 10 lines
drivers/spi/spi-fsi.c new file +10
-0+dev_dbg (dev,"SPI mux not set, aborting probe.\n" ); +return -ENODEV ; + } + +for_each_available_child_of_node (dev->of_node, np) { +u32 base; +struct fsi_spi *ctx; +struct spi_controller *ctlr; + +if (of_property_read_u32 (np,"reg" , &base))It looks like this has a device tree binding. Can you add a document describing it too?
Sure.
Quoted 30 lines
Quoted 30 lines
drivers/spi/spi-fsi.c new file +29
-0+continue ; + + ctlr =spi_alloc_master (dev,sizeof (*ctx)); +if (!ctlr) +break ; + + ctlr->dev.of_node = np; + ctlr->num_chipselect =of_get_available_child_count (np) ?:1 ; + ctlr->flags =SPI_CONTROLLER_HALF_DUPLEX ; + ctlr->max_transfer_size = fsi_spi_max_transfer_size; + ctlr->transfer_one_message = fsi_spi_transfer_one_message; + + ctx =spi_controller_get_devdata (ctlr); + ctx->dev = &ctlr->dev; + ctx->fsi = fsi; + ctx->base = base +SPI_FSI_BASE ; + + rc =devm_spi_register_controller (dev, ctlr); +if (rc) +spi_controller_put (ctlr); +else + num_controllers_registered++; + } + +if (!num_controllers_registered) +return -ENODEV ; + +return 0 ; +}