[PATCH] Revert "spi: zynq_qspi: Use dummy buswidth in dummy byte calculation"

[Stefan Herbrechtsmeier]

Hi Ashok,

have you test your patches with 1-4-4 mode?

I think your patches are a nop for 1-1-4 mode and break other modes:

spi-nor-core
op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;

zynq_qspi:
dummy_bytes = op->dummy.nbytes / op->dummy.buswidth;

=> dummy_bytes = (nor->read_dummy * op.dummy.buswidth) / 8) /
op->dummy.buswidth
=> dummy_bytes = nor->read_dummy / 8

You changes ignores the bus width and breaks if the addr bus width is not 1!

Regards
   Stefan

Am 18.04.2023 um 12:27 schrieb Stefan Herbrechtsmeier:
> Hi Ashok,
>
> Am 18.04.2023 um 10:43 schrieb Soma, Ashok Reddy:
>
>>
>>> -----Original Message-----
>>> From: Simek, Michal <michal.simek at amd.com>
>>> Sent: Monday, April 17, 2023 3:47 PM
>>> To: Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier-oss at weidmueller.com>;
>>> u-boot at lists.denx.de; Soma, Ashok Reddy <ashok.reddy.soma at amd.com>
>>> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>;
>>> Jagan Teki <jagan at amarulasolutions.com>
>>> Subject: Re: [PATCH] Revert "spi: zynq_qspi: Use dummy buswidth in
>>> dummy
>>> byte calculation"
>>>
>>>
>>>
>>> On 3/31/23 16:44, Stefan Herbrechtsmeier wrote:
>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>
>>>> This reverts commit e09784728689de7949d4cdd559a9590e0bfcc702. The
>>>> commit wrongly divides the dummy bytes by dummy bus width to calculate
>>>> the dummy bytes. The framework already converts the dummy cycles to
>>>> the number of bytes and the controller use the SPI flash command to
>>>> determine the dummy cycles via the address width.
>> As per my understanding dummy bus width should be equal to data
>> buswidth and not equal to address bus width.
>> Please let me know if this understanding in incorrect.
>
> Why? The kernel use the addr bus width too.
>
> Independent of the correct answer the core should handle this because
> it forward the dummy *bytes* to the driver:
>
>     /* convert the dummy cycles to the number of bytes */
>     op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
>
>
> Furthermore, the driver send the dummy as part of the address:
>
>     /* 1st transfer: opcode + address + dummy cycles */
>
>> Based on the above understanding we have changed in Xilinx repo, at
>> below code
>> https://github.com/Xilinx/u-boot-xlnx/blob/024eb37c1e38ab811abe5408d42069fbd7901824/drivers/mtd/spi/spi-nor-core.c#L262
>>
>> from
>> op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>> to
>> op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
>>
>> I have sent the same as RFC. Please see the attached patch.
>>
>> Currently this patch is present as part of Xilinx repo, and hence
>> based on this below implementation dymmy_bytes are recalculated.
>
> In this case the patch should be reverted until the other patch is
> accepted.
>
>> I have sent controller driver code to upstream but, I have not yet
>> sent spi-nor-core.c code, which I will be sending soon with some
>> other changes.
>>
>> Please let me know your thoughts about this patch and the changes.
>
> It is unclear to me why the dummy should be part of the data. The
> current implementation is wrong because it doesn't respects the
> address width and send the dummy bytes as part of the addr.
>
> Regards
>   Stefan
>
________________________________
Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, Dr. Sebastian Durst, André Sombecki;
USt-ID-Nr. DE124599660