| Message ID | 20200520163053.24357-5-p.yadav@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: add xSPI Octal DTR support | expand |
On Wed, 20 May 2020 22:00:38 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called > the "command extension". There can be 3 types of extensions in xSPI: > repeat, invert, and hex. When the extension type is "repeat", the same > opcode is sent twice. When it is "invert", the second byte is the > inverse of the opcode. When it is "hex" an additional opcode byte based > is sent with the command whose value can be anything. > > So, make opcode a 16-bit value and add a 'nbytes', similar to how > multiple address widths are handled. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > include/linux/spi/spi-mem.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > index e3dcb956bf61..731bb64c6ba6 100644 > --- a/include/linux/spi/spi-mem.h > +++ b/include/linux/spi/spi-mem.h > @@ -69,6 +69,8 @@ enum spi_mem_data_dir { > > /** > * struct spi_mem_op - describes a SPI memory operation > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > + * sent MSB-first. > * @cmd.buswidth: number of IO lines used to transmit the command > * @cmd.opcode: operation opcode > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not > @@ -94,9 +96,10 @@ enum spi_mem_data_dir { > */ > struct spi_mem_op { > struct { > + u8 nbytes; > u8 buswidth; > u8 dtr : 1; > - u8 opcode; > + u16 opcode; > } cmd; > > struct { As mentioned in one of my previous review, you should patch the mxic driver before extending the opcode field: --->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;
On 21/05/20 08:22PM, Boris Brezillon wrote: > On Wed, 20 May 2020 22:00:38 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called > > the "command extension". There can be 3 types of extensions in xSPI: > > repeat, invert, and hex. When the extension type is "repeat", the same > > opcode is sent twice. When it is "invert", the second byte is the > > inverse of the opcode. When it is "hex" an additional opcode byte based > > is sent with the command whose value can be anything. > > > > So, make opcode a 16-bit value and add a 'nbytes', similar to how > > multiple address widths are handled. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > include/linux/spi/spi-mem.h | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > index e3dcb956bf61..731bb64c6ba6 100644 > > --- a/include/linux/spi/spi-mem.h > > +++ b/include/linux/spi/spi-mem.h > > @@ -69,6 +69,8 @@ enum spi_mem_data_dir { > > > > /** > > * struct spi_mem_op - describes a SPI memory operation > > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > > + * sent MSB-first. > > * @cmd.buswidth: number of IO lines used to transmit the command > > * @cmd.opcode: operation opcode > > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not > > @@ -94,9 +96,10 @@ enum spi_mem_data_dir { > > */ > > struct spi_mem_op { > > struct { > > + u8 nbytes; > > u8 buswidth; > > u8 dtr : 1; > > - u8 opcode; > > + u16 opcode; > > } cmd; > > > > struct { > > As mentioned in one of my previous review, you should patch the mxic > driver before extending the opcode field: IIUC, this patchset doesn't break original functionality of the driver. It will work like before with 1-byte opcodes. So I don't think it is the responsibility of this patchset to enhance the driver. It didn't work before with 2-byte opcodes, it won't work now. IMO this should be a separate, independent change. > --->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; >
On 22/05/20 01:11AM, Pratyush Yadav wrote: > On 21/05/20 08:22PM, Boris Brezillon wrote: > > On Wed, 20 May 2020 22:00:38 +0530 > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called > > > the "command extension". There can be 3 types of extensions in xSPI: > > > repeat, invert, and hex. When the extension type is "repeat", the same > > > opcode is sent twice. When it is "invert", the second byte is the > > > inverse of the opcode. When it is "hex" an additional opcode byte based > > > is sent with the command whose value can be anything. > > > > > > So, make opcode a 16-bit value and add a 'nbytes', similar to how > > > multiple address widths are handled. > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > --- > > > include/linux/spi/spi-mem.h | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > > index e3dcb956bf61..731bb64c6ba6 100644 > > > --- a/include/linux/spi/spi-mem.h > > > +++ b/include/linux/spi/spi-mem.h > > > @@ -69,6 +69,8 @@ enum spi_mem_data_dir { > > > > > > /** > > > * struct spi_mem_op - describes a SPI memory operation > > > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > > > + * sent MSB-first. > > > * @cmd.buswidth: number of IO lines used to transmit the command > > > * @cmd.opcode: operation opcode > > > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not > > > @@ -94,9 +96,10 @@ enum spi_mem_data_dir { > > > */ > > > struct spi_mem_op { > > > struct { > > > + u8 nbytes; > > > u8 buswidth; > > > u8 dtr : 1; > > > - u8 opcode; > > > + u16 opcode; > > > } cmd; > > > > > > struct { > > > > As mentioned in one of my previous review, you should patch the mxic > > driver before extending the opcode field: > > IIUC, this patchset doesn't break original functionality of the driver. > It will work like before with 1-byte opcodes. So I don't think it is the > responsibility of this patchset to enhance the driver. It didn't work > before with 2-byte opcodes, it won't work now. IMO this should be a > separate, independent change. Scratch that. Big/little endian issue. If you'd drop your Signed-off-by, I'll write the commit message and patch it in. > > --->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; > >
On Fri, 22 May 2020 01:33:15 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > On 22/05/20 01:11AM, Pratyush Yadav wrote: > > On 21/05/20 08:22PM, Boris Brezillon wrote: > > > On Wed, 20 May 2020 22:00:38 +0530 > > > Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > In xSPI mode, flashes expect 2-byte opcodes. The second byte is called > > > > the "command extension". There can be 3 types of extensions in xSPI: > > > > repeat, invert, and hex. When the extension type is "repeat", the same > > > > opcode is sent twice. When it is "invert", the second byte is the > > > > inverse of the opcode. When it is "hex" an additional opcode byte based > > > > is sent with the command whose value can be anything. > > > > > > > > So, make opcode a 16-bit value and add a 'nbytes', similar to how > > > > multiple address widths are handled. > > > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > include/linux/spi/spi-mem.h | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h > > > > index e3dcb956bf61..731bb64c6ba6 100644 > > > > --- a/include/linux/spi/spi-mem.h > > > > +++ b/include/linux/spi/spi-mem.h > > > > @@ -69,6 +69,8 @@ enum spi_mem_data_dir { > > > > > > > > /** > > > > * struct spi_mem_op - describes a SPI memory operation > > > > + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is > > > > + * sent MSB-first. > > > > * @cmd.buswidth: number of IO lines used to transmit the command > > > > * @cmd.opcode: operation opcode > > > > * @cmd.dtr: whether the command opcode should be sent in DTR mode or not > > > > @@ -94,9 +96,10 @@ enum spi_mem_data_dir { > > > > */ > > > > struct spi_mem_op { > > > > struct { > > > > + u8 nbytes; > > > > u8 buswidth; > > > > u8 dtr : 1; > > > > - u8 opcode; > > > > + u16 opcode; > > > > } cmd; > > > > > > > > struct { > > > > > > As mentioned in one of my previous review, you should patch the mxic > > > driver before extending the opcode field: > > > > IIUC, this patchset doesn't break original functionality of the driver. > > It will work like before with 1-byte opcodes. So I don't think it is the > > responsibility of this patchset to enhance the driver. It didn't work > > before with 2-byte opcodes, it won't work now. IMO this should be a > > separate, independent change. > > Scratch that. Big/little endian issue. If you'd drop your Signed-off-by, > I'll write the commit message and patch it in. Just add a Suggested-by, that should be fine. > > > > --->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; > > > >
Hi Boris, On 21/05/20 08:22PM, Boris Brezillon wrote: > On Wed, 20 May 2020 22:00:38 +0530 > Pratyush Yadav <p.yadav@ti.com> wrote: > > As mentioned in one of my previous review, you should patch the mxic > driver before extending the opcode field: > > --->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]; Regarding your comment about bisect-ability, how about I change this to: u8 cmd[sizeof(op->cmd.opcode)]; and put this patch before the change to 2-byte opcodes. This should also make it resistent to further changes in opcode size. Does that sound like a sane idea? > 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; >
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h index e3dcb956bf61..731bb64c6ba6 100644 --- a/include/linux/spi/spi-mem.h +++ b/include/linux/spi/spi-mem.h @@ -69,6 +69,8 @@ enum spi_mem_data_dir { /** * struct spi_mem_op - describes a SPI memory operation + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is + * sent MSB-first. * @cmd.buswidth: number of IO lines used to transmit the command * @cmd.opcode: operation opcode * @cmd.dtr: whether the command opcode should be sent in DTR mode or not @@ -94,9 +96,10 @@ enum spi_mem_data_dir { */ struct spi_mem_op { struct { + u8 nbytes; u8 buswidth; u8 dtr : 1; - u8 opcode; + u16 opcode; } cmd; struct {
In xSPI mode, flashes expect 2-byte opcodes. The second byte is called the "command extension". There can be 3 types of extensions in xSPI: repeat, invert, and hex. When the extension type is "repeat", the same opcode is sent twice. When it is "invert", the second byte is the inverse of the opcode. When it is "hex" an additional opcode byte based is sent with the command whose value can be anything. So, make opcode a 16-bit value and add a 'nbytes', similar to how multiple address widths are handled. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- include/linux/spi/spi-mem.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)