Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Tue, 5 May 2020 11:44:43 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Tue, 5 May 2020 17:31:45 +0800
> masonccyang@xxxxxxxxxxx wrote:
> 
> 
> > > > > > I quickly went through your patches but can't reply them in each     
> > your   
> > > > > > patches.
> > > > > > 
> > > > > > i.e,.
> > > > > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > > > > > 
> > > > > > -                                u8 opcode;
> > > > > > +                                u16 opcode;
> > > > > > 
> > > > > > big/little Endian issue, right? 
> > > > > > why not just u8 ext_opcode;
> > > > > > No any impact for exist code and actually only xSPI device use     
> > > > extension     
> > > > > > command.    
> > > > > 
> > > > > Boris already explained the reasoning behind it.    
> > > > 
> > > > yup, I got his point and please make sure CPU data access.
> > > > 
> > > > i.e,.
> > > > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > > > 
> > > > and your patch,
> > > > +                                ext = spi_nor_get_cmd_ext(nor, op);
> > > > +                                op->cmd.opcode = (op->cmd.opcode <<     
> > 8) |   
> > > > ext;
> > > > +                                op->cmd.nbytes = 2;
> > > > 
> > > > I think maybe using u8 opcode[2] could avoid endianness.    
> > > 
> > > Again, thanks Boris for answering this. FWIW, I don't see anything wrong     
> >   
> > > with his suggestion.
> > > 
> > > To clarify a bit more, the idea is that we transmit the opcode MSB 
> > > first, just we do for the address. Assume we want to issue the command 
> > > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat     
> >   
> > > is as a 1-byte value, so the MSB is the same as the LSB. We directly 
> > > send 0x5 on the bus.    
> > 
> > There are many SPI controllers driver use "op->cmd.opcode" directly,
> > so is spi-mxic.c.
> > 
> > i.e,.
> > ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);  
> 
> Just because you do it doesn't mean it's right. And most controllers use
> the opcode value, they don't dereference the pointer as you do here.
> 
> >   
> > > 
> > > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension 
> > > is invert of command). So we send the MSB (0x05) first, and LSB (0xFA) 
> > > next.    
> > 
> > My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> > 
> > In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> > in 8D-8D-8D mode, I need to patch
> > 
> > i.e.,
> > op->cmd.opcode = op->cmd.opcode | (ext << 8);
> > 
> > rather than your patch.  
> 
> Seriously, how hard is it to extract each byte from the u16 if your
> controller needs to pass things in a different order? I mean, that's
> already how it's done for the address cycle, so why is it a problem
> here? This sounds like bikeshedding to me. If the order is properly
> documented in the kernel doc, I see no problem having it grouped in one
> u16, with the first cmd cycle placed in the MSB and the second one in
> the LSB.

So, I gave it a try, and we're talking about something as simple as the
below diff. And yes, the mxic controller needs to be patched before
extending the cmd.opcode field, but we're talking about one driver here
(all other drivers should be fine). Oh, and if you look a few lines below
the changed lines, you'll notice that we do exactly the same for the
address.

--->8---
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 69491f3a515d..c3f4136a7c1d 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -356,6 +356,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_set_freq(mxic, mem->spi->max_speed_hz);
        if (ret)
@@ -393,7 +394,10 @@ 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);
+       for (i = 0; i < op->cmd.nbytes; i++)
+               cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
+
+       ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
        if (ret)
                goto out;