[PATCH 0/5] Improvements to spi-bcm-qspi

[PATCH 0/5] Improvements to spi-bcm-qspi

[Mark Tomlinson]

This series of patches came from a single large Broadcom patch that
implements drivers for a number of their integrated switch chips. Mostly
this is just splitting the qspi driver into smaller parts and doesn't
include much original from me.

Mark Tomlinson (5):
  spi: bcm-qspi: Add support for setting BSPI clock
  spi: bcm-qspi: Improve debug reading SPI data
  spi: bcm-qspi: Do not split transfers into small chunks
  spi: bcm-qspi: Make multiple data blocks interrupt-driven
  spi: bcm-qspi: Improve interrupt handling

 drivers/spi/spi-bcm-qspi.c | 189 ++++++++++++++++++++++---------------
 drivers/spi/spi-bcm-qspi.h |   5 +-
 2 files changed, 115 insertions(+), 79 deletions(-)

-- 
2.27.0

[PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock

[Mark Tomlinson]

On iProc devices (unlike previous BCM SoCs) the clock rate of the SPI
can be set. This patch adds the appropriate code for setting that.

Reviewed-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 681d09085175..8fc5b9b3757b 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -117,6 +117,13 @@
 
 #define MSPI_MSPI_STATUS_SPIF			BIT(0)
 
+#define CRU_CTRL_REG				0x0
+#define QSPI_CLK_SEL_25M			0x00
+#define QSPI_CLK_SEL_50M			0x02
+#define QSPI_CLK_SEL_31M25			0x04
+#define QSPI_CLK_SEL_62M5			0x06
+#define QSPI_CLK_SEL_MASK			0x06
+
 #define INTR_BASE_BIT_SHIFT			0x02
 #define INTR_COUNT				0x07
 
@@ -170,6 +177,7 @@ enum base_type {
 	MSPI,
 	BSPI,
 	CHIP_SELECT,
+	CRU_CTRL,
 	BASEMAX,
 };
 
@@ -625,6 +633,7 @@ static void bcm_qspi_update_parms(struct bcm_qspi *qspi,
 static int bcm_qspi_setup(struct spi_device *spi)
 {
 	struct bcm_qspi_parms *xp;
+	struct bcm_qspi *qspi = spi_master_get_devdata(spi->master);
 
 	if (spi->bits_per_word > 16)
 		return -EINVAL;
@@ -639,6 +648,21 @@ static int bcm_qspi_setup(struct spi_device *spi)
 	xp->speed_hz = spi->max_speed_hz;
 	xp->mode = spi->mode;
 
+	if (qspi->base[CRU_CTRL]) {
+		u32 tmp = bcm_qspi_read(qspi, CRU_CTRL, CRU_CTRL_REG);
+
+		/* Set BSPI clock rate */
+		tmp &= ~QSPI_CLK_SEL_MASK;
+		if (spi->max_speed_hz >= 62500000)
+			tmp |= QSPI_CLK_SEL_62M5;
+		else if (spi->max_speed_hz >= 50000000)
+			tmp |= QSPI_CLK_SEL_50M;
+		else if (spi->max_speed_hz >= 31250000)
+			tmp |= QSPI_CLK_SEL_31M25;
+		/* default is 25MHz */
+		bcm_qspi_write(qspi, CRU_CTRL, CRU_CTRL_REG, tmp);
+	}
+
 	if (spi->bits_per_word)
 		xp->bits_per_word = spi->bits_per_word;
 	else
@@ -1459,6 +1483,16 @@ int bcm_qspi_probe(struct platform_device *pdev,
 		qspi->soc_intc = NULL;
 	}
 
+	/* iProc BSPI clock is set through CRU control */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cru_ctrl");
+	if (res) {
+		qspi->base[CRU_CTRL] = devm_ioremap_resource(dev, res);
+		if (IS_ERR(qspi->base[CRU_CTRL])) {
+			ret = PTR_ERR(qspi->base[CRU_CTRL]);
+			goto qspi_probe_err;
+		}
+	}
+
 	ret = clk_prepare_enable(qspi->clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
-- 
2.27.0

[PATCH 2/5] spi: bcm-qspi: Improve debug reading SPI data

[Mark Tomlinson]

This patch prevents device debug when data is not read from hardware
(i.e. when there is no receive buffer).

Reviewed-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/spi/spi-bcm-qspi.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 8fc5b9b3757b..92e04d24359f 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -749,21 +749,22 @@ static void read_from_hw(struct bcm_qspi *qspi, int slots)
 	tp = qspi->trans_pos;
 
 	for (slot = 0; slot < slots; slot++) {
-		if (tp.trans->bits_per_word <= 8) {
-			u8 *buf = tp.trans->rx_buf;
-
-			if (buf)
-				buf[tp.byte] = read_rxram_slot_u8(qspi, slot);
-			dev_dbg(&qspi->pdev->dev, "RD %02x\n",
-				buf ? buf[tp.byte] : 0x0);
-		} else {
-			u16 *buf = tp.trans->rx_buf;
-
-			if (buf)
-				buf[tp.byte / 2] = read_rxram_slot_u16(qspi,
-								      slot);
-			dev_dbg(&qspi->pdev->dev, "RD %04x\n",
-				buf ? buf[tp.byte / 2] : 0x0);
+		if (tp.trans->rx_buf) {
+			if (tp.trans->bits_per_word <= 8) {
+				u8 *buf = tp.trans->rx_buf;
+
+				buf[tp.byte] =
+					read_rxram_slot_u8(qspi, slot);
+				dev_dbg(&qspi->pdev->dev, "RD %02x\n",
+					buf[tp.byte]);
+			} else {
+				u16 *buf = tp.trans->rx_buf;
+
+				buf[tp.byte / 2] =
+					read_rxram_slot_u16(qspi, slot);
+				dev_dbg(&qspi->pdev->dev, "RD %04x\n",
+					buf[tp.byte / 2]);
+			}
 		}
 
 		update_qspi_trans_byte_count(qspi, &tp,
-- 
2.27.0

[PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks

[Mark Tomlinson]

Instead of splitting transfers into smaller parts, just perform the
operation that the higher level asked for.

Reviewed-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/spi/spi-bcm-qspi.c | 69 +++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 92e04d24359f..ce30ebf27f06 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -78,8 +78,6 @@
 #define BSPI_BPP_MODE_SELECT_MASK		BIT(8)
 #define BSPI_BPP_ADDR_SELECT_MASK		BIT(16)
 
-#define BSPI_READ_LENGTH			256
-
 /* MSPI register offsets */
 #define MSPI_SPCR0_LSB				0x000
 #define MSPI_SPCR0_MSB				0x004
@@ -888,9 +886,9 @@ static int bcm_qspi_bspi_exec_mem_op(struct spi_device *spi,
 				     const struct spi_mem_op *op)
 {
 	struct bcm_qspi *qspi = spi_master_get_devdata(spi->master);
-	u32 addr = 0, len, rdlen, len_words, from = 0;
+	u32 addr = 0, len, len_words, from = 0;
 	int ret = 0;
-	unsigned long timeo = msecs_to_jiffies(100);
+	unsigned long timeo = msecs_to_jiffies(1500);
 	struct bcm_qspi_soc_intc *soc_intc = qspi->soc_intc;
 
 	if (bcm_qspi_bspi_ver_three(qspi))
@@ -925,47 +923,34 @@ static int bcm_qspi_bspi_exec_mem_op(struct spi_device *spi,
 	 * into RAF buffer read lengths
 	 */
 	len = op->data.nbytes;
+	reinit_completion(&qspi->bspi_done);
+	bcm_qspi_enable_bspi(qspi);
+	len_words = (len + 3) >> 2;
+	qspi->bspi_rf_op = op;
+	qspi->bspi_rf_op_status = 0;
 	qspi->bspi_rf_op_idx = 0;
+	qspi->bspi_rf_op_len = len;
+	dev_dbg(&qspi->pdev->dev, "bspi xfr addr 0x%x len 0x%x", addr, len);
 
-	do {
-		if (len > BSPI_READ_LENGTH)
-			rdlen = BSPI_READ_LENGTH;
-		else
-			rdlen = len;
-
-		reinit_completion(&qspi->bspi_done);
-		bcm_qspi_enable_bspi(qspi);
-		len_words = (rdlen + 3) >> 2;
-		qspi->bspi_rf_op = op;
-		qspi->bspi_rf_op_status = 0;
-		qspi->bspi_rf_op_len = rdlen;
-		dev_dbg(&qspi->pdev->dev,
-			"bspi xfr addr 0x%x len 0x%x", addr, rdlen);
-		bcm_qspi_write(qspi, BSPI, BSPI_RAF_START_ADDR, addr);
-		bcm_qspi_write(qspi, BSPI, BSPI_RAF_NUM_WORDS, len_words);
-		bcm_qspi_write(qspi, BSPI, BSPI_RAF_WATERMARK, 0);
-		if (qspi->soc_intc) {
-			/*
-			 * clear soc MSPI and BSPI interrupts and enable
-			 * BSPI interrupts.
-			 */
-			soc_intc->bcm_qspi_int_ack(soc_intc, MSPI_BSPI_DONE);
-			soc_intc->bcm_qspi_int_set(soc_intc, BSPI_DONE, true);
-		}
-
-		/* Must flush previous writes before starting BSPI operation */
-		mb();
-		bcm_qspi_bspi_lr_start(qspi);
-		if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) {
-			dev_err(&qspi->pdev->dev, "timeout waiting for BSPI\n");
-			ret = -ETIMEDOUT;
-			break;
-		}
+	bcm_qspi_write(qspi, BSPI, BSPI_RAF_START_ADDR, addr);
+	bcm_qspi_write(qspi, BSPI, BSPI_RAF_NUM_WORDS, len_words);
+	bcm_qspi_write(qspi, BSPI, BSPI_RAF_WATERMARK, 0);
+	if (qspi->soc_intc) {
+		/*
+		 * clear soc MSPI and BSPI interrupts and enable
+		 * BSPI interrupts.
+		 */
+		soc_intc->bcm_qspi_int_ack(soc_intc, MSPI_BSPI_DONE);
+		soc_intc->bcm_qspi_int_set(soc_intc, BSPI_DONE, true);
+	}
 
-		/* set msg return length */
-		addr += rdlen;
-		len -= rdlen;
-	} while (len);
+	/* Must flush previous writes before starting BSPI operation */
+	mb();
+	bcm_qspi_bspi_lr_start(qspi);
+	if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) {
+		dev_err(&qspi->pdev->dev, "timeout waiting for BSPI\n");
+		ret = -ETIMEDOUT;
+	}
 
 	return ret;
 }
-- 
2.27.0

[PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven

[Mark Tomlinson]

When needing to send/receive data in small chunks, make this interrupt
driven rather than waiting for a completion event for each small section
of data.

Reviewed-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/spi/spi-bcm-qspi.c | 44 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index ce30ebf27f06..0cc51bcda300 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -200,12 +200,14 @@ struct bcm_qspi_dev_id {
 struct qspi_trans {
 	struct spi_transfer *trans;
 	int byte;
+	int slots;
 	bool mspi_last_trans;
 };
 
 struct bcm_qspi {
 	struct platform_device *pdev;
 	struct spi_master *master;
+	struct spi_device *spi_dev;
 	struct clk *clk;
 	u32 base_clk;
 	u32 max_speed_hz;
@@ -731,12 +733,14 @@ static inline u16 read_rxram_slot_u16(struct bcm_qspi *qspi, int slot)
 		((bcm_qspi_read(qspi, MSPI, msb_offset) & 0xff) << 8);
 }
 
-static void read_from_hw(struct bcm_qspi *qspi, int slots)
+static void read_from_hw(struct bcm_qspi *qspi)
 {
 	struct qspi_trans tp;
-	int slot;
+	int slot, slots;
 
 	bcm_qspi_disable_bspi(qspi);
+	tp = qspi->trans_pos;
+	slots = tp.slots;
 
 	if (slots > MSPI_NUM_CDRAM) {
 		/* should never happen */
@@ -744,8 +748,6 @@ static void read_from_hw(struct bcm_qspi *qspi, int slots)
 		return;
 	}
 
-	tp = qspi->trans_pos;
-
 	for (slot = 0; slot < slots; slot++) {
 		if (tp.trans->rx_buf) {
 			if (tp.trans->bits_per_word <= 8) {
@@ -803,11 +805,12 @@ static inline void write_cdram_slot(struct bcm_qspi *qspi, int slot, u32 val)
 }
 
 /* Return number of slots written */
-static int write_to_hw(struct bcm_qspi *qspi, struct spi_device *spi)
+static int write_to_hw(struct bcm_qspi *qspi)
 {
 	struct qspi_trans tp;
 	int slot = 0, tstatus = 0;
 	u32 mspi_cdram = 0;
+	struct spi_device *spi = qspi->spi_dev;
 
 	bcm_qspi_disable_bspi(qspi);
 	tp = qspi->trans_pos;
@@ -846,6 +849,9 @@ static int write_to_hw(struct bcm_qspi *qspi, struct spi_device *spi)
 		slot++;
 	}
 
+	/* save slot number for read_from_hw() */
+	qspi->trans_pos.slots = slot;
+
 	if (!slot) {
 		dev_err(&qspi->pdev->dev, "%s: no data to send?", __func__);
 		goto done;
@@ -960,24 +966,21 @@ static int bcm_qspi_transfer_one(struct spi_master *master,
 				 struct spi_transfer *trans)
 {
 	struct bcm_qspi *qspi = spi_master_get_devdata(master);
-	int slots;
-	unsigned long timeo = msecs_to_jiffies(100);
+	unsigned long timeo = msecs_to_jiffies(1000);
 
 	if (!spi->cs_gpiod)
 		bcm_qspi_chip_select(qspi, spi->chip_select);
 	qspi->trans_pos.trans = trans;
 	qspi->trans_pos.byte = 0;
+	qspi->spi_dev = spi;
 
-	while (qspi->trans_pos.byte < trans->len) {
-		reinit_completion(&qspi->mspi_done);
+	reinit_completion(&qspi->mspi_done);
 
-		slots = write_to_hw(qspi, spi);
-		if (!wait_for_completion_timeout(&qspi->mspi_done, timeo)) {
-			dev_err(&qspi->pdev->dev, "timeout waiting for MSPI\n");
-			return -ETIMEDOUT;
-		}
+	write_to_hw(qspi);
 
-		read_from_hw(qspi, slots);
+	if (!wait_for_completion_timeout(&qspi->mspi_done, timeo)) {
+		dev_err(&qspi->pdev->dev, "timeout waiting for MSPI\n");
+		return -ETIMEDOUT;
 	}
 	bcm_qspi_enable_bspi(qspi);
 
@@ -1092,7 +1095,16 @@ static irqreturn_t bcm_qspi_mspi_l2_isr(int irq, void *dev_id)
 		bcm_qspi_write(qspi, MSPI, MSPI_MSPI_STATUS, status);
 		if (qspi->soc_intc)
 			soc_intc->bcm_qspi_int_ack(soc_intc, MSPI_DONE);
-		complete(&qspi->mspi_done);
+
+		read_from_hw(qspi);
+
+		if (qspi->trans_pos.trans) {
+			write_to_hw(qspi);
+		} else {
+			complete(&qspi->mspi_done);
+			spi_finalize_current_transfer(qspi->master);
+		}
+
 		return IRQ_HANDLED;
 	}
 
-- 
2.27.0

[PATCH 5/5] spi: bcm-qspi: Improve interrupt handling

[Mark Tomlinson]

Acknowledge interrupts correctly and add support for fifo-full
interrupt, distinguishing it from the done interrupt.

Reviewed-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/spi/spi-bcm-qspi.c | 11 ++++++-----
 drivers/spi/spi-bcm-qspi.h |  5 ++++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 0cc51bcda300..3753eff8a154 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -1123,6 +1123,8 @@ static irqreturn_t bcm_qspi_bspi_lr_l2_isr(int irq, void *dev_id)
 		if (qspi->bspi_rf_op_len == 0) {
 			qspi->bspi_rf_op = NULL;
 			if (qspi->soc_intc) {
+				/* Ack BSPI done interrupt */
+				soc_intc->bcm_qspi_int_ack(soc_intc, BSPI_DONE);
 				/* disable soc BSPI interrupt */
 				soc_intc->bcm_qspi_int_set(soc_intc, BSPI_DONE,
 							   false);
@@ -1134,11 +1136,10 @@ static irqreturn_t bcm_qspi_bspi_lr_l2_isr(int irq, void *dev_id)
 				bcm_qspi_bspi_lr_clear(qspi);
 			else
 				bcm_qspi_bspi_flush_prefetch_buffers(qspi);
-		}
-
-		if (qspi->soc_intc)
-			/* clear soc BSPI interrupt */
-			soc_intc->bcm_qspi_int_ack(soc_intc, BSPI_DONE);
+		} else  if (qspi->soc_intc)
+			/* Ack FIFO full interrupt */
+			soc_intc->bcm_qspi_int_ack(soc_intc,
+						   BSPI_FIFO_FULL);
 	}
 
 	status &= INTR_BSPI_LR_SESSION_DONE_MASK;
diff --git a/drivers/spi/spi-bcm-qspi.h b/drivers/spi/spi-bcm-qspi.h
index 01aec6460108..b68e275bc721 100644
--- a/drivers/spi/spi-bcm-qspi.h
+++ b/drivers/spi/spi-bcm-qspi.h
@@ -48,7 +48,8 @@ enum {
 	MSPI_DONE = 0x1,
 	BSPI_DONE = 0x2,
 	BSPI_ERR = 0x4,
-	MSPI_BSPI_DONE = 0x7
+	MSPI_BSPI_DONE = 0x7,
+	BSPI_FIFO_FULL = 0x8
 };
 
 struct bcm_qspi_soc_intc {
@@ -84,6 +85,8 @@ static inline u32 get_qspi_mask(int type)
 		return INTR_MSPI_DONE_MASK;
 	case BSPI_DONE:
 		return BSPI_LR_INTERRUPTS_ALL;
+	case BSPI_FIFO_FULL:
+		return INTR_BSPI_LR_FULLNESS_REACHED_MASK;
 	case MSPI_BSPI_DONE:
 		return QSPI_INTERRUPTS_ALL;
 	case BSPI_ERR:
-- 
2.27.0

Re: [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Mon, Jun 15, 2020 at 04:05:55PM +1200, Mark Tomlinson wrote:
> Instead of splitting transfers into smaller parts, just perform the
> operation that the higher level asked for.

I don't understand this change - presumably there was some reason for
splitting the transfers into smaller chunks (issues keeping up with
FIFOs or something)?  How is whatever that reason is handled?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]

On Mon, Jun 15, 2020 at 04:05:56PM +1200, Mark Tomlinson wrote:

> When needing to send/receive data in small chunks, make this interrupt
> driven rather than waiting for a completion event for each small section
> of data.

Again was this done for a reason and if so do we understand why doing
this from interrupt context is safe - how long can the interrupts be
when stuffing the FIFO from interrupt context?

> @@ -731,12 +733,14 @@ static inline u16 read_rxram_slot_u16(struct bcm_qspi *qspi, int slot)
>  		((bcm_qspi_read(qspi, MSPI, msb_offset) & 0xff) << 8);
>  }
>  
> -static void read_from_hw(struct bcm_qspi *qspi, int slots)
> +static void read_from_hw(struct bcm_qspi *qspi)
>  {

Things might be clearer if this refactoring were split out into a
separate patch.

> @@ -960,24 +966,21 @@ static int bcm_qspi_transfer_one(struct spi_master *master,
>  				 struct spi_transfer *trans)
>  {
>  	struct bcm_qspi *qspi = spi_master_get_devdata(master);
> -	int slots;
> -	unsigned long timeo = msecs_to_jiffies(100);
> +	unsigned long timeo = msecs_to_jiffies(1000);

That's a randomly chosen value - if we're now doing the entire transfer
then we should be trying to estimate the length of time the transfer
will take, for a very large transfer on a slow bus it's possible that
even a second won't be enough.

> -		complete(&qspi->mspi_done);
> +
> +		read_from_hw(qspi);
> +
> +		if (qspi->trans_pos.trans) {
> +			write_to_hw(qspi);
> +		} else {
> +			complete(&qspi->mspi_done);
> +			spi_finalize_current_transfer(qspi->master);
> +		}
> +

This is adding a spi_finalize_current_transfer() which we didn't have
before, and still leaving us doing cleanup work in the driver in another
thread.  This is confused, the driver should only need to finalize the
transfer explicitly if it returned a timeout from transfer_one() but
nothing's changed there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] Improvements to spi-bcm-qspi

[Kamal Dasu]

Mark,

This block is used on multiple Broadcom SoCs and would like to get
comments from all who deal with iProc and have touched this file as
well.
Please copy :
Florian Fainelli <f.fainelli@gmail.com>
Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
and
bcm-kernel-feedback-list@broadcom.com (maintainer:BROADCOM SPI DRIVER)

Kamal

On Mon, Jun 15, 2020 at 12:06 AM Mark Tomlinson
<mark.tomlinson@alliedtelesis.co.nz> wrote:
>
> This series of patches came from a single large Broadcom patch that
> implements drivers for a number of their integrated switch chips. Mostly
> this is just splitting the qspi driver into smaller parts and doesn't
> include much original from me.
>
> Mark Tomlinson (5):
>   spi: bcm-qspi: Add support for setting BSPI clock
>   spi: bcm-qspi: Improve debug reading SPI data
>   spi: bcm-qspi: Do not split transfers into small chunks
>   spi: bcm-qspi: Make multiple data blocks interrupt-driven
>   spi: bcm-qspi: Improve interrupt handling
>
>  drivers/spi/spi-bcm-qspi.c | 189 ++++++++++++++++++++++---------------
>  drivers/spi/spi-bcm-qspi.h |   5 +-
>  2 files changed, 115 insertions(+), 79 deletions(-)
>
> --
> 2.27.0
>

Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven

[Mark Tomlinson]

On Mon, 2020-06-15 at 15:32 +0100, Mark Brown wrote:
> On Mon, Jun 15, 2020 at 04:05:56PM +1200, Mark Tomlinson wrote:
> 
> > When needing to send/receive data in small chunks, make this interrupt
> > driven rather than waiting for a completion event for each small section
> > of data.
> 
> Again was this done for a reason and if so do we understand why doing
> this from interrupt context is safe - how long can the interrupts be
> when stuffing the FIFO from interrupt context?

As I'm porting a Broadcom patch, I'm hoping someone else can add
something to this. From the history it appears there was a hard limit
(no small chunks), and this was changed to doing it in chunks with
patch 345309fa7c0c92, apparently to improve performance. I believe this
change further improves performance, but as the patch arrived without
any documentation, I'm not certain.


> > @@ -731,12 +733,14 @@ static inline u16 read_rxram_slot_u16(struct bcm_qspi *qspi, int slot)
> >  		((bcm_qspi_read(qspi, MSPI, msb_offset) & 0xff) << 8);
> >  }
> >  
> > -static void read_from_hw(struct bcm_qspi *qspi, int slots)
> > +static void read_from_hw(struct bcm_qspi *qspi)
> >  {
> 
> Things might be clearer if this refactoring were split out into a
> separate patch.

Done.

> > @@ -960,24 +966,21 @@ static int bcm_qspi_transfer_one(struct spi_master *master,
> >  				 struct spi_transfer *trans)
> >  {
> >  	struct bcm_qspi *qspi = spi_master_get_devdata(master);
> > -	int slots;
> > -	unsigned long timeo = msecs_to_jiffies(100);
> > +	unsigned long timeo = msecs_to_jiffies(1000);
> 
> That's a randomly chosen value - if we're now doing the entire transfer
> then we should be trying to estimate the length of time the transfer
> will take, for a very large transfer on a slow bus it's possible that
> even a second won't be enough.
> 
Again, the value came from Broadcom. Using the data length as an
estimate sounds like a good idea.

> > -		complete(&qspi->mspi_done);
> > +
> > +		read_from_hw(qspi);
> > +
> > +		if (qspi->trans_pos.trans) {
> > +			write_to_hw(qspi);
> > +		} else {
> > +			complete(&qspi->mspi_done);
> > +			spi_finalize_current_transfer(qspi->master);
> > +		}
> > +
> 
> This is adding a spi_finalize_current_transfer() which we didn't have
> before, and still leaving us doing cleanup work in the driver in another
> thread.  This is confused, the driver should only need to finalize the
> transfer explicitly if it returned a timeout from transfer_one() but
> nothing's changed there.

I can remove the call to spi_finalize_current_transfer() from this
patch. I'll try to check what does happen in the timeout case.

Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On Tue, Jun 16, 2020 at 03:07:17AM +0000, Mark Tomlinson wrote:
> On Mon, 2020-06-15 at 15:32 +0100, Mark Brown wrote:

> > Again was this done for a reason and if so do we understand why doing
> > this from interrupt context is safe - how long can the interrupts be
> > when stuffing the FIFO from interrupt context?

> As I'm porting a Broadcom patch, I'm hoping someone else can add
> something to this. From the history it appears there was a hard limit

If you didn't write this code then it should have at least one signoff
from the original source, I can't do anything with this without signoffs
- please see Documentation/process/submitting-patches.rst for what this
is and why it's important.

> (no small chunks), and this was changed to doing it in chunks with
> patch 345309fa7c0c92, apparently to improve performance. I believe this
> change further improves performance, but as the patch arrived without
> any documentation, I'm not certain.

Have you tested the impact on performance?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]