Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands

[Cédric Le Goater]

Hello,

On 4/27/21 10:54 AM, Francisco Iglesias wrote:
> On [2021 Apr 27] Tue 15:56:10, Alistair Francis wrote:
>> On Fri, Apr 23, 2021 at 4:46 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Mon, Feb 8, 2021 at 10:41 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias
>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>
>>>>> Hi Bin,
>>>>>
>>>>> On [2021 Jan 21] Thu 16:59:51, Bin Meng wrote:
>>>>>> Hi Francisco,
>>>>>>
>>>>>> On Thu, Jan 21, 2021 at 4:50 PM Francisco Iglesias
>>>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>>>
>>>>>>> Dear Bin,
>>>>>>>
>>>>>>> On [2021 Jan 20] Wed 22:20:25, Bin Meng wrote:
>>>>>>>> Hi Francisco,
>>>>>>>>
>>>>>>>> On Tue, Jan 19, 2021 at 9:01 PM Francisco Iglesias
>>>>>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Bin,
>>>>>>>>>
>>>>>>>>> On [2021 Jan 18] Mon 20:32:19, Bin Meng wrote:
>>>>>>>>>> Hi Francisco,
>>>>>>>>>>
>>>>>>>>>> On Mon, Jan 18, 2021 at 6:06 PM Francisco Iglesias
>>>>>>>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Bin,
>>>>>>>>>>>
>>>>>>>>>>> On [2021 Jan 15] Fri 22:38:18, Bin Meng wrote:
>>>>>>>>>>>> Hi Francisco,
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jan 15, 2021 at 8:26 PM Francisco Iglesias
>>>>>>>>>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Bin,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On [2021 Jan 15] Fri 10:07:52, Bin Meng wrote:
>>>>>>>>>>>>>> Hi Francisco,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias
>>>>>>>>>>>>>> <frasse.iglesias@gmail.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Bin,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
>>>>>>>>>>>>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The m25p80 model uses s->needed_bytes to indicate how many 
>>>>>>>>>>>>>>>> follow-up
>>>>>>>>>>>>>>>> bytes are expected to be received after it receives a command. 
>>>>>>>>>>>>>>>> For
>>>>>>>>>>>>>>>> example, depending on the address mode, either 3-byte address 
>>>>>>>>>>>>>>>> or
>>>>>>>>>>>>>>>> 4-byte address is needed.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> For fast read family commands, some dummy cycles are required 
>>>>>>>>>>>>>>>> after
>>>>>>>>>>>>>>>> sending the address bytes, and the dummy cycles need to be 
>>>>>>>>>>>>>>>> counted
>>>>>>>>>>>>>>>> in s->needed_bytes. This is where the mess began.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> As the variable name (needed_bytes) indicates, the unit is in 
>>>>>>>>>>>>>>>> byte.
>>>>>>>>>>>>>>>> It is not in bit, or cycle. However for some reason the model 
>>>>>>>>>>>>>>>> has
>>>>>>>>>>>>>>>> been using the number of dummy cycles for s->needed_bytes. The 
>>>>>>>>>>>>>>>> right
>>>>>>>>>>>>>>>> approach is to convert the number of dummy cycles to bytes 
>>>>>>>>>>>>>>>> based on
>>>>>>>>>>>>>>>> the SPI protocol, for example, 6 dummy cycles for the Fast 
>>>>>>>>>>>>>>>> Read Quad
>>>>>>>>>>>>>>>> I/O (EBh) should be converted to 3 bytes per the formula (6 * 
>>>>>>>>>>>>>>>> 4 / 8).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> While not being the original implementor I must assume that 
>>>>>>>>>>>>>>> above solution was
>>>>>>>>>>>>>>> considered but not chosen by the developers due to it is 
>>>>>>>>>>>>>>> inaccuracy (it
>>>>>>>>>>>>>>> wouldn't be possible to model exacly 6 dummy cycles, only a 
>>>>>>>>>>>>>>> multiple of 8,
>>>>>>>>>>>>>>> meaning that if the controller is wrongly programmed to 
>>>>>>>>>>>>>>> generate 7 the error
>>>>>>>>>>>>>>> wouldn't be caught and the controller will still be considered 
>>>>>>>>>>>>>>> "correct"). Now
>>>>>>>>>>>>>>> that we have this detail in the implementation I'm in favor of 
>>>>>>>>>>>>>>> keeping it, this
>>>>>>>>>>>>>>> also because the detail is already in use for catching exactly 
>>>>>>>>>>>>>>> above error.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I found no clue from the commit message that my proposed 
>>>>>>>>>>>>>> solution here
>>>>>>>>>>>>>> was ever considered, otherwise all SPI controller models 
>>>>>>>>>>>>>> supporting
>>>>>>>>>>>>>> software generation should have been found out seriously broken 
>>>>>>>>>>>>>> long
>>>>>>>>>>>>>> time ago!
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> The controllers you are referring to might lack support for 
>>>>>>>>>>>>> commands requiring
>>>>>>>>>>>>> dummy clock cycles but I really hope they work with the other 
>>>>>>>>>>>>> commands? If so I
>>>>>>>>>>>>
>>>>>>>>>>>> I am not sure why you view dummy clock cycles as something special
>>>>>>>>>>>> that needs some special support from the SPI controller. For the 
>>>>>>>>>>>> case
>>>>>>>>>>>> 1 controller, it's nothing special from the controller perspective,
>>>>>>>>>>>> just like sending out a command, or address bytes, or data. The
>>>>>>>>>>>> controller just shifts data bit by bit from its tx fifo and that's 
>>>>>>>>>>>> it.
>>>>>>>>>>>> In the Xilinx GQSPI controller case, the dummy cycles can either be
>>>>>>>>>>>> sent via a regular data (the case 1 controller) in the tx fifo, or
>>>>>>>>>>>> automatically generated (case 2 controller) by the hardware.
>>>>>>>>>>>
>>>>>>>>>>> Ok, I'll try to explain my view point a little differently. For 
>>>>>>>>>>> that we also
>>>>>>>>>>> need to keep in mind that QEMU models HW, and any binary that runs 
>>>>>>>>>>> on a HW
>>>>>>>>>>> board supported in QEMU should ideally run on that board inside 
>>>>>>>>>>> QEMU aswell
>>>>>>>>>>> (this can be a bare metal application equaly well as a modified 
>>>>>>>>>>> u-boot/Linux
>>>>>>>>>>> using SPI commands with a non multiple of 8 number of dummy clock 
>>>>>>>>>>> cycles).
>>>>>>>>>>>
>>>>>>>>>>> Once functionality has been introduced into QEMU it is not easy to 
>>>>>>>>>>> know which
>>>>>>>>>>> intentional or untentional features provided by the functionality 
>>>>>>>>>>> are being
>>>>>>>>>>> used by users. One of the (perhaps not well known) features I'm 
>>>>>>>>>>> aware of that
>>>>>>>>>>> is in use and is provided by the accurate dummy clock cycle 
>>>>>>>>>>> modeling inside
>>>>>>>>>>> m25p80 is the be ability to test drivers accurately regarding the 
>>>>>>>>>>> dummy clock
>>>>>>>>>>> cycles (even when using commands with a non-multiple of 8 number of 
>>>>>>>>>>> dummy clock
>>>>>>>>>>> cycles), but there might be others aswell. So by removing this 
>>>>>>>>>>> functionality
>>>>>>>>>>> above use case will brake, this since those test will not be 
>>>>>>>>>>> reliable.
>>>>>>>>>>> Furthermore, since users tend to be creative it is not possible to 
>>>>>>>>>>> know if
>>>>>>>>>>> there are other use cases that will be affected. This means that in 
>>>>>>>>>>> case [1]
>>>>>>>>>>> needs to be followed the safe path is to add functionality instead 
>>>>>>>>>>> of removing.
>>>>>>>>>>> Luckily it also easier in this case, see below.
>>>>>>>>>>
>>>>>>>>>> I understand there might be users other than U-Boot/Linux that use an
>>>>>>>>>> odd number of dummy bits (not multiple of 8). If your concern was
>>>>>>>>>> about model behavior changes, sure I can update
>>>>>>>>>> qemu/docs/system/deprecated.rst to mention that some flashes in the
>>>>>>>>>> m25p80 model now implement dummy cycles as bytes.
>>>>>>>>>
>>>>>>>>> Yes, something like that. My concern is that since this functionality 
>>>>>>>>> has been
>>>>>>>>> in tree for while, users have found known or unknown features that got
>>>>>>>>> introduced by it. By removing the functionality (and the known/uknown 
>>>>>>>>> features)
>>>>>>>>> we are riscing to brake our user's use cases (currently I'm aware of 
>>>>>>>>> one
>>>>>>>>> feature/use case but it is not unlikely that there are more). [1] 
>>>>>>>>> states that
>>>>>>>>> "In general features are intended to be supported indefinitely once 
>>>>>>>>> introduced
>>>>>>>>> into QEMU", to me that makes very much sense because the opposite 
>>>>>>>>> would mean
>>>>>>>>> that we were not reliable. So in case [1] needs to be honored it 
>>>>>>>>> looks to be
>>>>>>>>> safer to add functionality instead of removing (and riscing the 
>>>>>>>>> removal of use
>>>>>>>>> cases/features). Luckily I still believe in this case that it will be 
>>>>>>>>> easier to
>>>>>>>>> go forward (even if I also agree on what you are saying below about 
>>>>>>>>> what I
>>>>>>>>> proposed).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Even if the implementation is buggy and we need to keep the buggy
>>>>>>>> implementation forever? I think that's why
>>>>>>>> qemu/docs/system/deprecated.rst was created for deprecating such
>>>>>>>> feature.
>>>>>>>
>>>>>>> With the RFC I posted all commands in m25p80 are working for both the 
>>>>>>> case 1
>>>>>>> controller (using a txfifo) and the case 2 controller (no txfifo, as 
>>>>>>> GQSPI).
>>>>>>> Because of this, I, with all respect, will have to disagree that this 
>>>>>>> is buggy.
>>>>>>
>>>>>> Well, the existing m25p80 implementation that uses dummy cycle
>>>>>> accuracy for those flashes prevents all SPI controllers that use tx
>>>>>> fifo to work with those flashes. Hence it is buggy.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> don't think it is fair to call them 'seriously broken' (and else 
>>>>>>>>>>>>> we should
>>>>>>>>>>>>> probably let the maintainers know about it). Most likely the lack 
>>>>>>>>>>>>> of support
>>>>>>>>>>>>
>>>>>>>>>>>> I called it "seriously broken" because current implementation only
>>>>>>>>>>>> considered one type of SPI controllers while completely ignoring 
>>>>>>>>>>>> the
>>>>>>>>>>>> other type.
>>>>>>>>>>>
>>>>>>>>>>> If we change view and see this from the perspective of m25p80, it 
>>>>>>>>>>> models the
>>>>>>>>>>> commands a certain way and provides an API that the SPI controllers 
>>>>>>>>>>> need to
>>>>>>>>>>> implement for interacting with it. It is true that there are SPI 
>>>>>>>>>>> controllers
>>>>>>>>>>> referred to above that do not support the portion of that API that 
>>>>>>>>>>> corresponds
>>>>>>>>>>> to commands with dummy clock cycles, but I don't think it is true 
>>>>>>>>>>> that this is
>>>>>>>>>>> broken since there is also one SPI controller that has a working 
>>>>>>>>>>> implementation
>>>>>>>>>>> of m25p80's full API also when transfering through a tx fifo (use 
>>>>>>>>>>> case 1). But
>>>>>>>>>>> as mentioned above, by doing a minor extension and improvement to 
>>>>>>>>>>> m25p80's API
>>>>>>>>>>> and allow for toggling the accuracy from dummy clock cycles to 
>>>>>>>>>>> dummy bytes [1]
>>>>>>>>>>> will still be honored as in the same time making it possible to 
>>>>>>>>>>> have full
>>>>>>>>>>> support for the API in the SPI controllers that currently do not 
>>>>>>>>>>> (please reread
>>>>>>>>>>> the proposal in my previous reply that attempts to do this). I 
>>>>>>>>>>> myself see this
>>>>>>>>>>> as win/win situation, also because no controller should need 
>>>>>>>>>>> modifications.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I am afraid your proposal does not work. Your proposed new device
>>>>>>>>>> property 'model_dummy_bytes' to select to convert the accurate dummy
>>>>>>>>>> clock cycle count to dummy bytes inside m25p80, is hard to justify as
>>>>>>>>>> a property to the flash itself, as the behavior is tightly coupled to
>>>>>>>>>> how the SPI controller works.
>>>>>>>>>
>>>>>>>>> I agree on above. I decided though that instead of posting sample 
>>>>>>>>> code in here
>>>>>>>>> I'll post an RFC with hopefully an improved proposal. I'll cc you. 
>>>>>>>>> About below,
>>>>>>>>> Xilinx ZynqMP GQSPI should not need any modication in a first step.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Wait, (see below)
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Please take a look at the Xilinx GQSPI controller, which supports 
>>>>>>>>>> both
>>>>>>>>>> use cases, that the dummy cycles can be transferred via tx fifo, or
>>>>>>>>>> generated by the controller automatically. Please read the example
>>>>>>>>>> given in:
>>>>>>>>>>
>>>>>>>>>>     table 24‐22, an example of Generic FIFO Contents for Quad I/O 
>>>>>>>>>> Read
>>>>>>>>>> Command (EBh)
>>>>>>>>>>
>>>>>>>>>> in 
>>>>>>>>>> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>>>>>>>>>
>>>>>>>>>> If you choose to set the m25p80 device property 'model_dummy_bytes' 
>>>>>>>>>> to
>>>>>>>>>> true when working with the Xilinx GQSPI controller, you are bound to
>>>>>>>>>> only allow guest software to use tx fifo to transfer the dummy 
>>>>>>>>>> cycles,
>>>>>>>>>> and this is wrong.
>>>>>>>>>>
>>>>>>>>
>>>>>>>> You missed this part. I looked at your RFC, and as I mentioned above
>>>>>>>> your proposal cannot support the complicated controller like Xilinx
>>>>>>>> GQSPI. Please read the example of table 24-22. With your RFC, you
>>>>>>>> mandate guest software's GQSPI driver to only use hardware dummy cycle
>>>>>>>> generation, which is wrong.
>>>>>>>>
>>>>>>>
>>>>>>> First, thank you very much for looking into the RFC series, very much
>>>>>>> appreciated. Secondly, about above, the GQSPI model in QEMU transfers 
>>>>>>> from 2
>>>>>>> locations in the file, in 1 location the transfer referred to above is 
>>>>>>> done, in
>>>>>>> another location the transfer through the txfifo is done. The location 
>>>>>>> where
>>>>>>> transfer referred to above is done will not need any modifications (and 
>>>>>>> will
>>>>>>> thus work equally well as it does currently).
>>>>>>
>>>>>> Please explain this a little bit. How does your RFC series handle
>>>>>> cases as described in table 24-22, where the 6 dummy cycles are split
>>>>>> into 2 transfers, with one transfer using tx fifo, and the other one
>>>>>> using hardware dummy cycle generation?
>>>>>
>>>>> Sorry, I missunderstod. You are right, that won't work.
>>>>
>>>> +Edgar E. Iglesias
>>>>
>>>> So it looks by far the only way to implement dummy cycles correctly to
>>>> work with all SPI controller models is what I proposed here in this
>>>> patch series.
>>>>
>>>> Maintainers are quite silent, so I would like to hear your thoughts.
>>>>
>>>> @Alistair Francis @Philippe Mathieu-Daudé @Peter Maydell would you
>>>> please share your thoughts since you are the one who reviewed the
>>>> existing dummy implementation (based on commits history)
>>
>> I agree with Edgar, in that Francisco and Bin know this better than me
>> and that modelling things in cycles is a pain.
> 
> Hi Alistair,
> 
>>
>> As Bin points out it seems like currently we should be modelling bytes
>> (from the variable name) so it makes sense to keep it in bytes. I
>> would be in favour of this series in that case. Do we know what use
>> cases this will break? I know it's hard to answer but I don't think
>> there are too many SSI users in QEMU so it might not be too hard to
>> test most of the possible use cases.
> 
> The use case I'm aware of is regression testing of drivers. Ex: if a
> driver is using 10 dummy clock cycles with the commands and a patch
> accidentaly changes the driver to use 11 dummy clock cycles QEMU currently
> finds the problem, that won't be possible with this series. It's difficult
> to say but it is not impossible there are other use cases also.


It was breaking the Aspeed machines :

  
https://lore.kernel.org/qemu-devel/78a12882-1303-dd6d-6619-96c5e2cbf531@kaod.org/

QEMU 6.1 should have acceptance tests that will help in detecting
regressions in this area.

Thanks,

C.
 



> 
> More importantly IMO though is that the current use cases can be keept
> while still providing support for commands with dummy clock cycles into
> the QEMU SPI controllers lacking at the moment.
> 
> (If I recall correctly this series might also have another issue regarding
> the GQSPI SPI mode configuration, with that it is possible transmit 8
> dummy clock cycles as 1 data byte, 2 data bytes or 4 data bytes, so I
> think some form of calculation might be needed inside m25p80).
> 
> Best regards,
> Francisco
> 
> 
>>
>> Alistair
>>
>>>
>>> Hello maintainers,
>>>
>>> We apparently missed the 6.0 window to address this mess of the m25p80
>>> model. Please provide your inputs on this before I start working on
>>> the v2.
>>>
>>> Regards,
>>> Bin
>>>