<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
<div style="font-family: inherit; font-size: inherit; color: rgb(0, 0, 0);"></div>
<div style="font-family: inherit; font-size: inherit; color: rgb(0, 0, 0);">You dumb fuck stay in your nasty as disease infested poverty filled country </div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> linux-mtd <linux-mtd-bounces@lists.infradead.org> on behalf of Sergiu.Moga@microchip.com <Sergiu.Moga@microchip.com><br>
<b>Sent:</b> Monday, September 26, 2022 5:05 AM<br>
<b>To:</b> fancer.lancer@gmail.com <fancer.lancer@gmail.com><br>
<b>Cc:</b> Tudor.Ambarus@microchip.com <Tudor.Ambarus@microchip.com>; pratyush@kernel.org <pratyush@kernel.org>; michael@walle.cc <michael@walle.cc>; miquel.raynal@bootlin.com <miquel.raynal@bootlin.com>; richard@nod.at <richard@nod.at>; vigneshr@ti.com <vigneshr@ti.com>;
broonie@kernel.org <broonie@kernel.org>; Nicolas.Ferre@microchip.com <Nicolas.Ferre@microchip.com>; alexandre.belloni@bootlin.com <alexandre.belloni@bootlin.com>; Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>; chin-ting_kuo@aspeedtech.com <chin-ting_kuo@aspeedtech.com>;
clg@kaod.org <clg@kaod.org>; joel@jms.id.au <joel@jms.id.au>; andrew@aj.id.au <andrew@aj.id.au>; kdasu.kdev@gmail.com <kdasu.kdev@gmail.com>; han.xu@nxp.com <han.xu@nxp.com>; john.garry@huawei.com <john.garry@huawei.com>; matthias.bgg@gmail.com <matthias.bgg@gmail.com>;
avifishman70@gmail.com <avifishman70@gmail.com>; tmaimon77@gmail.com <tmaimon77@gmail.com>; tali.perry1@gmail.com <tali.perry1@gmail.com>; venture@google.com <venture@google.com>; yuenn@google.com <yuenn@google.com>; benjaminfair@google.com <benjaminfair@google.com>;
haibo.chen@nxp.com <haibo.chen@nxp.com>; yogeshgaur.83@gmail.com <yogeshgaur.83@gmail.com>; heiko@sntech.de <heiko@sntech.de>; mcoquelin.stm32@gmail.com <mcoquelin.stm32@gmail.com>; alexandre.torgue@foss.st.com <alexandre.torgue@foss.st.com>; michal.simek@xilinx.com
<michal.simek@xilinx.com>; bcm-kernel-feedback-list@broadcom.com <bcm-kernel-feedback-list@broadcom.com>; linux-mtd@lists.infradead.org <linux-mtd@lists.infradead.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-spi@vger.kernel.org
<linux-spi@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>; openbmc@lists.ozlabs.org <openbmc@lists.ozlabs.org>; linux-mediatek@lists.infradead.org
<linux-mediatek@lists.infradead.org>; linux-rockchip@lists.infradead.org <linux-rockchip@lists.infradead.org>; linux-stm32@st-md-mailman.stormreply.com <linux-stm32@st-md-mailman.stormreply.com><br>
<b>Subject:</b> Re: [PATCH] spi: Replace `dummy.nbytes` with `dummy.ncycles`</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On 26.09.2022 01:03, Serge Semin wrote:<br>
> Hello Sergiu<br>
> <br>
<br>
<br>
Hello Serge,<br>
<br>
<br>
> On Sun, Sep 11, 2022 at 08:45:53PM +0300, Sergiu Moga wrote:<br>
>> In order to properly represent the hardware functionality<br>
>> in the core, avoid reconverting the number of dummy cycles<br>
>> to the number of bytes and only work with the former.<br>
>> Instead, let the drivers that do need this conversion do<br>
>> it themselves.<br>
>><br>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com><br>
>> ---<br>
>> drivers/mtd/spi-nor/core.c | 22 ++++----------<br>
> <br>
> [...]<br>
> <br>
>> drivers/spi/spi-dw-core.c | 10 +++++--<br>
> <br>
> [...]<br>
> <br>
>> drivers/spi/spi-mem.c | 27 +++++++++++------<br>
> <br>
> [...]<br>
> <br>
>> drivers/spi/spi-mtk-nor.c | 48 +++++++++++++++++--------------<br>
> <br>
> [...]<br>
> <br>
>> drivers/spi/spi-zynq-qspi.c | 15 ++++++----<br>
>> drivers/spi/spi-zynqmp-gqspi.c | 8 ++++--<br>
>> include/linux/spi/spi-mem.h | 10 +++----<br>
>> 25 files changed, 234 insertions(+), 147 deletions(-)<br>
>><br>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c<br>
>> index f2c64006f8d7..cc8ca824f912 100644<br>
>> --- a/drivers/mtd/spi-nor/core.c<br>
>> +++ b/drivers/mtd/spi-nor/core.c<br>
>> @@ -88,7 +88,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,<br>
>> if (op->addr.nbytes)<br>
>> op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);<br>
>><br>
> <br>
> <br>
> <br>
>> - if (op->dummy.nbytes)<br>
>> + if (op->dummy.ncycles)<br>
>> op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);<br>
>><br>
>> if (op->data.nbytes)<br>
>> @@ -106,9 +106,6 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,<br>
>> op->dummy.dtr = true;<br>
>> op->data.dtr = true;<br>
>><br>
>> - /* 2 bytes per clock cycle in DTR mode. */<br>
>> - op->dummy.nbytes *= 2;<br>
>> -<br>
>> ext = spi_nor_get_cmd_ext(nor, op);<br>
>> op->cmd.opcode = (op->cmd.opcode << 8) | ext;<br>
>> op->cmd.nbytes = 2;<br>
>> @@ -207,10 +204,7 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,<br>
>><br>
>> spi_nor_spimem_setup_op(nor, &op, nor->read_proto);<br>
>><br>
>> - /* convert the dummy cycles to the number of bytes */<br>
>> - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;<br>
>> - if (spi_nor_protocol_is_dtr(nor->read_proto))<br>
>> - op.dummy.nbytes *= 2;<br>
>> + op.dummy.ncycles = nor->read_dummy;<br>
> <br>
> So according to this modification and what is done in the rest of the<br>
> patch, the dummy part of the SPI-mem operations now contains the number<br>
> of cycles only. Am I right to think that it means a number of dummy<br>
> clock oscillations? (Judging from what I've seen in the HW-manuals of<br>
> the SPI NOR memory devices most likely I am...)<br>
<br>
<br>
<br>
Yes, you are correct.<br>
<br>
<br>
> If so the "ncycles" field<br>
> is now free from the "data" semantic. Then what is the meaning of the<br>
> "buswidth and "dtr" fields in the spi_mem_op.dummy field?<br>
> <br>
<br>
<br>
It is still meaningful as it is used for the conversion by some drivers <br>
to nbytes and I do not see how it goes out of the specification in any <br>
way. So, at least for now, I do not see any reason to remove these fields.<br>
<br>
<br>
>><br>
>> usebouncebuf = spi_nor_spimem_bounce(nor, &op);<br>
>><br>
>> @@ -455,7 +449,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)<br>
>><br>
>> if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {<br>
>> op.addr.nbytes = nor->params->rdsr_addr_nbytes;<br>
>> - op.dummy.nbytes = nor->params->rdsr_dummy;<br>
>> + op.dummy.ncycles = nor->params->rdsr_dummy;<br>
>> /*<br>
>> * We don't want to read only one byte in DTR mode. So,<br>
>> * read 2 and then discard the second byte.<br>
>> @@ -1913,10 +1907,7 @@ static int spi_nor_spimem_check_readop(struct spi_nor *nor,<br>
>><br>
>> spi_nor_spimem_setup_op(nor, &op, read->proto);<br>
>><br>
>> - /* convert the dummy cycles to the number of bytes */<br>
>> - op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;<br>
>> - if (spi_nor_protocol_is_dtr(nor->read_proto))<br>
>> - op.dummy.nbytes *= 2;<br>
>> + op.dummy.ncycles = nor->read_dummy;<br>
>><br>
>> return spi_nor_spimem_check_op(nor, &op);<br>
>> }<br>
>> @@ -3034,10 +3025,7 @@ static int spi_nor_create_read_dirmap(struct spi_nor *nor)<br>
>><br>
>> spi_nor_spimem_setup_op(nor, op, nor->read_proto);<br>
>><br>
>> - /* convert the dummy cycles to the number of bytes */<br>
>> - op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;<br>
>> - if (spi_nor_protocol_is_dtr(nor->read_proto))<br>
>> - op->dummy.nbytes *= 2;<br>
>> + op->dummy.ncycles = nor->read_dummy;<br>
>><br>
>> /*<br>
>> * Since spi_nor_spimem_setup_op() only sets buswidth when the number<br>
> <br>
> [...]<br>
> <br>
>> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c<br>
>> index f87d97ccd2d6..0ba5c7d0e66e 100644<br>
>> --- a/drivers/spi/spi-dw-core.c<br>
>> +++ b/drivers/spi/spi-dw-core.c<br>
>> @@ -498,13 +498,17 @@ static bool dw_spi_supports_mem_op(struct spi_mem *mem,<br>
>> static int dw_spi_init_mem_buf(struct dw_spi *dws, const struct spi_mem_op *op)<br>
>> {<br>
>> unsigned int i, j, len;<br>
>> - u8 *out;<br>
>> + u8 *out, dummy_nbytes;<br>
>><br>
>> /*<br>
>> * Calculate the total length of the EEPROM command transfer and<br>
>> * either use the pre-allocated buffer or create a temporary one.<br>
>> */<br>
>> - len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;<br>
> <br>
>> + dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
> <br>
> 1. What about using the BITS_PER_BYTE macro (linux/bits.h) here? Since<br>
> you are adding a similar modification to so many drivers what about using<br>
> that macro there too?<br>
> <br>
<br>
<br>
AFAICT BIT_PER_BYTE is meant to transparently indicate how many bits per <br>
byte an arch has. Although, there is no place in the kernel from what I <br>
can see that has BITS_PER_BYTE with a value other than 8, you cannot <br>
deny that there exist architectures whose number of bits per byte may be <br>
different from 8.<br>
<br>
Meanwhile, the JESD216E specification tells us in the Terms and <br>
definitions chapter that<br>
"DWORD: Four consecutive 8-bit bytes used as the basic 32-bit building <br>
block for headers and parameter tables." So it explicitly says that a <br>
byte has 8 bits regardless of the arch.<br>
<br>
Therefore, I do not agree with replacing 8 with the BITS_PER_BYTE macro <br>
as, IMO, it does not represent the same thing as the number of bits per <br>
byte that the terms and definitions of the JESD216E specification refer to.<br>
<br>
<br>
> 2. buswidth is supposed to be always 1 in this driver (see the<br>
> dw_spi_supports_mem_op() method). So it can be dropped from the<br>
> statement above.<br>
> <br>
> 3. Since the ncycles now contains a number of clock cycles there is no<br>
> point in taking the SPI bus-width into account at all. What is<br>
> meaningful is how many oscillations are supposed to be placed on the<br>
> CLK line before the data is available. So the op->dummy.ncycles /<br>
> BITS_PER_BYTE statement would be more appropriate here in any case.<br>
> <br>
<br>
<br>
I can agee with this in the case of this driver, sure.<br>
<br>
<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> DTR is unsupported by the controller. See, no spi_controller_mem_caps<br>
> initialized. So this part is redundant. The same is most likely<br>
> applicable for some of the DTR-related updates in this patch too<br>
> since the spi_controller_mem_caps structure is initialized in a few<br>
> drivers only.<br>
> <br>
<br>
<br>
Agreed. Initially, wherever I was not sure, I just placed this if <br>
condition to avoid breaking anything in case the driver does support <br>
DTR. The same goes for your other related observations to other driver <br>
modifications, with which I agree :).<br>
<br>
<br>
>> +<br>
>> + len = op->cmd.nbytes + op->addr.nbytes + dummy_nbytes;<br>
>> if (op->data.dir == SPI_MEM_DATA_OUT)<br>
>> len += op->data.nbytes;<br>
>><br>
>> @@ -525,7 +529,7 @@ static int dw_spi_init_mem_buf(struct dw_spi *dws, const struct spi_mem_op *op)<br>
>> out[i] = DW_SPI_GET_BYTE(op->cmd.opcode, op->cmd.nbytes - i - 1);<br>
>> for (j = 0; j < op->addr.nbytes; ++i, ++j)<br>
>> out[i] = DW_SPI_GET_BYTE(op->addr.val, op->addr.nbytes - j - 1);<br>
>> - for (j = 0; j < op->dummy.nbytes; ++i, ++j)<br>
>> + for (j = 0; j < dummy_nbytes; ++i, ++j)<br>
>> out[i] = 0x0;<br>
>><br>
>> if (op->data.dir == SPI_MEM_DATA_OUT)<br>
> <br>
> [...]<br>
> <br>
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c<br>
>> index 0c79193d9697..7b204963bb62 100644<br>
>> --- a/drivers/spi/spi-mem.c<br>
>> +++ b/drivers/spi/spi-mem.c<br>
>> @@ -149,7 +149,7 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,<br>
>> spi_check_buswidth_req(mem, op->addr.buswidth, true))<br>
>> return false;<br>
>><br>
>> - if (op->dummy.nbytes &&<br>
>> + if (op->dummy.ncycles &&<br>
>> spi_check_buswidth_req(mem, op->dummy.buswidth, true))<br>
>> return false;<br>
>><br>
>> @@ -202,7 +202,7 @@ static int spi_mem_check_op(const struct spi_mem_op *op)<br>
>> return -EINVAL;<br>
>><br>
>> if ((op->addr.nbytes && !op->addr.buswidth) ||<br>
>> - (op->dummy.nbytes && !op->dummy.buswidth) ||<br>
>> + (op->dummy.ncycles && !op->dummy.buswidth) ||<br>
>> (op->data.nbytes && !op->data.buswidth))<br>
>> return -EINVAL;<br>
>><br>
>> @@ -315,7 +315,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)<br>
> <br>
>> struct spi_controller *ctlr = mem->spi->controller;<br>
>> struct spi_transfer xfers[4] = { };<br>
>> struct spi_message msg;<br>
>> - u8 *tmpbuf;<br>
>> + u8 *tmpbuf, dummy_nbytes;<br>
>> int ret;<br>
> <br>
> Reverse xmas tree order?<br>
> <br>
>><br>
>> ret = spi_mem_check_op(op);<br>
>> @@ -343,7 +343,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)<br>
>> return ret;<br>
>> }<br>
>><br>
> <br>
>> - tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;<br>
>> + dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
> <br>
> So ncycles now indeed is a number of CLK line oscillations. This most<br>
> likely will break the SPI Nand driver then, which still passes the<br>
> number of bytes to the SPI_MEM_OP_DUMMY() macro.<br>
> <br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> Generic SPI-mem ops don't take the DTR mode into account. So I don't<br>
> see this necessary.<br>
> <br>
<br>
<br>
You may be right, but this part of the code does take into consideration <br>
the number of dummy.nbytes to calculate the xfer length. Therefore, <br>
shouldn't this code block also know if the number of dummy nbytes is <br>
actually double the amount that it calculated through the conversion <br>
formula?<br>
<br>
<br>
>> +<br>
>> + tmpbufsize = op->cmd.nbytes + op->addr.nbytes + dummy_nbytes;<br>
>><br>
>> /*<br>
>> * Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so<br>
>> @@ -379,15 +383,15 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)<br>
>> totalxferlen += op->addr.nbytes;<br>
>> }<br>
>><br>
>> - if (op->dummy.nbytes) {<br>
>> - memset(tmpbuf + op->addr.nbytes + 1, 0xff, op->dummy.nbytes);<br>
>> + if (dummy_nbytes) {<br>
>> + memset(tmpbuf + op->addr.nbytes + 1, 0xff, dummy_nbytes);<br>
>> xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;<br>
>> - xfers[xferpos].len = op->dummy.nbytes;<br>
>> + xfers[xferpos].len = dummy_nbytes;<br>
>> xfers[xferpos].tx_nbits = op->dummy.buswidth;<br>
>> xfers[xferpos].dummy_data = 1;<br>
>> spi_message_add_tail(&xfers[xferpos], &msg);<br>
>> xferpos++;<br>
>> - totalxferlen += op->dummy.nbytes;<br>
>> + totalxferlen += dummy_nbytes;<br>
>> }<br>
>><br>
>> if (op->data.nbytes) {<br>
>> @@ -456,12 +460,17 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)<br>
>> {<br>
> <br>
>> struct spi_controller *ctlr = mem->spi->controller;<br>
>> size_t len;<br>
>> + u8 dummy_nbytes;<br>
> <br>
> reverse xmas tree?<br>
> <br>
>><br>
>> if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)<br>
>> return ctlr->mem_ops->adjust_op_size(mem, op);<br>
>><br>
>> + dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
>> +<br>
>> if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {<br>
>> - len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;<br>
>> + len = op->cmd.nbytes + op->addr.nbytes + dummy_nbytes;<br>
>><br>
>> if (len > spi_max_transfer_size(mem->spi))<br>
>> return -EINVAL;<br>
> <br>
> [...]<br>
> <br>
>> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c<br>
>> index d167699a1a96..f6870c6e911a 100644<br>
>> --- a/drivers/spi/spi-mtk-nor.c<br>
>> +++ b/drivers/spi/spi-mtk-nor.c<br>
>> @@ -171,23 +171,18 @@ static bool need_bounce(struct mtk_nor *sp, const struct spi_mem_op *op)<br>
>><br>
>> static bool mtk_nor_match_read(const struct spi_mem_op *op)<br>
>> {<br>
>> - int dummy = 0;<br>
>> -<br>
>> - if (op->dummy.nbytes)<br>
>> - dummy = op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth;<br>
>> -<br>
>> if ((op->data.buswidth == 2) || (op->data.buswidth == 4)) {<br>
>> if (op->addr.buswidth == 1)<br>
>> - return dummy == 8;<br>
>> + return op->dummy.ncycles == 8;<br>
>> else if (op->addr.buswidth == 2)<br>
>> - return dummy == 4;<br>
>> + return op->dummy.ncycles == 4;<br>
>> else if (op->addr.buswidth == 4)<br>
>> - return dummy == 6;<br>
>> + return op->dummy.ncycles == 6;<br>
>> } else if ((op->addr.buswidth == 1) && (op->data.buswidth == 1)) {<br>
>> if (op->cmd.opcode == 0x03)<br>
>> - return dummy == 0;<br>
>> + return op->dummy.ncycles == 0;<br>
>> else if (op->cmd.opcode == 0x0b)<br>
>> - return dummy == 8;<br>
>> + return op->dummy.ncycles == 8;<br>
>> }<br>
>> return false;<br>
>> }<br>
>> @@ -195,6 +190,10 @@ static bool mtk_nor_match_read(const struct spi_mem_op *op)<br>
>> static bool mtk_nor_match_prg(const struct spi_mem_op *op)<br>
>> {<br>
>> int tx_len, rx_len, prg_len, prg_left;<br>
> <br>
>> + u8 dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
>> +<br>
> <br>
> IMO it's better to move the initialization statement to a separate<br>
> line here.<br>
> <br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> Does the MTK SPI driver support DTR? AFAICS it doesn't.<br>
> <br>
>><br>
>> // prg mode is spi-only.<br>
>> if ((op->cmd.buswidth > 1) || (op->addr.buswidth > 1) ||<br>
>> @@ -205,7 +204,7 @@ static bool mtk_nor_match_prg(const struct spi_mem_op *op)<br>
>><br>
>> if (op->data.dir == SPI_MEM_DATA_OUT) {<br>
>> // count dummy bytes only if we need to write data after it<br>
>> - tx_len += op->dummy.nbytes;<br>
>> + tx_len += dummy_nbytes;<br>
>><br>
>> // leave at least one byte for data<br>
>> if (tx_len > MTK_NOR_REG_PRGDATA_MAX)<br>
>> @@ -221,7 +220,7 @@ static bool mtk_nor_match_prg(const struct spi_mem_op *op)<br>
>> return false;<br>
>><br>
>> rx_len = op->data.nbytes;<br>
>> - prg_left = MTK_NOR_PRG_CNT_MAX / 8 - tx_len - op->dummy.nbytes;<br>
>> + prg_left = MTK_NOR_PRG_CNT_MAX / 8 - tx_len - dummy_nbytes;<br>
>> if (prg_left > MTK_NOR_REG_SHIFT_MAX + 1)<br>
>> prg_left = MTK_NOR_REG_SHIFT_MAX + 1;<br>
>> if (rx_len > prg_left) {<br>
>> @@ -230,11 +229,11 @@ static bool mtk_nor_match_prg(const struct spi_mem_op *op)<br>
>> rx_len = prg_left;<br>
>> }<br>
>><br>
>> - prg_len = tx_len + op->dummy.nbytes + rx_len;<br>
>> + prg_len = tx_len + dummy_nbytes + rx_len;<br>
>> if (prg_len > MTK_NOR_PRG_CNT_MAX / 8)<br>
>> return false;<br>
>> } else {<br>
>> - prg_len = tx_len + op->dummy.nbytes;<br>
>> + prg_len = tx_len + dummy_nbytes;<br>
>> if (prg_len > MTK_NOR_PRG_CNT_MAX / 8)<br>
>> return false;<br>
>> }<br>
>> @@ -244,15 +243,19 @@ static bool mtk_nor_match_prg(const struct spi_mem_op *op)<br>
>> static void mtk_nor_adj_prg_size(struct spi_mem_op *op)<br>
>> {<br>
>> int tx_len, tx_left, prg_left;<br>
> <br>
>> + u8 dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
>> +<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> ditto<br>
> <br>
>><br>
>> tx_len = op->cmd.nbytes + op->addr.nbytes;<br>
>> if (op->data.dir == SPI_MEM_DATA_OUT) {<br>
>> - tx_len += op->dummy.nbytes;<br>
>> + tx_len += dummy_nbytes;<br>
>> tx_left = MTK_NOR_REG_PRGDATA_MAX + 1 - tx_len;<br>
>> if (op->data.nbytes > tx_left)<br>
>> op->data.nbytes = tx_left;<br>
>> } else if (op->data.dir == SPI_MEM_DATA_IN) {<br>
>> - prg_left = MTK_NOR_PRG_CNT_MAX / 8 - tx_len - op->dummy.nbytes;<br>
>> + prg_left = MTK_NOR_PRG_CNT_MAX / 8 - tx_len - dummy_nbytes;<br>
>> if (prg_left > MTK_NOR_REG_SHIFT_MAX + 1)<br>
>> prg_left = MTK_NOR_REG_SHIFT_MAX + 1;<br>
>> if (op->data.nbytes > prg_left)<br>
>> @@ -312,7 +315,7 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,<br>
>> break;<br>
>> case SPI_MEM_DATA_OUT:<br>
>> if ((op->addr.buswidth == 1) &&<br>
>> - (op->dummy.nbytes == 0) &&<br>
>> + (op->dummy.ncycles == 0) &&<br>
>> (op->data.buswidth == 1))<br>
>> return true;<br>
>> break;<br>
>> @@ -515,17 +518,20 @@ static int mtk_nor_spi_mem_prg(struct mtk_nor *sp, const struct spi_mem_op *op)<br>
>> int tx_len, prg_len;<br>
>> int i, ret;<br>
>> void __iomem *reg;<br>
> <br>
>> - u8 bufbyte;<br>
>> + u8 bufbyte, dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
>> +<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> ditto<br>
> <br>
>><br>
>> tx_len = op->cmd.nbytes + op->addr.nbytes;<br>
>><br>
>> // count dummy bytes only if we need to write data after it<br>
>> if (op->data.dir == SPI_MEM_DATA_OUT)<br>
>> - tx_len += op->dummy.nbytes + op->data.nbytes;<br>
>> + tx_len += dummy_nbytes + op->data.nbytes;<br>
>> else if (op->data.dir == SPI_MEM_DATA_IN)<br>
>> rx_len = op->data.nbytes;<br>
>><br>
>> - prg_len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes +<br>
>> + prg_len = op->cmd.nbytes + op->addr.nbytes + dummy_nbytes +<br>
>> op->data.nbytes;<br>
>><br>
>> // an invalid op may reach here if the caller calls exec_op without<br>
>> @@ -550,7 +556,7 @@ static int mtk_nor_spi_mem_prg(struct mtk_nor *sp, const struct spi_mem_op *op)<br>
>> }<br>
>><br>
>> if (op->data.dir == SPI_MEM_DATA_OUT) {<br>
>> - for (i = 0; i < op->dummy.nbytes; i++, reg_offset--) {<br>
>> + for (i = 0; i < dummy_nbytes; i++, reg_offset--) {<br>
>> reg = sp->base + MTK_NOR_REG_PRGDATA(reg_offset);<br>
>> writeb(0, reg);<br>
>> }<br>
> <br>
> [...]<br>
> <br>
>> diff --git a/drivers/spi/spi-zynq-qspi.c b/drivers/spi/spi-zynq-qspi.c<br>
>> index 78f31b61a2aa..84b7db85548c 100644<br>
>> --- a/drivers/spi/spi-zynq-qspi.c<br>
>> +++ b/drivers/spi/spi-zynq-qspi.c<br>
>> @@ -527,7 +527,10 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem,<br>
>> {<br>
>> struct zynq_qspi *xqspi = spi_controller_get_devdata(mem->spi->master);<br>
>> int err = 0, i;<br>
>> - u8 *tmpbuf;<br>
>> + u8 *tmpbuf, dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
> <br>
> Separate line?<br>
> <br>
>> +<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> Is DTR supported by the driver?<br>
> <br>
<br>
<br>
Not from what I can see, but I was not 100% sure so I placed this if <br>
statement here just in case.<br>
<br>
<br>
<br>
>><br>
>> dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n",<br>
>> op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,<br>
>> @@ -568,17 +571,17 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem,<br>
>> err = -ETIMEDOUT;<br>
>> }<br>
>><br>
>> - if (op->dummy.nbytes) {<br>
>> - tmpbuf = kzalloc(op->dummy.nbytes, GFP_KERNEL);<br>
>> + if (dummy_nbytes) {<br>
>> + tmpbuf = kzalloc(dummy_nbytes, GFP_KERNEL);<br>
>> if (!tmpbuf)<br>
>> return -ENOMEM;<br>
>><br>
>> - memset(tmpbuf, 0xff, op->dummy.nbytes);<br>
>> + memset(tmpbuf, 0xff, dummy_nbytes);<br>
>> reinit_completion(&xqspi->data_completion);<br>
>> xqspi->txbuf = tmpbuf;<br>
>> xqspi->rxbuf = NULL;<br>
>> - xqspi->tx_bytes = op->dummy.nbytes;<br>
>> - xqspi->rx_bytes = op->dummy.nbytes;<br>
>> + xqspi->tx_bytes = dummy_nbytes;<br>
>> + xqspi->rx_bytes = dummy_nbytes;<br>
>> zynq_qspi_write_op(xqspi, ZYNQ_QSPI_FIFO_DEPTH, true);<br>
>> zynq_qspi_write(xqspi, ZYNQ_QSPI_IEN_OFFSET,<br>
>> ZYNQ_QSPI_IXR_RXTX_MASK);<br>
>> diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c<br>
>> index c760aac070e5..b41abadef9a6 100644<br>
>> --- a/drivers/spi/spi-zynqmp-gqspi.c<br>
>> +++ b/drivers/spi/spi-zynqmp-gqspi.c<br>
>> @@ -948,6 +948,10 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,<br>
>> u32 genfifoentry = 0;<br>
>> u16 opcode = op->cmd.opcode;<br>
>> u64 opaddr;<br>
> <br>
>> + u8 dummy_nbytes = (op->dummy.ncycles * op->dummy.buswidth) / 8;<br>
>> +<br>
>> + if (op->dummy.dtr)<br>
>> + dummy_nbytes *= 2;<br>
> <br>
> ditto<br>
> <br>
>><br>
>> dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n",<br>
>> op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,<br>
>> @@ -1006,14 +1010,14 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem,<br>
>> }<br>
>> }<br>
>><br>
>> - if (op->dummy.nbytes) {<br>
>> + if (dummy_nbytes) {<br>
>> xqspi->txbuf = NULL;<br>
>> xqspi->rxbuf = NULL;<br>
>> /*<br>
>> * xqspi->bytes_to_transfer here represents the dummy circles<br>
>> * which need to be sent.<br>
>> */<br>
>> - xqspi->bytes_to_transfer = op->dummy.nbytes * 8 / op->dummy.buswidth;<br>
>> + xqspi->bytes_to_transfer = dummy_nbytes;<br>
>> xqspi->bytes_to_receive = 0;<br>
>> /*<br>
>> * Using op->data.buswidth instead of op->dummy.buswidth here because<br>
>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h<br>
>> index 2ba044d0d5e5..5fd45800af03 100644<br>
>> --- a/include/linux/spi/spi-mem.h<br>
>> +++ b/include/linux/spi/spi-mem.h<br>
>> @@ -29,9 +29,9 @@<br>
>><br>
>> #define SPI_MEM_OP_NO_ADDR { }<br>
>><br>
> <br>
>> -#define SPI_MEM_OP_DUMMY(__nbytes, __buswidth) \<br>
>> +#define SPI_MEM_OP_DUMMY(__ncycles, __buswidth) \<br>
>> { \<br>
> <br>
>> - .nbytes = __nbytes, \<br>
>> + .ncycles = __ncycles, \<br>
>> .buswidth = __buswidth, \<br>
> <br>
> Please make sure this update and the drivers/spi/spi-mem.c driver<br>
> alterations are coherent with the SPI Nand driver. See the macro usages:<br>
> include/linux/mtd/spinand.h: SPINAND_PAGE_READ_FROM_*().<br>
> <br>
> -Sergey<br>
><br>
<br>
<br>
Yes, indeed, I should have paid more attention here. As I have <br>
previously said, I simply replaced dummy.nbytes with the code sequences <br>
you now see. I should have checked for SPI_MEM_OP_DUMMY usages as well <br>
since I changed its definition. Thank you! :)<br>
<br>
<br>
>> }<br>
>><br>
>> @@ -83,8 +83,8 @@ enum spi_mem_data_dir {<br>
>> * Note that only @addr.nbytes are taken into account in this<br>
>> * address value, so users should make sure the value fits in the<br>
>> * assigned number of bytes.<br>
>> - * @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can<br>
>> - * be zero if the operation does not require dummy bytes<br>
>> + * @dummy.ncycles: number of dummy cycles after an opcode or address. Can<br>
>> + * be zero if the operation does not require dummy cycles<br>
>> * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes<br>
>> * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not<br>
>> * @data.buswidth: number of IO lanes used to send/receive the data<br>
>> @@ -112,7 +112,7 @@ struct spi_mem_op {<br>
>> } addr;<br>
>><br>
>> struct {<br>
>> - u8 nbytes;<br>
>> + u8 ncycles;<br>
>> u8 buswidth;<br>
>> u8 dtr : 1;<br>
>> } dummy;<br>
>> --<br>
>> 2.34.1<br>
>><br>
<br>
<br>
Regards,<br>
Sergiu<br>
______________________________________________________<br>
Linux MTD discussion mailing list<br>
<a href="http://lists.infradead.org/mailman/listinfo/linux-mtd/">http://lists.infradead.org/mailman/listinfo/linux-mtd/</a><br>
</div>
</span></font></div>
</body>
</html>