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

[Cédric Le Goater]

On 4/28/21 3:12 PM, Bin Meng wrote:
> Hi Cédric,
> 
> On Tue, Apr 27, 2021 at 10:32 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> 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/
> 
> Yes, as I mentioned in the series the modification was based on a pure
> guess from existing QEMU codes as I could not find a datasheet of the
> Aspeed SPI controller on the internet. Do you know if this is publicly
> available?

It is not but much of the register bitfields are described in the code.
I should be able to help you in making this work.

Thanks,

C. 


>> QEMU 6.1 should have acceptance tests that will help in detecting
>> regressions in this area.
>>
> 
> Regards,
> Bin
>