linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Improvements to spi-bcm-qspi
@ 2020-06-15  4:05 Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock Mark Tomlinson
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
@ 2020-06-15  4:05 ` Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 2/5] spi: bcm-qspi: Improve debug reading SPI data Mark Tomlinson
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/5] spi: bcm-qspi: Improve debug reading SPI data
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock Mark Tomlinson
@ 2020-06-15  4:05 ` Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks Mark Tomlinson
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock Mark Tomlinson
  2020-06-15  4:05 ` [PATCH 2/5] spi: bcm-qspi: Improve debug reading SPI data Mark Tomlinson
@ 2020-06-15  4:05 ` Mark Tomlinson
  2020-06-15 13:31   ` Mark Brown
  2020-06-15  4:05 ` [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven Mark Tomlinson
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
                   ` (2 preceding siblings ...)
  2020-06-15  4:05 ` [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks Mark Tomlinson
@ 2020-06-15  4:05 ` Mark Tomlinson
  2020-06-15 14:32   ` Mark Brown
  2020-06-15  4:05 ` [PATCH 5/5] spi: bcm-qspi: Improve interrupt handling Mark Tomlinson
  2020-06-15 19:05 ` [PATCH 0/5] Improvements to spi-bcm-qspi Kamal Dasu
  5 siblings, 1 reply; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 5/5] spi: bcm-qspi: Improve interrupt handling
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
                   ` (3 preceding siblings ...)
  2020-06-15  4:05 ` [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven Mark Tomlinson
@ 2020-06-15  4:05 ` Mark Tomlinson
  2020-06-15 19:05 ` [PATCH 0/5] Improvements to spi-bcm-qspi Kamal Dasu
  5 siblings, 0 replies; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-15  4:05 UTC (permalink / raw)
  To: broonie, kdasu.kdev; +Cc: linux-kernel, linux-spi, 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks
  2020-06-15  4:05 ` [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks Mark Tomlinson
@ 2020-06-15 13:31   ` Mark Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2020-06-15 13:31 UTC (permalink / raw)
  To: Mark Tomlinson; +Cc: kdasu.kdev, linux-kernel, linux-spi

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven
  2020-06-15  4:05 ` [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven Mark Tomlinson
@ 2020-06-15 14:32   ` Mark Brown
  2020-06-16  3:07     ` Mark Tomlinson
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2020-06-15 14:32 UTC (permalink / raw)
  To: Mark Tomlinson; +Cc: kdasu.kdev, linux-kernel, linux-spi

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/5] Improvements to spi-bcm-qspi
  2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
                   ` (4 preceding siblings ...)
  2020-06-15  4:05 ` [PATCH 5/5] spi: bcm-qspi: Improve interrupt handling Mark Tomlinson
@ 2020-06-15 19:05 ` Kamal Dasu
  5 siblings, 0 replies; 11+ messages in thread
From: Kamal Dasu @ 2020-06-15 19:05 UTC (permalink / raw)
  To: Mark Tomlinson; +Cc: Mark Brown, Linux Kernel Mailing List, linux-spi

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
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven
  2020-06-15 14:32   ` Mark Brown
@ 2020-06-16  3:07     ` Mark Tomlinson
  2020-06-16  8:26       ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Tomlinson @ 2020-06-16  3:07 UTC (permalink / raw)
  To: broonie; +Cc: linux-spi, kdasu.kdev, linux-kernel

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.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven
  2020-06-16  3:07     ` Mark Tomlinson
@ 2020-06-16  8:26       ` Mark Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2020-06-16  8:26 UTC (permalink / raw)
  To: Mark Tomlinson; +Cc: linux-spi, kdasu.kdev, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-06-16  8:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15  4:05 [PATCH 0/5] Improvements to spi-bcm-qspi Mark Tomlinson
2020-06-15  4:05 ` [PATCH 1/5] spi: bcm-qspi: Add support for setting BSPI clock Mark Tomlinson
2020-06-15  4:05 ` [PATCH 2/5] spi: bcm-qspi: Improve debug reading SPI data Mark Tomlinson
2020-06-15  4:05 ` [PATCH 3/5] spi: bcm-qspi: Do not split transfers into small chunks Mark Tomlinson
2020-06-15 13:31   ` Mark Brown
2020-06-15  4:05 ` [PATCH 4/5] spi: bcm-qspi: Make multiple data blocks interrupt-driven Mark Tomlinson
2020-06-15 14:32   ` Mark Brown
2020-06-16  3:07     ` Mark Tomlinson
2020-06-16  8:26       ` Mark Brown
2020-06-15  4:05 ` [PATCH 5/5] spi: bcm-qspi: Improve interrupt handling Mark Tomlinson
2020-06-15 19:05 ` [PATCH 0/5] Improvements to spi-bcm-qspi Kamal Dasu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).