Re: [PATCH RFC 05/18] spi: spi-mem: mxic: Add support for DTR and Octo mode

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Boris,

Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote on Fri, 12 Oct 2018
10:48:12 +0200:

> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> ---
>  drivers/spi/spi-mxic.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index ce59ea2ecfe2..70e6bc9a099e 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -281,10 +281,11 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
>  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  				     const struct spi_mem_op *op)
>  {
> -	if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
> -	    op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
> +	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> +	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
>  		return false;
>  
> +

Extra space here

>  	if (op->data.nbytes && op->dummy.nbytes &&
>  	    op->data.buswidth != op->dummy.buswidth)
>  		return false;
> @@ -302,6 +303,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>  	int nio = 1, i, ret;
>  	u32 ss_ctrl;
>  	u8 addr[8];
> +	u8 cmd[2];
>  
>  	ret = mxic_spi_clk_setup(mem->spi);
>  	if (ret)
> @@ -311,6 +313,8 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>  	if (ret)
>  		return ret;
>  
> +	if (mem->spi->mode & (SPI_RX_OCTO | SPI_TX_OCTO))
> +		nio = 8;
>  	if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
>  		nio = 4;
>  	else if (mem->spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
> @@ -323,17 +327,21 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>  	       mxic->regs + HC_CFG);
>  	writel(HC_EN_BIT, mxic->regs + HC_EN);
>  
> -	ss_ctrl = OP_CMD_BYTES(1) | OP_CMD_BUSW(fls(op->cmd.buswidth) - 1);
> +	ss_ctrl = OP_CMD_BYTES(op->cmd.nbytes) |
> +		  OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
> +		  (op->cmd.dtr ? OP_CMD_DDR : 0);
>  
>  	if (op->addr.nbytes)
>  		ss_ctrl |= OP_ADDR_BYTES(op->addr.nbytes) |
> -			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1);
> +			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
> +			   (op->addr.dtr ? OP_ADDR_DDR : 0);
>  
>  	if (op->dummy.nbytes)
>  		ss_ctrl |= OP_DUMMY_CYC(op->dummy.nbytes);
>  
>  	if (op->data.nbytes) {
> -		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1);
> +		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
> +			   (op->data.dtr ? OP_DATA_DDR : 0);
>  		if (op->data.dir == SPI_MEM_DATA_IN)
>  			ss_ctrl |= OP_READ;
>  	}
> @@ -343,7 +351,14 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>  	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
>  	       mxic->regs + HC_CFG);
>  
> -	ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> +	if (op->cmd.nbytes == 2) {
> +		cmd[0] = op->cmd.opcode >> 8;
> +		cmd[1] = op->cmd.opcode;
> +	} else {
> +		cmd[0] = op->cmd.opcode;
> +	}

I haven't played with this code yet and maybe I'll regret this but
wouldn't be easier for developers to have this in patch 4:

 struct spi_mem_op {
 	struct {
+		u8 nbytes;
 		u8 buswidth;
 		bool dtr;
-		u8 opcode;
+		u8 opcode[2]; /* <- an array of opcodes instead of an u16? */
 	} cmd;

This way I think we would avoid endianness considerations and reading
would be eased.

> +
> +	ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
>  	if (ret)
>  		goto out;
>  


Thanks,
Miquèl