This patchset adds 36bit dma address and power management
supports for mt8192-nor.
Changes in v3:
- Fix a bugfix of v2 in checking spi memory operation.
- split read_dma function into two (normal/bounce)
- Support 7bytes generic spi xfer
Changes in v2:
- Add power management support
- Fix bugs in checking spi memory operation.
- use dma_alloc_coherent for allocating bounce buffer
- code cleanups
Ikjoon Jang (6):
dt-bindings: spi: add mt8192-nor compatible string
spi: spi-mtk-nor: fix mishandled logics in checking SPI memory
operation
spi: spi-mtk-nor: support 7 bytes transfer of generic spi
spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer
spi: spi-mtk-nor: support 36bit dma addressing
spi: spi-mtk-nor: Add power management support
.../bindings/spi/mediatek,spi-mtk-nor.yaml | 1 +
drivers/spi/spi-mtk-nor.c | 333 ++++++++++++------
2 files changed, 230 insertions(+), 104 deletions(-)
--
2.28.0.681.g6f77f65b4e-goog
Add MT8192 spi-nor controller support.
Signed-off-by: Ikjoon Jang <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
(no changes since v1)
Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml
index 42c9205ac991..55c239446a5b 100644
--- a/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml
@@ -30,6 +30,7 @@ properties:
- mediatek,mt7622-nor
- mediatek,mt7623-nor
- mediatek,mt7629-nor
+ - mediatek,mt8192-nor
- enum:
- mediatek,mt8173-nor
- items:
--
2.28.0.681.g6f77f65b4e-goog
Fix a bug which limits its protocol availability in supports_op().
Fixes: a59b2c7c56bf ("spi: spi-mtk-nor: support standard spi properties")
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 6e6ca2b8e6c8..0f7d4ec68730 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -211,28 +211,24 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
if (op->cmd.buswidth != 1)
return false;
+ if (!spi_mem_default_supports_op(mem, op))
+ return false;
+
if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
- switch(op->data.dir) {
- case SPI_MEM_DATA_IN:
- if (!mtk_nor_match_read(op))
- return false;
- break;
- case SPI_MEM_DATA_OUT:
- if ((op->addr.buswidth != 1) ||
- (op->dummy.nbytes != 0) ||
- (op->data.buswidth != 1))
- return false;
- break;
- default:
- break;
- }
+ if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
+ return true;
+ else if (op->data.dir == SPI_MEM_DATA_OUT)
+ return (op->addr.buswidth == 1) &&
+ (op->dummy.nbytes == 0) &&
+ (op->data.buswidth == 1);
}
+
len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
if ((len > MTK_NOR_PRG_MAX_SIZE) ||
((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
return false;
- return spi_mem_default_supports_op(mem, op);
+ return true;
}
static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
--
2.28.0.681.g6f77f65b4e-goog
When mtk-nor fallbacks to generic spi transfers, it can actually
transfer up to 7 bytes.
This patch fixes adjust_op_size() and supports_op() to explicitly
check 7 bytes range and also fixes possible under/overflow conditions
in register offsets calculation.
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
1 file changed, 76 insertions(+), 26 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 0f7d4ec68730..e7719d249095 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -79,7 +79,11 @@
#define MTK_NOR_REG_DMA_DADR 0x720
#define MTK_NOR_REG_DMA_END_DADR 0x724
+/* maximum bytes of TX in PRG mode */
#define MTK_NOR_PRG_MAX_SIZE 6
+/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
+#define MTK_NOR_PRG_MAX_CYCLES 7
+
// Reading DMA src/dst addresses have to be 16-byte aligned
#define MTK_NOR_DMA_ALIGN 16
#define MTK_NOR_DMA_ALIGN_MASK (MTK_NOR_DMA_ALIGN - 1)
@@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
return false;
}
+static bool mtk_nor_check_prg(const struct spi_mem_op *op)
+{
+ size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+ if (len > MTK_NOR_PRG_MAX_SIZE)
+ return false;
+
+ if (!op->data.nbytes)
+ return true;
+
+ if (op->data.dir == SPI_MEM_DATA_OUT)
+ return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
+ else if (op->data.dir == SPI_MEM_DATA_IN)
+ return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
+ else
+ return true;
+}
+
static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
{
size_t len;
@@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
}
}
- len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
- op->dummy.nbytes;
- if (op->data.nbytes > len)
- op->data.nbytes = len;
+ if (mtk_nor_check_prg(op))
+ return 0;
+
+ len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+ if (op->data.dir == SPI_MEM_DATA_OUT) {
+ if (len == MTK_NOR_PRG_MAX_SIZE)
+ return -EINVAL;
+ op->data.nbytes = min_t(unsigned int, op->data.nbytes,
+ MTK_NOR_PRG_MAX_SIZE - len);
+ } else {
+ if (len == MTK_NOR_PRG_MAX_CYCLES)
+ return -EINVAL;
+ op->data.nbytes = min_t(unsigned int, op->data.nbytes,
+ MTK_NOR_PRG_MAX_CYCLES - len);
+ }
return 0;
}
@@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
static bool mtk_nor_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
- size_t len;
-
if (op->cmd.buswidth != 1)
return false;
@@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
(op->data.buswidth == 1);
}
- len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
- if ((len > MTK_NOR_PRG_MAX_SIZE) ||
- ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
+ /* fallback to generic spi xfer */
+ if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
return false;
- return true;
+ return mtk_nor_check_prg(op);
}
static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
@@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
int stat = 0;
int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
void __iomem *reg;
- const u8 *txbuf;
- u8 *rxbuf;
- int i;
+ int i, tx_len = 0, rx_len = 0;
list_for_each_entry(t, &m->transfers, transfer_list) {
- txbuf = t->tx_buf;
- for (i = 0; i < t->len; i++, reg_offset--) {
+ const u8 *txbuf = t->tx_buf;
+
+ if (!txbuf) {
+ rx_len += t->len;
+ continue;
+ }
+
+ if (rx_len) {
+ stat = -EPROTO;
+ goto msg_done;
+ }
+
+ for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
- if (txbuf)
- writeb(txbuf[i], reg);
- else
- writeb(0, reg);
+ writeb(txbuf[i], reg);
+ tx_len++;
}
- trx_len += t->len;
}
+ while (reg_offset >= 0) {
+ writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
+ reg_offset--;
+ }
+
+ rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
+ trx_len = tx_len + rx_len;
+
writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
@@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
if (stat < 0)
goto msg_done;
- reg_offset = trx_len - 1;
- list_for_each_entry(t, &m->transfers, transfer_list) {
- rxbuf = t->rx_buf;
- for (i = 0; i < t->len; i++, reg_offset--) {
- reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
- if (rxbuf)
+ if (rx_len > 0) {
+ reg_offset = rx_len - 1;
+ list_for_each_entry(t, &m->transfers, transfer_list) {
+ u8 *rxbuf = t->rx_buf;
+
+ if (!rxbuf)
+ continue;
+
+ for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
+ reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
rxbuf[i] = readb(reg);
+ }
}
}
--
2.28.0.681.g6f77f65b4e-goog
Use dma_alloc_coherent() for bounce buffer instead of kmalloc() to
make sure the bounce buffer to be allocated within its DMAable range.
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 93 +++++++++++++++++++++------------------
1 file changed, 51 insertions(+), 42 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index e7719d249095..8dbafee7f431 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -100,6 +100,7 @@ struct mtk_nor {
struct device *dev;
void __iomem *base;
u8 *buffer;
+ dma_addr_t buffer_dma;
struct clk *spi_clk;
struct clk *ctlr_clk;
unsigned int spi_freq;
@@ -148,6 +149,11 @@ static void mtk_nor_set_addr(struct mtk_nor *sp, const struct spi_mem_op *op)
}
}
+static bool need_bounce(struct mtk_nor *sp, const struct spi_mem_op *op)
+{
+ return ((uintptr_t)op->data.buf.in & MTK_NOR_DMA_ALIGN_MASK);
+}
+
static bool mtk_nor_match_read(const struct spi_mem_op *op)
{
int dummy = 0;
@@ -191,6 +197,7 @@ static bool mtk_nor_check_prg(const struct spi_mem_op *op)
static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
{
+ struct mtk_nor *sp = spi_controller_get_devdata(mem->spi->master);
size_t len;
if (!op->data.nbytes)
@@ -202,8 +209,7 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
(op->data.nbytes < MTK_NOR_DMA_ALIGN))
op->data.nbytes = 1;
- else if (!((ulong)(op->data.buf.in) &
- MTK_NOR_DMA_ALIGN_MASK))
+ else if (!need_bounce(sp, op))
op->data.nbytes &= ~MTK_NOR_DMA_ALIGN_MASK;
else if (op->data.nbytes > MTK_NOR_BOUNCE_BUF_SIZE)
op->data.nbytes = MTK_NOR_BOUNCE_BUF_SIZE;
@@ -288,19 +294,12 @@ static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
mtk_nor_rmw(sp, MTK_NOR_REG_BUSCFG, reg, MTK_NOR_BUS_MODE_MASK);
}
-static int mtk_nor_read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
- u8 *buffer)
+static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
+ dma_addr_t dma_addr)
{
int ret = 0;
ulong delay;
u32 reg;
- dma_addr_t dma_addr;
-
- dma_addr = dma_map_single(sp->dev, buffer, length, DMA_FROM_DEVICE);
- if (dma_mapping_error(sp->dev, dma_addr)) {
- dev_err(sp->dev, "failed to map dma buffer.\n");
- return -EINVAL;
- }
writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
@@ -325,30 +324,49 @@ static int mtk_nor_read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
(delay + 1) * 100);
}
- dma_unmap_single(sp->dev, dma_addr, length, DMA_FROM_DEVICE);
if (ret < 0)
dev_err(sp->dev, "dma read timeout.\n");
return ret;
}
-static int mtk_nor_read_bounce(struct mtk_nor *sp, u32 from,
- unsigned int length, u8 *buffer)
+static int mtk_nor_read_bounce(struct mtk_nor *sp, const struct spi_mem_op *op)
{
unsigned int rdlen;
int ret;
- if (length & MTK_NOR_DMA_ALIGN_MASK)
- rdlen = (length + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
+ if (op->data.nbytes & MTK_NOR_DMA_ALIGN_MASK)
+ rdlen = (op->data.nbytes + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
else
- rdlen = length;
+ rdlen = op->data.nbytes;
- ret = mtk_nor_read_dma(sp, from, rdlen, sp->buffer);
- if (ret)
- return ret;
+ ret = mtk_nor_dma_exec(sp, op->addr.val, rdlen, sp->buffer_dma);
- memcpy(buffer, sp->buffer, length);
- return 0;
+ if (!ret)
+ memcpy(op->data.buf.in, sp->buffer, op->data.nbytes);
+
+ return ret;
+}
+
+static int mtk_nor_read_dma(struct mtk_nor *sp, const struct spi_mem_op *op)
+{
+ int ret;
+ dma_addr_t dma_addr;
+
+ if (need_bounce(sp, op))
+ return mtk_nor_read_bounce(sp, op);
+
+ dma_addr = dma_map_single(sp->dev, op->data.buf.in,
+ op->data.nbytes, DMA_FROM_DEVICE);
+
+ if (dma_mapping_error(sp->dev, dma_addr))
+ return -EINVAL;
+
+ ret = mtk_nor_dma_exec(sp, op->addr.val, op->data.nbytes, dma_addr);
+
+ dma_unmap_single(sp->dev, dma_addr, op->data.nbytes, DMA_FROM_DEVICE);
+
+ return ret;
}
static int mtk_nor_read_pio(struct mtk_nor *sp, const struct spi_mem_op *op)
@@ -452,15 +470,8 @@ static int mtk_nor_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
if (op->data.nbytes == 1) {
mtk_nor_set_addr(sp, op);
return mtk_nor_read_pio(sp, op);
- } else if (((ulong)(op->data.buf.in) &
- MTK_NOR_DMA_ALIGN_MASK)) {
- return mtk_nor_read_bounce(sp, op->addr.val,
- op->data.nbytes,
- op->data.buf.in);
} else {
- return mtk_nor_read_dma(sp, op->addr.val,
- op->data.nbytes,
- op->data.buf.in);
+ return mtk_nor_read_dma(sp, op);
}
}
@@ -634,7 +645,6 @@ static int mtk_nor_probe(struct platform_device *pdev)
struct spi_controller *ctlr;
struct mtk_nor *sp;
void __iomem *base;
- u8 *buffer;
struct clk *spi_clk, *ctlr_clk;
int ret, irq;
@@ -650,16 +660,6 @@ static int mtk_nor_probe(struct platform_device *pdev)
if (IS_ERR(ctlr_clk))
return PTR_ERR(ctlr_clk);
- buffer = devm_kmalloc(&pdev->dev,
- MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
- GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
-
- if ((ulong)buffer & MTK_NOR_DMA_ALIGN_MASK)
- buffer = (u8 *)(((ulong)buffer + MTK_NOR_DMA_ALIGN) &
- ~MTK_NOR_DMA_ALIGN_MASK);
-
ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp));
if (!ctlr) {
dev_err(&pdev->dev, "failed to allocate spi controller\n");
@@ -679,13 +679,22 @@ static int mtk_nor_probe(struct platform_device *pdev)
sp = spi_controller_get_devdata(ctlr);
sp->base = base;
- sp->buffer = buffer;
sp->has_irq = false;
sp->wbuf_en = false;
sp->ctlr = ctlr;
sp->dev = &pdev->dev;
sp->spi_clk = spi_clk;
sp->ctlr_clk = ctlr_clk;
+ sp->buffer = dmam_alloc_coherent(&pdev->dev,
+ MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
+ &sp->buffer_dma, GFP_KERNEL);
+ if (!sp->buffer)
+ return -ENOMEM;
+
+ if ((uintptr_t)sp->buffer & MTK_NOR_DMA_ALIGN_MASK) {
+ dev_err(sp->dev, "misaligned allocation of internal buffer.\n");
+ return -ENOMEM;
+ }
irq = platform_get_irq_optional(pdev, 0);
if (irq < 0) {
--
2.28.0.681.g6f77f65b4e-goog
This patch adds dev_pm_ops to mtk-nor to support suspend/resume,
auto suspend delay is set to -1 by default.
Accessing registers are only permitted after its clock is enabled
to deal with unknown state of operating clk at probe time,
Signed-off-by: Ikjoon Jang <[email protected]>
---
Changes in v3:
- Fix a bugfix of v2 in checking spi memory operation.
- split read_dma function into two (normal/bounce)
- Support 7bytes generic spi xfer
Changes in v2:
- Add power management support
- Fix bugs in checking spi memory operation.
- use dma_alloc_coherent for allocating bounce buffer
- code cleanups
drivers/spi/spi-mtk-nor.c | 98 ++++++++++++++++++++++++++++++---------
1 file changed, 76 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 35205635ed42..bde4c846ce65 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -14,6 +14,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
#include <linux/spi/spi.h>
#include <linux/spi/spi-mem.h>
#include <linux/string.h>
@@ -592,22 +593,15 @@ static int mtk_nor_enable_clk(struct mtk_nor *sp)
return 0;
}
-static int mtk_nor_init(struct mtk_nor *sp)
+static void mtk_nor_init(struct mtk_nor *sp)
{
- int ret;
-
- ret = mtk_nor_enable_clk(sp);
- if (ret)
- return ret;
-
- sp->spi_freq = clk_get_rate(sp->spi_clk);
+ writel(0, sp->base + MTK_NOR_REG_IRQ_EN);
+ writel(MTK_NOR_IRQ_MASK, sp->base + MTK_NOR_REG_IRQ_STAT);
writel(MTK_NOR_ENABLE_SF_CMD, sp->base + MTK_NOR_REG_WP);
mtk_nor_rmw(sp, MTK_NOR_REG_CFG2, MTK_NOR_WR_CUSTOM_OP_EN, 0);
mtk_nor_rmw(sp, MTK_NOR_REG_CFG3,
MTK_NOR_DISABLE_WREN | MTK_NOR_DISABLE_SR_POLL, 0);
-
- return ret;
}
static irqreturn_t mtk_nor_irq_handler(int irq, void *data)
@@ -690,6 +684,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
ctlr->num_chipselect = 1;
ctlr->setup = mtk_nor_setup;
ctlr->transfer_one_message = mtk_nor_transfer_one_message;
+ ctlr->auto_runtime_pm = true;
dev_set_drvdata(&pdev->dev, ctlr);
@@ -712,12 +707,19 @@ static int mtk_nor_probe(struct platform_device *pdev)
return -ENOMEM;
}
+ ret = mtk_nor_enable_clk(sp);
+ if (ret < 0)
+ return ret;
+
+ sp->spi_freq = clk_get_rate(sp->spi_clk);
+
+ mtk_nor_init(sp);
+
irq = platform_get_irq_optional(pdev, 0);
+
if (irq < 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
- writel(MTK_NOR_IRQ_MASK, base + MTK_NOR_REG_IRQ_STAT);
- writel(0, base + MTK_NOR_REG_IRQ_EN);
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
pdev->name, sp);
if (ret < 0) {
@@ -728,34 +730,86 @@ static int mtk_nor_probe(struct platform_device *pdev)
}
}
- ret = mtk_nor_init(sp);
- if (ret < 0) {
- kfree(ctlr);
- return ret;
- }
+ pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+ pm_runtime_use_autosuspend(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_noresume(&pdev->dev);
+
+ ret = devm_spi_register_controller(&pdev->dev, ctlr);
+ if (ret < 0)
+ goto err_probe;
+
+ pm_runtime_mark_last_busy(&pdev->dev);
+ pm_runtime_put_autosuspend(&pdev->dev);
dev_info(&pdev->dev, "spi frequency: %d Hz\n", sp->spi_freq);
- return devm_spi_register_controller(&pdev->dev, ctlr);
+ return 0;
+
+err_probe:
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ pm_runtime_dont_use_autosuspend(&pdev->dev);
+
+ mtk_nor_disable_clk(sp);
+
+ return ret;
}
static int mtk_nor_remove(struct platform_device *pdev)
{
- struct spi_controller *ctlr;
- struct mtk_nor *sp;
+ struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
+ struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
- ctlr = dev_get_drvdata(&pdev->dev);
- sp = spi_controller_get_devdata(ctlr);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+ pm_runtime_dont_use_autosuspend(&pdev->dev);
+
+ mtk_nor_disable_clk(sp);
+
+ return 0;
+}
+
+static int __maybe_unused mtk_nor_runtime_suspend(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
mtk_nor_disable_clk(sp);
return 0;
}
+static int __maybe_unused mtk_nor_runtime_resume(struct device *dev)
+{
+ struct spi_controller *ctlr = dev_get_drvdata(dev);
+ struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
+
+ return mtk_nor_enable_clk(sp);
+}
+
+static int __maybe_unused mtk_nor_suspend(struct device *dev)
+{
+ return pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused mtk_nor_resume(struct device *dev)
+{
+ return pm_runtime_force_resume(dev);
+}
+
+static const struct dev_pm_ops mtk_nor_pm_ops = {
+ SET_RUNTIME_PM_OPS(mtk_nor_runtime_suspend,
+ mtk_nor_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(mtk_nor_suspend, mtk_nor_resume)
+};
+
static struct platform_driver mtk_nor_driver = {
.driver = {
.name = DRIVER_NAME,
.of_match_table = mtk_nor_match,
+ .pm = &mtk_nor_pm_ops,
},
.probe = mtk_nor_probe,
.remove = mtk_nor_remove,
--
2.28.0.681.g6f77f65b4e-goog
This patch enables 36bit dma address support to spi-mtk-nor.
Currently this is enabled only for mt8192-nor.
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 8dbafee7f431..35205635ed42 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -78,6 +78,8 @@
#define MTK_NOR_REG_DMA_FADR 0x71c
#define MTK_NOR_REG_DMA_DADR 0x720
#define MTK_NOR_REG_DMA_END_DADR 0x724
+#define MTK_NOR_REG_DMA_DADR_HB 0x738
+#define MTK_NOR_REG_DMA_END_DADR_HB 0x73c
/* maximum bytes of TX in PRG mode */
#define MTK_NOR_PRG_MAX_SIZE 6
@@ -106,6 +108,7 @@ struct mtk_nor {
unsigned int spi_freq;
bool wbuf_en;
bool has_irq;
+ bool high_dma;
struct completion op_done;
};
@@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
+ if (sp->high_dma) {
+ writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
+ writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
+ }
+
if (sp->has_irq) {
reinit_completion(&sp->op_done);
mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
@@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
};
static const struct of_device_id mtk_nor_match[] = {
- { .compatible = "mediatek,mt8173-nor" },
+ { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
+ { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mtk_nor_match);
@@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
void __iomem *base;
struct clk *spi_clk, *ctlr_clk;
int ret, irq;
+ unsigned long dma_bits;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
@@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
if (IS_ERR(ctlr_clk))
return PTR_ERR(ctlr_clk);
+ dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
+ if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
+ dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
+ return -EINVAL;
+ }
+
ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp));
if (!ctlr) {
dev_err(&pdev->dev, "failed to allocate spi controller\n");
--
2.28.0.681.g6f77f65b4e-goog
Hi!
On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <[email protected]> wrote:
>
> When mtk-nor fallbacks to generic spi transfers, it can actually
> transfer up to 7 bytes.
generic transfer_one_message should support full-duplex transfers,
not transfers with special format requirements. (e.g. here the last
byte is rx only.) These transfers with format requirements should
be implemented with spi-mem interface instead.
>
> This patch fixes adjust_op_size() and supports_op() to explicitly
> check 7 bytes range and also fixes possible under/overflow conditions
> in register offsets calculation.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
I was notified by Bayi about your discussion and sent some
patches yesterday for the same purpose. Whoops...
As transfer_one_message isn't the proper place to implement
this, maybe we could work on my version instead?
> ---
>
> (no changes since v1)
This should be "new patch" not "no changes" :P
>
> drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
> 1 file changed, 76 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 0f7d4ec68730..e7719d249095 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -79,7 +79,11 @@
> #define MTK_NOR_REG_DMA_DADR 0x720
> #define MTK_NOR_REG_DMA_END_DADR 0x724
>
> +/* maximum bytes of TX in PRG mode */
> #define MTK_NOR_PRG_MAX_SIZE 6
> +/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
> +#define MTK_NOR_PRG_MAX_CYCLES 7
> +
> // Reading DMA src/dst addresses have to be 16-byte aligned
> #define MTK_NOR_DMA_ALIGN 16
> #define MTK_NOR_DMA_ALIGN_MASK (MTK_NOR_DMA_ALIGN - 1)
> @@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
> return false;
> }
>
> +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> +{
> + size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> + if (len > MTK_NOR_PRG_MAX_SIZE)
> + return false;
> +
> + if (!op->data.nbytes)
> + return true;
> +
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> + else if (op->data.dir == SPI_MEM_DATA_IN)
> + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
> + else
> + return true;
> +}
> +
> static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> {
> size_t len;
> @@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> }
> }
>
> - len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> - op->dummy.nbytes;
> - if (op->data.nbytes > len)
> - op->data.nbytes = len;
> + if (mtk_nor_check_prg(op))
> + return 0;
> +
> + len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> + if (op->data.dir == SPI_MEM_DATA_OUT) {
> + if (len == MTK_NOR_PRG_MAX_SIZE)
> + return -EINVAL;
> + op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> + MTK_NOR_PRG_MAX_SIZE - len);
> + } else {
> + if (len == MTK_NOR_PRG_MAX_CYCLES)
> + return -EINVAL;
> + op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> + MTK_NOR_PRG_MAX_CYCLES - len);
> + }
>
> return 0;
> }
> @@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> static bool mtk_nor_supports_op(struct spi_mem *mem,
> const struct spi_mem_op *op)
> {
> - size_t len;
> -
> if (op->cmd.buswidth != 1)
> return false;
>
> @@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> (op->data.buswidth == 1);
> }
>
> - len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> - if ((len > MTK_NOR_PRG_MAX_SIZE) ||
> - ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
> + /* fallback to generic spi xfer */
> + if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
> return false;
Rejecting an op in supports_op doesn't tell it to fall back to generic
spi transfer.
It instead tells caller to abort this transfer completely.
A fallback only happens when exec_op returns -ENOTSUPP.
This comment is incorrect. I'd put this buswidth checking in mtk_nor_check_prg
instead because mtk_nor_check_prg is checking whether an op is supported
by prg mode, thus it should reject ops with buswidth > 1.
>
> - return true;
> + return mtk_nor_check_prg(op);
> }
>
> static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> @@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> int stat = 0;
> int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
> void __iomem *reg;
> - const u8 *txbuf;
> - u8 *rxbuf;
> - int i;
> + int i, tx_len = 0, rx_len = 0;
>
> list_for_each_entry(t, &m->transfers, transfer_list) {
> - txbuf = t->tx_buf;
> - for (i = 0; i < t->len; i++, reg_offset--) {
> + const u8 *txbuf = t->tx_buf;
> +
> + if (!txbuf) {
> + rx_len += t->len;
> + continue;
> + }
> +
> + if (rx_len) {
> + stat = -EPROTO;
> + goto msg_done;
> + }
NACK. you are unnecessarily rejecting possible transfers.
> +
> + for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
> - if (txbuf)
> - writeb(txbuf[i], reg);
> - else
> - writeb(0, reg);
> + writeb(txbuf[i], reg);
> + tx_len++;
According to SPI standard, during a rx transfer, tx should be kept low.
These PROGDATA registers doesn't clear itself so it'll keep sending
data from last transfer, which violates this rule. That's
why the original code writes 0 to PRGDATA for rx bytes.
> }
> - trx_len += t->len;
> }
>
> + while (reg_offset >= 0) {
> + writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
> + reg_offset--;
> + }
> +
> + rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
> + trx_len = tx_len + rx_len;
> +
> writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
>
> stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
> @@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> if (stat < 0)
> goto msg_done;
>
> - reg_offset = trx_len - 1;
> - list_for_each_entry(t, &m->transfers, transfer_list) {
> - rxbuf = t->rx_buf;
> - for (i = 0; i < t->len; i++, reg_offset--) {
> - reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> - if (rxbuf)
> + if (rx_len > 0) {
> + reg_offset = rx_len - 1;
> + list_for_each_entry(t, &m->transfers, transfer_list) {
> + u8 *rxbuf = t->rx_buf;
> +
> + if (!rxbuf)
> + continue;
> +
> + for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> + reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> rxbuf[i] = readb(reg);
> + }
I think this is replacing original code with some equivalent ones, which
seems unnecessary.
> }
> }
>
--
Regards,
Chuanhong Guo
HI!
One more comment:
On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <[email protected]> wrote:
> +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> +{
> + size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> + if (len > MTK_NOR_PRG_MAX_SIZE)
> + return false;
> +
> + if (!op->data.nbytes)
> + return true;
> +
> + if (op->data.dir == SPI_MEM_DATA_OUT)
> + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> + else if (op->data.dir == SPI_MEM_DATA_IN)
> + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
You need to consider the existence of adjust_op_size in supports_op as well.
This mtk_nor_check_prg still rejects SFDP reading command from spi-nor
driver altogether.
--
Regards,
Chuanhong Guo
Hi!
On Fri, Sep 25, 2020 at 2:56 PM Ikjoon Jang <[email protected]> wrote:
>
> Use dma_alloc_coherent() for bounce buffer instead of kmalloc() to
> make sure the bounce buffer to be allocated within its DMAable range.
I think the introduction of need_bounce can be in its own commit or
at least mentioned here.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
>
> ---
>
> (no changes since v1)
There have been a lot of changes since v2 :)
Reviewed-by: Chuanhong Guo <[email protected]>
--
Regards,
Chuanhong Guo
Hi!
On Fri, Sep 25, 2020 at 2:56 PM Ikjoon Jang <[email protected]> wrote:
>
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently this is enabled only for mt8192-nor.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 8dbafee7f431..35205635ed42 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
> #define MTK_NOR_REG_DMA_FADR 0x71c
> #define MTK_NOR_REG_DMA_DADR 0x720
> #define MTK_NOR_REG_DMA_END_DADR 0x724
> +#define MTK_NOR_REG_DMA_DADR_HB 0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB 0x73c
>
> /* maximum bytes of TX in PRG mode */
> #define MTK_NOR_PRG_MAX_SIZE 6
> @@ -106,6 +108,7 @@ struct mtk_nor {
> unsigned int spi_freq;
> bool wbuf_en;
> bool has_irq;
> + bool high_dma;
> struct completion op_done;
> };
>
> @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>
> + if (sp->high_dma) {
> + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> + writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> + }
I remembered kbuild test robot reported a warning on this on 32-bit platforms
in your v1. [0]
I don't know what's the fix for this though :(
[0] https://marc.info/?l=linux-spi&m=159982425706940&w=2
--
Regards,
Chuanhong Guo
On Fri, Sep 25, 2020 at 4:27 PM Chuanhong Guo <[email protected]> wrote:
>
[snip]
> > + if (sp->high_dma) {
> > + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> > + writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> > + }
>
> I remembered kbuild test robot reported a warning on this on 32-bit platforms
> in your v1. [0]
> I don't know what's the fix for this though :(
>
> [0] https://marc.info/?l=linux-spi&m=159982425706940&w=2
yeah, I'm not sure how to handle this properly,
"warning: shift count >= width of type",
(sp->high_dma) is always false on 32bit arm kernel.
I think adding size check on here is unnecessary, should I fix for this warning?
> --
> Regards,
> Chuanhong Guo
Sorry for resending, Chuanhong.
On Fri, Sep 25, 2020 at 3:47 PM Chuanhong Guo <[email protected]> wrote:
>
> Hi!
>
> On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <[email protected]> wrote:
> >
> > When mtk-nor fallbacks to generic spi transfers, it can actually
> > transfer up to 7 bytes.
>
> generic transfer_one_message should support full-duplex transfers,
> not transfers with special format requirements. (e.g. here the last
> byte is rx only.) These transfers with format requirements should
> be implemented with spi-mem interface instead.
yep, that's correct.
>
> >
> > This patch fixes adjust_op_size() and supports_op() to explicitly
> > check 7 bytes range and also fixes possible under/overflow conditions
> > in register offsets calculation.
> >
> > Signed-off-by: Ikjoon Jang <[email protected]>
>
> I was notified by Bayi about your discussion and sent some
> patches yesterday for the same purpose. Whoops...
> As transfer_one_message isn't the proper place to implement
> this, maybe we could work on my version instead?
>
I didn't noticed that before,
Sure, please go ahead, I'll follow up with your patch in v4.
> > ---
> >
> > (no changes since v1)
>
> This should be "new patch" not "no changes" :P
oops, it seems my script did something wrong.
>
>
> >
> > drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
> > 1 file changed, 76 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 0f7d4ec68730..e7719d249095 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -79,7 +79,11 @@
> > #define MTK_NOR_REG_DMA_DADR 0x720
> > #define MTK_NOR_REG_DMA_END_DADR 0x724
> >
> > +/* maximum bytes of TX in PRG mode */
> > #define MTK_NOR_PRG_MAX_SIZE 6
> > +/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
> > +#define MTK_NOR_PRG_MAX_CYCLES 7
> > +
> > // Reading DMA src/dst addresses have to be 16-byte aligned
> > #define MTK_NOR_DMA_ALIGN 16
> > #define MTK_NOR_DMA_ALIGN_MASK (MTK_NOR_DMA_ALIGN - 1)
> > @@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
> > return false;
> > }
> >
> > +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> > +{
> > + size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > + if (len > MTK_NOR_PRG_MAX_SIZE)
> > + return false;
> > +
> > + if (!op->data.nbytes)
> > + return true;
> > +
> > + if (op->data.dir == SPI_MEM_DATA_OUT)
> > + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> > + else if (op->data.dir == SPI_MEM_DATA_IN)
> > + return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
> > + else
> > + return true;
> > +}
> > +
> > static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > {
> > size_t len;
> > @@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > }
> > }
> >
> > - len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> > - op->dummy.nbytes;
> > - if (op->data.nbytes > len)
> > - op->data.nbytes = len;
> > + if (mtk_nor_check_prg(op))
> > + return 0;
> > +
> > + len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > + if (op->data.dir == SPI_MEM_DATA_OUT) {
> > + if (len == MTK_NOR_PRG_MAX_SIZE)
> > + return -EINVAL;
> > + op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> > + MTK_NOR_PRG_MAX_SIZE - len);
> > + } else {
> > + if (len == MTK_NOR_PRG_MAX_CYCLES)
> > + return -EINVAL;
> > + op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> > + MTK_NOR_PRG_MAX_CYCLES - len);
> > + }
> >
> > return 0;
> > }
> > @@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > static bool mtk_nor_supports_op(struct spi_mem *mem,
> > const struct spi_mem_op *op)
> > {
> > - size_t len;
> > -
> > if (op->cmd.buswidth != 1)
> > return false;
> >
> > @@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> > (op->data.buswidth == 1);
> > }
> >
> > - len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > - if ((len > MTK_NOR_PRG_MAX_SIZE) ||
> > - ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
> > + /* fallback to generic spi xfer */
> > + if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
> > return false;
>
> Rejecting an op in supports_op doesn't tell it to fall back to generic
> spi transfer.
> It instead tells caller to abort this transfer completely.
> A fallback only happens when exec_op returns -ENOTSUPP.
yep but I think that case always going PRG mode in exec_op() with the
same condition?
> This comment is incorrect. I'd put this buswidth checking in mtk_nor_check_prg
> instead because mtk_nor_check_prg is checking whether an op is supported
> by prg mode, thus it should reject ops with buswidth > 1.
>
> >
> > - return true;
> > + return mtk_nor_check_prg(op);
> > }
> >
> > static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> > @@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> > int stat = 0;
> > int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
> > void __iomem *reg;
> > - const u8 *txbuf;
> > - u8 *rxbuf;
> > - int i;
> > + int i, tx_len = 0, rx_len = 0;
> >
> > list_for_each_entry(t, &m->transfers, transfer_list) {
> > - txbuf = t->tx_buf;
> > - for (i = 0; i < t->len; i++, reg_offset--) {
> > + const u8 *txbuf = t->tx_buf;
> > +
> > + if (!txbuf) {
> > + rx_len += t->len;
> > + continue;
> > + }
> > +
> > + if (rx_len) {
> > + stat = -EPROTO;
> > + goto msg_done;
> > + }
>
> NACK. you are unnecessarily rejecting possible transfers.
yep, ditto
>
> > +
> > + for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> > reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
> > - if (txbuf)
> > - writeb(txbuf[i], reg);
> > - else
> > - writeb(0, reg);
> > + writeb(txbuf[i], reg);
> > + tx_len++;
>
> According to SPI standard, during a rx transfer, tx should be kept low.
> These PROGDATA registers doesn't clear itself so it'll keep sending
> data from last transfer, which violates this rule. That's
> why the original code writes 0 to PRGDATA for rx bytes.
following lines with while() will set 0s to the rest of registers.
>
> > }
> > - trx_len += t->len;
> > }
> >
> > + while (reg_offset >= 0) {
> > + writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
> > + reg_offset--;
> > + }
> > +
> > + rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
> > + trx_len = tx_len + rx_len;
> > +
> > writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
> >
> > stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
> > @@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> > if (stat < 0)
> > goto msg_done;
> >
> > - reg_offset = trx_len - 1;
> > - list_for_each_entry(t, &m->transfers, transfer_list) {
> > - rxbuf = t->rx_buf;
> > - for (i = 0; i < t->len; i++, reg_offset--) {
> > - reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> > - if (rxbuf)
> > + if (rx_len > 0) {
> > + reg_offset = rx_len - 1;
> > + list_for_each_entry(t, &m->transfers, transfer_list) {
> > + u8 *rxbuf = t->rx_buf;
> > +
> > + if (!rxbuf)
> > + continue;
> > +
> > + for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> > + reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> > rxbuf[i] = readb(reg);
> > + }
>
> I think this is replacing original code with some equivalent ones, which
> seems unnecessary.
This patch addressed the issue with 1+6 bytes transfer (e.g JEDEC ID)
can have negative reg_offset.
And there's skipping the loop if (rx_len < 0)
anyway I'd like to follow with your new patch. :-)
Thanks!
>
> > }
> > }
> >
> --
> Regards,
> Chuanhong Guo
On Fri, Sep 25, 2020 at 2:54 PM Ikjoon Jang <[email protected]> wrote:
>
> Fix a bug which limits its protocol availability in supports_op().
>
> Fixes: a59b2c7c56bf ("spi: spi-mtk-nor: support standard spi properties")
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
This is also duplicated work of https://patchwork.kernel.org/patch/11797723/,
I'm going to drop this patch in v4.
>
> (no changes since v1)
>
> drivers/spi/spi-mtk-nor.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 6e6ca2b8e6c8..0f7d4ec68730 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -211,28 +211,24 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> if (op->cmd.buswidth != 1)
> return false;
>
> + if (!spi_mem_default_supports_op(mem, op))
> + return false;
> +
> if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> - switch(op->data.dir) {
> - case SPI_MEM_DATA_IN:
> - if (!mtk_nor_match_read(op))
> - return false;
> - break;
> - case SPI_MEM_DATA_OUT:
> - if ((op->addr.buswidth != 1) ||
> - (op->dummy.nbytes != 0) ||
> - (op->data.buswidth != 1))
> - return false;
> - break;
> - default:
> - break;
> - }
> + if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
> + return true;
> + else if (op->data.dir == SPI_MEM_DATA_OUT)
> + return (op->addr.buswidth == 1) &&
> + (op->dummy.nbytes == 0) &&
> + (op->data.buswidth == 1);
> }
> +
> len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> if ((len > MTK_NOR_PRG_MAX_SIZE) ||
> ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
> return false;
>
> - return spi_mem_default_supports_op(mem, op);
> + return true;
> }
>
> static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> --
> 2.28.0.681.g6f77f65b4e-goog
>
On Fri, 2020-09-25 at 14:54 +0800, Ikjoon Jang wrote:
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently this is enabled only for mt8192-nor.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 8dbafee7f431..35205635ed42 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
> #define MTK_NOR_REG_DMA_FADR 0x71c
> #define MTK_NOR_REG_DMA_DADR 0x720
> #define MTK_NOR_REG_DMA_END_DADR 0x724
> +#define MTK_NOR_REG_DMA_DADR_HB 0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB 0x73c
>
> /* maximum bytes of TX in PRG mode */
> #define MTK_NOR_PRG_MAX_SIZE 6
> @@ -106,6 +108,7 @@ struct mtk_nor {
> unsigned int spi_freq;
> bool wbuf_en;
> bool has_irq;
> + bool high_dma;
> struct completion op_done;
> };
>
> @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>
> + if (sp->high_dma) {
> + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> + writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> + }
> +
Maybe use upper_32_bits() ?
> if (sp->has_irq) {
> reinit_completion(&sp->op_done);
> mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> @@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
> };
>
> static const struct of_device_id mtk_nor_match[] = {
> - { .compatible = "mediatek,mt8173-nor" },
> + { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> + { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mtk_nor_match);
> @@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
> void __iomem *base;
> struct clk *spi_clk, *ctlr_clk;
> int ret, irq;
> + unsigned long dma_bits;
>
> base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base))
> @@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
> if (IS_ERR(ctlr_clk))
> return PTR_ERR(ctlr_clk);
>
> + dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
> + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
> + dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
> + return -EINVAL;
> + }
> +
As said in previous version. I don't see any place enable high_dma, so I
think this patch won't set >32bits for anychip. We need something like:
sp->hidh_dma = dma_bits > 32;
Am I missing anything?
Joe.C
On Sun, Sep 27, 2020 at 4:30 PM Yingjoe Chen <[email protected]> wrote:
>
> On Fri, 2020-09-25 at 14:54 +0800, Ikjoon Jang wrote:
> > This patch enables 36bit dma address support to spi-mtk-nor.
> > Currently this is enabled only for mt8192-nor.
> >
> > Signed-off-by: Ikjoon Jang <[email protected]>
> > ---
> >
> > (no changes since v1)
> >
> > drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 8dbafee7f431..35205635ed42 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -78,6 +78,8 @@
> > #define MTK_NOR_REG_DMA_FADR 0x71c
> > #define MTK_NOR_REG_DMA_DADR 0x720
> > #define MTK_NOR_REG_DMA_END_DADR 0x724
> > +#define MTK_NOR_REG_DMA_DADR_HB 0x738
> > +#define MTK_NOR_REG_DMA_END_DADR_HB 0x73c
> >
> > /* maximum bytes of TX in PRG mode */
> > #define MTK_NOR_PRG_MAX_SIZE 6
> > @@ -106,6 +108,7 @@ struct mtk_nor {
> > unsigned int spi_freq;
> > bool wbuf_en;
> > bool has_irq;
> > + bool high_dma;
> > struct completion op_done;
> > };
> >
> > @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> > writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> > writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
> >
> > + if (sp->high_dma) {
> > + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> > + writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> > + }
> > +
>
> Maybe use upper_32_bits() ?
Thanks, good to know that!
>
>
> > if (sp->has_irq) {
> > reinit_completion(&sp->op_done);
> > mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> > @@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
> > };
> >
> > static const struct of_device_id mtk_nor_match[] = {
> > - { .compatible = "mediatek,mt8173-nor" },
> > + { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> > + { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, mtk_nor_match);
> > @@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
> > void __iomem *base;
> > struct clk *spi_clk, *ctlr_clk;
> > int ret, irq;
> > + unsigned long dma_bits;
> >
> > base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(base))
> > @@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
> > if (IS_ERR(ctlr_clk))
> > return PTR_ERR(ctlr_clk);
> >
> > + dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
> > + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
> > + dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
> > + return -EINVAL;
> > + }
> > +
>
> As said in previous version. I don't see any place enable high_dma, so I
> think this patch won't set >32bits for anychip. We need something like:
>
> sp->hidh_dma = dma_bits > 32;
>
> Am I missing anything?
Yeah, you're right, that line disappeared between v2 ~ v3 (by mistake).
>
> Joe.C
>