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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Sun, 18 Nov 2018 18:21:11 +0100
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> 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? */

Just wanted to stay consistent with what we have for the address
cycles, but maybe I should clarify how the opcode should be
transmitted on the wire (MSB first).

>  	} 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