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

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

+	switch (width) {
+	case SPI_NBITS_QUAD:
+		/* clear dual mode and set quad mode */
+		data &= ~BSPI_STRAP_OVERRIDE_CTRL_DATA_DUAL;
+		data |= BSPI_STRAP_OVERRIDE_CTRL_DATA_QUAD;
+		break;
+	case SPI_NBITS_DUAL:
+		/* clear quad mode set dual mode */
+		data &= ~BSPI_STRAP_OVERRIDE_CTRL_DATA_QUAD;

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

+/* MSPI helpers */
+
+/* stop at end of transfer, no other reason */
+#define FNB_BREAK_NONE			0
+/* stop at end of spi_message */
+#define FNB_BREAK_EOM			1
+/* stop at end of spi_transfer if delay */
+#define FNB_BREAK_DELAY			2
+/* stop at end of spi_transfer if cs_change */

Quoted 6 lines

drivers/spi/spi-bcm-qspi.c new file +6 -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).
+	 *
+	 * Other architectures (e.g., ARM) either do not support big endian, or

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

Quoted 6 lines

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

+	 * else leave I/O in little endian mode.
+	 */
+	if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		return ioread32be(qspi->base[type] + offset);
+	else
+		return readl_relaxed(qspi->base[type] + offset);

Just put this in the DT like we do for other MIPS IPs.

Quoted 1 line · Mark Brown

Just put this in the DT like we do for other MIPS IPs.

As in, putting a native-endian property for this node, right?

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

Quoted 3 lines · Kamal Dasu

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

+#define STATE_IDLE                           0
+#define STATE_RUNNING                                1
+#define STATE_SHUTDOWN                               2

There's a lot of defines with very generic names that look like they should be namespaced.

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

Quoted 2 lines

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

+#define DWORD_ALIGNED(a)                     IS_ALIGNED((uintptr_t)(a), 4)
+#define ADDR_TO_4MBYTE_SEGMENT(addr)         (((u32)(addr)) >> 22)

This just doesn't belong in a driver, there's nothing driver specific about it. It should go in a generic header if it's useful.

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

Quoted 12 lines

drivers/spi/spi-bcm-qspi.c new file +12 -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).
+      *
+      * 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 ioread32be(qspi->base[type] + offset);
+     else
+             return readl_relaxed(qspi->base[type] + offset);

Just put this in the DT like we do for other MIPS IPs.

Quoted 12 lines

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

+/* Interrupt helpers when not using brcm intc driver */
+static void bcm_qspi_enable_interrupt(struct bcm_qspi *qspi, u32 mask)
+{
+     unsigned int val;
+
+     if (qspi->hif_spi_mode) {
+             bcm_qspi_write(qspi, INTR, HIF_SPI_INTR2_CPU_MASK_CLEAR, mask);
+     } else {
+             val = bcm_qspi_read(qspi, INTR, 0);
+             val = val | (mask << INTR_BASE_BIT_SHIFT);
+             bcm_qspi_write(qspi, INTR, 0, val);

Review

Quoted 2 lines

(quoted diff) +2 -0

+     }
+}

All this interrupt code and especially the fact that it's a completely separate register range in the binding looks very much like it's just an interrupt controller IP that's not particularly anything to do with the SPI controller and should therefore be in a separate driver. Why is this part of the SPI controller driver?

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

Quoted 7 lines

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

+static int bcm_qspi_bspi_set_override(struct bcm_qspi *qspi, int width,
+                                   int addrlen, int hp)
+{
+     u32 data = bcm_qspi_read(qspi, BSPI, BSPI_STRAP_OVERRIDE_CTRL);
+
+     pr_debug("set override mode w %x addrlen %x hp %d\n",
+              width, addrlen, hp);

dev_dbg(). If you've got a device prints should use it, it makes reading logs vastly easier.

Quoted 9 lines

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

+     switch (width) {
+     case SPI_NBITS_QUAD:
+             /* clear dual mode and set quad mode */
+             data &= ~BSPI_STRAP_OVERRIDE_CTRL_DATA_DUAL;
+             data |= BSPI_STRAP_OVERRIDE_CTRL_DATA_QUAD;
+             break;
+     case SPI_NBITS_DUAL:
+             /* clear quad mode set dual mode */
+             data &= ~BSPI_STRAP_OVERRIDE_CTRL_DATA_QUAD;

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

Quoted 10 lines

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

+             data |= BSPI_STRAP_OVERRIDE_CTRL_DATA_DUAL;
+             break;
+     case SPI_NBITS_SINGLE:
+             /* clear quad/dual mode */
+             data &= ~(BSPI_STRAP_OVERRIDE_CTRL_DATA_QUAD |
+                       BSPI_STRAP_OVERRIDE_CTRL_DATA_DUAL);
+             break;
+     default:
+             break;
+     }

We just ignore other widths?

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

Quoted 5 lines

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

+static void bcm_qspi_enable_bspi(struct bcm_qspi *qspi)
+{
+
+     if ((!qspi->base[BSPI]) || (qspi->bspi_enabled))
+             return;

Why would it be OK to call this if we don't have BSPI?

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

Quoted 2 lines

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

+     xp->mode = spi->mode;
+     xp->bits_per_word = spi->bits_per_word ? spi->bits_per_word : 8;

Write normal if statements for legibility please.

Quoted 9 lines

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

+/* MSPI helpers */
+
+/* stop at end of transfer, no other reason */
+#define FNB_BREAK_NONE                       0
+/* stop at end of spi_message */
+#define FNB_BREAK_EOM                        1
+/* stop at end of spi_transfer if delay */
+#define FNB_BREAK_DELAY                      2
+/* stop at end of spi_transfer if cs_change */

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

Quoted 10 lines

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

+#define FNB_BREAK_CS_CHANGE          4
+/* stop if we run out of bytes */
+#define FNB_BREAK_NO_BYTES           8
+
+/* events that make us stop filling TX slots */
+#define FNB_BREAK_TX                 (FNB_BREAK_EOM | FNB_BREAK_DELAY | \
+                                      FNB_BREAK_CS_CHANGE)
+
+/* events that make us deassert CS */
+#define FNB_BREAK_DESELECT           (FNB_BREAK_EOM | FNB_BREAK_CS_CHANGE)

This block of defies is just randomly in the middle of the driver?

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

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;

I'm unclear what this is intended to do, I suspect other readers might be too.

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

Quoted 2 lines

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

+static int bcm_qspi_flash_read(struct spi_device *spi,
+                            struct spi_flash_read_message *msg)

There's a lot of flash code in the driver, it would make review a lot easier to split this out into a followup patch so we have one patch with the standard SPI controller and some more adding the flash acceleration.

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

Quoted 3 lines

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

+static void bcm_qspi_trans_mode(struct bcm_qspi *qspi,
+                             struct spi_device *spi,
+                             struct spi_transfer *trans)

This still appears to be trying to parse the data in SPI transfers. Please don't do this, remove this code in favour of using the accelerated flash operations.

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

Quoted 4 lines

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

+     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hif_mspi");
+     if (!res)
+             res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+                                                "mspi");

Why support both names?

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

Quoted 9 lines

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

+     if (res) {
+             qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
+             if (IS_ERR(qspi->base[MSPI])) {
+                     ret = PTR_ERR(qspi->base[MSPI]);
+                     goto err2;
+             }
+     } else
+             goto err2;

Coding style, { } on both sides of the if.

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

Quoted 2 lines

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

+     if (!qspi->bspi_mode)
+             master->bus_num += 1;

Why is the driver attempting to set a bus number manually? The core will assign bus numbers automatically and anything that relies on them is going to be fragile.

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

Quoted 8 lines

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

+     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg");
+     if (res) {
+             qspi->base[CHIP_SELECT]  = devm_ioremap_resource(dev, res);
+             if (IS_ERR(qspi->base[CHIP_SELECT])) {
+                     ret = PTR_ERR(qspi->base[CHIP_SELECT]);
+                     goto err2;
+             }
+     }

As with the interrupt controller is this really part of the IP rather than just a GPIO block?

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

Quoted 5 lines

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

+     qspi->clk = devm_clk_get(&pdev->dev, NULL);
+     if (IS_ERR(qspi->clk)) {
+             dev_warn(dev, "unable to get clock, using defaults\n");
+             qspi->clk = NULL;
+     }

This is broken - it's just ignoring errors. That's going to be broken for probe deferral or any other case where a clock is specified but fails to be found for some reason. Given that this is a completely new binding and it's trivial to specify fixed clocks I can see no reason why we'd even want to support missing clocks, it's going to be much more robust to just require that the DT specifies the clock.

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

Quoted 3 lines

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

+     bcm_qspi_hw_init(qspi);
+     init_completion(&qspi->mspi_done);
+     init_completion(&qspi->bspi_done);

Can we have mspi and bspi simultaneously?

Quoted 5 lines

All this interrupt code and especially the fact that it's a completely separate register range in the binding looks very much like it's just an interrupt controller IP that's not particularly anything to do with the SPI controller and should therefore be in a separate driver. Why is this part of the SPI controller driver?

Some SoCs need this since they do not implement a separate interrupt controller and have dedicated l1 interrupt for spi. Also the handling is not generic enough to cover other ips as well in those case. Hence have to handle it within the driver.

Quoted 5 lines

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

Quoted 3 lines

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

+     default:
+             break;
+     }

We just ignore other widths?

These are the only supported widths will make SPI_NBITS_SINGLE default.

Quoted 10 lines · Kamal Dasu

Quoted 5 lines

All this interrupt code and especially the fact that it's a completely separate register range in the binding looks very much like it's just an interrupt controller IP that's not particularly anything to do with the SPI controller and should therefore be in a separate driver. Why is this part of the SPI controller driver?

Some SoCs need this since they do not implement a separate interrupt controller and have dedicated l1 interrupt for spi. Also the handling is not generic enough to cover other ips as well in those case. Hence have to handle it within the driver.

That doesn't seem to match what the code is actually doing. The register block this is controlling is separate to the rest of the IP. It's perfectly OK to have a driver for an interrupt controller which is only used in one place, though you may find one of the generic ones might be able to handle it anyway.

The NS* SoC SPI blocks has hw glue logic specific to MSPI+BSPI, so the re-usability of an interrupt controller driver for this logic is limited. The spi only (spi protocol) block does use l2 controller, however for the MSPI+BSPI block used for the spi-nor flash had to introduce the code to handle l1 interrupts within the code itself. In this case can the driver be accepted without having to introduce a new intc driver ?.

Quoted 7 lines · Kamal Dasu

The NS* SoC SPI blocks has hw glue logic specific to MSPI+BSPI, so the re-usability of an interrupt controller driver for this logic is limited. The spi only (spi protocol) block does use l2 controller, however for the MSPI+BSPI block used for the spi-nor flash had to introduce the code to handle l1 interrupts within the code itself. In this case can the driver be accepted without having to introduce a new intc driver ?.

It's not really about reuse, it's about maintainability. All the code for handling this within the driver is quite unusual and makes the driver harder to understand and review. That's going to have an impact now and make things harder to follow in future too. Fitting in with the frameworks means that we get the benefit of the structure and support code that the frameworks provide while minimizing the amount of unusal code in the driver.

Quoted 7 lines · Mark Brown

It's not really about reuse, it's about maintainability. All the code for handling this within the driver is quite unusual and makes the driver harder to understand and review. That's going to have an impact now and make things harder to follow in future too. Fitting in with the frameworks means that we get the benefit of the structure and support code that the frameworks provide while minimizing the amount of unusal code in the driver.

This is not unusual at all, there are tons of peripherals that embed a L2/L3 interrupt controller, yet keep the code localized because the interrupt sources are not visible outside of the specific block and it would not make sense to demultiplex these different interrupt sources as individually requestable interrupt lines when the consumer is also local and self contained.

This is exactly what is going on here, and it this makes the driver more self contained without external dependencies to an irqchip driver to an interrupt controller provider.

The reusability argument is kind of the only one that makes sense here, maintaining two drivers instead of one seems like more maintenance burden imho.

Quoted 10 lines · Florian Fainelli

Quoted 7 lines · Mark Brown

It's not really about reuse, it's about maintainability. All the code for handling this within the driver is quite unusual and makes the driver harder to understand and review. That's going to have an impact now and make things harder to follow in future too. Fitting in with the frameworks means that we get the benefit of the structure and support code that the frameworks provide while minimizing the amount of unusal code in the driver.

Quoted 6 lines

This is not unusual at all, there are tons of peripherals that embed a L2/L3 interrupt controller, yet keep the code localized because the interrupt sources are not visible outside of the specific block and it would not make sense to demultiplex these different interrupt sources as individually requestable interrupt lines when the consumer is also local and self contained.

If it was unconditionally in the perhipheral it'd be fairly normal and standard but it's not, it's a completely separate and optional register block with a bunch of separate code in a driver which already has above average complexity even for the bits that belong in it.

Quoted 4 lines

This is exactly what is going on here, and it this makes the driver more self contained without external dependencies to an irqchip driver to an interrupt controller provider.

Like I say it just isn't, it's an obviously separate interrupt controller.

Quoted 4 lines

The reusability argument is kind of the only one that makes sense here, maintaining two drivers instead of one seems like more maintenance burden imho.

It's definitely adding to the review complexity as things stand mainly due to the obvious disconnect from the actual IP. It should take very little time to refactor and it's really hard to see it ever taking much time to maintain in future, though it might be helpful if one of your hardware engineers ever tries to reuse that IP.

Quoted 4 lines · Mark Brown

If it was unconditionally in the perhipheral it'd be fairly normal and standard but it's not, it's a completely separate and optional register block with a bunch of separate code in a driver which already has above average complexity even for the bits that belong in it.

It is not separate nor optional, the fact that the register range seems discontiguous is just an integration choice here, but it contains bits that are relevant exclusively to the SPI controller.

It is not exclusively an interrupt controller, there are other bits in there are do not functionally belong into an interrupt controller driver per-se, things like a clock enable, arbiter control and a random bit to control MSPI flush behavior.

Oh, and please don't tell me regmap is the solution here if we need to control both the interrupt-related bits and the other bits within this register.

Considering that this block aggregates interrupt enable bits, but not just, there is no reason for this block to change over time, and clearly this won't be handled via a generic interrupt controller driver either.

Quoted 6 lines · Florian Fainelli

Quoted 4 lines · Mark Brown

If it was unconditionally in the perhipheral it'd be fairly normal and standard but it's not, it's a completely separate and optional register block with a bunch of separate code in a driver which already has above average complexity even for the bits that belong in it.

Quoted 3 lines

It is not separate nor optional, the fact that the register range seems discontiguous is just an integration choice here, but it contains bits that are relevant exclusively to the SPI controller.

Some versions of the block have it, some versions of the block don't have it. That suggests that it's optional. It is a completely separate register map, that suggests that it is an external block.

Quoted 5 lines

It is not exclusively an interrupt controller, there are other bits in there are do not functionally belong into an interrupt controller driver per-se, things like a clock enable, arbiter control and a random bit to control MSPI flush behavior.

Quoted 3 lines

Oh, and please don't tell me regmap is the solution here if we need to control both the interrupt-related bits and the other bits within this register.

That does sound system controllerish if those bits do need to be managed. Are we sure future designs won't just integrate this into the main system controller?

Quoted 4 lines

Considering that this block aggregates interrupt enable bits, but not just, there is no reason for this block to change over time, and clearly this won't be handled via a generic interrupt controller driver either.

There must be *some* logic behind the way the hardware has been structured, and right now the code just isn't making any of this apparent which makes it look like it needs refactoring.

Now, as to whether it makes sense to model the IDM enable/disable (intr_regs resource) and the 7 32-bits interrupt status/acknowledge words at the end of the MSPI+BSPI block (intr_status_regs resource) as an interrupt controller makes sense or not, it is kind of hard to say, because really the IDM IO_CONTROL aggregates more than just interrupt enable/disable bits here, but the overall ownership of this IDM IO_CONTROL is clear and it belongs to the SPI function of the system.

Quoted 7 lines · Florian Fainelli

Now, as to whether it makes sense to model the IDM enable/disable (intr_regs resource) and the 7 32-bits interrupt status/acknowledge words at the end of the MSPI+BSPI block (intr_status_regs resource) as an interrupt controller makes sense or not, it is kind of hard to say, because really the IDM IO_CONTROL aggregates more than just interrupt enable/disable bits here, but the overall ownership of this IDM IO_CONTROL is clear and it belongs to the SPI function of the system.

TBH if that's all it's doing then I'm surprised it's not simple to handle it with irq_setup_generic_chip() - have you looked at that at all?

Quoted 3 lines · Mark Brown

TBH if that's all it's doing then I'm surprised it's not simple to handle it with irq_setup_generic_chip() - have you looked at that at all?

The generic IRQ chip model assumes that the mask/unmask/ack logic will operate against the same register base address, which won't be the case for the NS/NSP/NS2 platforms where the mask/unmask is in one register set (coined IDM), and the ack is in another one (actually 7 * 32-bits words).

Quoted 6 lines · Florian Fainelli

Quoted 4 lines · Mark Brown

If it was unconditionally in the perhipheral it'd be fairly normal and standard but it's not, it's a completely separate and optional register block with a bunch of separate code in a driver which already has above average complexity even for the bits that belong in it.

Quoted 3 lines

It is not separate nor optional, the fact that the register range seems discontiguous is just an integration choice here, but it contains bits that are relevant exclusively to the SPI controller.

Some versions of the block have it, some versions of the block don't have it. That suggests that it's optional. It is a completely separate register map, that suggests that it is an external block.

Mark, would you oppose to a driver structure that looks like the Broadcom NAND driver under drivers/mtd/nand/brcmnand/, where we have something like this:

- the core driver only deals with individual interrupt lines which are available in the original version of the IP block as integrated on Set-top-box/Cable Modem chips

Mark, would you oppose to a driver structure that looks like the Broadcom NAND driver under drivers/mtd/nand/brcmnand/, where we have something like this:

- the core driver only deals with individual interrupt lines which are available in the original version of the IP block as integrated on Set-top-box/Cable Modem chips

- the core driver allows for callbacks to perform additional interupt work when that is required for the iProc (Northstar, Northstar Plus, Northstar 2) and DSL SoCs where we have additional logic to enable/disable/acknowledge interrupt lines

In m25p80_read() even though spi_flash_read() is supported by some drivers, under certain circumstances like unaligned buffer, address or address range limitations on certain SoCs let it fallback to core spi reads. Such drivers are expected to return -EAGAIN so that the m25p80_read() uses standard spi transfer.

Added device tree bindings documentation for SoCs supported by the new spi-bcm-qspi driver.

Quoted 2 lines · Kamal Dasu

Added device tree bindings documentation for SoCs supported by the new spi-bcm-qspi driver.

To repeat what I said on your previous version:

| So this is a perfect example of why you should use standard formats for
| subject lines, if things don't look relevant they're likely to get
| missed.  In this case it's both the prefix and the fact that the version
| is added in a weird way.  Frankly I didn't even notice that it wasn't a
| cover letter.

Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed.

Documentation/devicetree/bindings/spi/brcm,spi-bcm-qspi.txt line 17

Quoted 4 lines

+- compatible:

Documentation/devicetree/bindings/spi/brcm,spi-bcm-qspi.txt new file +3 -0

+    Must be one of :
+    "brcm,spi-bcm-qspi"
+    "brcm,spi-brmstb" spi-nor and/or "brcm,spi-brmstb-mspi" unmanaged SPI Master

What do these compatible strings mean, what are the differences between them? The last two sound like configuration of a single IP rather than descriptions of hardware.

Quoted 4 lines · Mark Brown

Quoted 4 lines

+- compatible:

Documentation/devicetree/bindings/spi/brcm,spi-bcm-qspi.txt new file +3 -0

+    Must be one of :
+    "brcm,spi-bcm-qspi"
+    "brcm,spi-brmstb" spi-nor and/or "brcm,spi-brmstb-mspi" unmanaged SPI Master

Quoted 3 lines

What do these compatible strings mean, what are the differences between them? The last two sound like configuration of a single IP rather than descriptions of hardware.

The Broadcom STB SoCs have two instances of this SPI HW block, one which is MSPI+BSPI capable and another one which is MSPI only (or that's how we call them), the difference in the compatible strings tries to denote and capture that.