[PATCH v3 0/6] spi: spi-mtk-nor: Add mt8192 support

[PATCH v3 0/6] spi: spi-mtk-nor: Add mt8192 support

[Ikjoon Jang]

This patchset adds 36bit dma address and power management
supports for mt8192-nor.

Changes in v3:
- Fix a bugfix of v2 in checking spi memory operation.
- split read_dma function into two (normal/bounce)
- Support 7bytes generic spi xfer

Changes in v2:
- Add power management support
- Fix bugs in checking spi memory operation.
- use dma_alloc_coherent for allocating bounce buffer
- code cleanups

Ikjoon Jang (6):
  dt-bindings: spi: add mt8192-nor compatible string
  spi: spi-mtk-nor: fix mishandled logics in checking SPI memory
    operation
  spi: spi-mtk-nor: support 7 bytes transfer of generic spi
  spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer
  spi: spi-mtk-nor: support 36bit dma addressing
  spi: spi-mtk-nor: Add power management support

 .../bindings/spi/mediatek,spi-mtk-nor.yaml    |   1 +
 drivers/spi/spi-mtk-nor.c                     | 333 ++++++++++++------
 2 files changed, 230 insertions(+), 104 deletions(-)

-- 
2.28.0.681.g6f77f65b4e-goog

[PATCH v3 1/6] dt-bindings: spi: add mt8192-nor compatible string

[Ikjoon Jang]

Add MT8192 spi-nor controller support.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
Acked-by: Rob Herring <robh@kernel.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

[PATCH v3 2/6] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation

[Ikjoon Jang]

Fix a bug which limits its protocol availability in supports_op().

Fixes: a59b2c7c56bf ("spi: spi-mtk-nor: support standard spi properties")
Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---

(no changes since v1)

 drivers/spi/spi-mtk-nor.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 6e6ca2b8e6c8..0f7d4ec68730 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -211,28 +211,24 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
 	if (op->cmd.buswidth != 1)
 		return false;
 
+	if (!spi_mem_default_supports_op(mem, op))
+		return false;
+
 	if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
-		switch(op->data.dir) {
-		case SPI_MEM_DATA_IN:
-			if (!mtk_nor_match_read(op))
-				return false;
-			break;
-		case SPI_MEM_DATA_OUT:
-			if ((op->addr.buswidth != 1) ||
-			    (op->dummy.nbytes != 0) ||
-			    (op->data.buswidth != 1))
-				return false;
-			break;
-		default:
-			break;
-		}
+		if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
+			return true;
+		else if (op->data.dir == SPI_MEM_DATA_OUT)
+			return (op->addr.buswidth == 1) &&
+			       (op->dummy.nbytes == 0) &&
+			       (op->data.buswidth == 1);
 	}
+
 	len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
 	if ((len > MTK_NOR_PRG_MAX_SIZE) ||
 	    ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
 		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return true;
 }
 
 static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH v3 2/6] spi: spi-mtk-nor: fix mishandled logics in checking SPI memory operation

[Ikjoon Jang]

On Fri, Sep 25, 2020 at 2:54 PM Ikjoon Jang <ikjn@chromium.org> wrote:
>
> Fix a bug which limits its protocol availability in supports_op().
>
> Fixes: a59b2c7c56bf ("spi: spi-mtk-nor: support standard spi properties")
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---

This is also duplicated work of https://patchwork.kernel.org/patch/11797723/,
I'm going to drop this patch in v4.

>
> (no changes since v1)
>
>  drivers/spi/spi-mtk-nor.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 6e6ca2b8e6c8..0f7d4ec68730 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -211,28 +211,24 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
>         if (op->cmd.buswidth != 1)
>                 return false;
>
> +       if (!spi_mem_default_supports_op(mem, op))
> +               return false;
> +
>         if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
> -               switch(op->data.dir) {
> -               case SPI_MEM_DATA_IN:
> -                       if (!mtk_nor_match_read(op))
> -                               return false;
> -                       break;
> -               case SPI_MEM_DATA_OUT:
> -                       if ((op->addr.buswidth != 1) ||
> -                           (op->dummy.nbytes != 0) ||
> -                           (op->data.buswidth != 1))
> -                               return false;
> -                       break;
> -               default:
> -                       break;
> -               }
> +               if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
> +                       return true;
> +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> +                       return (op->addr.buswidth == 1) &&
> +                              (op->dummy.nbytes == 0) &&
> +                              (op->data.buswidth == 1);
>         }
> +
>         len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
>         if ((len > MTK_NOR_PRG_MAX_SIZE) ||
>             ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
>                 return false;
>
> -       return spi_mem_default_supports_op(mem, op);
> +       return true;
>  }
>
>  static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> --
> 2.28.0.681.g6f77f65b4e-goog
>

[PATCH v3 3/6] spi: spi-mtk-nor: support 7 bytes transfer of generic spi

[Ikjoon Jang]

When mtk-nor fallbacks to generic spi transfers, it can actually
transfer up to 7 bytes.

This patch fixes adjust_op_size() and supports_op() to explicitly
check 7 bytes range and also fixes possible under/overflow conditions
in register offsets calculation.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---

(no changes since v1)

 drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
 1 file changed, 76 insertions(+), 26 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 0f7d4ec68730..e7719d249095 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -79,7 +79,11 @@
 #define MTK_NOR_REG_DMA_DADR		0x720
 #define MTK_NOR_REG_DMA_END_DADR	0x724
 
+/* maximum bytes of TX in PRG mode */
 #define MTK_NOR_PRG_MAX_SIZE		6
+/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
+#define MTK_NOR_PRG_MAX_CYCLES		7
+
 // Reading DMA src/dst addresses have to be 16-byte aligned
 #define MTK_NOR_DMA_ALIGN		16
 #define MTK_NOR_DMA_ALIGN_MASK		(MTK_NOR_DMA_ALIGN - 1)
@@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
 	return false;
 }
 
+static bool mtk_nor_check_prg(const struct spi_mem_op *op)
+{
+	size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+	if (len > MTK_NOR_PRG_MAX_SIZE)
+		return false;
+
+	if (!op->data.nbytes)
+		return true;
+
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
+	else if (op->data.dir == SPI_MEM_DATA_IN)
+		return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
+	else
+		return true;
+}
+
 static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 {
 	size_t len;
@@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 		}
 	}
 
-	len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
-	      op->dummy.nbytes;
-	if (op->data.nbytes > len)
-		op->data.nbytes = len;
+	if (mtk_nor_check_prg(op))
+		return 0;
+
+	len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
+
+	if (op->data.dir == SPI_MEM_DATA_OUT) {
+		if (len == MTK_NOR_PRG_MAX_SIZE)
+			return -EINVAL;
+		op->data.nbytes = min_t(unsigned int, op->data.nbytes,
+					MTK_NOR_PRG_MAX_SIZE - len);
+	} else  {
+		if (len == MTK_NOR_PRG_MAX_CYCLES)
+			return -EINVAL;
+		op->data.nbytes = min_t(unsigned int, op->data.nbytes,
+					MTK_NOR_PRG_MAX_CYCLES - len);
+	}
 
 	return 0;
 }
@@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 static bool mtk_nor_supports_op(struct spi_mem *mem,
 				const struct spi_mem_op *op)
 {
-	size_t len;
-
 	if (op->cmd.buswidth != 1)
 		return false;
 
@@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
 			       (op->data.buswidth == 1);
 	}
 
-	len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
-	if ((len > MTK_NOR_PRG_MAX_SIZE) ||
-	    ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
+	/* fallback to generic spi xfer */
+	if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
 		return false;
 
-	return true;
+	return mtk_nor_check_prg(op);
 }
 
 static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
@@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
 	int stat = 0;
 	int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
 	void __iomem *reg;
-	const u8 *txbuf;
-	u8 *rxbuf;
-	int i;
+	int i, tx_len = 0, rx_len = 0;
 
 	list_for_each_entry(t, &m->transfers, transfer_list) {
-		txbuf = t->tx_buf;
-		for (i = 0; i < t->len; i++, reg_offset--) {
+		const u8 *txbuf = t->tx_buf;
+
+		if (!txbuf) {
+			rx_len += t->len;
+			continue;
+		}
+
+		if (rx_len) {
+			stat = -EPROTO;
+			goto msg_done;
+		}
+
+		for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
 			reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
-			if (txbuf)
-				writeb(txbuf[i], reg);
-			else
-				writeb(0, reg);
+			writeb(txbuf[i], reg);
+			tx_len++;
 		}
-		trx_len += t->len;
 	}
 
+	while (reg_offset >= 0) {
+		writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
+		reg_offset--;
+	}
+
+	rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
+	trx_len = tx_len + rx_len;
+
 	writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
 
 	stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
@@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
 	if (stat < 0)
 		goto msg_done;
 
-	reg_offset = trx_len - 1;
-	list_for_each_entry(t, &m->transfers, transfer_list) {
-		rxbuf = t->rx_buf;
-		for (i = 0; i < t->len; i++, reg_offset--) {
-			reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
-			if (rxbuf)
+	if (rx_len > 0) {
+		reg_offset = rx_len - 1;
+		list_for_each_entry(t, &m->transfers, transfer_list) {
+			u8 *rxbuf = t->rx_buf;
+
+			if (!rxbuf)
+				continue;
+
+			for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
+				reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
 				rxbuf[i] = readb(reg);
+			}
 		}
 	}
 
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH v3 3/6] spi: spi-mtk-nor: support 7 bytes transfer of generic spi

[Chuanhong Guo]

Hi!

On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <ikjn@chromium.org> wrote:
>
> When mtk-nor fallbacks to generic spi transfers, it can actually
> transfer up to 7 bytes.

generic transfer_one_message should support full-duplex transfers,
not transfers with special format requirements. (e.g. here the last
byte is rx only.) These transfers with format requirements should
be implemented with spi-mem interface instead.

>
> This patch fixes adjust_op_size() and supports_op() to explicitly
> check 7 bytes range and also fixes possible under/overflow conditions
> in register offsets calculation.
>
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>

I was notified by Bayi about your discussion and sent some
patches yesterday for the same purpose. Whoops...
As transfer_one_message isn't the proper place to implement
this, maybe we could work on my version instead?

> ---
>
> (no changes since v1)

This should be "new patch" not "no changes" :P


>
>  drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 0f7d4ec68730..e7719d249095 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -79,7 +79,11 @@
>  #define MTK_NOR_REG_DMA_DADR           0x720
>  #define MTK_NOR_REG_DMA_END_DADR       0x724
>
> +/* maximum bytes of TX in PRG mode */
>  #define MTK_NOR_PRG_MAX_SIZE           6
> +/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
> +#define MTK_NOR_PRG_MAX_CYCLES         7
> +
>  // Reading DMA src/dst addresses have to be 16-byte aligned
>  #define MTK_NOR_DMA_ALIGN              16
>  #define MTK_NOR_DMA_ALIGN_MASK         (MTK_NOR_DMA_ALIGN - 1)
> @@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
>         return false;
>  }
>
> +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> +{
> +       size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> +       if (len > MTK_NOR_PRG_MAX_SIZE)
> +               return false;
> +
> +       if (!op->data.nbytes)
> +               return true;
> +
> +       if (op->data.dir == SPI_MEM_DATA_OUT)
> +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> +       else if (op->data.dir == SPI_MEM_DATA_IN)
> +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
> +       else
> +               return true;
> +}
> +
>  static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  {
>         size_t len;
> @@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>                 }
>         }
>
> -       len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> -             op->dummy.nbytes;
> -       if (op->data.nbytes > len)
> -               op->data.nbytes = len;
> +       if (mtk_nor_check_prg(op))
> +               return 0;
> +
> +       len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> +       if (op->data.dir == SPI_MEM_DATA_OUT) {
> +               if (len == MTK_NOR_PRG_MAX_SIZE)
> +                       return -EINVAL;
> +               op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> +                                       MTK_NOR_PRG_MAX_SIZE - len);
> +       } else  {
> +               if (len == MTK_NOR_PRG_MAX_CYCLES)
> +                       return -EINVAL;
> +               op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> +                                       MTK_NOR_PRG_MAX_CYCLES - len);
> +       }
>
>         return 0;
>  }
> @@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  static bool mtk_nor_supports_op(struct spi_mem *mem,
>                                 const struct spi_mem_op *op)
>  {
> -       size_t len;
> -
>         if (op->cmd.buswidth != 1)
>                 return false;
>
> @@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
>                                (op->data.buswidth == 1);
>         }
>
> -       len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> -       if ((len > MTK_NOR_PRG_MAX_SIZE) ||
> -           ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
> +       /* fallback to generic spi xfer */
> +       if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
>                 return false;

Rejecting an op in supports_op doesn't tell it to fall back to generic
spi transfer.
It instead tells caller to abort this transfer completely.
A fallback only happens when exec_op returns -ENOTSUPP.
This comment is incorrect. I'd put this buswidth checking in mtk_nor_check_prg
instead because mtk_nor_check_prg is checking whether an op is supported
by prg mode, thus it should reject ops with buswidth > 1.

>
> -       return true;
> +       return mtk_nor_check_prg(op);
>  }
>
>  static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> @@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
>         int stat = 0;
>         int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
>         void __iomem *reg;
> -       const u8 *txbuf;
> -       u8 *rxbuf;
> -       int i;
> +       int i, tx_len = 0, rx_len = 0;
>
>         list_for_each_entry(t, &m->transfers, transfer_list) {
> -               txbuf = t->tx_buf;
> -               for (i = 0; i < t->len; i++, reg_offset--) {
> +               const u8 *txbuf = t->tx_buf;
> +
> +               if (!txbuf) {
> +                       rx_len += t->len;
> +                       continue;
> +               }
> +
> +               if (rx_len) {
> +                       stat = -EPROTO;
> +                       goto msg_done;
> +               }

NACK. you are unnecessarily rejecting possible transfers.

> +
> +               for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
>                         reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
> -                       if (txbuf)
> -                               writeb(txbuf[i], reg);
> -                       else
> -                               writeb(0, reg);
> +                       writeb(txbuf[i], reg);
> +                       tx_len++;

According to SPI standard, during a rx transfer, tx should be kept low.
These PROGDATA registers doesn't clear itself so it'll keep sending
data from last transfer, which violates this rule. That's
why the original code writes 0 to PRGDATA for rx bytes.

>                 }
> -               trx_len += t->len;
>         }
>
> +       while (reg_offset >= 0) {
> +               writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
> +               reg_offset--;
> +       }
> +
> +       rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
> +       trx_len = tx_len + rx_len;
> +
>         writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
>
>         stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
> @@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
>         if (stat < 0)
>                 goto msg_done;
>
> -       reg_offset = trx_len - 1;
> -       list_for_each_entry(t, &m->transfers, transfer_list) {
> -               rxbuf = t->rx_buf;
> -               for (i = 0; i < t->len; i++, reg_offset--) {
> -                       reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> -                       if (rxbuf)
> +       if (rx_len > 0) {
> +               reg_offset = rx_len - 1;
> +               list_for_each_entry(t, &m->transfers, transfer_list) {
> +                       u8 *rxbuf = t->rx_buf;
> +
> +                       if (!rxbuf)
> +                               continue;
> +
> +                       for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> +                               reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
>                                 rxbuf[i] = readb(reg);
> +                       }

I think this is replacing original code with some equivalent ones, which
seems unnecessary.

>                 }
>         }
>
-- 
Regards,
Chuanhong Guo

Re: [PATCH v3 3/6] spi: spi-mtk-nor: support 7 bytes transfer of generic spi

[Ikjoon Jang]

On Fri, Sep 25, 2020 at 3:47 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> Hi!
>
> On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <ikjn@chromium.org> wrote:
> >
> > When mtk-nor fallbacks to generic spi transfers, it can actually
> > transfer up to 7 bytes.
>
> generic transfer_one_message should support full-duplex transfers,
> not transfers with special format requirements. (e.g. here the last
> byte is rx only.) These transfers with format requirements should
> be implemented with spi-mem interface instead.

yep, that's correct.

>
> >
> > This patch fixes adjust_op_size() and supports_op() to explicitly
> > check 7 bytes range and also fixes possible under/overflow conditions
> > in register offsets calculation.
> >
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
>
> I was notified by Bayi about your discussion and sent some
> patches yesterday for the same purpose. Whoops...
> As transfer_one_message isn't the proper place to implement
> this, maybe we could work on my version instead?
>

I didn't noticed that before,
Sure, please go ahead, I'll follow up with your patch in v4.

> > ---
> >
> > (no changes since v1)
>
> This should be "new patch" not "no changes" :P

oops, it seems my script did something wrong.

>
>
> >
> >  drivers/spi/spi-mtk-nor.c | 102 ++++++++++++++++++++++++++++----------
> >  1 file changed, 76 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 0f7d4ec68730..e7719d249095 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -79,7 +79,11 @@
> >  #define MTK_NOR_REG_DMA_DADR           0x720
> >  #define MTK_NOR_REG_DMA_END_DADR       0x724
> >
> > +/* maximum bytes of TX in PRG mode */
> >  #define MTK_NOR_PRG_MAX_SIZE           6
> > +/* maximum bytes of TX + RX is 7, last 1 byte is always being sent as zero */
> > +#define MTK_NOR_PRG_MAX_CYCLES         7
> > +
> >  // Reading DMA src/dst addresses have to be 16-byte aligned
> >  #define MTK_NOR_DMA_ALIGN              16
> >  #define MTK_NOR_DMA_ALIGN_MASK         (MTK_NOR_DMA_ALIGN - 1)
> > @@ -167,6 +171,24 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)
> >         return false;
> >  }
> >
> > +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> > +{
> > +       size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > +       if (len > MTK_NOR_PRG_MAX_SIZE)
> > +               return false;
> > +
> > +       if (!op->data.nbytes)
> > +               return true;
> > +
> > +       if (op->data.dir == SPI_MEM_DATA_OUT)
> > +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> > +       else if (op->data.dir == SPI_MEM_DATA_IN)
> > +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);
> > +       else
> > +               return true;
> > +}
> > +
> >  static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> >  {
> >         size_t len;
> > @@ -195,10 +217,22 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> >                 }
> >         }
> >
> > -       len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
> > -             op->dummy.nbytes;
> > -       if (op->data.nbytes > len)
> > -               op->data.nbytes = len;
> > +       if (mtk_nor_check_prg(op))
> > +               return 0;
> > +
> > +       len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > +
> > +       if (op->data.dir == SPI_MEM_DATA_OUT) {
> > +               if (len == MTK_NOR_PRG_MAX_SIZE)
> > +                       return -EINVAL;
> > +               op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> > +                                       MTK_NOR_PRG_MAX_SIZE - len);
> > +       } else  {
> > +               if (len == MTK_NOR_PRG_MAX_CYCLES)
> > +                       return -EINVAL;
> > +               op->data.nbytes = min_t(unsigned int, op->data.nbytes,
> > +                                       MTK_NOR_PRG_MAX_CYCLES - len);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -206,8 +240,6 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> >  static bool mtk_nor_supports_op(struct spi_mem *mem,
> >                                 const struct spi_mem_op *op)
> >  {
> > -       size_t len;
> > -
> >         if (op->cmd.buswidth != 1)
> >                 return false;
> >
> > @@ -223,12 +255,11 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
> >                                (op->data.buswidth == 1);
> >         }
> >
> > -       len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > -       if ((len > MTK_NOR_PRG_MAX_SIZE) ||
> > -           ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
> > +       /* fallback to generic spi xfer */
> > +       if (op->cmd.buswidth > 1 || op->addr.buswidth > 1 || op->data.buswidth > 1)
> >                 return false;
>
> Rejecting an op in supports_op doesn't tell it to fall back to generic
> spi transfer.
> It instead tells caller to abort this transfer completely.
> A fallback only happens when exec_op returns -ENOTSUPP.

yep but I think that case always going PRG mode in exec_op() with the
same condition?

> This comment is incorrect. I'd put this buswidth checking in mtk_nor_check_prg
> instead because mtk_nor_check_prg is checking whether an op is supported
> by prg mode, thus it should reject ops with buswidth > 1.
>
> >
> > -       return true;
> > +       return mtk_nor_check_prg(op);
> >  }
> >
> >  static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
> > @@ -459,22 +490,36 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> >         int stat = 0;
> >         int reg_offset = MTK_NOR_REG_PRGDATA_MAX;
> >         void __iomem *reg;
> > -       const u8 *txbuf;
> > -       u8 *rxbuf;
> > -       int i;
> > +       int i, tx_len = 0, rx_len = 0;
> >
> >         list_for_each_entry(t, &m->transfers, transfer_list) {
> > -               txbuf = t->tx_buf;
> > -               for (i = 0; i < t->len; i++, reg_offset--) {
> > +               const u8 *txbuf = t->tx_buf;
> > +
> > +               if (!txbuf) {
> > +                       rx_len += t->len;
> > +                       continue;
> > +               }
> > +
> > +               if (rx_len) {
> > +                       stat = -EPROTO;
> > +                       goto msg_done;
> > +               }
>
> NACK. you are unnecessarily rejecting possible transfers.

yep, ditto

>
> > +
> > +               for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> >                         reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);
> > -                       if (txbuf)
> > -                               writeb(txbuf[i], reg);
> > -                       else
> > -                               writeb(0, reg);
> > +                       writeb(txbuf[i], reg);
> > +                       tx_len++;
>
> According to SPI standard, during a rx transfer, tx should be kept low.
> These PROGDATA registers doesn't clear itself so it'll keep sending
> data from last transfer, which violates this rule. That's
> why the original code writes 0 to PRGDATA for rx bytes.

following lines with while() will set 0s to the rest of registers.

>
> >                 }
> > -               trx_len += t->len;
> >         }
> >
> > +       while (reg_offset >= 0) {
> > +               writeb(0, sp->base + MTK_NOR_REG_PRGDATA(reg_offset));
> > +               reg_offset--;
> > +       }
> > +
> > +       rx_len = min_t(unsigned long, MTK_NOR_PRG_MAX_CYCLES - tx_len, rx_len);
> > +       trx_len = tx_len + rx_len;
> > +
> >         writel(trx_len * BITS_PER_BYTE, sp->base + MTK_NOR_REG_PRG_CNT);
> >
> >         stat = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
> > @@ -482,13 +527,18 @@ static int mtk_nor_transfer_one_message(struct spi_controller *master,
> >         if (stat < 0)
> >                 goto msg_done;
> >
> > -       reg_offset = trx_len - 1;
> > -       list_for_each_entry(t, &m->transfers, transfer_list) {
> > -               rxbuf = t->rx_buf;
> > -               for (i = 0; i < t->len; i++, reg_offset--) {
> > -                       reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> > -                       if (rxbuf)
> > +       if (rx_len > 0) {
> > +               reg_offset = rx_len - 1;
> > +               list_for_each_entry(t, &m->transfers, transfer_list) {
> > +                       u8 *rxbuf = t->rx_buf;
> > +
> > +                       if (!rxbuf)
> > +                               continue;
> > +
> > +                       for (i = 0; i < t->len && reg_offset >= 0; i++, reg_offset--) {
> > +                               reg = sp->base + MTK_NOR_REG_SHIFT(reg_offset);
> >                                 rxbuf[i] = readb(reg);
> > +                       }
>
> I think this is replacing original code with some equivalent ones, which
> seems unnecessary.

This patch addressed the issue with 1+6 bytes transfer (e.g JEDEC ID)
can have negative reg_offset.
And there's skipping the loop if (rx_len < 0)
anyway I'd like to follow with your new patch. :-)

Thanks!

>
> >                 }
> >         }
> >
> --
> Regards,
> Chuanhong Guo

Re: [PATCH v3 3/6] spi: spi-mtk-nor: support 7 bytes transfer of generic spi

[Chuanhong Guo]

HI!
One more comment:
On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <ikjn@chromium.org> wrote:
> +static bool mtk_nor_check_prg(const struct spi_mem_op *op)
> +{
> +       size_t len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> +
> +       if (len > MTK_NOR_PRG_MAX_SIZE)
> +               return false;
> +
> +       if (!op->data.nbytes)
> +               return true;
> +
> +       if (op->data.dir == SPI_MEM_DATA_OUT)
> +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_SIZE);
> +       else if (op->data.dir == SPI_MEM_DATA_IN)
> +               return ((len + op->data.nbytes) <= MTK_NOR_PRG_MAX_CYCLES);

You need to consider the existence of adjust_op_size in supports_op as well.
This mtk_nor_check_prg still rejects SFDP reading command from spi-nor
driver altogether.

-- 
Regards,
Chuanhong Guo

[PATCH v3 4/6] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer

[Ikjoon Jang]

Use dma_alloc_coherent() for bounce buffer instead of kmalloc() to
make sure the bounce buffer to be allocated within its DMAable range.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>

---

(no changes since v1)

 drivers/spi/spi-mtk-nor.c | 93 +++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index e7719d249095..8dbafee7f431 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -100,6 +100,7 @@ struct mtk_nor {
 	struct device *dev;
 	void __iomem *base;
 	u8 *buffer;
+	dma_addr_t buffer_dma;
 	struct clk *spi_clk;
 	struct clk *ctlr_clk;
 	unsigned int spi_freq;
@@ -148,6 +149,11 @@ static void mtk_nor_set_addr(struct mtk_nor *sp, const struct spi_mem_op *op)
 	}
 }
 
+static bool need_bounce(struct mtk_nor *sp, const struct spi_mem_op *op)
+{
+	return ((uintptr_t)op->data.buf.in & MTK_NOR_DMA_ALIGN_MASK);
+}
+
 static bool mtk_nor_match_read(const struct spi_mem_op *op)
 {
 	int dummy = 0;
@@ -191,6 +197,7 @@ static bool mtk_nor_check_prg(const struct spi_mem_op *op)
 
 static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 {
+	struct mtk_nor *sp = spi_controller_get_devdata(mem->spi->master);
 	size_t len;
 
 	if (!op->data.nbytes)
@@ -202,8 +209,7 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 			if ((op->addr.val & MTK_NOR_DMA_ALIGN_MASK) ||
 			    (op->data.nbytes < MTK_NOR_DMA_ALIGN))
 				op->data.nbytes = 1;
-			else if (!((ulong)(op->data.buf.in) &
-				   MTK_NOR_DMA_ALIGN_MASK))
+			else if (!need_bounce(sp, op))
 				op->data.nbytes &= ~MTK_NOR_DMA_ALIGN_MASK;
 			else if (op->data.nbytes > MTK_NOR_BOUNCE_BUF_SIZE)
 				op->data.nbytes = MTK_NOR_BOUNCE_BUF_SIZE;
@@ -288,19 +294,12 @@ static void mtk_nor_setup_bus(struct mtk_nor *sp, const struct spi_mem_op *op)
 	mtk_nor_rmw(sp, MTK_NOR_REG_BUSCFG, reg, MTK_NOR_BUS_MODE_MASK);
 }
 
-static int mtk_nor_read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
-			    u8 *buffer)
+static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
+			    dma_addr_t dma_addr)
 {
 	int ret = 0;
 	ulong delay;
 	u32 reg;
-	dma_addr_t dma_addr;
-
-	dma_addr = dma_map_single(sp->dev, buffer, length, DMA_FROM_DEVICE);
-	if (dma_mapping_error(sp->dev, dma_addr)) {
-		dev_err(sp->dev, "failed to map dma buffer.\n");
-		return -EINVAL;
-	}
 
 	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
 	writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
@@ -325,30 +324,49 @@ static int mtk_nor_read_dma(struct mtk_nor *sp, u32 from, unsigned int length,
 					 (delay + 1) * 100);
 	}
 
-	dma_unmap_single(sp->dev, dma_addr, length, DMA_FROM_DEVICE);
 	if (ret < 0)
 		dev_err(sp->dev, "dma read timeout.\n");
 
 	return ret;
 }
 
-static int mtk_nor_read_bounce(struct mtk_nor *sp, u32 from,
-			       unsigned int length, u8 *buffer)
+static int mtk_nor_read_bounce(struct mtk_nor *sp, const struct spi_mem_op *op)
 {
 	unsigned int rdlen;
 	int ret;
 
-	if (length & MTK_NOR_DMA_ALIGN_MASK)
-		rdlen = (length + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
+	if (op->data.nbytes & MTK_NOR_DMA_ALIGN_MASK)
+		rdlen = (op->data.nbytes + MTK_NOR_DMA_ALIGN) & ~MTK_NOR_DMA_ALIGN_MASK;
 	else
-		rdlen = length;
+		rdlen = op->data.nbytes;
 
-	ret = mtk_nor_read_dma(sp, from, rdlen, sp->buffer);
-	if (ret)
-		return ret;
+	ret = mtk_nor_dma_exec(sp, op->addr.val, rdlen, sp->buffer_dma);
 
-	memcpy(buffer, sp->buffer, length);
-	return 0;
+	if (!ret)
+		memcpy(op->data.buf.in, sp->buffer, op->data.nbytes);
+
+	return ret;
+}
+
+static int mtk_nor_read_dma(struct mtk_nor *sp, const struct spi_mem_op *op)
+{
+	int ret;
+	dma_addr_t dma_addr;
+
+	if (need_bounce(sp, op))
+		return mtk_nor_read_bounce(sp, op);
+
+	dma_addr = dma_map_single(sp->dev, op->data.buf.in,
+				  op->data.nbytes, DMA_FROM_DEVICE);
+
+	if (dma_mapping_error(sp->dev, dma_addr))
+		return -EINVAL;
+
+	ret = mtk_nor_dma_exec(sp, op->addr.val, op->data.nbytes, dma_addr);
+
+	dma_unmap_single(sp->dev, dma_addr, op->data.nbytes, DMA_FROM_DEVICE);
+
+	return ret;
 }
 
 static int mtk_nor_read_pio(struct mtk_nor *sp, const struct spi_mem_op *op)
@@ -452,15 +470,8 @@ static int mtk_nor_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		if (op->data.nbytes == 1) {
 			mtk_nor_set_addr(sp, op);
 			return mtk_nor_read_pio(sp, op);
-		} else if (((ulong)(op->data.buf.in) &
-			    MTK_NOR_DMA_ALIGN_MASK)) {
-			return mtk_nor_read_bounce(sp, op->addr.val,
-						   op->data.nbytes,
-						   op->data.buf.in);
 		} else {
-			return mtk_nor_read_dma(sp, op->addr.val,
-						op->data.nbytes,
-						op->data.buf.in);
+			return mtk_nor_read_dma(sp, op);
 		}
 	}
 
@@ -634,7 +645,6 @@ static int mtk_nor_probe(struct platform_device *pdev)
 	struct spi_controller *ctlr;
 	struct mtk_nor *sp;
 	void __iomem *base;
-	u8 *buffer;
 	struct clk *spi_clk, *ctlr_clk;
 	int ret, irq;
 
@@ -650,16 +660,6 @@ static int mtk_nor_probe(struct platform_device *pdev)
 	if (IS_ERR(ctlr_clk))
 		return PTR_ERR(ctlr_clk);
 
-	buffer = devm_kmalloc(&pdev->dev,
-			      MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
-			      GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	if ((ulong)buffer & MTK_NOR_DMA_ALIGN_MASK)
-		buffer = (u8 *)(((ulong)buffer + MTK_NOR_DMA_ALIGN) &
-				~MTK_NOR_DMA_ALIGN_MASK);
-
 	ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp));
 	if (!ctlr) {
 		dev_err(&pdev->dev, "failed to allocate spi controller\n");
@@ -679,13 +679,22 @@ static int mtk_nor_probe(struct platform_device *pdev)
 
 	sp = spi_controller_get_devdata(ctlr);
 	sp->base = base;
-	sp->buffer = buffer;
 	sp->has_irq = false;
 	sp->wbuf_en = false;
 	sp->ctlr = ctlr;
 	sp->dev = &pdev->dev;
 	sp->spi_clk = spi_clk;
 	sp->ctlr_clk = ctlr_clk;
+	sp->buffer = dmam_alloc_coherent(&pdev->dev,
+				MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
+				&sp->buffer_dma, GFP_KERNEL);
+	if (!sp->buffer)
+		return -ENOMEM;
+
+	if ((uintptr_t)sp->buffer & MTK_NOR_DMA_ALIGN_MASK) {
+		dev_err(sp->dev, "misaligned allocation of internal buffer.\n");
+		return -ENOMEM;
+	}
 
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0) {
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH v3 4/6] spi: spi-mtk-nor: use dma_alloc_coherent() for bounce buffer

[Chuanhong Guo]

Hi!

On Fri, Sep 25, 2020 at 2:56 PM Ikjoon Jang <ikjn@chromium.org> wrote:
>
> Use dma_alloc_coherent() for bounce buffer instead of kmalloc() to
> make sure the bounce buffer to be allocated within its DMAable range.

I think the introduction of need_bounce can be in its own commit or
at least mentioned here.

>
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
>
> ---
>
> (no changes since v1)

 There have been a lot of changes since v2 :)

Reviewed-by: Chuanhong Guo <gch981213@gmail.com>
-- 
Regards,
Chuanhong Guo

[PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Ikjoon Jang]

This patch enables 36bit dma address support to spi-mtk-nor.
Currently this is enabled only for mt8192-nor.

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---

(no changes since v1)

 drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 8dbafee7f431..35205635ed42 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -78,6 +78,8 @@
 #define MTK_NOR_REG_DMA_FADR		0x71c
 #define MTK_NOR_REG_DMA_DADR		0x720
 #define MTK_NOR_REG_DMA_END_DADR	0x724
+#define MTK_NOR_REG_DMA_DADR_HB		0x738
+#define MTK_NOR_REG_DMA_END_DADR_HB	0x73c
 
 /* maximum bytes of TX in PRG mode */
 #define MTK_NOR_PRG_MAX_SIZE		6
@@ -106,6 +108,7 @@ struct mtk_nor {
 	unsigned int spi_freq;
 	bool wbuf_en;
 	bool has_irq;
+	bool high_dma;
 	struct completion op_done;
 };
 
@@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
 	writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
 	writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
 
+	if (sp->high_dma) {
+		writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
+		writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
+	}
+
 	if (sp->has_irq) {
 		reinit_completion(&sp->op_done);
 		mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
@@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
 };
 
 static const struct of_device_id mtk_nor_match[] = {
-	{ .compatible = "mediatek,mt8173-nor" },
+	{ .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
+	{ .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mtk_nor_match);
@@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
 	void __iomem *base;
 	struct clk *spi_clk, *ctlr_clk;
 	int ret, irq;
+	unsigned long dma_bits;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
 	if (IS_ERR(ctlr_clk))
 		return PTR_ERR(ctlr_clk);
 
+	dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
+		dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
+		return -EINVAL;
+	}
+
 	ctlr = spi_alloc_master(&pdev->dev, sizeof(*sp));
 	if (!ctlr) {
 		dev_err(&pdev->dev, "failed to allocate spi controller\n");
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Chuanhong Guo]

Hi!

On Fri, Sep 25, 2020 at 2:56 PM Ikjoon Jang <ikjn@chromium.org> wrote:
>
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently this is enabled only for mt8192-nor.
>
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 8dbafee7f431..35205635ed42 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
>  #define MTK_NOR_REG_DMA_FADR           0x71c
>  #define MTK_NOR_REG_DMA_DADR           0x720
>  #define MTK_NOR_REG_DMA_END_DADR       0x724
> +#define MTK_NOR_REG_DMA_DADR_HB                0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB    0x73c
>
>  /* maximum bytes of TX in PRG mode */
>  #define MTK_NOR_PRG_MAX_SIZE           6
> @@ -106,6 +108,7 @@ struct mtk_nor {
>         unsigned int spi_freq;
>         bool wbuf_en;
>         bool has_irq;
> +       bool high_dma;
>         struct completion op_done;
>  };
>
> @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>         writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
>         writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>
> +       if (sp->high_dma) {
> +               writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> +               writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> +       }

I remembered kbuild test robot reported a warning on this on 32-bit platforms
in your v1. [0]
I don't know what's the fix for this though :(

[0] https://marc.info/?l=linux-spi&m=159982425706940&w=2
-- 
Regards,
Chuanhong Guo

Re: [PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Ikjoon Jang]

On Fri, Sep 25, 2020 at 4:27 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>

[snip]

> > +       if (sp->high_dma) {
> > +               writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> > +               writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> > +       }
>
> I remembered kbuild test robot reported a warning on this on 32-bit platforms
> in your v1. [0]
> I don't know what's the fix for this though :(
>
> [0] https://marc.info/?l=linux-spi&m=159982425706940&w=2

yeah, I'm not sure how to handle this properly,

"warning: shift count >= width of type",
(sp->high_dma) is always false on 32bit arm kernel.
I think adding size check on here is unnecessary, should I fix for this warning?

> --
> Regards,
> Chuanhong Guo

Sorry for resending, Chuanhong.

Re: [PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Yingjoe Chen]

On Fri, 2020-09-25 at 14:54 +0800, Ikjoon Jang wrote:
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently this is enabled only for mt8192-nor.
> 
> Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 8dbafee7f431..35205635ed42 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
>  #define MTK_NOR_REG_DMA_FADR		0x71c
>  #define MTK_NOR_REG_DMA_DADR		0x720
>  #define MTK_NOR_REG_DMA_END_DADR	0x724
> +#define MTK_NOR_REG_DMA_DADR_HB		0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB	0x73c
>  
>  /* maximum bytes of TX in PRG mode */
>  #define MTK_NOR_PRG_MAX_SIZE		6
> @@ -106,6 +108,7 @@ struct mtk_nor {
>  	unsigned int spi_freq;
>  	bool wbuf_en;
>  	bool has_irq;
> +	bool high_dma;
>  	struct completion op_done;
>  };
>  
> @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>  	writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
>  	writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>  
> +	if (sp->high_dma) {
> +		writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> +		writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> +	}
> +

Maybe use upper_32_bits() ?


>  	if (sp->has_irq) {
>  		reinit_completion(&sp->op_done);
>  		mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> @@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
>  };
>  
>  static const struct of_device_id mtk_nor_match[] = {
> -	{ .compatible = "mediatek,mt8173-nor" },
> +	{ .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> +	{ .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mtk_nor_match);
> @@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	struct clk *spi_clk, *ctlr_clk;
>  	int ret, irq;
> +	unsigned long dma_bits;
>  
>  	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
> @@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
>  	if (IS_ERR(ctlr_clk))
>  		return PTR_ERR(ctlr_clk);
>  
> +	dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
> +	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
> +		dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
> +		return -EINVAL;
> +	}
> +

As said in previous version. I don't see any place enable high_dma, so I
think this patch won't set >32bits for anychip. We need something like:

	sp->hidh_dma = dma_bits > 32;

Am I missing anything?

Joe.C

Re: [PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Ikjoon Jang]

On Sun, Sep 27, 2020 at 4:30 PM Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>
> On Fri, 2020-09-25 at 14:54 +0800, Ikjoon Jang wrote:
> > This patch enables 36bit dma address support to spi-mtk-nor.
> > Currently this is enabled only for mt8192-nor.
> >
> > Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/spi/spi-mtk-nor.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index 8dbafee7f431..35205635ed42 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -78,6 +78,8 @@
> >  #define MTK_NOR_REG_DMA_FADR         0x71c
> >  #define MTK_NOR_REG_DMA_DADR         0x720
> >  #define MTK_NOR_REG_DMA_END_DADR     0x724
> > +#define MTK_NOR_REG_DMA_DADR_HB              0x738
> > +#define MTK_NOR_REG_DMA_END_DADR_HB  0x73c
> >
> >  /* maximum bytes of TX in PRG mode */
> >  #define MTK_NOR_PRG_MAX_SIZE         6
> > @@ -106,6 +108,7 @@ struct mtk_nor {
> >       unsigned int spi_freq;
> >       bool wbuf_en;
> >       bool has_irq;
> > +     bool high_dma;
> >       struct completion op_done;
> >  };
> >
> > @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> >       writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
> >       writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
> >
> > +     if (sp->high_dma) {
> > +             writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> > +             writel((dma_addr + length) >> 32, sp->base + MTK_NOR_REG_DMA_END_DADR_HB);
> > +     }
> > +
>
> Maybe use upper_32_bits() ?

Thanks, good to know that!

>
>
> >       if (sp->has_irq) {
> >               reinit_completion(&sp->op_done);
> >               mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> > @@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops mtk_nor_mem_ops = {
> >  };
> >
> >  static const struct of_device_id mtk_nor_match[] = {
> > -     { .compatible = "mediatek,mt8173-nor" },
> > +     { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> > +     { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
> >       { /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, mtk_nor_match);
> > @@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
> >       void __iomem *base;
> >       struct clk *spi_clk, *ctlr_clk;
> >       int ret, irq;
> > +     unsigned long dma_bits;
> >
> >       base = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(base))
> > @@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
> >       if (IS_ERR(ctlr_clk))
> >               return PTR_ERR(ctlr_clk);
> >
> > +     dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
> > +     if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(dma_bits))) {
> > +             dev_err(&pdev->dev, "failed to set dma mask(%lu)\n", dma_bits);
> > +             return -EINVAL;
> > +     }
> > +
>
> As said in previous version. I don't see any place enable high_dma, so I
> think this patch won't set >32bits for anychip. We need something like:
>
>         sp->hidh_dma = dma_bits > 32;
>
> Am I missing anything?

Yeah, you're right, that line disappeared between v2 ~ v3 (by mistake).

>
> Joe.C
>

[PATCH v3 6/6] spi: spi-mtk-nor: Add power management support

[Ikjoon Jang]

This patch adds dev_pm_ops to mtk-nor to support suspend/resume,
auto suspend delay is set to -1 by default.

Accessing registers are only permitted after its clock is enabled
to deal with unknown state of operating clk at probe time,

Signed-off-by: Ikjoon Jang <ikjn@chromium.org>
---

Changes in v3:
- Fix a bugfix of v2 in checking spi memory operation.
- split read_dma function into two (normal/bounce)
- Support 7bytes generic spi xfer

Changes in v2:
- Add power management support
- Fix bugs in checking spi memory operation.
- use dma_alloc_coherent for allocating bounce buffer
- code cleanups

 drivers/spi/spi-mtk-nor.c | 98 ++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 35205635ed42..bde4c846ce65 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
 #include <linux/string.h>
@@ -592,22 +593,15 @@ static int mtk_nor_enable_clk(struct mtk_nor *sp)
 	return 0;
 }
 
-static int mtk_nor_init(struct mtk_nor *sp)
+static void mtk_nor_init(struct mtk_nor *sp)
 {
-	int ret;
-
-	ret = mtk_nor_enable_clk(sp);
-	if (ret)
-		return ret;
-
-	sp->spi_freq = clk_get_rate(sp->spi_clk);
+	writel(0, sp->base + MTK_NOR_REG_IRQ_EN);
+	writel(MTK_NOR_IRQ_MASK, sp->base + MTK_NOR_REG_IRQ_STAT);
 
 	writel(MTK_NOR_ENABLE_SF_CMD, sp->base + MTK_NOR_REG_WP);
 	mtk_nor_rmw(sp, MTK_NOR_REG_CFG2, MTK_NOR_WR_CUSTOM_OP_EN, 0);
 	mtk_nor_rmw(sp, MTK_NOR_REG_CFG3,
 		    MTK_NOR_DISABLE_WREN | MTK_NOR_DISABLE_SR_POLL, 0);
-
-	return ret;
 }
 
 static irqreturn_t mtk_nor_irq_handler(int irq, void *data)
@@ -690,6 +684,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
 	ctlr->num_chipselect = 1;
 	ctlr->setup = mtk_nor_setup;
 	ctlr->transfer_one_message = mtk_nor_transfer_one_message;
+	ctlr->auto_runtime_pm = true;
 
 	dev_set_drvdata(&pdev->dev, ctlr);
 
@@ -712,12 +707,19 @@ static int mtk_nor_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ret = mtk_nor_enable_clk(sp);
+	if (ret < 0)
+		return ret;
+
+	sp->spi_freq = clk_get_rate(sp->spi_clk);
+
+	mtk_nor_init(sp);
+
 	irq = platform_get_irq_optional(pdev, 0);
+
 	if (irq < 0) {
 		dev_warn(sp->dev, "IRQ not available.");
 	} else {
-		writel(MTK_NOR_IRQ_MASK, base + MTK_NOR_REG_IRQ_STAT);
-		writel(0, base + MTK_NOR_REG_IRQ_EN);
 		ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
 				       pdev->name, sp);
 		if (ret < 0) {
@@ -728,34 +730,86 @@ static int mtk_nor_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = mtk_nor_init(sp);
-	if (ret < 0) {
-		kfree(ctlr);
-		return ret;
-	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+
+	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	if (ret < 0)
+		goto err_probe;
+
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	dev_info(&pdev->dev, "spi frequency: %d Hz\n", sp->spi_freq);
 
-	return devm_spi_register_controller(&pdev->dev, ctlr);
+	return 0;
+
+err_probe:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+
+	mtk_nor_disable_clk(sp);
+
+	return ret;
 }
 
 static int mtk_nor_remove(struct platform_device *pdev)
 {
-	struct spi_controller *ctlr;
-	struct mtk_nor *sp;
+	struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
+	struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
 
-	ctlr = dev_get_drvdata(&pdev->dev);
-	sp = spi_controller_get_devdata(ctlr);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+
+	mtk_nor_disable_clk(sp);
+
+	return 0;
+}
+
+static int __maybe_unused mtk_nor_runtime_suspend(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
 
 	mtk_nor_disable_clk(sp);
 
 	return 0;
 }
 
+static int __maybe_unused mtk_nor_runtime_resume(struct device *dev)
+{
+	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct mtk_nor *sp = spi_controller_get_devdata(ctlr);
+
+	return mtk_nor_enable_clk(sp);
+}
+
+static int __maybe_unused mtk_nor_suspend(struct device *dev)
+{
+	return pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused mtk_nor_resume(struct device *dev)
+{
+	return pm_runtime_force_resume(dev);
+}
+
+static const struct dev_pm_ops mtk_nor_pm_ops = {
+	SET_RUNTIME_PM_OPS(mtk_nor_runtime_suspend,
+			   mtk_nor_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(mtk_nor_suspend, mtk_nor_resume)
+};
+
 static struct platform_driver mtk_nor_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
 		.of_match_table = mtk_nor_match,
+		.pm = &mtk_nor_pm_ops,
 	},
 	.probe = mtk_nor_probe,
 	.remove = mtk_nor_remove,
-- 
2.28.0.681.g6f77f65b4e-goog