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
Instead of splitting transfers into smaller parts, just perform the
operation that the higher level asked for.
Reviewed-by: Callum Sinclair <[email protected]>
Reviewed-by: Chris Packham <[email protected]>
Signed-off-by: Mark Tomlinson <[email protected]>
---
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
Acknowledge interrupts correctly and add support for fifo-full
interrupt, distinguishing it from the done interrupt.
Reviewed-by: Callum Sinclair <[email protected]>
Reviewed-by: Chris Packham <[email protected]>
Signed-off-by: Mark Tomlinson <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Chris Packham <[email protected]>
Signed-off-by: Mark Tomlinson <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Chris Packham <[email protected]>
Signed-off-by: Mark Tomlinson <[email protected]>
---
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
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?
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.
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 <[email protected]>
Rayagonda Kokatanur <[email protected]>
and
[email protected] (maintainer:BROADCOM SPI DRIVER)
Kamal
On Mon, Jun 15, 2020 at 12:06 AM Mark Tomlinson
<[email protected]> 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
>
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.
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?