The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
controllers support enhanced SPI which includes Dual mode, Quad mode and
Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
which can be used to prevent FIFO underflow and overflow conditions while
transmitting or receiving the data respectively.
This is almost a complete rework based on the review from Serge.
--
Regards
Sudip
Sudip Mukherjee (15):
spi: dw: Introduce spi_frf and STD_SPI
spi: dw: update NDF while using enhanced spi mode
spi: dw: update SPI_CTRLR0 register
spi: dw: add check for support of enhanced spi
spi: dw: Introduce enhanced mem_op
spi: dw: Introduce dual/quad/octal spi
spi: dw: send cmd and addr to start the spi transfer
spi: dw: update irq setup to use multiple handler
spi: dw: use irq handler for enhanced spi
spi: dw: Calculate Receive FIFO Threshold Level
spi: dw: adjust size of mem_op
spi: dw: Add retry for enhanced spi mode
spi: dw: detect enhanced spi mode
spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version
spi: dw: initialize dwc-ssi controller
.../bindings/spi/snps,dw-apb-ssi.yaml | 1 +
drivers/spi/spi-dw-core.c | 347 +++++++++++++++++-
drivers/spi/spi-dw-mmio.c | 1 +
drivers/spi/spi-dw.h | 27 ++
4 files changed, 364 insertions(+), 12 deletions(-)
--
2.30.2
If the SPI transfer is being done in enhanced mode then SPI_CTRLR0
register needs to be updated to mention the instruction length, address
length, address and instruction transfer format, wait cycles. And, we
also need to enable clock stretching.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 14 +++++++++++++-
drivers/spi/spi-dw.h | 11 +++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 8c47a4d14b666..d59401f16c47a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -320,7 +320,7 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
{
struct dw_spi_chip_data *chip = spi_get_ctldata(spi);
u32 cr0 = chip->cr0;
- u32 speed_hz;
+ u32 speed_hz, spi_ctrlr0;
u16 clk_div;
/* CTRLR0[ 4/3: 0] or CTRLR0[ 20: 16] Data Frame Size */
@@ -365,6 +365,18 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
dw_writel(dws, DW_SPI_RX_SAMPLE_DLY, chip->rx_sample_dly);
dws->cur_rx_sample_dly = chip->rx_sample_dly;
}
+
+ if (cfg->spi_frf != DW_SPI_CTRLR0_SPI_FRF_STD_SPI) {
+ spi_ctrlr0 = DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN;
+ spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK,
+ cfg->wait_c);
+ spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_INST_L_MASK,
+ cfg->inst_l);
+ spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_ADDR_L_MASK,
+ cfg->addr_l);
+ spi_ctrlr0 |= cfg->trans_t;
+ dw_writel(dws, DW_SPI_SPI_CTRLR0, spi_ctrlr0);
+ }
}
EXPORT_SYMBOL_NS_GPL(dw_spi_update_config, SPI_DW_CORE);
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 414a415deb42a..f29d89d05f34b 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -63,6 +63,7 @@
#define DW_SPI_DR 0x60
#define DW_SPI_RX_SAMPLE_DLY 0xf0
#define DW_SPI_CS_OVERRIDE 0xf4
+#define DW_SPI_SPI_CTRLR0 0xf4
/* Bit fields in CTRLR0 (DWC APB SSI) */
#define DW_PSSI_CTRLR0_DFS_MASK GENMASK(3, 0)
@@ -126,6 +127,12 @@
#define DW_SPI_DMACR_RDMAE BIT(0)
#define DW_SPI_DMACR_TDMAE BIT(1)
+/* Bit fields in SPI_CTRLR0 */
+#define DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN BIT(30)
+#define DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK GENMASK(15, 11)
+#define DW_SPI_SPI_CTRLR0_INST_L_MASK GENMASK(9, 8)
+#define DW_SPI_SPI_CTRLR0_ADDR_L_MASK GENMASK(5, 2)
+
/* Mem/DMA operations helpers */
#define DW_SPI_WAIT_RETRIES 5
#define DW_SPI_BUF_SIZE \
@@ -141,6 +148,10 @@ struct dw_spi_cfg {
u32 ndf;
u32 freq;
u8 spi_frf;
+ u8 trans_t;
+ u8 inst_l;
+ u8 addr_l;
+ u8 wait_c;
};
struct dw_spi;
--
2.30.2
The current mem_ops is not using interrupt based transfer so
dw_spi_irq_setup() only has one interrput handler to handle the non
mem_ops transfers. We will use interrupt based transfers in enhanced
spi and so we need a way to specify which irq handler to use.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index ecab0fbc847c7..f540165245a89 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -260,7 +260,8 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
if (!irq_status)
return IRQ_NONE;
- if (!master->cur_msg) {
+ if (!master->cur_msg && dws->transfer_handler ==
+ dw_spi_transfer_handler) {
dw_spi_mask_intr(dws, 0xff);
return IRQ_HANDLED;
}
@@ -380,7 +381,8 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
}
EXPORT_SYMBOL_NS_GPL(dw_spi_update_config, SPI_DW_CORE);
-static void dw_spi_irq_setup(struct dw_spi *dws)
+static void dw_spi_irq_setup(struct dw_spi *dws,
+ irqreturn_t (*t_handler)(struct dw_spi *))
{
u16 level;
u8 imask;
@@ -394,7 +396,7 @@ static void dw_spi_irq_setup(struct dw_spi *dws)
dw_writel(dws, DW_SPI_TXFTLR, level);
dw_writel(dws, DW_SPI_RXFTLR, level - 1);
- dws->transfer_handler = dw_spi_transfer_handler;
+ dws->transfer_handler = t_handler;
imask = DW_SPI_INT_TXEI | DW_SPI_INT_TXOI |
DW_SPI_INT_RXUI | DW_SPI_INT_RXOI | DW_SPI_INT_RXFI;
@@ -486,7 +488,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
else if (dws->irq == IRQ_NOTCONNECTED)
return dw_spi_poll_transfer(dws, transfer);
- dw_spi_irq_setup(dws);
+ dw_spi_irq_setup(dws, dw_spi_transfer_handler);
return 1;
}
--
2.30.2
The DW APB SSI controllers of v4.x and newer and DW AHB SSI controllers
supports enhanced SPI modes which can be defined from SPI_FRF of
DW_SPI_CTRLR0 register. Without enhanced mode, these controllers will
work in the standard spi mode.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 13 ++++++++++++-
drivers/spi/spi-dw.h | 6 ++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 99edddf9958b9..77c23772bb3d9 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -333,6 +333,16 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
/* CTRLR0[11:10] Transfer Mode */
cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_TMOD_MASK, cfg->tmode);
+ if (dw_spi_ver_is_ge(dws, HSSI, 103A)) {
+ cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;
+ cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,
+ cfg->spi_frf);
+ } else if (dw_spi_ver_is_ge(dws, PSSI, 400A)) {
+ cr0 &= ~DW_PSSI_CTRLR0_SPI_FRF_MASK;
+ cr0 |= FIELD_PREP(DW_PSSI_CTRLR0_SPI_FRF_MASK,
+ cfg->spi_frf);
+ }
+
dw_writel(dws, DW_SPI_CTRLR0, cr0);
if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
@@ -422,6 +432,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
.tmode = DW_SPI_CTRLR0_TMOD_TR,
.dfs = transfer->bits_per_word,
.freq = transfer->speed_hz,
+ .spi_frf = DW_SPI_CTRLR0_SPI_FRF_STD_SPI,
};
int ret;
@@ -664,7 +675,7 @@ static void dw_spi_stop_mem_op(struct dw_spi *dws, struct spi_device *spi)
static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
{
struct dw_spi *dws = spi_controller_get_devdata(mem->spi->controller);
- struct dw_spi_cfg cfg;
+ struct dw_spi_cfg cfg = {0};
unsigned long flags;
int ret;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 9e8eb2b52d5c7..414a415deb42a 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -17,6 +17,8 @@
/* Synopsys DW SSI component versions (FourCC sequence) */
#define DW_HSSI_102A 0x3130322a
+#define DW_HSSI_103A 0x3130332a
+#define DW_PSSI_400A 0x3430302a
/* DW SSI IP-core ID and version check helpers */
#define dw_spi_ip_is(_dws, _ip) \
@@ -94,6 +96,9 @@
#define DW_HSSI_CTRLR0_TMOD_MASK GENMASK(11, 10)
#define DW_HSSI_CTRLR0_SRL BIT(13)
#define DW_HSSI_CTRLR0_MST BIT(31)
+#define DW_HSSI_CTRLR0_SPI_FRF_MASK GENMASK(23, 22)
+#define DW_PSSI_CTRLR0_SPI_FRF_MASK GENMASK(22, 21)
+#define DW_SPI_CTRLR0_SPI_FRF_STD_SPI 0x0
/* Bit fields in CTRLR1 */
#define DW_SPI_NDF_MASK GENMASK(15, 0)
@@ -135,6 +140,7 @@ struct dw_spi_cfg {
u8 dfs;
u32 ndf;
u32 freq;
+ u8 spi_frf;
};
struct dw_spi;
--
2.30.2
Add new snps,dw-ahb-ssi version to the bindings.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index d33b72fabc5d8..af36df67a4c0e 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -45,6 +45,7 @@ properties:
enum:
- snps,dw-apb-ssi
- snps,dwc-ssi-1.01a
+ - snps,dw-ahb-ssi
- description: Microsemi Ocelot/Jaguar2 SoC SPI Controller
items:
- enum:
--
2.30.2
If the spi transfer is using dual/quad/octal spi mode, then we need to
update the SPI_CTRLR0 register. The SPI_CTRLR0 register will be updated
in dw_spi_update_config() via the values in dw_spi_cfg.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
Note: DW_SPI_SPI_CTRLR0_INST_L_INST_L16 will not work yet as
spi_mem_default_supports_op() checks for op->cmd.nbytes != 1.
drivers/spi/spi-dw-core.c | 46 +++++++++++++++++++++++++++++++++++++++
drivers/spi/spi-dw.h | 9 ++++++++
2 files changed, 55 insertions(+)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 89438ae2df17d..06169aa3f37bf 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -836,10 +836,56 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
{
struct spi_controller *ctlr = mem->spi->controller;
struct dw_spi *dws = spi_controller_get_devdata(ctlr);
+ struct dw_spi_cfg cfg;
+
+ switch (op->data.buswidth) {
+ case 2:
+ cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_DUAL_SPI;
+ break;
+ case 4:
+ cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_QUAD_SPI;
+ break;
+ case 8:
+ cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_OCT_SPI;
+ break;
+ default:
+ return dw_spi_exec_mem_op(mem, op);
+ }
/* Collect cmd and addr into a single buffer */
dw_spi_init_enh_mem_buf(dws, op);
+ cfg.dfs = 8;
+ cfg.freq = clamp(mem->spi->max_speed_hz, 0U, dws->max_mem_freq);
+ cfg.ndf = op->data.nbytes;
+ if (op->data.dir == SPI_MEM_DATA_IN)
+ cfg.tmode = DW_SPI_CTRLR0_TMOD_RO;
+ else
+ cfg.tmode = DW_SPI_CTRLR0_TMOD_TO;
+ if (op->data.buswidth == op->addr.buswidth &&
+ op->data.buswidth == op->cmd.buswidth)
+ cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT2;
+ else if (op->data.buswidth == op->addr.buswidth)
+ cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT1;
+ else
+ cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT0;
+
+ cfg.addr_l = clamp(op->addr.nbytes * 2, 0, 0xf);
+ if (op->cmd.nbytes > 1)
+ cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L16;
+ else if (op->cmd.nbytes == 1)
+ cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L8;
+ else
+ cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L0;
+
+ cfg.wait_c = (op->dummy.nbytes * (BITS_PER_BYTE / op->dummy.buswidth));
+
+ dw_spi_enable_chip(dws, 0);
+
+ dw_spi_update_config(dws, mem->spi, &cfg);
+
+ dw_spi_enable_chip(dws, 1);
+
return 0;
}
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 327d037bdb10e..494b830ad1026 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -101,6 +101,9 @@
#define DW_HSSI_CTRLR0_SPI_FRF_MASK GENMASK(23, 22)
#define DW_PSSI_CTRLR0_SPI_FRF_MASK GENMASK(22, 21)
#define DW_SPI_CTRLR0_SPI_FRF_STD_SPI 0x0
+#define DW_SPI_CTRLR0_SPI_FRF_DUAL_SPI 0x1
+#define DW_SPI_CTRLR0_SPI_FRF_QUAD_SPI 0x2
+#define DW_SPI_CTRLR0_SPI_FRF_OCT_SPI 0x3
/* Bit fields in CTRLR1 */
#define DW_SPI_NDF_MASK GENMASK(15, 0)
@@ -132,7 +135,13 @@
#define DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN BIT(30)
#define DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK GENMASK(15, 11)
#define DW_SPI_SPI_CTRLR0_INST_L_MASK GENMASK(9, 8)
+#define DW_SPI_SPI_CTRLR0_INST_L_INST_L0 0x0
+#define DW_SPI_SPI_CTRLR0_INST_L_INST_L8 0x2
+#define DW_SPI_SPI_CTRLR0_INST_L_INST_L16 0x3
#define DW_SPI_SPI_CTRLR0_ADDR_L_MASK GENMASK(5, 2)
+#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT0 0x0
+#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT1 0x1
+#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT2 0x2
/* Mem/DMA operations helpers */
#define DW_SPI_WAIT_RETRIES 5
--
2.30.2
In enhanced mode adjust the size of the data that can be sent or received
as this will then be used to set the NDF.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 75f5ce5f377ca..dff7b419af304 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -546,6 +546,13 @@ static void dw_spi_handle_err(struct spi_controller *master,
dw_spi_reset_chip(dws);
}
+static int dw_spi_adjust_enh_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+{
+ op->data.nbytes = clamp_val(op->data.nbytes, 0, DW_SPI_NDF_MASK + 1);
+
+ return 0;
+}
+
static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
{
if (op->data.dir == SPI_MEM_DATA_IN)
@@ -997,13 +1004,14 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
{
if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
!dws->set_cs) {
- dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
if (dws->caps & DW_SPI_CAP_EMODE) {
dws->mem_ops.exec_op = dw_spi_exec_enh_mem_op;
dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
+ dws->mem_ops.adjust_op_size = dw_spi_adjust_enh_mem_op_size;
} else {
dws->mem_ops.exec_op = dw_spi_exec_mem_op;
dws->mem_ops.supports_op = dw_spi_supports_mem_op;
+ dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
}
if (!dws->max_mem_freq)
dws->max_mem_freq = dws->max_freq;
--
2.30.2
In enhanced spi mode, read or write will start by sending the cmd
and address (if present).
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 06169aa3f37bf..ecab0fbc847c7 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -832,6 +832,29 @@ static void dw_spi_init_enh_mem_buf(struct dw_spi *dws, const struct spi_mem_op
}
}
+static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_op *op)
+{
+ void *buf = dws->buf;
+ u32 txw;
+
+ /* Send cmd as 32 bit value */
+ if (buf) {
+ txw = *(u32 *)(buf);
+ dw_write_io_reg(dws, DW_SPI_DR, txw);
+ buf += dws->reg_io_width;
+ if (op->addr.nbytes) {
+ txw = *(u32 *)(buf);
+ dw_write_io_reg(dws, DW_SPI_DR, txw);
+ if (op->addr.nbytes > 4) {
+ /* address more than 32bit */
+ buf += dws->reg_io_width;
+ txw = *(u32 *)(buf);
+ dw_write_io_reg(dws, DW_SPI_DR, txw);
+ }
+ }
+ }
+}
+
static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
{
struct spi_controller *ctlr = mem->spi->controller;
@@ -886,6 +909,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
dw_spi_enable_chip(dws, 1);
+ dw_spi_enh_write_cmd_addr(dws, op);
+
return 0;
}
--
2.30.2
If the connection to the spi device is not stable then the transfer can
fail. Add retry for DW_SPI_WAIT_RETRIES times and print error if it still
fails.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index dff7b419af304..cef56acd8d8fd 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -906,7 +906,7 @@ static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_o
}
}
-static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
+static int dw_spi_try_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
{
struct spi_controller *ctlr = mem->spi->controller;
struct dw_spi *dws = spi_controller_get_devdata(ctlr);
@@ -991,6 +991,21 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
return ret;
}
+static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+ struct spi_controller *ctlr = mem->spi->controller;
+ struct dw_spi *dws = spi_controller_get_devdata(ctlr);
+ int retry, ret = -EIO;
+
+ for (retry = 0; retry < DW_SPI_WAIT_RETRIES && ret != 0; retry++)
+ ret = dw_spi_try_enh_mem_op(mem, op);
+
+ if (retry == DW_SPI_WAIT_RETRIES)
+ dev_err(&dws->master->dev, "Retry of enh_mem_op failed\n");
+
+ return ret;
+}
+
/*
* Initialize the default memory operations if a glue layer hasn't specified
* custom ones. Direct mapping operations will be preserved anyway since DW SPI
--
2.30.2
In enhanced mode we need to calculate RXFTLR based on the length of data
we are expecting to receive or the fifo length.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 10d453228368f..75f5ce5f377ca 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -428,6 +428,14 @@ static void dw_spi_irq_setup(struct dw_spi *dws,
*/
level = min_t(u16, dws->fifo_len / 2, dws->tx_len);
dw_writel(dws, DW_SPI_TXFTLR, level);
+
+ /*
+ * In enhanced mode if we are reading then tx_len is 0 as we
+ * have nothing to transmit. Calculate DW_SPI_RXFTLR with
+ * rx_len.
+ */
+ if (t_handler == dw_spi_enh_handler)
+ level = min_t(u16, dws->fifo_len / 2, dws->rx_len);
dw_writel(dws, DW_SPI_RXFTLR, level - 1);
dws->transfer_handler = t_handler;
--
2.30.2
Before doing the mem op, spi controller will be queried about the
buswidths it supports. Add the dual/quad/octal if the controller
has the DW_SPI_CAP_EMODE capability.
The DW_SPI_CAP_EMODE capability will be enabled in a later patch.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 25 ++++++++++++++++++++++++-
drivers/spi/spi-dw.h | 1 +
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index d59401f16c47a..49fad58ceb94a 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -510,6 +510,26 @@ static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
return 0;
}
+static bool dw_spi_supports_enh_mem_op(struct spi_mem *mem,
+ const struct spi_mem_op *op)
+{
+ if (op->addr.nbytes != 0 && op->addr.buswidth != 1 &&
+ op->addr.buswidth != op->data.buswidth)
+ return false;
+
+ if (op->cmd.buswidth != 1 && op->cmd.buswidth != op->addr.buswidth &&
+ op->cmd.buswidth != op->data.buswidth)
+ return false;
+
+ if (op->dummy.nbytes != 0 && op->data.dir == SPI_MEM_DATA_OUT)
+ return false;
+
+ if (op->dummy.nbytes != 0 && op->dummy.nbytes / op->dummy.buswidth > 4)
+ return false;
+
+ return spi_mem_default_supports_op(mem, op);
+}
+
static bool dw_spi_supports_mem_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
@@ -792,7 +812,10 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
!dws->set_cs) {
dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
- dws->mem_ops.supports_op = dw_spi_supports_mem_op;
+ if (dws->caps & DW_SPI_CAP_EMODE)
+ dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
+ else
+ dws->mem_ops.supports_op = dw_spi_supports_mem_op;
dws->mem_ops.exec_op = dw_spi_exec_mem_op;
if (!dws->max_mem_freq)
dws->max_mem_freq = dws->max_freq;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index f29d89d05f34b..327d037bdb10e 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -34,6 +34,7 @@
/* DW SPI controller capabilities */
#define DW_SPI_CAP_CS_OVERRIDE BIT(0)
#define DW_SPI_CAP_DFS32 BIT(1)
+#define DW_SPI_CAP_EMODE BIT(2)
/* Register offsets (Generic for both DWC APB SSI and DWC SSI IP-cores) */
#define DW_SPI_CTRLR0 0x00
--
2.30.2
Introduce the interrupt handler for enhanced spi to read or write based
on the generated irq. Also, use the xfer_completion from spi_controller
to wait for a timeout or completion from irq handler.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 62 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index f540165245a89..10d453228368f 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -251,6 +251,34 @@ static irqreturn_t dw_spi_transfer_handler(struct dw_spi *dws)
return IRQ_HANDLED;
}
+static irqreturn_t dw_spi_enh_handler(struct dw_spi *dws)
+{
+ u16 irq_status = dw_readl(dws, DW_SPI_ISR);
+
+ if (dw_spi_check_status(dws, false)) {
+ spi_finalize_current_transfer(dws->master);
+ return IRQ_HANDLED;
+ }
+
+ if (irq_status & DW_SPI_INT_RXFI) {
+ dw_reader(dws);
+ if (dws->rx_len <= dw_readl(dws, DW_SPI_RXFTLR))
+ dw_writel(dws, DW_SPI_RXFTLR, dws->rx_len - 1);
+ }
+
+ if (irq_status & DW_SPI_INT_TXEI)
+ dw_writer(dws);
+
+ if (!dws->tx_len && dws->rx_len) {
+ dw_spi_mask_intr(dws, DW_SPI_INT_TXEI);
+ } else if (!dws->rx_len && !dws->tx_len) {
+ dw_spi_mask_intr(dws, 0xff);
+ spi_finalize_current_transfer(dws->master);
+ }
+
+ return IRQ_HANDLED;
+}
+
static irqreturn_t dw_spi_irq(int irq, void *dev_id)
{
struct spi_controller *master = dev_id;
@@ -265,6 +293,12 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
dw_spi_mask_intr(dws, 0xff);
return IRQ_HANDLED;
}
+ if ((dws->transfer_handler == dw_spi_enh_handler &&
+ !dws->rx_len && !dws->tx_len)) {
+ dw_spi_mask_intr(dws, 0xff);
+ spi_finalize_current_transfer(master);
+ return IRQ_HANDLED;
+ }
return dws->transfer_handler(dws);
}
@@ -862,6 +896,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
struct spi_controller *ctlr = mem->spi->controller;
struct dw_spi *dws = spi_controller_get_devdata(ctlr);
struct dw_spi_cfg cfg;
+ int ret = 0;
+ unsigned long long ms;
switch (op->data.buswidth) {
case 2:
@@ -909,11 +945,35 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
dw_spi_update_config(dws, mem->spi, &cfg);
+ dw_spi_mask_intr(dws, 0xff);
+ reinit_completion(&ctlr->xfer_completion);
dw_spi_enable_chip(dws, 1);
dw_spi_enh_write_cmd_addr(dws, op);
+ dw_spi_set_cs(mem->spi, false);
+ dw_spi_irq_setup(dws, dw_spi_enh_handler);
- return 0;
+ /* Use timeout calculation from spi_transfer_wait() */
+ ms = 8LL * MSEC_PER_SEC * (dws->rx_len ? dws->rx_len : dws->tx_len);
+ do_div(ms, dws->current_freq);
+
+ /*
+ * Increase it twice and add 200 ms tolerance, use
+ * predefined maximum in case of overflow.
+ */
+ ms += ms + 200;
+ if (ms > UINT_MAX)
+ ms = UINT_MAX;
+
+ ms = wait_for_completion_timeout(&ctlr->xfer_completion,
+ msecs_to_jiffies(ms));
+
+ dw_spi_stop_mem_op(dws, mem->spi);
+
+ if (ms == 0)
+ ret = -EIO;
+
+ return ret;
}
/*
--
2.30.2
Use the generic snps,dw-ahb-ssi version to initialize the controller.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-mmio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 26c40ea6dd129..431ba768ea861 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -352,6 +352,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
{ .compatible = "intel,thunderbay-ssi", .data = dw_spi_intel_init},
{ .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init},
{ .compatible = "canaan,k210-spi", dw_spi_canaan_k210_init},
+ { .compatible = "snps,dw-ahb-ssi", .data = dw_spi_hssi_init},
{ /* end of table */}
};
MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
--
2.30.2
If the DW_SPI_CAP_EMODE capability is enabled then dw_spi_exec_enh_mem_op()
will be used as the new enhanced mem_op. Lets initialize the buffer and
get the pointers to receive and transmit data buffers.
The DW_SPI_CAP_EMODE capability will be enabled in a later patch.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 53 ++++++++++++++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 49fad58ceb94a..89438ae2df17d 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -798,6 +798,51 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
return ret;
}
+static void dw_spi_init_enh_mem_buf(struct dw_spi *dws, const struct spi_mem_op *op)
+{
+ unsigned int i, j;
+ u8 *out;
+
+ out = dws->buf;
+ for (i = 0; i < DW_SPI_BUF_SIZE; ++i)
+ out[i] = 0;
+
+ for (i = 0, j = op->cmd.nbytes; i < op->cmd.nbytes; ++i, --j)
+ out[i] = DW_SPI_GET_BYTE(op->cmd.opcode, op->cmd.nbytes - j);
+
+ for (j = op->addr.nbytes, i = dws->reg_io_width; j > 0; ++i, --j)
+ out[i] = DW_SPI_GET_BYTE(op->addr.val, op->addr.nbytes - j);
+
+ dws->n_bytes = 1;
+ if (op->data.dir == SPI_MEM_DATA_IN) {
+ dws->rx = op->data.buf.in;
+ dws->rx_len = op->data.nbytes;
+ dws->tx = NULL;
+ dws->tx_len = 0;
+ } else if (op->data.dir == SPI_MEM_DATA_OUT) {
+ dws->tx_len = op->data.nbytes;
+ dws->tx = (void *)op->data.buf.out;
+ dws->rx = NULL;
+ dws->rx_len = 0;
+ } else {
+ dws->rx = NULL;
+ dws->rx_len = 0;
+ dws->tx = NULL;
+ dws->tx_len = 0;
+ }
+}
+
+static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+ struct spi_controller *ctlr = mem->spi->controller;
+ struct dw_spi *dws = spi_controller_get_devdata(ctlr);
+
+ /* Collect cmd and addr into a single buffer */
+ dw_spi_init_enh_mem_buf(dws, op);
+
+ return 0;
+}
+
/*
* Initialize the default memory operations if a glue layer hasn't specified
* custom ones. Direct mapping operations will be preserved anyway since DW SPI
@@ -812,11 +857,13 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
!dws->set_cs) {
dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
- if (dws->caps & DW_SPI_CAP_EMODE)
+ if (dws->caps & DW_SPI_CAP_EMODE) {
+ dws->mem_ops.exec_op = dw_spi_exec_enh_mem_op;
dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
- else
+ } else {
+ dws->mem_ops.exec_op = dw_spi_exec_mem_op;
dws->mem_ops.supports_op = dw_spi_supports_mem_op;
- dws->mem_ops.exec_op = dw_spi_exec_mem_op;
+ }
if (!dws->max_mem_freq)
dws->max_mem_freq = dws->max_freq;
}
--
2.30.2
If the transfer of Transmit only mode is using dual/quad/octal SPI then
NDF needs to be updated with the number of data frames.
If the Transmit FIFO goes empty in-between, DWC_ssi masks the serial
clock and wait for rest of the data until the programmed amount of
frames are transferred successfully.
Signed-off-by: Sudip Mukherjee <[email protected]>
---
drivers/spi/spi-dw-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index 77c23772bb3d9..8c47a4d14b666 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -346,7 +346,9 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
dw_writel(dws, DW_SPI_CTRLR0, cr0);
if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
- cfg->tmode == DW_SPI_CTRLR0_TMOD_RO)
+ cfg->tmode == DW_SPI_CTRLR0_TMOD_RO ||
+ (cfg->tmode == DW_SPI_CTRLR0_TMOD_TO &&
+ cfg->spi_frf != DW_SPI_CTRLR0_SPI_FRF_STD_SPI))
dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf ? cfg->ndf - 1 : 0);
/* Note DW APB SSI clock divider doesn't support odd numbers */
--
2.30.2
On Mon, Dec 12, 2022 at 06:07:31PM +0000, Sudip Mukherjee wrote:
> Add new snps,dw-ahb-ssi version to the bindings.
Yes, I can see that in the diff. Tell me something useful here. Why
do we need a new compatible? What does this compatible imply in the
h/w that the existing ones don't. How do I know which compatible to
use?
Really, this should probably only be a fallback with an SoC specific
compatible. Future quirk properties which are not board specific only
will be rejected. You've been warned.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index d33b72fabc5d8..af36df67a4c0e 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -45,6 +45,7 @@ properties:
> enum:
> - snps,dw-apb-ssi
> - snps,dwc-ssi-1.01a
> + - snps,dw-ahb-ssi
> - description: Microsemi Ocelot/Jaguar2 SoC SPI Controller
> items:
> - enum:
> --
> 2.30.2
>
>
On Tue, Dec 13, 2022 at 10:32:09AM -0600, Rob Herring wrote:
> On Mon, Dec 12, 2022 at 06:07:31PM +0000, Sudip Mukherjee wrote:
> > Add new snps,dw-ahb-ssi version to the bindings.
> Really, this should probably only be a fallback with an SoC specific
> compatible. Future quirk properties which are not board specific only
> will be rejected. You've been warned.
Given how widely used DesignWare stuff is and usage in FPGAs it does
seem reasonable to have compatibles for just the IP rather than SoC
specific ones - we do have quirked versions that have been modified but
these are things that people manage to deploy without needing that and
SoC specific compatibles for FPGA instantiations would get painful.
On Tue, Dec 13, 2022 at 4:59 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Dec 13, 2022 at 10:32:09AM -0600, Rob Herring wrote:
> > On Mon, Dec 12, 2022 at 06:07:31PM +0000, Sudip Mukherjee wrote:
>
> > > Add new snps,dw-ahb-ssi version to the bindings.
>
> > Really, this should probably only be a fallback with an SoC specific
> > compatible. Future quirk properties which are not board specific only
> > will be rejected. You've been warned.
>
> Given how widely used DesignWare stuff is and usage in FPGAs it does
> seem reasonable to have compatibles for just the IP rather than SoC
> specific ones - we do have quirked versions that have been modified but
> these are things that people manage to deploy without needing that and
> SoC specific compatibles for FPGA instantiations would get painful.
Also, this patchset adds the autodetect procedure as discussed in the review
of the previous series at
https://lore.kernel.org/lkml/20220826233116.uulisbo663cxiadt@mobilestation/
So, we should be able to replace "snps,dw-apb-ssi" and
"snps,dwc-ssi-1.01a" with "snps,dw-ahb-ssi" after this.
And, also this generic compatible has been tested with the new 1.03a
version we are working with, which was
mentioned in my v1 at
https://lore.kernel.org/lkml/20220826233305.5ugpukokzldum7y5@mobilestation/
--
Regards
Sudip
On Tue, Dec 13, 2022 at 05:47:53PM +0000, Sudip Mukherjee wrote:
> On Tue, Dec 13, 2022 at 4:59 PM Mark Brown <[email protected]> wrote:
> >
> > On Tue, Dec 13, 2022 at 10:32:09AM -0600, Rob Herring wrote:
> > > On Mon, Dec 12, 2022 at 06:07:31PM +0000, Sudip Mukherjee wrote:
> >
> > > > Add new snps,dw-ahb-ssi version to the bindings.
> >
> > > Really, this should probably only be a fallback with an SoC specific
> > > compatible. Future quirk properties which are not board specific only
> > > will be rejected. You've been warned.
> >
> > Given how widely used DesignWare stuff is and usage in FPGAs it does
> > seem reasonable to have compatibles for just the IP rather than SoC
> > specific ones - we do have quirked versions that have been modified but
> > these are things that people manage to deploy without needing that and
> > SoC specific compatibles for FPGA instantiations would get painful.
>
> Also, this patchset adds the autodetect procedure as discussed in the review
> of the previous series at
> https://lore.kernel.org/lkml/20220826233116.uulisbo663cxiadt@mobilestation/
>
> So, we should be able to replace "snps,dw-apb-ssi" and
> "snps,dwc-ssi-1.01a" with "snps,dw-ahb-ssi" after this.
Just "snps,dwc-ssi-1.01a". That is the IP-core
https://www.synopsys.com/dw/ipdir.php?c=dwc_ssi
which support was added in
https://lore.kernel.org/linux-spi/[email protected]/
Since the IP-core version is auto-detectable there is no need in
having the version attached to the compatible string. That's why I
asked @Sudip to introduce a new generic device name free of the
version suffix. It should be used instead of "snps,dwc-ssi-1.01a"
from now.
The "snps,dw-apb-ssi" compatible string will stay since it corresponds
to another IP-core:
https://www.synopsys.com/dw/ipdir.php?c=DW_apb_ssi
Answering to the @Rob note regarding the quirk properties. All the
features added by Sudip here were supposed to be auto-detectable.
So the generic IP-core name will be still useful as before with no
need in adding any quirks.
@Rob AFAIR you used to be against the generic fallback compatible
names, but had nothing against just generic compatibles. Has something
changed?
-Serge(y)
> And, also this generic compatible has been tested with the new 1.03a
> version we are working with, which was
> mentioned in my v1 at
> https://lore.kernel.org/lkml/20220826233305.5ugpukokzldum7y5@mobilestation/
>
> --
> Regards
> Sudip
Hi Sudip
On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> controllers support enhanced SPI which includes Dual mode, Quad mode and
> Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> which can be used to prevent FIFO underflow and overflow conditions while
> transmitting or receiving the data respectively.
>
> This is almost a complete rework based on the review from Serge.
Thank you very much for the series. I'll have a look at it on the next
week.
-Serge(y)
>
>
> --
> Regards
> Sudip
>
> Sudip Mukherjee (15):
> spi: dw: Introduce spi_frf and STD_SPI
> spi: dw: update NDF while using enhanced spi mode
> spi: dw: update SPI_CTRLR0 register
> spi: dw: add check for support of enhanced spi
> spi: dw: Introduce enhanced mem_op
> spi: dw: Introduce dual/quad/octal spi
> spi: dw: send cmd and addr to start the spi transfer
> spi: dw: update irq setup to use multiple handler
> spi: dw: use irq handler for enhanced spi
> spi: dw: Calculate Receive FIFO Threshold Level
> spi: dw: adjust size of mem_op
> spi: dw: Add retry for enhanced spi mode
> spi: dw: detect enhanced spi mode
> spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version
> spi: dw: initialize dwc-ssi controller
>
> .../bindings/spi/snps,dw-apb-ssi.yaml | 1 +
> drivers/spi/spi-dw-core.c | 347 +++++++++++++++++-
> drivers/spi/spi-dw-mmio.c | 1 +
> drivers/spi/spi-dw.h | 27 ++
> 4 files changed, 364 insertions(+), 12 deletions(-)
>
> --
> 2.30.2
>
Hi Sudip
On Sun, Dec 18, 2022 at 08:45:26PM +0300, Serge Semin wrote:
> Hi Sudip
>
> On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> > The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> > controllers support enhanced SPI which includes Dual mode, Quad mode and
> > Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> > which can be used to prevent FIFO underflow and overflow conditions while
> > transmitting or receiving the data respectively.
> >
> > This is almost a complete rework based on the review from Serge.
>
> Thank you very much for the series. I'll have a look at it on the next
> week.
Just so you know. I haven't forgot about the series. There are some
problematic parts which I need to give more thinking than I originally
expected. I'll submit my comments very soon. Sorry for the delay.
Good news is that I've got the HW-manual for the DW SSI v1.01a
IP-core. So I'll no longer need to ask of you about that device
implementation specifics.
-Serge(y)
>
> -Serge(y)
>
> >
> >
> > --
> > Regards
> > Sudip
> >
> > Sudip Mukherjee (15):
> > spi: dw: Introduce spi_frf and STD_SPI
> > spi: dw: update NDF while using enhanced spi mode
> > spi: dw: update SPI_CTRLR0 register
> > spi: dw: add check for support of enhanced spi
> > spi: dw: Introduce enhanced mem_op
> > spi: dw: Introduce dual/quad/octal spi
> > spi: dw: send cmd and addr to start the spi transfer
> > spi: dw: update irq setup to use multiple handler
> > spi: dw: use irq handler for enhanced spi
> > spi: dw: Calculate Receive FIFO Threshold Level
> > spi: dw: adjust size of mem_op
> > spi: dw: Add retry for enhanced spi mode
> > spi: dw: detect enhanced spi mode
> > spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version
> > spi: dw: initialize dwc-ssi controller
> >
> > .../bindings/spi/snps,dw-apb-ssi.yaml | 1 +
> > drivers/spi/spi-dw-core.c | 347 +++++++++++++++++-
> > drivers/spi/spi-dw-mmio.c | 1 +
> > drivers/spi/spi-dw.h | 27 ++
> > 4 files changed, 364 insertions(+), 12 deletions(-)
> >
> > --
> > 2.30.2
> >
Hello Sudip
On Thu, Jan 05, 2023 at 01:20:39AM +0300, Serge Semin wrote:
> Hi Sudip
>
> On Sun, Dec 18, 2022 at 08:45:26PM +0300, Serge Semin wrote:
> > Hi Sudip
> >
> > On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> > > The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> > > controllers support enhanced SPI which includes Dual mode, Quad mode and
> > > Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> > > which can be used to prevent FIFO underflow and overflow conditions while
> > > transmitting or receiving the data respectively.
> > >
> > > This is almost a complete rework based on the review from Serge.
> >
> > Thank you very much for the series. I'll have a look at it on the next
> > week.
>
> Just so you know. I haven't forgot about the series. There are some
> problematic parts which I need to give more thinking than I originally
> expected. I'll submit my comments very soon. Sorry for the delay.
>
> Good news is that I've got the HW-manual for the DW SSI v1.01a
> IP-core. So I'll no longer need to ask of you about that device
> implementation specifics.
Finally I managed to consolidate my thoughts regarding your patchset.
Here is the summary. Some specific comments will be sent in-reply to
the corresponding patches.
First of all there is a crucial difference between eSPI capability
available on DW APB SSI and DW AHB SSI controllers:
DW APB SSI 4.x:
+ Tx until FIFO is empty
+ No clock stretching at all
DW AHB SSI 1.x:
+ Tx until CTRLR1.NDF if clock stretching is available
+ If no clock stretching then Tx until FIFO is empty.
See the DW APB SSI IP-cores don't have the clock stretching feature.
That should be kept in mind while implementing the portable eSPI
support in the DW SSI driver. Your version of the eSPI support is only
applicable for the devices with the eSPI clock-stretching capability
which significantly narrows down its applicability. Anyway you should
convert your code to working only if the clock-stretching feature is
detected.
Moreover your implementation will only work on the platforms with the
native chip-selects. If the GPIO-based CS setup is detected the standard
SPI-messages/transfers-based kernel API will be utilized (see the
spi_mem_exec_op() method implementation) which currently imply the
single-laned SPI bus only. For the same reason the DMA capability won't
work in your eSPI implementation.
If you want that to be fixed then you'll need to update the standard
transfer (+DMA) procedure so one would take the
spi_transfer.{tx_nbits,rx_nbits} fields into account activating the
eSPI modes accordingly. The dw_spi_transfer_handler() method and its
DMA counterpart shall be altered too since the eSPI implies the
Tx-only or Rx-only modes.
Here are several notes applicable to all your patches:
1. Please use the dw_spi_enh* prefix for all eSPI-related methods. That
shall unify the eSPI-code a bit in the same way as it was done in the
DMA-modules (see it has the method defined with the dw_spi_dma_* prefix).
2. Please define the enhanced versions of the MEM-op methods below their
non-enhanced counterparts. Thus we'll have a clearer driver code.
3. It isn't normally welcome to add some code in one patch and fix it in
some following up patch.
4. I am pretty much sure you shouldn't touch the spi_controller data
internals if it isn't platform-specific settings on the controller
probe procedure. Mark won't bless such change. The
spi_controller.xfer_complete field is the internal completion handler
and should be used by the SPI core only.
Regarding the patchset in general. It's ok to provide the eSPI mode
support for the native chip-selects only. But since there are going to be
not a few requests to you to fix I'd suggest to refactor the series in the
next manner:
[PATCH 1] spi: dw: Add Enhanced capabilities flags
Just define the new capability flags
+ DW_SPI_CAP_ENH
+ DW_SPI_CAP_ENH_CLK_STR
Note the capabilities auto-detection will be added later in this patchset.
Yeah, I remember asking you to add the DW_SPI_CAP_EMODE macro, but adding
the _ENH suffix instead seems more readable and would refer to the
eSPI-related methods.
[PATCH 2] spi: dw: Update enhanced frame forward config
In this patch please fix the dw_spi_update_config() method so one would
perform both standard and enhanced config setups:
+ Add new field dw_spi_cfg.enh_frf.
+ Declare new structure struct dw_spi_enh_cfg with the wait_c, addr_l,
inst_l and trans_t fields.
+ Convert the dw_spi_update_config() method to accepting the optional
struct dw_spi_enh_cfg *ecfg pointer.
+ Update the CTRLR1.NDF field for Tx-only transfers if
DW_SPI_CAP_ENH_CLK_STR capability is available.
[PATCH 3] spi: dw: Reduce mem-ops init method indentations
+ This is a prerequisite patch before adding the eSPI-related
MEM-op methods to make the following up patches simpler and more
coherent. It implies updating the dw_spi_init_mem_ops()
function so one would return straight away if the platform-specific CS-methods
or MEM-ops methods are specified. Thus further function updates would be
performed in the more coherent patches with simpler changelog. (See my
comments to your patch "[PATCH v2 04/15] spi: dw: add check for support of enhanced spi".)
[PATCH 4] spi: dw: Add enhanced mem-op verification method
+ Almost the same as your patch "[PATCH v2 04/15] spi: dw: add check for support of enhanced spi"
except the code will have one less indentation due to the patch #3.
Some other issues shall be fixed too. See my comments to your patch for
details. (init if DW_SPI_CAP_ENH_CLK_STR, op->dummy.nbytes / op->dummy.buswidth >= 4)
[PATCH 5] spi: dw: Add enhanced mem-op size adjustment method
+ The same as your patch "[PATCH v2 11/15] spi: dw: adjust size of mem_op"
[PATCH 6] spi: dw: Mask out IRQs if no xfer data available
+ Instead of checking the master->cur_msg pointer to be non-NULL I guess
it would be ok to disable the IRQs if !dws->rx_len && !dws->tx_len in the
dw_spi_irq() method. Although I have doubts that after the commit
a1d5aa6f7f97 ("spi: dw: Disable all IRQs when controller is unused") there
is need in that conditional statement especially seeing the dw_reader()
and dw_writer() methods won't do anything if no data available to
transfer.
[PATCH 7] spi: dw: Move wait-function to the driver core
+ Instead of re-implementing the xfer completion wait function one more
time (as you've done in "[PATCH v2 09/15] spi: dw: use irq handler for
enhanced spi") just move the dw_spi_dma_wait() method from spi-dw-dma.c to
spi-dw-core.c, accordingly fix the prototype and implementation, rename
the dw_spi.dma_completion field to something like xfer_completion and use
the new method in the DW SSI core and DMA modules.
[PATCH 8] spi: dw: spi: dw: Add enhanced mem-op execution method
+ Just add the methods particularly implementing the mem-op execution
process like:
dw_spi_enh_irq_setup(): instead of updating the dw_spi_irq_setup() method
just create a new one which would initialize the IRQ-handler and unmask
IRQs accordingly (depending on the rx_len/rx_len values?).
dw_spi_enh_transfer_handler(): similar to your implementation of the
IRQ-handler except it will use the internal wait-for-completion infrastructure.
dw_spi_enh_write_then_read(): shall write the cmd+addr and activate the
CS line. After that the transfer shall begin.
dw_spi_enh_exec_mem_op(): similar to the dw_spi_exec_mem_op() method. It
shall update the controller config taking the enhanced part into account,
activate the IRQs using the dw_spi_enh_irq_setup() method, call the
dw_spi_enh_write_then_read() function and then wait until the IRQ-based
transfer is completed.
[PATCH 9] spi: dw: Add enhanced mem-op capability auto-detection
+ Just check whether the CTRLR0.SPI_FRF field is writable and values it
accepts. Based on that set the DW_SPI_CAP_ENH capability flag and update
spi_controller.mode_bits field. Similarly check whether the
SPI_CTRLR0.CLK_STRETCH_EN bit is writable and set the
DW_SPI_CAP_ENH_CLK_STR capability flag.
+ Note first you need to try detecting the eSPI capability and only
then the eSPI clock stretching capability.
+ Note the best place for that is the dw_spi_hw_init() method where all
the HW-init and auto-detection is done.
[PATCH 10] spi: dt-bindings: dw-apb-ssi: Add DW AHB SSI compatible string
+ The same as your "[PATCH v2 14/15] spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version"
[PATCH 11] spi: dw: Add DW AHB SSI compatible string
+ The same as your "[PATCH v2 15/15] spi: dw: initialize dwc-ssi controller"
Some implementation-specific comments I'll submit in-reply to the
corresponding patches.
-Serge(y)
>
> -Serge(y)
>
> >
> > -Serge(y)
> >
> > >
> > >
> > > --
> > > Regards
> > > Sudip
> > >
> > > Sudip Mukherjee (15):
> > > spi: dw: Introduce spi_frf and STD_SPI
> > > spi: dw: update NDF while using enhanced spi mode
> > > spi: dw: update SPI_CTRLR0 register
> > > spi: dw: add check for support of enhanced spi
> > > spi: dw: Introduce enhanced mem_op
> > > spi: dw: Introduce dual/quad/octal spi
> > > spi: dw: send cmd and addr to start the spi transfer
> > > spi: dw: update irq setup to use multiple handler
> > > spi: dw: use irq handler for enhanced spi
> > > spi: dw: Calculate Receive FIFO Threshold Level
> > > spi: dw: adjust size of mem_op
> > > spi: dw: Add retry for enhanced spi mode
> > > spi: dw: detect enhanced spi mode
> > > spi: dt-bindings: snps,dw-ahb-ssi: Add generic dw-ahb-ssi version
> > > spi: dw: initialize dwc-ssi controller
> > >
> > > .../bindings/spi/snps,dw-apb-ssi.yaml | 1 +
> > > drivers/spi/spi-dw-core.c | 347 +++++++++++++++++-
> > > drivers/spi/spi-dw-mmio.c | 1 +
> > > drivers/spi/spi-dw.h | 27 ++
> > > 4 files changed, 364 insertions(+), 12 deletions(-)
> > >
> > > --
> > > 2.30.2
> > >
On Mon, Dec 12, 2022 at 06:07:19PM +0000, Sudip Mukherjee wrote:
> If the transfer of Transmit only mode is using dual/quad/octal SPI then
> NDF needs to be updated with the number of data frames.
> If the Transmit FIFO goes empty in-between, DWC_ssi masks the serial
> clock and wait for rest of the data until the programmed amount of
> frames are transferred successfully.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 77c23772bb3d9..8c47a4d14b666 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -346,7 +346,9 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> dw_writel(dws, DW_SPI_CTRLR0, cr0);
>
> if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> - cfg->tmode == DW_SPI_CTRLR0_TMOD_RO)
> + cfg->tmode == DW_SPI_CTRLR0_TMOD_RO ||
> + (cfg->tmode == DW_SPI_CTRLR0_TMOD_TO &&
> + cfg->spi_frf != DW_SPI_CTRLR0_SPI_FRF_STD_SPI))
First CTRLR1.NDF is meaningful for the Tx-only mode if non-zero eSPI
mode is enabled and the clock-stretching feature is activated. Second
the conditional statement already looks too bulky. Adding new parts
will make it even harder to read. What about converting it to
something like:
< if (cfg->ndf)
< dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf - 1);
What do you think?
-Serge(y)
> dw_writel(dws, DW_SPI_CTRLR1, cfg->ndf ? cfg->ndf - 1 : 0);
>
> /* Note DW APB SSI clock divider doesn't support odd numbers */
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:18PM +0000, Sudip Mukherjee wrote:
> The DW APB SSI controllers of v4.x and newer and DW AHB SSI controllers
> supports enhanced SPI modes which can be defined from SPI_FRF of
> DW_SPI_CTRLR0 register. Without enhanced mode, these controllers will
> work in the standard spi mode.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 13 ++++++++++++-
> drivers/spi/spi-dw.h | 6 ++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 99edddf9958b9..77c23772bb3d9 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -333,6 +333,16 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> /* CTRLR0[11:10] Transfer Mode */
> cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_TMOD_MASK, cfg->tmode);
>
> + if (dw_spi_ver_is_ge(dws, HSSI, 103A)) {
eSPI has been available most likely since 1.00a (at least 1.01a
has that feature).
> + cr0 &= ~DW_HSSI_CTRLR0_SPI_FRF_MASK;
No need in masking that field because the cr0 variable is
pre-initialized with the device-specific value anyway.
> + cr0 |= FIELD_PREP(DW_HSSI_CTRLR0_SPI_FRF_MASK,
> + cfg->spi_frf);
The HW-manual defines that field as SPI_FRF, but the SPI_ prefix looks
vague because it doesn't differentiate it from just "frf" field. I'd
suggest to use the "enh_frf" name instead.
> + } else if (dw_spi_ver_is_ge(dws, PSSI, 400A)) {
> + cr0 &= ~DW_PSSI_CTRLR0_SPI_FRF_MASK;
> + cr0 |= FIELD_PREP(DW_PSSI_CTRLR0_SPI_FRF_MASK,
> + cfg->spi_frf);
The same comments as above.
> + }
> +
> dw_writel(dws, DW_SPI_CTRLR0, cr0);
>
> if (cfg->tmode == DW_SPI_CTRLR0_TMOD_EPROMREAD ||
> @@ -422,6 +432,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
<--------+
> .tmode = DW_SPI_CTRLR0_TMOD_TR, |
> .dfs = transfer->bits_per_word, |
> .freq = transfer->speed_hz, |
|
> + .spi_frf = DW_SPI_CTRLR0_SPI_FRF_STD_SPI, +
You also forgot to update the spi-dw-bt1.c driver.
> };
> int ret;
>
> @@ -664,7 +675,7 @@ static void dw_spi_stop_mem_op(struct dw_spi *dws, struct spi_device *spi)
> static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> {
> struct dw_spi *dws = spi_controller_get_devdata(mem->spi->controller);
> - struct dw_spi_cfg cfg;
> + struct dw_spi_cfg cfg = {0};
Please explicitly initialize the enh_frf field in the method below in
the same way as it's done for the rest of the fields.
> unsigned long flags;
> int ret;
>
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 9e8eb2b52d5c7..414a415deb42a 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -17,6 +17,8 @@
>
> /* Synopsys DW SSI component versions (FourCC sequence) */
<-+
> #define DW_HSSI_102A 0x3130322a |
> +#define DW_HSSI_103A 0x3130332a |
|
> +#define DW_PSSI_400A 0x3430302a -+
Please define the PSSI-macros above the HSSI ones.
>
> /* DW SSI IP-core ID and version check helpers */
> #define dw_spi_ip_is(_dws, _ip) \
> @@ -94,6 +96,9 @@
> #define DW_HSSI_CTRLR0_TMOD_MASK GENMASK(11, 10)
> #define DW_HSSI_CTRLR0_SRL BIT(13)
<---------+
> #define DW_HSSI_CTRLR0_MST BIT(31) |
|
> +#define DW_HSSI_CTRLR0_SPI_FRF_MASK GENMASK(23, 22) -+
This macro should be placed above the DW_HSSI_CTRLR0_MST one. Also
rename SPI_FRF to ENH_FRF.
> +#define DW_PSSI_CTRLR0_SPI_FRF_MASK GENMASK(22, 21)
> +#define DW_SPI_CTRLR0_SPI_FRF_STD_SPI 0x0
1. Move these macros to the DW APB SSI group of the CSR fields macros.
2. Drop the SPI suffix from the DW_SPI_CTRLR0_SPI_FRF_STD_SPI macro.
3. Replace SPI_FRF with ENH_FRF name.
>
> /* Bit fields in CTRLR1 */
> #define DW_SPI_NDF_MASK GENMASK(15, 0)
> @@ -135,6 +140,7 @@ struct dw_spi_cfg {
> u8 dfs;
> u32 ndf;
> u32 freq;
> + u8 spi_frf;
Please move it to the head of the structure and rename to "enh_frf".
-Serge(y)
> };
>
> struct dw_spi;
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:20PM +0000, Sudip Mukherjee wrote:
> If the SPI transfer is being done in enhanced mode then SPI_CTRLR0
> register needs to be updated to mention the instruction length, address
> length, address and instruction transfer format, wait cycles. And, we
> also need to enable clock stretching.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 14 +++++++++++++-
> drivers/spi/spi-dw.h | 11 +++++++++++
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 8c47a4d14b666..d59401f16c47a 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -320,7 +320,7 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> {
> struct dw_spi_chip_data *chip = spi_get_ctldata(spi);
> u32 cr0 = chip->cr0;
I suggest to update the dw_spi_update_config() semantic to accepting
optional eSPI configs by means of passing an additional argument
struct dw_spi_enh_cfg *ecfg. If it's null, then no need in updating
the SPI_CTRLR0 register.
> - u32 speed_hz;
> + u32 speed_hz, spi_ctrlr0;
Just reuse the cr0 variable.
> u16 clk_div;
>
> /* CTRLR0[ 4/3: 0] or CTRLR0[ 20: 16] Data Frame Size */
> @@ -365,6 +365,18 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> dw_writel(dws, DW_SPI_RX_SAMPLE_DLY, chip->rx_sample_dly);
> dws->cur_rx_sample_dly = chip->rx_sample_dly;
> }
> +
> + if (cfg->spi_frf != DW_SPI_CTRLR0_SPI_FRF_STD_SPI) {
> + spi_ctrlr0 = DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN;
> + spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK,
> + cfg->wait_c);
> + spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_INST_L_MASK,
> + cfg->inst_l);
> + spi_ctrlr0 |= FIELD_PREP(DW_SPI_SPI_CTRLR0_ADDR_L_MASK,
> + cfg->addr_l);
> + spi_ctrlr0 |= cfg->trans_t;
Should be also handled by the FIELD_PREP() macro.
> + dw_writel(dws, DW_SPI_SPI_CTRLR0, spi_ctrlr0);
> + }
> }
> EXPORT_SYMBOL_NS_GPL(dw_spi_update_config, SPI_DW_CORE);
>
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 414a415deb42a..f29d89d05f34b 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -63,6 +63,7 @@
> #define DW_SPI_DR 0x60
> #define DW_SPI_RX_SAMPLE_DLY 0xf0
<-+
> #define DW_SPI_CS_OVERRIDE 0xf4 |
|
> +#define DW_SPI_SPI_CTRLR0 0xf4 -+
Please replace SPI_CTRLR0 with ENH_CTRLR0 and move the macro
definition to where the arrow points to.
>
> /* Bit fields in CTRLR0 (DWC APB SSI) */
> #define DW_PSSI_CTRLR0_DFS_MASK GENMASK(3, 0)
> @@ -126,6 +127,12 @@
> #define DW_SPI_DMACR_RDMAE BIT(0)
> #define DW_SPI_DMACR_TDMAE BIT(1)
>
> +/* Bit fields in SPI_CTRLR0 */
> +#define DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN BIT(30)
> +#define DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK GENMASK(15, 11)
> +#define DW_SPI_SPI_CTRLR0_INST_L_MASK GENMASK(9, 8)
> +#define DW_SPI_SPI_CTRLR0_ADDR_L_MASK GENMASK(5, 2)
First add DW_SPI_ENH_CTRLR0_TRANS_T_MASK macro too. Second please
replace SPI_CTRLR0 with ENH_CTRLR0.
> +
> /* Mem/DMA operations helpers */
> #define DW_SPI_WAIT_RETRIES 5
> #define DW_SPI_BUF_SIZE \
> @@ -141,6 +148,10 @@ struct dw_spi_cfg {
> u32 ndf;
> u32 freq;
> u8 spi_frf;
> + u8 trans_t;
> + u8 inst_l;
> + u8 addr_l;
> + u8 wait_c;
Please move these to a separate structure:
struct dw_spi_enh_cfg {
u8 wait_l;
u8 inst_l;
u8 addr_l;
u8 trans_t;
};
Thus we'll be able to add an optional argument to the
dw_spi_update_config() method.
-Serge(y)
> };
>
> struct dw_spi;
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:21PM +0000, Sudip Mukherjee wrote:
> Before doing the mem op, spi controller will be queried about the
> buswidths it supports. Add the dual/quad/octal if the controller
> has the DW_SPI_CAP_EMODE capability.
> The DW_SPI_CAP_EMODE capability will be enabled in a later patch.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 25 ++++++++++++++++++++++++-
> drivers/spi/spi-dw.h | 1 +
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index d59401f16c47a..49fad58ceb94a 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -510,6 +510,26 @@ static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> return 0;
> }
>
> +static bool dw_spi_supports_enh_mem_op(struct spi_mem *mem,
> + const struct spi_mem_op *op)
> +{
> + if (op->addr.nbytes != 0 && op->addr.buswidth != 1 &&
> + op->addr.buswidth != op->data.buswidth)
> + return false;
> +
> + if (op->cmd.buswidth != 1 && op->cmd.buswidth != op->addr.buswidth &&
> + op->cmd.buswidth != op->data.buswidth)
> + return false;
> +
> + if (op->dummy.nbytes != 0 && op->data.dir == SPI_MEM_DATA_OUT)
> + return false;
> +
> + if (op->dummy.nbytes != 0 && op->dummy.nbytes / op->dummy.buswidth > 4)
> + return false;
> +
> + return spi_mem_default_supports_op(mem, op);
> +}
> +
> static bool dw_spi_supports_mem_op(struct spi_mem *mem,
> const struct spi_mem_op *op)
> {
> @@ -792,7 +812,10 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
> if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
> !dws->set_cs) {
> dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
> - dws->mem_ops.supports_op = dw_spi_supports_mem_op;
Please see my comment to the cover letter. In order to have a more
readable method I'd suggest to convert it to something like this:
< dw_spi_init_mem_ops() {
< if (dws->mem_ops.exec_op || dws->caps & DW_SPI_CAP_CS_OVERRIDE ||
< dws->set_cs)
< return;
<
< if (dws->caps & DW_SPI_CAP_ENH_CLK_STR) {
< dws->mem_ops.adjust_op_size = dw_spi_enh_adjust_mem_op;
< dws->mem_ops.supports_op = dw_spi_enh_supports_mem_op;
< dws->mem_ops.exec_op = dw_spi_enh_exec_mem_op;
<
< return;
< }
<
< dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
< dws->mem_ops.supports_op = dw_spi_supports_mem_op;
< dws->mem_ops.exec_op = dw_spi_exec_mem_op;
<
< return;
< }
> + if (dws->caps & DW_SPI_CAP_EMODE)
Your implementation is working only if the clock-stretching feature is
available.
> + dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
> + else
> + dws->mem_ops.supports_op = dw_spi_supports_mem_op;
> dws->mem_ops.exec_op = dw_spi_exec_mem_op;
> if (!dws->max_mem_freq)
> dws->max_mem_freq = dws->max_freq;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index f29d89d05f34b..327d037bdb10e 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -34,6 +34,7 @@
> /* DW SPI controller capabilities */
> #define DW_SPI_CAP_CS_OVERRIDE BIT(0)
> #define DW_SPI_CAP_DFS32 BIT(1)
> +#define DW_SPI_CAP_EMODE BIT(2)
As I suggested in the cover letter let's make it DW_SPI_CAP_ENH (any
better suggestion?). Then the clock-stretching capability flag will be
DW_SPI_CAP_ENH_CLK_STR.
-Serge(y)
>
> /* Register offsets (Generic for both DWC APB SSI and DWC SSI IP-cores) */
> #define DW_SPI_CTRLR0 0x00
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:23PM +0000, Sudip Mukherjee wrote:
> If the spi transfer is using dual/quad/octal spi mode, then we need to
> update the SPI_CTRLR0 register. The SPI_CTRLR0 register will be updated
> in dw_spi_update_config() via the values in dw_spi_cfg.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
>
> Note: DW_SPI_SPI_CTRLR0_INST_L_INST_L16 will not work yet as
> spi_mem_default_supports_op() checks for op->cmd.nbytes != 1.
>
> drivers/spi/spi-dw-core.c | 46 +++++++++++++++++++++++++++++++++++++++
> drivers/spi/spi-dw.h | 9 ++++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 89438ae2df17d..06169aa3f37bf 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -836,10 +836,56 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
> {
> struct spi_controller *ctlr = mem->spi->controller;
> struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> + struct dw_spi_cfg cfg;
> +
> + switch (op->data.buswidth) {
> + case 2:
> + cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_DUAL_SPI;
> + break;
> + case 4:
> + cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_QUAD_SPI;
> + break;
> + case 8:
> + cfg.spi_frf = DW_SPI_CTRLR0_SPI_FRF_OCT_SPI;
> + break;
> + default:
> + return dw_spi_exec_mem_op(mem, op);
case 1:
return dw_spi_exec_mem_op(mem, op);
case 2:
cfg.enh_frf = DW_SPI_CTRLR0_SPI_FRF_DUAL_SPI;
break;
...
default:
return -EINVAL;
> + }
>
> /* Collect cmd and addr into a single buffer */
> dw_spi_init_enh_mem_buf(dws, op);
>
> + cfg.dfs = 8;
> + cfg.freq = clamp(mem->spi->max_speed_hz, 0U, dws->max_mem_freq);
> + cfg.ndf = op->data.nbytes;
> + if (op->data.dir == SPI_MEM_DATA_IN)
> + cfg.tmode = DW_SPI_CTRLR0_TMOD_RO;
> + else
> + cfg.tmode = DW_SPI_CTRLR0_TMOD_TO;
Newline, please.
> + if (op->data.buswidth == op->addr.buswidth &&
> + op->data.buswidth == op->cmd.buswidth)
> + cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT2;
> + else if (op->data.buswidth == op->addr.buswidth)
> + cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT1;
> + else
> + cfg.trans_t = DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT0;
> +
> + cfg.addr_l = clamp(op->addr.nbytes * 2, 0, 0xf);
First the address length should be checked in the
spi_controller_mem_ops.supports_op method. Thus the clamping procedure
would be redundant. Second instead of using the multiplication
operator I would suggest to have the bitwise left-shift op utilized
here, (cfg.addr_l = op->addr.nbytes << 2). This shall look a bit more
coherent.
> + if (op->cmd.nbytes > 1)
> + cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L16;
No. Firstly INST_L16 means 2-bytes length. Using greater-than op here
is incorrect. Secondly the command part length should be
checked in the spi_controller_mem_ops.supports_op callback.
> + else if (op->cmd.nbytes == 1)
> + cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L8;
> + else
> + cfg.inst_l = DW_SPI_SPI_CTRLR0_INST_L_INST_L0;
> +
> + cfg.wait_c = (op->dummy.nbytes * (BITS_PER_BYTE / op->dummy.buswidth));
Hm, what if buswidth is zero?
> +
> + dw_spi_enable_chip(dws, 0);
> +
> + dw_spi_update_config(dws, mem->spi, &cfg);
> +
> + dw_spi_enable_chip(dws, 1);
> +
> return 0;
> }
>
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 327d037bdb10e..494b830ad1026 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -101,6 +101,9 @@
> #define DW_HSSI_CTRLR0_SPI_FRF_MASK GENMASK(23, 22)
> #define DW_PSSI_CTRLR0_SPI_FRF_MASK GENMASK(22, 21)
> #define DW_SPI_CTRLR0_SPI_FRF_STD_SPI 0x0
> +#define DW_SPI_CTRLR0_SPI_FRF_DUAL_SPI 0x1
> +#define DW_SPI_CTRLR0_SPI_FRF_QUAD_SPI 0x2
> +#define DW_SPI_CTRLR0_SPI_FRF_OCT_SPI 0x3
Please replace SPI_FRF with ENH_FRF and drop _SPI suffix from the
macros.
>
> /* Bit fields in CTRLR1 */
> #define DW_SPI_NDF_MASK GENMASK(15, 0)
> @@ -132,7 +135,13 @@
> #define DW_SPI_SPI_CTRLR0_CLK_STRETCH_EN BIT(30)
> #define DW_SPI_SPI_CTRLR0_WAIT_CYCLE_MASK GENMASK(15, 11)
> #define DW_SPI_SPI_CTRLR0_INST_L_MASK GENMASK(9, 8)
> +#define DW_SPI_SPI_CTRLR0_INST_L_INST_L0 0x0
> +#define DW_SPI_SPI_CTRLR0_INST_L_INST_L8 0x2
> +#define DW_SPI_SPI_CTRLR0_INST_L_INST_L16 0x3
> #define DW_SPI_SPI_CTRLR0_ADDR_L_MASK GENMASK(5, 2)
> +#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT0 0x0
> +#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT1 0x1
> +#define DW_SPI_SPI_CTRLR0_TRANS_TYPE_TT2 0x2
Please replace SPI_CTRLR0 with ENH_CTRLR0.
-Serge(y)
>
> /* Mem/DMA operations helpers */
> #define DW_SPI_WAIT_RETRIES 5
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:24PM +0000, Sudip Mukherjee wrote:
> In enhanced spi mode, read or write will start by sending the cmd
> and address (if present).
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 06169aa3f37bf..ecab0fbc847c7 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -832,6 +832,29 @@ static void dw_spi_init_enh_mem_buf(struct dw_spi *dws, const struct spi_mem_op
> }
> }
>
> +static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_op *op)
> +{
> + void *buf = dws->buf;
> + u32 txw;
> +
> + /* Send cmd as 32 bit value */
> + if (buf) {
> + txw = *(u32 *)(buf);
> + dw_write_io_reg(dws, DW_SPI_DR, txw);
> + buf += dws->reg_io_width;
> + if (op->addr.nbytes) {
> + txw = *(u32 *)(buf);
> + dw_write_io_reg(dws, DW_SPI_DR, txw);
> + if (op->addr.nbytes > 4) {
> + /* address more than 32bit */
> + buf += dws->reg_io_width;
> + txw = *(u32 *)(buf);
> + dw_write_io_reg(dws, DW_SPI_DR, txw);
> + }
> + }
> + }
Just put the command and address directly to the CSR. There is no
point in using the temporary buffer in your case unless I miss
something.
-Serge(y)
> +}
> +
> static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> {
> struct spi_controller *ctlr = mem->spi->controller;
> @@ -886,6 +909,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>
> dw_spi_enable_chip(dws, 1);
>
> + dw_spi_enh_write_cmd_addr(dws, op);
> +
> return 0;
> }
>
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:22PM +0000, Sudip Mukherjee wrote:
> If the DW_SPI_CAP_EMODE capability is enabled then dw_spi_exec_enh_mem_op()
> will be used as the new enhanced mem_op. Lets initialize the buffer and
> get the pointers to receive and transmit data buffers.
> The DW_SPI_CAP_EMODE capability will be enabled in a later patch.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 53 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 49fad58ceb94a..89438ae2df17d 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -798,6 +798,51 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> return ret;
> }
>
> +static void dw_spi_init_enh_mem_buf(struct dw_spi *dws, const struct spi_mem_op *op)
> +{
> + unsigned int i, j;
> + u8 *out;
> +
> + out = dws->buf;
> + for (i = 0; i < DW_SPI_BUF_SIZE; ++i)
> + out[i] = 0;
> +
> + for (i = 0, j = op->cmd.nbytes; i < op->cmd.nbytes; ++i, --j)
> + out[i] = DW_SPI_GET_BYTE(op->cmd.opcode, op->cmd.nbytes - j);
> +
> + for (j = op->addr.nbytes, i = dws->reg_io_width; j > 0; ++i, --j)
> + out[i] = DW_SPI_GET_BYTE(op->addr.val, op->addr.nbytes - j);
In case of the non-eSPI implementation the outbound data consolidation
was required to get the most optimal loop of data transfer. In this
case I don't see it was required since the clock stretching feature is
available and the IRQ-based xfer procedure is implemented. Do I miss
something?
-Serge(y)
> +
> + dws->n_bytes = 1;
> + if (op->data.dir == SPI_MEM_DATA_IN) {
> + dws->rx = op->data.buf.in;
> + dws->rx_len = op->data.nbytes;
> + dws->tx = NULL;
> + dws->tx_len = 0;
> + } else if (op->data.dir == SPI_MEM_DATA_OUT) {
> + dws->tx_len = op->data.nbytes;
> + dws->tx = (void *)op->data.buf.out;
> + dws->rx = NULL;
> + dws->rx_len = 0;
> + } else {
> + dws->rx = NULL;
> + dws->rx_len = 0;
> + dws->tx = NULL;
> + dws->tx_len = 0;
> + }
> +}
> +
> +static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> + struct spi_controller *ctlr = mem->spi->controller;
> + struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> +
> + /* Collect cmd and addr into a single buffer */
> + dw_spi_init_enh_mem_buf(dws, op);
> +
> + return 0;
> +}
> +
> /*
> * Initialize the default memory operations if a glue layer hasn't specified
> * custom ones. Direct mapping operations will be preserved anyway since DW SPI
> @@ -812,11 +857,13 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
> if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
> !dws->set_cs) {
> dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
> - if (dws->caps & DW_SPI_CAP_EMODE)
> + if (dws->caps & DW_SPI_CAP_EMODE) {
> + dws->mem_ops.exec_op = dw_spi_exec_enh_mem_op;
> dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
> - else
> + } else {
> + dws->mem_ops.exec_op = dw_spi_exec_mem_op;
> dws->mem_ops.supports_op = dw_spi_supports_mem_op;
> - dws->mem_ops.exec_op = dw_spi_exec_mem_op;
> + }
> if (!dws->max_mem_freq)
> dws->max_mem_freq = dws->max_freq;
> }
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:25PM +0000, Sudip Mukherjee wrote:
> The current mem_ops is not using interrupt based transfer so
> dw_spi_irq_setup() only has one interrput handler to handle the non
> mem_ops transfers. We will use interrupt based transfers in enhanced
> spi and so we need a way to specify which irq handler to use.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index ecab0fbc847c7..f540165245a89 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -260,7 +260,8 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
> if (!irq_status)
> return IRQ_NONE;
>
> - if (!master->cur_msg) {
> + if (!master->cur_msg && dws->transfer_handler ==
> + dw_spi_transfer_handler) {
What about replacing this with the (!dws->rx_len && !dws->tx_len)
statement?
> dw_spi_mask_intr(dws, 0xff);
> return IRQ_HANDLED;
> }
> @@ -380,7 +381,8 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi,
> }
> EXPORT_SYMBOL_NS_GPL(dw_spi_update_config, SPI_DW_CORE);
>
> -static void dw_spi_irq_setup(struct dw_spi *dws)
> +static void dw_spi_irq_setup(struct dw_spi *dws,
> + irqreturn_t (*t_handler)(struct dw_spi *))
> {
> u16 level;
> u8 imask;
> @@ -394,7 +396,7 @@ static void dw_spi_irq_setup(struct dw_spi *dws)
> dw_writel(dws, DW_SPI_TXFTLR, level);
> dw_writel(dws, DW_SPI_RXFTLR, level - 1);
>
> - dws->transfer_handler = dw_spi_transfer_handler;
> + dws->transfer_handler = t_handler;
>
> imask = DW_SPI_INT_TXEI | DW_SPI_INT_TXOI |
> DW_SPI_INT_RXUI | DW_SPI_INT_RXOI | DW_SPI_INT_RXFI;
I'd suggest to create a separate dw_spi_enh_irq_setup() method which
would unmask only the required IRQs, initialize the threshold level
depending on the transfer type and set the
dw_spi_enh_transfer_handler() handler.
-Serge(y)
> @@ -486,7 +488,7 @@ static int dw_spi_transfer_one(struct spi_controller *master,
> else if (dws->irq == IRQ_NOTCONNECTED)
> return dw_spi_poll_transfer(dws, transfer);
>
> - dw_spi_irq_setup(dws);
> + dw_spi_irq_setup(dws, dw_spi_transfer_handler);
>
> return 1;
> }
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:26PM +0000, Sudip Mukherjee wrote:
> Introduce the interrupt handler for enhanced spi to read or write based
> on the generated irq. Also, use the xfer_completion from spi_controller
> to wait for a timeout or completion from irq handler.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 62 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index f540165245a89..10d453228368f 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -251,6 +251,34 @@ static irqreturn_t dw_spi_transfer_handler(struct dw_spi *dws)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t dw_spi_enh_handler(struct dw_spi *dws)
> +{
> + u16 irq_status = dw_readl(dws, DW_SPI_ISR);
> +
> + if (dw_spi_check_status(dws, false)) {
> + spi_finalize_current_transfer(dws->master);
As I suggested in the cover-letter please use the dw_spi_dma_wait()
function for that.
> + return IRQ_HANDLED;
> + }
> +
> + if (irq_status & DW_SPI_INT_RXFI) {
> + dw_reader(dws);
> + if (dws->rx_len <= dw_readl(dws, DW_SPI_RXFTLR))
> + dw_writel(dws, DW_SPI_RXFTLR, dws->rx_len - 1);
> + }
> +
> + if (irq_status & DW_SPI_INT_TXEI)
> + dw_writer(dws);
> +
> + if (!dws->tx_len && dws->rx_len) {
> + dw_spi_mask_intr(dws, DW_SPI_INT_TXEI);
> + } else if (!dws->rx_len && !dws->tx_len) {
> + dw_spi_mask_intr(dws, 0xff);
> + spi_finalize_current_transfer(dws->master);
Why so complicated? You have two types of the transfers: Tx-only and
Rx-only. Thus you can unmask only one type of the IRQs and terminate
the process upon both lengths are zero.
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t dw_spi_irq(int irq, void *dev_id)
> {
> struct spi_controller *master = dev_id;
> @@ -265,6 +293,12 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
> dw_spi_mask_intr(dws, 0xff);
> return IRQ_HANDLED;
> }
> + if ((dws->transfer_handler == dw_spi_enh_handler &&
> + !dws->rx_len && !dws->tx_len)) {
> + dw_spi_mask_intr(dws, 0xff);
> + spi_finalize_current_transfer(master);
> + return IRQ_HANDLED;
Why? You already have this statement in the handler above.
> + }
>
> return dws->transfer_handler(dws);
> }
> @@ -862,6 +896,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
> struct spi_controller *ctlr = mem->spi->controller;
> struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> struct dw_spi_cfg cfg;
> + int ret = 0;
> + unsigned long long ms;
>
> switch (op->data.buswidth) {
> case 2:
> @@ -909,11 +945,35 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>
> dw_spi_update_config(dws, mem->spi, &cfg);
>
> + dw_spi_mask_intr(dws, 0xff);
> + reinit_completion(&ctlr->xfer_completion);
> dw_spi_enable_chip(dws, 1);
>
> dw_spi_enh_write_cmd_addr(dws, op);
> + dw_spi_set_cs(mem->spi, false);
> + dw_spi_irq_setup(dws, dw_spi_enh_handler);
>
> - return 0;
> + /* Use timeout calculation from spi_transfer_wait() */
> + ms = 8LL * MSEC_PER_SEC * (dws->rx_len ? dws->rx_len : dws->tx_len);
> + do_div(ms, dws->current_freq);
> +
> + /*
> + * Increase it twice and add 200 ms tolerance, use
> + * predefined maximum in case of overflow.
> + */
> + ms += ms + 200;
> + if (ms > UINT_MAX)
> + ms = UINT_MAX;
> +
> + ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> + msecs_to_jiffies(ms));
All of that is already implemented in the dw_spi_dma_wait() method.
Moreover addr+cmd write procedure, IRQ setup and wait-for-completion
can be consolidate in the dw_spi_enh_write_then_read() function thus
having the dw_spi_enh_exec_mem_op method looking similar to the
standard dw_spi_exec_mem_op().
-Serge(y)
> +
> + dw_spi_stop_mem_op(dws, mem->spi);
> +
> + if (ms == 0)
> + ret = -EIO;
> +
> + return ret;
> }
>
> /*
> --
> 2.30.2
>
On Mon, Dec 12, 2022 at 06:07:29PM +0000, Sudip Mukherjee wrote:
> If the connection to the spi device is not stable then the transfer can
> fail. Add retry for DW_SPI_WAIT_RETRIES times and print error if it still
> fails.
>
> Signed-off-by: Sudip Mukherjee <[email protected]>
> ---
> drivers/spi/spi-dw-core.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index dff7b419af304..cef56acd8d8fd 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -906,7 +906,7 @@ static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_o
> }
> }
>
> -static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +static int dw_spi_try_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> {
> struct spi_controller *ctlr = mem->spi->controller;
> struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> @@ -991,6 +991,21 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
> return ret;
> }
>
> +static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> + struct spi_controller *ctlr = mem->spi->controller;
> + struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> + int retry, ret = -EIO;
> +
> + for (retry = 0; retry < DW_SPI_WAIT_RETRIES && ret != 0; retry++)
> + ret = dw_spi_try_enh_mem_op(mem, op);
> +
> + if (retry == DW_SPI_WAIT_RETRIES)
> + dev_err(&dws->master->dev, "Retry of enh_mem_op failed\n");
> +
> + return ret;
No. If something goes wrong during the transfer you should return an
error and let the upper layer to decide whether to retry or pass the
failure further.
-Serge(y)
> +}
> +
> /*
> * Initialize the default memory operations if a glue layer hasn't specified
> * custom ones. Direct mapping operations will be preserved anyway since DW SPI
> --
> 2.30.2
>
On Mon, Jan 9, 2023 at 4:25 PM Serge Semin <[email protected]> wrote:
>
> Hello Sudip
>
> On Thu, Jan 05, 2023 at 01:20:39AM +0300, Serge Semin wrote:
> > Hi Sudip
> >
> > On Sun, Dec 18, 2022 at 08:45:26PM +0300, Serge Semin wrote:
> > > Hi Sudip
> > >
> > > On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> > > > The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> > > > controllers support enhanced SPI which includes Dual mode, Quad mode and
> > > > Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> > > > which can be used to prevent FIFO underflow and overflow conditions while
> > > > transmitting or receiving the data respectively.
> > > >
> > > > This is almost a complete rework based on the review from Serge.
> > >
> > > Thank you very much for the series. I'll have a look at it on the next
> > > week.
> >
> > Just so you know. I haven't forgot about the series. There are some
> > problematic parts which I need to give more thinking than I originally
> > expected. I'll submit my comments very soon. Sorry for the delay.
> >
> > Good news is that I've got the HW-manual for the DW SSI v1.01a
> > IP-core. So I'll no longer need to ask of you about that device
> > implementation specifics.
>
> Finally I managed to consolidate my thoughts regarding your patchset.
> Here is the summary. Some specific comments will be sent in-reply to
> the corresponding patches.
>
> First of all there is a crucial difference between eSPI capability
> available on DW APB SSI and DW AHB SSI controllers:
> DW APB SSI 4.x:
> + Tx until FIFO is empty
> + No clock stretching at all
Thanks for your detailed review and all the additional details about
DW APB SSI. I did not have this datasheet to check.
So, that will mean I can remove the APB versiom detection from my next series.
But unfortunately, I don't have access to the hardware currently to
prepare and test the v3 series. It will be delayed a bit and I am
hoping I will be able to work on this by early March.
--
Regards
Sudip
On Thu, Jan 19, 2023 at 04:26:58PM +0000, Sudip Mukherjee wrote:
> On Mon, Jan 9, 2023 at 4:25 PM Serge Semin <[email protected]> wrote:
> >
> > Hello Sudip
> >
> > On Thu, Jan 05, 2023 at 01:20:39AM +0300, Serge Semin wrote:
> > > Hi Sudip
> > >
> > > On Sun, Dec 18, 2022 at 08:45:26PM +0300, Serge Semin wrote:
> > > > Hi Sudip
> > > >
> > > > On Mon, Dec 12, 2022 at 06:07:17PM +0000, Sudip Mukherjee wrote:
> > > > > The is v2 of the patch series adding enhanced SPI support. Some Synopsys SSI
> > > > > controllers support enhanced SPI which includes Dual mode, Quad mode and
> > > > > Octal mode. DWC_ssi includes clock stretching feature in enhanced SPI modes
> > > > > which can be used to prevent FIFO underflow and overflow conditions while
> > > > > transmitting or receiving the data respectively.
> > > > >
> > > > > This is almost a complete rework based on the review from Serge.
> > > >
> > > > Thank you very much for the series. I'll have a look at it on the next
> > > > week.
> > >
> > > Just so you know. I haven't forgot about the series. There are some
> > > problematic parts which I need to give more thinking than I originally
> > > expected. I'll submit my comments very soon. Sorry for the delay.
> > >
> > > Good news is that I've got the HW-manual for the DW SSI v1.01a
> > > IP-core. So I'll no longer need to ask of you about that device
> > > implementation specifics.
> >
> > Finally I managed to consolidate my thoughts regarding your patchset.
> > Here is the summary. Some specific comments will be sent in-reply to
> > the corresponding patches.
> >
> > First of all there is a crucial difference between eSPI capability
> > available on DW APB SSI and DW AHB SSI controllers:
> > DW APB SSI 4.x:
> > + Tx until FIFO is empty
> > + No clock stretching at all
>
> Thanks for your detailed review and all the additional details about
> DW APB SSI. I did not have this datasheet to check.
> So, that will mean I can remove the APB versiom detection from my next series.
> But unfortunately, I don't have access to the hardware currently to
> prepare and test the v3 series. It will be delayed a bit and I am
> hoping I will be able to work on this by early March.
Ok. Thanks for the update. Whenever you're ready I'll be here for review.
-Serge(y)
>
>
> --
> Regards
> Sudip