* [PATCH v2 0/5] spi: spi-mtk-nor: Add mt8192 support. @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Bayi Cheng, Chuanhong Guo, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 0/5] spi: spi-mtk-nor: Add mt8192 support. @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 0/5] spi: spi-mtk-nor: Add mt8192 support. @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel 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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 0/5] spi: spi-mtk-nor: Add mt8192 support. @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 8:31 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Bayi Cheng, Chuanhong Guo, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek Add compatible string for mt8192 SoC. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel Add compatible string for mt8192 SoC. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel Add compatible string for mt8192 SoC. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: linux-kernel, Bayi Cheng, linux-mediatek, Matthias Brugger, Chuanhong Guo, Ikjoon Jang, linux-arm-kernel Add compatible string for mt8192 SoC. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-23 20:42 ` Rob Herring -1 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2020-09-23 20:42 UTC (permalink / raw) To: Ikjoon Jang Cc: Bayi Cheng, linux-arm-kernel, Mark Brown, devicetree, Chuanhong Guo, Matthias Brugger, linux-mtd, linux-spi, linux-kernel, Rob Herring, linux-mediatek On Fri, 18 Sep 2020 16:31:19 +0800, Ikjoon Jang wrote: > Add compatible string for mt8192 SoC. > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > > (no changes since v1) > > Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-23 20:42 ` Rob Herring 0 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2020-09-23 20:42 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Bayi Cheng, linux-spi, Rob Herring, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, Chuanhong Guo, linux-arm-kernel On Fri, 18 Sep 2020 16:31:19 +0800, Ikjoon Jang wrote: > Add compatible string for mt8192 SoC. > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > > (no changes since v1) > > Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-23 20:42 ` Rob Herring 0 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2020-09-23 20:42 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Bayi Cheng, linux-spi, Rob Herring, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, Chuanhong Guo, linux-arm-kernel On Fri, 18 Sep 2020 16:31:19 +0800, Ikjoon Jang wrote: > Add compatible string for mt8192 SoC. > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > > (no changes since v1) > > Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org> _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string @ 2020-09-23 20:42 ` Rob Herring 0 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2020-09-23 20:42 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Bayi Cheng, linux-spi, Rob Herring, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, Chuanhong Guo, linux-arm-kernel On Fri, 18 Sep 2020 16:31:19 +0800, Ikjoon Jang wrote: > Add compatible string for mt8192 SoC. > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > > (no changes since v1) > > Documentation/devicetree/bindings/spi/mediatek,spi-mtk-nor.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring <robh@kernel.org> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 8:31 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek 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 <ikjn@chromium.org> --- (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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 13:09 ` Chuanhong Guo -1 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:09 UTC (permalink / raw) To: Ikjoon Jang Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:09 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:09 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:09 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:09 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:09 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:09 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation 2020-09-18 13:09 ` Chuanhong Guo (?) (?) @ 2020-09-18 13:33 ` Chuanhong Guo -1 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:33 UTC (permalink / raw) To: Ikjoon Jang Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:33 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:33 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:33 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:33 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-18 13:33 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:33 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation 2020-09-18 13:09 ` Chuanhong Guo (?) (?) @ 2020-09-21 6:10 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:10 UTC (permalink / raw) To: Chuanhong Guo Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-21 6:10 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:10 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-21 6:10 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:10 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation @ 2020-09-21 6:10 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:10 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:09 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:34 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 8:31 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek Use dma_alloc_coherent() for bounce buffer instead of kmalloc. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel Use dma_alloc_coherent() for bounce buffer instead of kmalloc. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel Use dma_alloc_coherent() for bounce buffer instead of kmalloc. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel Use dma_alloc_coherent() for bounce buffer instead of kmalloc. Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 13:25 ` Chuanhong Guo -1 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:25 UTC (permalink / raw) To: Ikjoon Jang Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 13:25 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:25 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 13:25 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:25 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-18 13:25 ` Chuanhong Guo 0 siblings, 0 replies; 56+ messages in thread From: Chuanhong Guo @ 2020-09-18 13:25 UTC (permalink / raw) To: Ikjoon Jang Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support Hi! On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > --- > > (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer 2020-09-18 13:25 ` Chuanhong Guo (?) (?) @ 2020-09-21 6:52 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:52 UTC (permalink / raw) To: Chuanhong Guo Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, open list, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:25 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-21 6:52 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:52 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:25 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-21 6:52 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:52 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:25 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer @ 2020-09-21 6:52 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:52 UTC (permalink / raw) To: Chuanhong Guo Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Fri, Sep 18, 2020 at 9:25 PM Chuanhong Guo <gch981213@gmail.com> wrote: > > Hi! > > On Fri, Sep 18, 2020 at 4:35 PM Ikjoon Jang <ikjn@chromium.org> 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 <ikjn@chromium.org> > > > > --- > > > > (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 8:31 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek 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 <ikjn@chromium.org> --- (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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-19 15:26 ` Yingjoe Chen -1 siblings, 0 replies; 56+ messages in thread From: Yingjoe Chen @ 2020-09-19 15:26 UTC (permalink / raw) To: Ikjoon Jang Cc: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd, Matthias Brugger, linux-mediatek, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> > --- > > (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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-19 15:26 ` Yingjoe Chen 0 siblings, 0 replies; 56+ messages in thread From: Yingjoe Chen @ 2020-09-19 15:26 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, linux-arm-kernel 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 <ikjn@chromium.org> > --- > > (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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-19 15:26 ` Yingjoe Chen 0 siblings, 0 replies; 56+ messages in thread From: Yingjoe Chen @ 2020-09-19 15:26 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, linux-arm-kernel 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 <ikjn@chromium.org> > --- > > (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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-19 15:26 ` Yingjoe Chen 0 siblings, 0 replies; 56+ messages in thread From: Yingjoe Chen @ 2020-09-19 15:26 UTC (permalink / raw) To: Ikjoon Jang Cc: devicetree, linux-kernel, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, linux-mediatek, linux-arm-kernel 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 <ikjn@chromium.org> > --- > > (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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek 2020-09-19 15:26 ` Yingjoe Chen (?) (?) @ 2020-09-21 6:54 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:54 UTC (permalink / raw) To: Yingjoe Chen Cc: Rob Herring, Mark Brown, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-spi, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support, open list On Sat, Sep 19, 2020 at 11:26 PM Yingjoe Chen <yingjoe.chen@mediatek.com> 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-21 6:54 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:54 UTC (permalink / raw) To: Yingjoe Chen Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Sat, Sep 19, 2020 at 11:26 PM Yingjoe Chen <yingjoe.chen@mediatek.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-21 6:54 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:54 UTC (permalink / raw) To: Yingjoe Chen Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Sat, Sep 19, 2020 at 11:26 PM Yingjoe Chen <yingjoe.chen@mediatek.com> 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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek @ 2020-09-21 6:54 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-21 6:54 UTC (permalink / raw) To: Yingjoe Chen Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list, Rob Herring, linux-spi, Mark Brown, linux-mtd, Matthias Brugger, moderated list:ARM/Mediatek SoC support, moderated list:ARM/Mediatek SoC support On Sat, Sep 19, 2020 at 11:26 PM Yingjoe Chen <yingjoe.chen@mediatek.com> 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 5/5] spi: spi-mtk-nor: Add power management support 2020-09-18 8:31 ` Ikjoon Jang (?) (?) @ 2020-09-18 8:31 ` Ikjoon Jang -1 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Ikjoon Jang, Matthias Brugger, linux-arm-kernel, linux-kernel, linux-mediatek 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 <ikjn@chromium.org> --- 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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 5/5] spi: spi-mtk-nor: Add power management support @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 5/5] spi: spi-mtk-nor: Add power management support @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- 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 _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH v2 5/5] spi: spi-mtk-nor: Add power management support @ 2020-09-18 8:31 ` Ikjoon Jang 0 siblings, 0 replies; 56+ messages in thread From: Ikjoon Jang @ 2020-09-18 8:31 UTC (permalink / raw) To: Rob Herring, Mark Brown, devicetree, linux-spi, linux-mtd Cc: Matthias Brugger, linux-mediatek, Ikjoon Jang, linux-arm-kernel, linux-kernel 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 <ikjn@chromium.org> --- 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 56+ messages in thread
end of thread, other threads:[~2020-09-23 20:43 UTC | newest] Thread overview: 56+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-18 8:31 [PATCH v2 0/5] spi: spi-mtk-nor: Add mt8192 support Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` [PATCH v2 1/5] dt-bindings: spi: add mt8192-nor compatible string Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-23 20:42 ` Rob Herring 2020-09-23 20:42 ` Rob Herring 2020-09-23 20:42 ` Rob Herring 2020-09-23 20:42 ` Rob Herring 2020-09-18 8:31 ` [PATCH v2 2/5] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 13:09 ` Chuanhong Guo 2020-09-18 13:09 ` Chuanhong Guo 2020-09-18 13:09 ` Chuanhong Guo 2020-09-18 13:09 ` Chuanhong Guo 2020-09-18 13:33 ` Chuanhong Guo 2020-09-18 13:33 ` Chuanhong Guo 2020-09-18 13:33 ` Chuanhong Guo 2020-09-18 13:33 ` Chuanhong Guo 2020-09-21 6:10 ` Ikjoon Jang 2020-09-21 6:10 ` Ikjoon Jang 2020-09-21 6:10 ` Ikjoon Jang 2020-09-21 6:10 ` Ikjoon Jang 2020-09-18 8:31 ` [PATCH v2 3/5] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 13:25 ` Chuanhong Guo 2020-09-18 13:25 ` Chuanhong Guo 2020-09-18 13:25 ` Chuanhong Guo 2020-09-18 13:25 ` Chuanhong Guo 2020-09-21 6:52 ` Ikjoon Jang 2020-09-21 6:52 ` Ikjoon Jang 2020-09-21 6:52 ` Ikjoon Jang 2020-09-21 6:52 ` Ikjoon Jang 2020-09-18 8:31 ` [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-19 15:26 ` Yingjoe Chen 2020-09-19 15:26 ` Yingjoe Chen 2020-09-19 15:26 ` Yingjoe Chen 2020-09-19 15:26 ` Yingjoe Chen 2020-09-21 6:54 ` Ikjoon Jang 2020-09-21 6:54 ` Ikjoon Jang 2020-09-21 6:54 ` Ikjoon Jang 2020-09-21 6:54 ` Ikjoon Jang 2020-09-18 8:31 ` [PATCH v2 5/5] spi: spi-mtk-nor: Add power management support Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang 2020-09-18 8:31 ` Ikjoon Jang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.