Re: [PATCH v3 3/6] spi: spi-mtk-nor: support 7 bytes transfer of generic spi
From: Chuanhong Guo
Date: Fri Sep 25 2020 - 03:47:45 EST
Hi!
On Fri, Sep 25, 2020 at 2:55 PM Ikjoon Jang <ikjn@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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