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