[PATCH] spi: Replace `dummy.nbytes` with `dummy.ncycles`
Sergiu.Moga at microchip.com
Sergiu.Moga at microchip.com
Tue Sep 27 18:21:34 AEST 2022
On 26.09.2022 20:24, Serge Semin wrote:
> @Mark, @Tudor, @Pratyush, @Michael could you please join the
> discussion regarding the dummy.buswidth and dummy.dtr fields in the
> spi_mem_op structure?
>
> On Mon, Sep 26, 2022 at 09:05:49AM +0000, Sergiu.Moga at microchip.com wrote:
>> On 26.09.2022 01:03, Serge Semin wrote:
>>> Hello Sergiu
>>>
>
> Sergiu, you didn't address all my comments. Please be more attentive.
>
Hello Serge,
My apologies, I did take note of them :D. I said while addressing your
comments for `drivers/spi/spi-dw-core.c` that I agree with the rest of
the comments, as I did not find it productive to say something among the
lines of "I agree" or "Noted" for each comment separately.
>>
>>
>> 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.
>
> I do see the way these fields are used in the SPI-mem drivers. I was
> wondering what do these bits mean in the framework of the SPI-mem
> core? AFAICS from the specification the dummy cycles are irrelevant to
> the data bus state. It says "the master tri-states the bus during
> 'dummy' cycles." If so I don't see a reason to have the DTR and
> buswidth fields in the spi_mem_op structure anymore. The number of
> cycles could be calculated right on the initialization stage based on
> the SPI NOR/NAND requirements.
>
> @Mark, @Tudor, @Pratyush, what do you think?
>
>>
>>
>>>>
>>>> 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.
>
> Judging by the way the macro is declared it isn't platform specific.
> So no, the kernel always expects the byte having eight bits.
>
>>
>> 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.
>
> Right. That's what the BITS_PER_BYTE macro is for.
>
>>
>> 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.
>
> It represents exactly the same in include/linux/bits.h .
>
> Ideally it would be good to use it in all your updates since you touch
> the corresponding parts anyway. But at the very least I would insist on
> using the macro in the drivers which already have it utilized like
> spi-dw-*, spi-mtk-snfi, spi-mtk-nor.
>
I guess you are right. I wrongly assumed it has something to do with
plaforms. I checked with commit f7589f28d7dd4586b4e90ac3b2a180409669053a
when it was first defined and I guess that yeah, it's not what I thought
it was, my apologies :).
>>
>>
>>> 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.
>
> Ok. thanks.
>
>>
>>
>>>> + 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 :).
>
> AFAICS the only drivers which support the DTR-capable transfers are
> the ones having the spi_controller_mem_caps structure defined with dtr
> set to true or the ones with custom SPI-mem ops. It means that the
> DTR-transfers are supported by the spi-mtk-snfi.c, spi-mxic.c,
> spi-cadence-quadspi.c and spi-intel.c drivers only. The rest of the
> SPI-controller drivers will fail to execute the SPI-mem ops with dtr
> flag set due to the spi_mem_default_supports_op() method semantics.
>
>>
>>
>>>> +
>>>> + 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?
>
> Please take this note into account. Preserving the locally defined
> coding-style convention is a very useful practice. It retains the code
> uniformity, which improves readability and maintainability for just no
> price.
>
>>>
>>>>
>>>> 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?
>
> Ok. This part turns to be debatable indeed. On the first glance the
> SPI-mem core doesn't anyhow handles the DTR-flag value. On the other
> hand SPI-controllers may have the dtr-capability flag set thus, for
> instance implicitly supporting the DTR transfers. Finally currently
> all the DTR-aware drivers are known to have the custom SPI-mem ops
> defined. So some aspects say for dropping the dummy.dtr field usage
> from here, some say against it. I'll leave it for you and @Mark,
> @Tudor, @Pratyush to decide.
>
I am fine with this either way. I placed it here at first since I found
it to be most appropriate here.
>>
>>
>>>> +
>>>> + 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?
>
> Please retain the local coding style convention.
>
>>>
>>>>
>>>> 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.
>
> Again. The initialization statement is too long. It makes the code
> harder to read. Just split the declaration and initialization up.
>
Alright, I did take note of it. As I previously said, I agree with all
of your styling related comments.
>>>
>>>> + if (op->dummy.dtr)
>>>> + dummy_nbytes *= 2;
>>>
>
>>> Does the MTK SPI driver support DTR? AFAICS it doesn't.
>
> I'll give an answer. It doesn't. The spi_mem_exec_op() will return the
> -ENOTSUPP error if an SPI-mem op with any dtr flag set is requested.
>
Alright, thanks.
>>>
>>>>
>>>> // 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?
>
> Too long. Split the declaration and initialization up.
>
>>>
>>>> +
>>>> + 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.
>
> spi_mem_default_supports_op() will return false for the DTR-available
> transfers anyway. So the spi_mem_exec_op() method will fail right at
> the start and this part will never be executed if the DTR-mode is
> requested.
>
> -Sergey
>
Noted.
Thanks,
Sergiu
>>
>>
>>
>>>>
>>>> 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
More information about the openbmc
mailing list