This patchset adds 36bit dma address and power management
supports for mt8192-nor.
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 (5):
dt-bindings: spi: add mt8192-nor compatible string
spi: spi-mtk-nor: fix mishandled logics in checking SPI memory
operation
spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer
spi: spi-mtk-nor: support 36bit dma addressing to mediatek
spi: spi-mtk-nor: Add power management support
.../bindings/spi/mediatek,spi-mtk-nor.yaml | 1 +
drivers/spi/spi-mtk-nor.c | 242 ++++++++++++------
2 files changed, 170 insertions(+), 73 deletions(-)
--
2.28.0.681.g6f77f65b4e-goog
Fix a simple bug which can limits its transfer size,
and add a simple helper function for code cleanups.
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 | 62 ++++++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 24 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 6e6ca2b8e6c8..54b2c0fde95b 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -167,52 +167,63 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
return false;
}
-static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+static bool need_bounce(void *cpu_addr, unsigned long len)
{
- size_t len;
+ return !!(((uintptr_t)cpu_addr) & MTK_NOR_DMA_ALIGN_MASK);
+}
+static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
+{
if (!op->data.nbytes)
return 0;
if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
- if ((op->data.dir == SPI_MEM_DATA_IN) &&
- mtk_nor_match_read(op)) {
+ switch (op->data.dir) {
+ case SPI_MEM_DATA_IN:
+ if (!mtk_nor_match_read(op))
+ return -EINVAL;
+ /* check if it's DMAable */
if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
- (op->data.nbytes < MTK_NOR_DMA_ALIGN))
+ (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(op->data.buf.in, op->data.nbytes) &&
+ (op->data.nbytes > MTK_NOR_BOUNCE_BUF_SIZE))
+ op->data.nbytes = MTK_NOR_BOUNCE_BUF_SIZE;
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;
- return 0;
- } else if (op->data.dir == SPI_MEM_DATA_OUT) {
+ }
+ break;
+ case SPI_MEM_DATA_OUT:
if (op->data.nbytes >= MTK_NOR_PP_SIZE)
op->data.nbytes = MTK_NOR_PP_SIZE;
else
op->data.nbytes = 1;
- return 0;
+ break;
+ default:
+ break;
}
+ } else {
+ u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+ if (len > MTK_NOR_PRG_MAX_SIZE)
+ return -EINVAL;
+ if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
+ return -EINVAL;
+ if (op->data.nbytes > (MTK_NOR_PRG_MAX_SIZE - len))
+ op->data.nbytes = MTK_NOR_PRG_MAX_SIZE - len;
}
- len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
- op->dummy.nbytes;
- if (op->data.nbytes > len)
- op->data.nbytes = len;
-
return 0;
}
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;
if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
- switch(op->data.dir) {
+ switch (op->data.dir) {
case SPI_MEM_DATA_IN:
if (!mtk_nor_match_read(op))
return false;
@@ -226,11 +237,14 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
default:
break;
}
+ } else {
+ u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+ if (len > MTK_NOR_PRG_MAX_SIZE)
+ return false;
+ if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
+ return false;
}
- 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);
}
--
2.28.0.681.g6f77f65b4e-goog
Use dma_alloc_coherent() for bounce buffer instead of kmalloc.
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 60 +++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 54b2c0fde95b..e14798a6e7d0 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -96,6 +96,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;
@@ -275,19 +276,16 @@ 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 read_dma(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");
+ if (WARN_ON((length & MTK_NOR_DMA_ALIGN_MASK) ||
+ (dma_addr & MTK_NOR_DMA_ALIGN_MASK)))
return -EINVAL;
- }
writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
@@ -312,30 +310,39 @@ 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_dma(struct mtk_nor *sp, u32 from,
+ unsigned int length, u8 *buffer)
{
- unsigned int rdlen;
int ret;
+ dma_addr_t dma_addr;
+ bool bounce = need_bounce(buffer, length);
- if (length & MTK_NOR_DMA_ALIGN_MASK)
- rdlen = (length + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
- else
- rdlen = length;
+ if (!bounce) {
+ 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;
+ }
+ } else {
+ dma_addr = sp->buffer_dma;
+ }
- ret = mtk_nor_read_dma(sp, from, rdlen, sp->buffer);
- if (ret)
- return ret;
+ ret = read_dma(sp, from, length, dma_addr);
- memcpy(buffer, sp->buffer, length);
- return 0;
+ if (!bounce)
+ dma_unmap_single(sp->dev, dma_addr, length,
+ DMA_FROM_DEVICE);
+ else
+ memcpy(buffer, sp->buffer, length);
+
+ return ret;
}
static int mtk_nor_read_pio(struct mtk_nor *sp, const struct spi_mem_op *op)
@@ -439,11 +446,6 @@ 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,
@@ -654,6 +656,10 @@ static int mtk_nor_probe(struct platform_device *pdev)
sp->dev = &pdev->dev;
sp->spi_clk = spi_clk;
sp->ctlr_clk = ctlr_clk;
+ sp->buffer = dma_alloc_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
+ &sp->buffer_dma, GFP_KERNEL);
+ if (!sp->buffer)
+ return -ENOMEM;
irq = platform_get_irq_optional(pdev, 0);
if (irq < 0) {
@@ -674,6 +680,8 @@ static int mtk_nor_probe(struct platform_device *pdev)
ret = mtk_nor_init(sp);
if (ret < 0) {
kfree(ctlr);
+ dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
+ sp->buffer, sp->buffer_dma);
return ret;
}
@@ -692,6 +700,8 @@ static int mtk_nor_remove(struct platform_device *pdev)
mtk_nor_disable_clk(sp);
+ dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
+ sp->buffer, sp->buffer_dma);
return 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 delayed after enabling clocks
to deal with unknown state of clocks at probe time,
Signed-off-by: Ikjoon Jang <[email protected]>
---
drivers/spi/spi-mtk-nor.c | 105 +++++++++++++++++++++++++++++---------
1 file changed, 80 insertions(+), 25 deletions(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 99dd5dca744e..5dcd575998d9 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>
@@ -551,22 +552,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)
@@ -630,6 +624,11 @@ static int mtk_nor_probe(struct platform_device *pdev)
if (IS_ERR(ctlr_clk))
return PTR_ERR(ctlr_clk);
+ irq = platform_get_irq_optional(pdev, 0);
+
+ if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(36)))
+ dev_warn(&pdev->dev, "failed to set dma mask(36)\n");
+
buffer = devm_kmalloc(&pdev->dev,
MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
GFP_KERNEL);
@@ -661,6 +660,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);
@@ -678,12 +678,17 @@ static int mtk_nor_probe(struct platform_device *pdev)
if (!sp->buffer)
return -ENOMEM;
- irq = platform_get_irq_optional(pdev, 0);
+ ret = mtk_nor_enable_clk(sp);
+ if (ret < 0)
+ return ret;
+
+ sp->spi_freq = clk_get_rate(sp->spi_clk);
+
+ mtk_nor_init(sp);
+
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) {
@@ -694,26 +699,41 @@ static int mtk_nor_probe(struct platform_device *pdev)
}
}
- ret = mtk_nor_init(sp);
- if (ret < 0) {
- kfree(ctlr);
- dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
- sp->buffer, sp->buffer_dma);
- 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);
@@ -722,10 +742,45 @@ static int mtk_nor_remove(struct platform_device *pdev)
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 36bit dma addressing is enabled only for mt8192-nor.
Signed-off-by: Ikjoon Jang <[email protected]>
---
(no changes since v1)
drivers/spi/spi-mtk-nor.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index e14798a6e7d0..99dd5dca744e 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
#define MTK_NOR_PRG_MAX_SIZE 6
// Reading DMA src/dst addresses have to be 16-byte aligned
@@ -102,6 +104,7 @@ struct mtk_nor {
unsigned int spi_freq;
bool wbuf_en;
bool has_irq;
+ bool high_dma;
struct completion op_done;
};
@@ -291,6 +294,11 @@ static int read_dma(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);
@@ -594,7 +602,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);
@@ -607,6 +616,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
u8 *buffer;
struct clk *spi_clk, *ctlr_clk;
int ret, irq;
+ unsigned long dma_bits;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
@@ -623,6 +633,13 @@ static int mtk_nor_probe(struct platform_device *pdev)
buffer = devm_kmalloc(&pdev->dev,
MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
GFP_KERNEL);
+
+ 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;
+ }
+
if (!buffer)
return -ENOMEM;
--
2.28.0.681.g6f77f65b4e-goog
Add compatible string for mt8192 SoC.
Signed-off-by: Ikjoon Jang <[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
Hi!
On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <[email protected]> wrote:
>
> Fix a simple bug which can limits its transfer size,
> and add a simple helper function for code cleanups.
>
> 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 | 62 ++++++++++++++++++++++++---------------
> 1 file changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 6e6ca2b8e6c8..54b2c0fde95b 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -167,52 +167,63 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
> return false;
> }
>
> -static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +static bool need_bounce(void *cpu_addr, unsigned long len)
> {
> - size_t len;
> + return !!(((uintptr_t)cpu_addr) & MTK_NOR_DMA_ALIGN_MASK);
> +}
parameter 'len' isn't used in this function.
>
> +static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> if (!op->data.nbytes)
> return 0;
>
> if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> - if ((op->data.dir == SPI_MEM_DATA_IN) &&
> - mtk_nor_match_read(op)) {
I think replacing a if/else if with a two-case switch is more
of a personal code preference rather than code cleanup.
I'd prefer only adding need_bounce to replace alignment
check using a separated commit and leave other stuff
untouched because:
1. This "cleanup" made unintended logic changes (see below)
2. The "cleanup" itself actually becomes the major part of
this patch, while the actual fix mentioned in commit
message is the minor part.
3. A fix commit should contain the fix itself. It shouldn't
mix with these code changes.
> + switch (op->data.dir) {
> + case SPI_MEM_DATA_IN:
> + if (!mtk_nor_match_read(op))
> + return -EINVAL;
You are changing the code logic here.
mtk_nor_match_read checks if the operation can be executed
using controller PIO/DMA reading. Even if it's not supported,
we can still use PRG mode to execute the operation.
One example of such an operation is SPI NOR SFDP reading.
Your change breaks that which then breaks 1_2_2 and 1_4_4
reading capability because spi-nor driver parses these op formats
from SFDP table.
> + /* check if it's DMAable */
> if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
> - (op->data.nbytes < MTK_NOR_DMA_ALIGN))
> + (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(op->data.buf.in, op->data.nbytes) &&
> + (op->data.nbytes > MTK_NOR_BOUNCE_BUF_SIZE))
> + op->data.nbytes = MTK_NOR_BOUNCE_BUF_SIZE;
> 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;
data length alignment is intentionally done only for DMA reading
without the bounce buffer.
My intention here:
If we use the bounce buffer, we can read more data than needed to.
Say we want 25 bytes of data, reading 32 bytes using DMA and
bounce buffer should be faster than reading 16 bytes with DMA
and another 9 bytes with PIO, because for every single byte of PIO
reading, adjust_op_size and exec_op is called once, we
program controller with new cmd/address, and controller need
to send extra cmd/address to flash.
I noticed that you removed this part of logic from DMA reading
execution in 3/5 as well. Please revert the logic change here
add in DMA reading function (see later comment in 3/5).
> - return 0;
> - } else if (op->data.dir == SPI_MEM_DATA_OUT) {
> + }
> + break;
> + case SPI_MEM_DATA_OUT:
> if (op->data.nbytes >= MTK_NOR_PP_SIZE)
> op->data.nbytes = MTK_NOR_PP_SIZE;
> else
> op->data.nbytes = 1;
> - return 0;
> + break;
> + default:
> + break;
> }
> + } else {
> + u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> + if (len > MTK_NOR_PRG_MAX_SIZE)
> + return -EINVAL;
> + if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
> + return -EINVAL;
> + if (op->data.nbytes > (MTK_NOR_PRG_MAX_SIZE - len))
> + op->data.nbytes = MTK_NOR_PRG_MAX_SIZE - len;
> }
>
> - len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> - op->dummy.nbytes;
> - if (op->data.nbytes > len)
> - op->data.nbytes = len;
> -
> return 0;
> }
>
> 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;
>
> if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> - switch(op->data.dir) {
> + switch (op->data.dir) {
> case SPI_MEM_DATA_IN:
> if (!mtk_nor_match_read(op))
> return false;
> @@ -226,11 +237,14 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> default:
> break;
> }
> + } else {
> + u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> + if (len > MTK_NOR_PRG_MAX_SIZE)
> + return false;
> + if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
> + return false;
> }
> - 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);
> }
> --
> 2.28.0.681.g6f77f65b4e-goog
>
--
Regards,
Chuanhong Guo
Hi!
On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <[email protected]> wrote:
>
> Use dma_alloc_coherent() for bounce buffer instead of kmalloc.
The commit message should explain why such a change is
needed. (i.e. why using dma_alloc_coherent here is better
than kmalloc.) And if there's no benefit for this change I'd prefer
leaving it untouched.
I remembered reading somewhere that stream DMA api is
prefered over dma_alloc_coherent for this kind of single-direction
DMA operation.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
>
> ---
>
> (no changes since v1)
>
> drivers/spi/spi-mtk-nor.c | 60 +++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 54b2c0fde95b..e14798a6e7d0 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -96,6 +96,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;
> @@ -275,19 +276,16 @@ 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 read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
This name is a bit confusing considering there's a mtk_nor_read_dma
below.
As this function now only executes dma readings and wait it to finish,
what about mtk_nor_dma_exec instead?
> + 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");
> + if (WARN_ON((length & MTK_NOR_DMA_ALIGN_MASK) ||
> + (dma_addr & MTK_NOR_DMA_ALIGN_MASK)))
These alignment is guaranteed by callers of this function if all
my comments below are addressed. This check isn't needed.
> return -EINVAL;
> - }
>
> writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> @@ -312,30 +310,39 @@ 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_dma(struct mtk_nor *sp, u32 from,
> + unsigned int length, u8 *buffer)
> {
> - unsigned int rdlen;
> int ret;
> + dma_addr_t dma_addr;
> + bool bounce = need_bounce(buffer, length);
>
> - if (length & MTK_NOR_DMA_ALIGN_MASK)
> - rdlen = (length + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
The intention of this rdlen alignment is explained in 2/5.
Please make sure this rdlen alignment logic is present
only for PIO reading.
> - else
> - rdlen = length;
> + if (!bounce) {
> + 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;
> + }
> + } else {
> + dma_addr = sp->buffer_dma;
> + }
>
> - ret = mtk_nor_read_dma(sp, from, rdlen, sp->buffer);
> - if (ret)
> - return ret;
> + ret = read_dma(sp, from, length, dma_addr);
>
> - memcpy(buffer, sp->buffer, length);
> - return 0;
> + if (!bounce)
> + dma_unmap_single(sp->dev, dma_addr, length,
> + DMA_FROM_DEVICE);
> + else
> + memcpy(buffer, sp->buffer, length);
> +
> + return ret;
> }
I think a separated read_dma and read_bounce function will be
cleaner than this if-else implementation:
read_dma:
1. call dma_map_single to get physical address
2. call read_dma to execute operation
3. call dma_unmap_single
read_bounce:
1. align reading length
2. call read_dma
3. call memcpy
>
> static int mtk_nor_read_pio(struct mtk_nor *sp, const struct spi_mem_op *op)
> @@ -439,11 +446,6 @@ 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,
> @@ -654,6 +656,10 @@ static int mtk_nor_probe(struct platform_device *pdev)
> sp->dev = &pdev->dev;
> sp->spi_clk = spi_clk;
> sp->ctlr_clk = ctlr_clk;
There is extra memory allocation code for sp->buffer in mtk_nor_probe.
If you intend to replace this with dma_alloc_coherent you should
drop those devm_kmalloc code as well.
> + sp->buffer = dma_alloc_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> + &sp->buffer_dma, GFP_KERNEL);
There's a devm variant: dmam_alloc_coherent(dev, size, dma_handle, gfp)
> + if (!sp->buffer)
> + return -ENOMEM;
This spi-nor controller requires all addresses to be 16-byte aligned.
Although it should be guaranteed by a usually way larger page
alignment address from dma_alloc_coherent I'd prefer an explicit
check for address alignment here rather than letting it probe
successfully and fail for every dma_read with bounce buffer.
>
> irq = platform_get_irq_optional(pdev, 0);
> if (irq < 0) {
> @@ -674,6 +680,8 @@ static int mtk_nor_probe(struct platform_device *pdev)
> ret = mtk_nor_init(sp);
> if (ret < 0) {
> kfree(ctlr);
> + dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> + sp->buffer, sp->buffer_dma);
> return ret;
> }
>
> @@ -692,6 +700,8 @@ static int mtk_nor_remove(struct platform_device *pdev)
>
> mtk_nor_disable_clk(sp);
>
> + dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> + sp->buffer, sp->buffer_dma);
> return 0;
> }
>
> --
> 2.28.0.681.g6f77f65b4e-goog
>
--
Regards,
Chuanhong Guo
Hi!
On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <[email protected]> wrote:
> On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <[email protected]> wrote:
> > [...]
> > + switch (op->data.dir) {
> > + case SPI_MEM_DATA_IN:
> > + if (!mtk_nor_match_read(op))
> > + return -EINVAL;
>
> You are changing the code logic here.
> mtk_nor_match_read checks if the operation can be executed
> using controller PIO/DMA reading. Even if it's not supported,
> we can still use PRG mode to execute the operation.
> One example of such an operation is SPI NOR SFDP reading.
> Your change breaks that which then breaks 1_2_2 and 1_4_4
> reading capability because spi-nor driver parses these op formats
> from SFDP table.
I just noticed that you already broke this in:
spi: spi-mtk-nor: support standard spi properties
Please also fix the same logic in mtk_nor_supports_op in your v3.
--
Regards,
Chuanhong Guo
On Fri, 2020-09-18 at 16:31 +0800, Ikjoon Jang wrote:
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently 36bit dma addressing is enabled only for mt8192-nor.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/spi/spi-mtk-nor.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index e14798a6e7d0..99dd5dca744e 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
>
> #define MTK_NOR_PRG_MAX_SIZE 6
> // Reading DMA src/dst addresses have to be 16-byte aligned
> @@ -102,6 +104,7 @@ struct mtk_nor {
> unsigned int spi_freq;
> bool wbuf_en;
> bool has_irq;
> + bool high_dma;
> struct completion op_done;
> };
>
> @@ -291,6 +294,11 @@ static int read_dma(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);
> @@ -594,7 +602,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);
> @@ -607,6 +616,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
> u8 *buffer;
> struct clk *spi_clk, *ctlr_clk;
> int ret, irq;
> + unsigned long dma_bits;
>
> base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base))
> @@ -623,6 +633,13 @@ static int mtk_nor_probe(struct platform_device *pdev)
> buffer = devm_kmalloc(&pdev->dev,
> MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
> GFP_KERNEL);
> +
> + 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;
> + }
> +
Do we need to set sp->high_dma when we have >32bits DMA?
Joe.C
On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <[email protected]> wrote:
>
> Hi!
>
> On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <[email protected]> wrote:
> >
> > Fix a simple bug which can limits its transfer size,
> > and add a simple helper function for code cleanups.
> >
> > 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 | 62 ++++++++++++++++++++++++---------------
> > 1 file changed, 38 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 6e6ca2b8e6c8..54b2c0fde95b 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -167,52 +167,63 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
> > return false;
> > }
> >
> > -static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > +static bool need_bounce(void *cpu_addr, unsigned long len)
> > {
> > - size_t len;
> > + return !!(((uintptr_t)cpu_addr) & MTK_NOR_DMA_ALIGN_MASK);
> > +}
>
> parameter 'len' isn't used in this function.
ACK.
>
> >
> > +static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > +{
> > if (!op->data.nbytes)
> > return 0;
> >
> > if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> > - if ((op->data.dir == SPI_MEM_DATA_IN) &&
> > - mtk_nor_match_read(op)) {
>
> I think replacing a if/else if with a two-case switch is more
> of a personal code preference rather than code cleanup.
> I'd prefer only adding need_bounce to replace alignment
> check using a separated commit and leave other stuff
> untouched because:
> 1. This "cleanup" made unintended logic changes (see below)
> 2. The "cleanup" itself actually becomes the major part of
> this patch, while the actual fix mentioned in commit
> message is the minor part.
> 3. A fix commit should contain the fix itself. It shouldn't
> mix with these code changes.
Got it, v3 will only include the fix itself.
>
> > + switch (op->data.dir) {
> > + case SPI_MEM_DATA_IN:
> > + if (!mtk_nor_match_read(op))
> > + return -EINVAL;
>
> You are changing the code logic here.
> mtk_nor_match_read checks if the operation can be executed
> using controller PIO/DMA reading. Even if it's not supported,
> we can still use PRG mode to execute the operation.
> One example of such an operation is SPI NOR SFDP reading.
> Your change breaks that which then breaks 1_2_2 and 1_4_4
> reading capability because spi-nor driver parses these op formats
> from SFDP table.
As you already noticed it, this is a bug from the last patch I made
and v2 isn't fixing this problem. This should be restored & fixed in v3.
And will not include other changes in a same patch.
>
> > + /* check if it's DMAable */
> > if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
> > - (op->data.nbytes < MTK_NOR_DMA_ALIGN))
> > + (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(op->data.buf.in, op->data.nbytes) &&
> > + (op->data.nbytes > MTK_NOR_BOUNCE_BUF_SIZE))
> > + op->data.nbytes = MTK_NOR_BOUNCE_BUF_SIZE;
> > 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;
>
> data length alignment is intentionally done only for DMA reading
> without the bounce buffer.
> My intention here:
> If we use the bounce buffer, we can read more data than needed to.
> Say we want 25 bytes of data, reading 32 bytes using DMA and
> bounce buffer should be faster than reading 16 bytes with DMA
> and another 9 bytes with PIO, because for every single byte of PIO
> reading, adjust_op_size and exec_op is called once, we
> program controller with new cmd/address, and controller need
> to send extra cmd/address to flash.
> I noticed that you removed this part of logic from DMA reading
> execution in 3/5 as well. Please revert the logic change here
> add in DMA reading function (see later comment in 3/5).
In v2, I wasn't sure whether this behavior is sane (read more than requested)
Now I think this is okay, I've missed the fact that flash address is
also aligned together.
I'll just keep the previous logics with padding in v3.
Thanks!
>
> > - return 0;
> > - } else if (op->data.dir == SPI_MEM_DATA_OUT) {
> > + }
> > + break;
> > + case SPI_MEM_DATA_OUT:
> > if (op->data.nbytes >= MTK_NOR_PP_SIZE)
> > op->data.nbytes = MTK_NOR_PP_SIZE;
> > else
> > op->data.nbytes = 1;
> > - return 0;
> > + break;
> > + default:
> > + break;
> > }
> > + } else {
> > + u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > + if (len > MTK_NOR_PRG_MAX_SIZE)
> > + return -EINVAL;
> > + if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
> > + return -EINVAL;
> > + if (op->data.nbytes > (MTK_NOR_PRG_MAX_SIZE - len))
> > + op->data.nbytes = MTK_NOR_PRG_MAX_SIZE - len;
> > }
> >
> > - len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> > - op->dummy.nbytes;
> > - if (op->data.nbytes > len)
> > - op->data.nbytes = len;
> > -
> > return 0;
> > }
> >
> > 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;
> >
> > if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> > - switch(op->data.dir) {
> > + switch (op->data.dir) {
> > case SPI_MEM_DATA_IN:
> > if (!mtk_nor_match_read(op))
> > return false;
> > @@ -226,11 +237,14 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> > default:
> > break;
> > }
> > + } else {
> > + u8 len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > + if (len > MTK_NOR_PRG_MAX_SIZE)
> > + return false;
> > + if (op->data.nbytes && !(MTK_NOR_PRG_MAX_SIZE - len))
> > + return false;
> > }
> > - 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);
> > }
> > --
> > 2.28.0.681.g6f77f65b4e-goog
> >
>
>
> --
> Regards,
> Chuanhong Guo
On Fri, Sep 18, 2020 at 9:25 PM Chuanhong Guo <[email protected]> wrote:
>
> Hi!
>
> On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <[email protected]> wrote:
> >
> > Use dma_alloc_coherent() for bounce buffer instead of kmalloc.
>
> The commit message should explain why such a change is
> needed. (i.e. why using dma_alloc_coherent here is better
> than kmalloc.) And if there's no benefit for this change I'd prefer
> leaving it untouched.
> I remembered reading somewhere that stream DMA api is
> prefered over dma_alloc_coherent for this kind of single-direction
> DMA operation.
>
I will add more description on why I changed it to dma_alloc_coherent():
- to explictly support devices like mt8173-nor which only supports
32bit addressing for dma.
And it also reminded me an another problem, (I won't address this
issue for now in v3):
as this device is using dma range as [start, end) format
where 'end' can be zero in that corner case of {start = 0xffff_f000; end = 0; }
> >
> > Signed-off-by: Ikjoon Jang <[email protected]>
> >
> > ---
> >
> > (no changes since v1)
> >
> > drivers/spi/spi-mtk-nor.c | 60 +++++++++++++++++++++++----------------
> > 1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 54b2c0fde95b..e14798a6e7d0 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -96,6 +96,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;
> > @@ -275,19 +276,16 @@ 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 read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
>
> This name is a bit confusing considering there's a mtk_nor_read_dma
> below.
> As this function now only executes dma readings and wait it to finish,
> what about mtk_nor_dma_exec instead?
yeah, good idea.
>
> > + 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");
> > + if (WARN_ON((length & MTK_NOR_DMA_ALIGN_MASK) ||
> > + (dma_addr & MTK_NOR_DMA_ALIGN_MASK)))
>
> These alignment is guaranteed by callers of this function if all
> my comments below are addressed. This check isn't needed.
ACK.
>
> > return -EINVAL;
> > - }
> >
> > writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> > @@ -312,30 +310,39 @@ 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_dma(struct mtk_nor *sp, u32 from,
> > + unsigned int length, u8 *buffer)
> > {
> > - unsigned int rdlen;
> > int ret;
> > + dma_addr_t dma_addr;
> > + bool bounce = need_bounce(buffer, length);
> >
> > - if (length & MTK_NOR_DMA_ALIGN_MASK)
> > - rdlen = (length + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
>
> The intention of this rdlen alignment is explained in 2/5.
> Please make sure this rdlen alignment logic is present
> only for PIO reading.
okay, I'll use padding again in v3.
>
> > - else
> > - rdlen = length;
> > + if (!bounce) {
> > + 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;
> > + }
> > + } else {
> > + dma_addr = sp->buffer_dma;
> > + }
> >
> > - ret = mtk_nor_read_dma(sp, from, rdlen, sp->buffer);
> > - if (ret)
> > - return ret;
> > + ret = read_dma(sp, from, length, dma_addr);
> >
> > - memcpy(buffer, sp->buffer, length);
> > - return 0;
> > + if (!bounce)
> > + dma_unmap_single(sp->dev, dma_addr, length,
> > + DMA_FROM_DEVICE);
> > + else
> > + memcpy(buffer, sp->buffer, length);
> > +
> > + return ret;
> > }
>
> I think a separated read_dma and read_bounce function will be
> cleaner than this if-else implementation:
> read_dma:
> 1. call dma_map_single to get physical address
> 2. call read_dma to execute operation
> 3. call dma_unmap_single
>
> read_bounce:
> 1. align reading length
> 2. call read_dma
> 3. call memcpy
ACK
>
> >
> > static int mtk_nor_read_pio(struct mtk_nor *sp, const struct spi_mem_op *op)
> > @@ -439,11 +446,6 @@ 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,
> > @@ -654,6 +656,10 @@ static int mtk_nor_probe(struct platform_device *pdev)
> > sp->dev = &pdev->dev;
> > sp->spi_clk = spi_clk;
> > sp->ctlr_clk = ctlr_clk;
>
> There is extra memory allocation code for sp->buffer in mtk_nor_probe.
> If you intend to replace this with dma_alloc_coherent you should
> drop those devm_kmalloc code as well.
>
> > + sp->buffer = dma_alloc_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> > + &sp->buffer_dma, GFP_KERNEL);
>
> There's a devm variant: dmam_alloc_coherent(dev, size, dma_handle, gfp)
ACK
>
> > + if (!sp->buffer)
> > + return -ENOMEM;
>
> This spi-nor controller requires all addresses to be 16-byte aligned.
> Although it should be guaranteed by a usually way larger page
> alignment address from dma_alloc_coherent I'd prefer an explicit
> check for address alignment here rather than letting it probe
> successfully and fail for every dma_read with bounce buffer.
>
Yep, I'll restore the padding.
>
> >
> > irq = platform_get_irq_optional(pdev, 0);
> > if (irq < 0) {
> > @@ -674,6 +680,8 @@ static int mtk_nor_probe(struct platform_device *pdev)
> > ret = mtk_nor_init(sp);
> > if (ret < 0) {
> > kfree(ctlr);
> > + dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> > + sp->buffer, sp->buffer_dma);
> > return ret;
> > }
> >
> > @@ -692,6 +700,8 @@ static int mtk_nor_remove(struct platform_device *pdev)
> >
> > mtk_nor_disable_clk(sp);
> >
> > + dma_free_coherent(&pdev->dev, MTK_NOR_BOUNCE_BUF_SIZE,
> > + sp->buffer, sp->buffer_dma);
> > return 0;
> > }
> >
> > --
> > 2.28.0.681.g6f77f65b4e-goog
> >
>
>
> --
> Regards,
> Chuanhong Guo
On Sat, Sep 19, 2020 at 11:26 PM Yingjoe Chen <[email protected]> wrote:
>
> On Fri, 2020-09-18 at 16:31 +0800, Ikjoon Jang wrote:
> > This patch enables 36bit dma address support to spi-mtk-nor.
> > Currently 36bit dma addressing is enabled only for mt8192-nor.
[snip]
>
> Do we need to set sp->high_dma when we have >32bits DMA?
>
Yes, to do so, you need to add new compatible strings if there are
more SoCs support >32bit other than mt8192
or just ust mt8192-nor for binding.
> Joe.C
On Fri, 18 Sep 2020 16:31:19 +0800, Ikjoon Jang wrote:
> Add compatible string for mt8192 SoC.
>
> Signed-off-by: Ikjoon Jang <[email protected]>
> ---
>
> (no changes since v1)
>
> Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring <[email protected]>