dt: bindings: spi-bcm-qspi: NSP, NS2, BRCMSTB SoC bindings

drivers/spi/spi-bcm-qspi.c new file +7 -0

+/* Read qspi controller register*/
+static inline u32 bcm_qspi_read(struct bcm_qspi *qspi, enum base_type type,
+				   unsigned int offset)
+{
+	if (!qspi->base[type])
+		return 0;
+

drivers/spi/spi-bcm-qspi.c new file +6 -0

+static int bcm_qspi_bspi_set_flex_mode(struct bcm_qspi *qspi, int width,
+				       int addrlen, int hp)
+{
+	int bpc = 0, bpp = 0;
+	u8 command = SPINOR_OP_READ_FAST;
+	int flex_mode = 1, rv = 0;

drivers/spi/spi-bcm-qspi.c new file +9 -0

+static int bcm_qspi_simple_transaction(struct bcm_qspi *qspi,
+	const void *tx_buf, int tx_len, void *rx_buf, int rx_len)
+{
+
+	struct bcm_qspi_parms *xp = &qspi->last_parms;
+	struct spi_message m;
+	struct spi_transfer t_tx, t_rx;
+	struct spi_device spi;
+	int ret;

+MODULE_AUTHOR("Broadcom");

drivers/spi/spi-bcm-qspi.c line 1813

Quoted 7 lines

drivers/spi/spi-bcm-qspi.c new file +7 -0

+static const struct of_device_id bcm_qspi_of_match[] = {
+	{ .compatible = "brcm,spi-bcm-qspi" },
+	{ .compatible = "brcm,qspi-brcmstb" },
+	{ .compatible = "brcm,spi-brcmstb-mspi"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm_qspi_of_match);

This is adding new DT bindings but there is no documentation, documentation is required for all DT bindings.

Quoted 10 lines · Mark Brown

drivers/spi/spi-bcm-qspi.c line 1813

Quoted 7 lines

drivers/spi/spi-bcm-qspi.c new file +7 -0

+static const struct of_device_id bcm_qspi_of_match[] = {
+	{ .compatible = "brcm,spi-bcm-qspi" },
+	{ .compatible = "brcm,qspi-brcmstb" },
+	{ .compatible = "brcm,spi-brcmstb-mspi"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm_qspi_of_match);

This is adding new DT bindings but there is no documentation, documentation is required for all DT bindings.

The binding documentation is added to patch 1, which you are listed as a recipient for.

Quoted 12 lines · Kamal Dasu

drivers/spi/spi-bcm-qspi.c new file +12 -0

+/* Read qspi controller register*/
+static inline u32 bcm_qspi_read(struct bcm_qspi *qspi, enum base_type type,
+                                unsigned int offset)
+{
+     if (!qspi->base[type])
+             return 0;
+
+     /*
+      * MIPS endianness is configured by boot strap, which also reverses all
+      * bus endianness (i.e., big-endian CPU + big endian bus ==> native
+      * endian I/O).
+      *

drivers/spi/spi-bcm-qspi.c line 289

Quoted 5 lines

drivers/spi/spi-bcm-qspi.c new file +5 -0

+      * Other architectures (e.g., ARM) either do not support big endian, or
+      * else leave I/O in little endian mode.
+      */
+     if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+             return __raw_readl(qspi->base[type] + offset);

You shouldn't use raw accessors outside of arch code. Use ioread32be() to read big endian values, and similarly for the writes.

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!qspi->base[INTR])
+             return;

All these bits where we just silently ignore operations if some value isn't initialized are a bit worrying - why is this safe and robust? I'd expect chip feature selection to be done at a higher level.

drivers/spi/spi-bcm-qspi.c line 318

Quoted 4 lines

drivers/spi/spi-bcm-qspi.c new file +4 -0

+
+     if (qspi->hif_spi_mode)
+             bcm_qspi_write(qspi, INTR, HIF_SPI_INTR2_CPU_MASK_CLEAR, mask);
+     else {

Coding style { } on both sides of the if if they're on one.

drivers/spi/spi-bcm-qspi.c line 447

Quoted 7 lines

drivers/spi/spi-bcm-qspi.c new file +7 -0

+static void bcm_qspi_bspi_lr_data_read(struct bcm_qspi *qspi)
+{
+     u32 *buf = (u32 *)qspi->bspi_xfer->rx_buf;
+     u32 data = 0;
+
+     DBG("xfer %p rx %p rxlen %d\n",
+         qspi->bspi_xfer, qspi->bspi_xfer->rx_buf, qspi->bspi_xfer_len);

Use the standard kernel trace infrastructure, don't invent your own.

Quoted 10 lines

drivers/spi/spi-bcm-qspi.c new file +10 -0

+static int bcm_qspi_bspi_set_flex_mode(struct bcm_qspi *qspi, int width,
+                                    int addrlen, int hp)
+{
+     int bpc = 0, bpp = 0;
+     u8 command = SPINOR_OP_READ_FAST;
+     int flex_mode = 1, rv = 0;
+     bool spans_4byte = false;
+
+     DBG("set flex mode w %x addrlen %x hp %d\n", width, addrlen, hp);
+

drivers/spi/spi-bcm-qspi.c line 498

Quoted 6 lines

drivers/spi/spi-bcm-qspi.c new file +6 -0

+     if (addrlen == BSPI_ADDRLEN_4BYTES) {
+             bpp = BSPI_BPP_ADDR_SELECT_MASK;
+             spans_4byte = true;
+     }
+
+     bpp |= 8; /* dummy cycles */

Dummy cycles?

drivers/spi/spi-bcm-qspi.c line 538

Quoted 2 lines

drivers/spi/spi-bcm-qspi.c new file +2 -0

+     default:
+             rv = -1;

Real error codes please.

drivers/spi/spi-bcm-qspi.c line 639

Quoted 9 lines

drivers/spi/spi-bcm-qspi.c new file +9 -0

+     if (!error) {
+             qspi->xfer_mode.width = width;
+             qspi->xfer_mode.addrlen = addrlen;
+             qspi->xfer_mode.hp = hp;
+             dev_info(&qspi->pdev->dev,
+                     "%d-lane output, %d-byte address%s\n",
+                     qspi->xfer_mode.width,
+                     qspi->xfer_mode.addrlen,
+                     qspi->xfer_mode.hp ? ", high-performance mode" : "");

This is far too noisy, it's going to spam the logs on every transfer.

drivers/spi/spi-bcm-qspi.c line 643

Quoted 5 lines

drivers/spi/spi-bcm-qspi.c new file +4 -0

+     } else
+             dev_warn(&qspi->pdev->dev,
+                     "INVALID COMBINATION: width=%d addrlen=%d hp=%d\n",
+                     width, addrlen, hp);

Why isn't this returning an error?

Quoted 5 lines

drivers/spi/spi-bcm-qspi.c new file +5 -0

+static int bcm_qspi_update_parms(struct bcm_qspi *qspi,
+                              struct spi_device *spidev,
+                              struct spi_transfer *trans, int override)
+{
+     struct bcm_qspi_parms xp;

drivers/spi/spi-bcm-qspi.c line 725

Quoted 4 lines

drivers/spi/spi-bcm-qspi.c new file +4 -0

+
+     xp.speed_hz = min(trans->speed_hz ? trans->speed_hz :
+                     (spidev->max_speed_hz ? spidev->max_speed_hz :
+                     qspi->max_speed_hz), qspi->max_speed_hz);

This is marginally legible, and you should just be able to trust what's in the transfer anyway as the core should be doing this - there's nothing device specific.

drivers/spi/spi-bcm-qspi.c line 740

Quoted 10 lines

drivers/spi/spi-bcm-qspi.c new file +10 -0

+     if ((override == PARMS_OVERRIDE) ||
+             ((xp.speed_hz == qspi->last_parms.speed_hz) &&
+              (xp.chip_select == qspi->last_parms.chip_select) &&
+              (xp.mode == qspi->last_parms.mode) &&
+              (xp.bits_per_word == qspi->last_parms.bits_per_word))) {
+             bcm_qspi_hw_set_parms(qspi, &xp);
+             return 0;
+     }
+     /* no override, and parms do not match */
+     return 1;

What is an override

drivers/spi/spi-bcm-qspi.c line 788

Quoted 4 lines

drivers/spi/spi-bcm-qspi.c new file +4 -0

+static int find_next_byte(struct bcm_qspi *qspi, struct position *p,
+                       int flags)
+{
+     int ret = FNB_BREAK_NONE;

What is this supposed to be doing?

drivers/spi/spi-bcm-qspi.c line 964

Quoted 6 lines

drivers/spi/spi-bcm-qspi.c new file +6 -0

+     /* deassert CS on the final byte */
+     if (fnb & FNB_BREAK_DESELECT) {
+             mspi_cdram = read_cdram_slot(qspi, slot - 1) &
+                     ~MSPI_CDRAM_CONT_BIT;
+             write_cdram_slot(qspi, slot - 1, mspi_cdram);
+     }

Let the core handle asserting and deasserting chip select.

drivers/spi/spi-bcm-qspi.c line 991

Quoted 6 lines

drivers/spi/spi-bcm-qspi.c new file +6 -0

+/* BSPI helpers */
+static int bcm_qspi_emulate_flash_read(struct bcm_qspi *qspi,
+                            struct spi_device *spi, struct spi_transfer *t)
+{
+     u32 addr, len, len_words;
+     u8 *buf;

What is this doing? This seems very worrying...

Quoted 6 lines

drivers/spi/spi-bcm-qspi.c new file +6 -0

+     if (trans && trans->len && trans->tx_buf) {
+             u8 command = ((u8 *)trans->tx_buf)[0];
+
+             if (trans->rx_nbits)
+                     nbits = trans->rx_nbits;
+

drivers/spi/spi-bcm-qspi.c line 1113

Quoted 3 lines

drivers/spi/spi-bcm-qspi.c new file +3 -0

+             switch (command) {
+             case SPINOR_OP_READ4_FAST:
+                     if (!bcm_qspi_is_4_byte_mode(qspi))

No, this is not a good idea. You should not be attempting to parse the data stream. If your controller has flash support it should implement the flash interfaces, otherwise it should just pass the data streams through.

drivers/spi/spi-bcm-qspi.c line 1229

Quoted 2 lines

drivers/spi/spi-bcm-qspi.c new file +2 -0

+static int bcm_qspi_transfer_one_message(struct spi_master *master,
+              struct spi_message *msg)

Implement transfer_one(), not transfer_one_message().

Quoted 16 lines

drivers/spi/spi-bcm-qspi.c new file +16 -0

+static int bcm_qspi_simple_transaction(struct bcm_qspi *qspi,
+     const void *tx_buf, int tx_len, void *rx_buf, int rx_len)
+{
+
+     struct bcm_qspi_parms *xp = &qspi->last_parms;
+     struct spi_message m;
+     struct spi_transfer t_tx, t_rx;
+     struct spi_device spi;
+     int ret;
+
+     memset(&spi, 0, sizeof(spi));
+     spi.max_speed_hz = xp->speed_hz;
+     spi.chip_select = xp->chip_select;
+     spi.mode = xp->mode;
+     spi.bits_per_word = xp->bits_per_word;
+     spi.master = qspi->master;

drivers/spi/spi-bcm-qspi.c line 1438

Quoted 3 lines

drivers/spi/spi-bcm-qspi.c new file +3 -0

+
+     spi_message_init(&m);
+     m.spi = &spi;

I don't know why your driver is creating and trying to do SPI transfers but that is completely inappropriate for a SPI controller. Whatever functionality this is implementing should be in a separate SPI device driver.

drivers/spi/spi-bcm-qspi.c line 1813

Quoted 7 lines

drivers/spi/spi-bcm-qspi.c new file +7 -0

+static const struct of_device_id bcm_qspi_of_match[] = {
+     { .compatible = "brcm,spi-bcm-qspi" },
+     { .compatible = "brcm,qspi-brcmstb" },
+     { .compatible = "brcm,spi-brcmstb-mspi"},
+     {},
+};
+MODULE_DEVICE_TABLE(of, bcm_qspi_of_match);

This is adding new DT bindings but there is no documentation, documentation is required for all DT bindings.

drivers/spi/spi-bcm-qspi.c line 1819

Quoted 5 lines

drivers/spi/spi-bcm-qspi.c new file +5 -0

+static struct platform_driver bcm_qspi_driver = {
+     .driver = {
+             .name = DRIVER_NAME,
+             .bus = &platform_bus_type,
+             .owner = THIS_MODULE,

You don't need to initalize bus or owner.

Quoted 1 line

+MODULE_AUTHOR("Broadcom");

This should be a person if it's going to be included.

Quoted 8 lines · Mark Brown

Quoted 6 lines · Kamal Dasu

drivers/spi/spi-bcm-qspi.c new file +6 -0

+     /* deassert CS on the final byte */
+     if (fnb & FNB_BREAK_DESELECT) {
+             mspi_cdram = read_cdram_slot(qspi, slot - 1) &
+                     ~MSPI_CDRAM_CONT_BIT;
+             write_cdram_slot(qspi, slot - 1, mspi_cdram);
+     }

Quoted 1 line

Let the core handle asserting and deasserting chip select.

Its actually a peripheral CS (PCS) bit in the MSPI block. Peripheral chip selects are used to select an external device for serial data transfer. PCS[i] corresponds to pin SS[i].

Quoted 8 lines

drivers/spi/spi-bcm-qspi.c line 1113

Quoted 3 lines

drivers/spi/spi-bcm-qspi.c new file +3 -0

+             switch (command) {
+             case SPINOR_OP_READ4_FAST:
+                     if (!bcm_qspi_is_4_byte_mode(qspi))

No, this is not a good idea. You should not be attempting to parse the data stream. If your controller has flash support it should implement the flash interfaces, otherwise it should just pass the data streams through.

This is only for read command we need to setup both tx and rx at the same time for improved performance.

Quoted 7 lines

drivers/spi/spi-bcm-qspi.c line 1438

Quoted 2 lines

drivers/spi/spi-bcm-qspi.c new file +2 -0

+     spi_message_init(&m);
+     m.spi = &spi;

I don't know why your driver is creating and trying to do SPI transfers but that is completely inappropriate for a SPI controller. Whatever functionality this is implementing should be in a separate SPI device driver.

Currently not used. But I did not see a good way to be able to send commands, like finding the ID, setting 3/4 byte addressing mode , quad mode, on a PM resume(). Are you suggesting I make a driver similar to m25p80 ?.

Quoted 4 lines

Quoted 1 line

+MODULE_DEVICE_TABLE(of, bcm_qspi_of_match);

This is adding new DT bindings but there is no documentation, documentation is required for all DT bindings.

This was part of Patch 1/4 as pointed to by Florian

Quoted 9 lines · Kamal Dasu

Quoted 8 lines · Mark Brown

Quoted 6 lines · Kamal Dasu

(quoted diff) +6 -0

+     /* deassert CS on the final byte */
+     if (fnb & FNB_BREAK_DESELECT) {
+             mspi_cdram = read_cdram_slot(qspi, slot - 1) &
+                     ~MSPI_CDRAM_CONT_BIT;
+             write_cdram_slot(qspi, slot - 1, mspi_cdram);
+     }

Quoted 5 lines

Quoted 1 line

Let the core handle asserting and deasserting chip select.

Its actually a peripheral CS (PCS) bit in the MSPI block. Peripheral chip selects are used to select an external device for serial data transfer. PCS[i] corresponds to pin SS[i].

That doesn't appear to address the issue, what you're describing sounds like the normal function of the chip select?

Quoted 9 lines

Quoted 9 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+             switch (command) {
+             case SPINOR_OP_READ4_FAST:
+                     if (!bcm_qspi_is_4_byte_mode(qspi))

No, this is not a good idea. You should not be attempting to parse the data stream. If your controller has flash support it should implement the flash interfaces, otherwise it should just pass the data streams through.

Quoted 2 lines

This is only for read command we need to setup both tx and rx at the same time for improved performance.

No, to repeat what I said if you want to support accelerated flash reads implement the standard accelerated flash read operation we have (spi_flash_read()). This is a fairly common feature and trying to open code this in individual drivers would at the very least lead to a lot of duplicated code and is the source of most of the problems in the code.

Quoted 7 lines

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     spi_message_init(&m);
+     m.spi = &spi;

I don't know why your driver is creating and trying to do SPI transfers but that is completely inappropriate for a SPI controller. Whatever functionality this is implementing should be in a separate SPI device driver.

Quoted 4 lines

Currently not used. But I did not see a good way to be able to send commands, like finding the ID, setting 3/4 byte addressing mode , quad mode, on a PM resume(). Are you suggesting I make a driver similar to m25p80 ?.

I am suggesting you implement the standard flash interfaces and as a result directly use m25p80.

I was looking at spi_flash_read() caled from m25p80_read() and spi core implementation. I have a case where when we have very small reads or unaligned buffers and addresses passed then I have to fall back to using standard MSPI reads. How best to handle this ?. One of the options without having to replicate code involves a minor change to m25p80 driver as shown below.

Quoted 7 lines

Quoted 4 lines

Currently not used. But I did not see a good way to be able to send commands, like finding the ID, setting 3/4 byte addressing mode , quad mode, on a PM resume(). Are you suggesting I make a driver similar to m25p80 ?.

I am suggesting you implement the standard flash interfaces and as a result directly use m25p80.

Are you suggesting to use something like spi_read_then_write() in cases where I want to set the addressing and single/quad modes ?.