Hello Edgar, On [2021 Feb 08] Mon 16:30:00, Edgar E. Iglesias wrote: > On Mon, Feb 8, 2021 at 3:42 PM Bin Meng <bmeng...@gmail.com> wrote: > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > <frasse.igles...@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.igles...@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.igles...@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.igles...@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.igles...@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.igles...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > From: Bin Meng <bin.m...@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) > > Francisco really knows this stuff better than me.... > I would tend to agree that it's unfortunate to model things in cycles, if > we could abstract things at a higher level that would be nice. Without > breaking existing use-cases. > Francisco, is it impossible to bring up the abstraction level to bytes and > keep existing use-cases?
Great question, I'm leaning on that it shouldn't be impossible to be honest (but I haven't been able to try anything yet though). Best regards, Francisco Iglesias > We have a bunch of test-cases, We'll publish some of them in source code, > others we can't publish since they use proprietary SW we're not allowed to > publish at all, but we can run tests and Ack if things work. > Best regards, > Edgar
