Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 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 wrote: > > On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: >> >> On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias >> 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 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 >> 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 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 >> 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 wrote: > > Hi Bin, > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: >> From: Bin Meng >> >> 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Cédric, On Tue, Apr 27, 2021 at 10:32 PM Cédric Le Goater 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 wrote: > >>> > >>> On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > 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 > >> 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 > 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 > >> 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 > 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 > >> wrote: > >>> > >>> Hi Bin, > >>> > >>> On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > From: Bin Meng > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 wrote: >>> >>> On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias 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 >> 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 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 >> 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 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 >> wrote: >>> >>> Hi Bin, >>> >>> On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: From: Bin Meng 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 >>>
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On [2021 Apr 27] Tue 15:56:10, Alistair Francis wrote: > On Fri, Apr 23, 2021 at 4:46 PM Bin Meng wrote: > > > > On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: > > > > > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > 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 > > > > > > > > > > > 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 > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On Fri, Apr 23, 2021 at 4:46 PM Bin Meng wrote: > > On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: > > > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > > 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 > > > > 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 > > > > > > 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 > > > > > > > > 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 > > > > > > > > > > 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 > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On Mon, Feb 8, 2021 at 10:41 PM Bin Meng wrote: > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > 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 > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > > > 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,
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 wrote: > > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > 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 > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > > > 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, >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On Mon, Feb 8, 2021 at 3:42 PM Bin Meng wrote: > On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias > 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > 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 > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On Thu, Jan 21, 2021 at 10:18 PM Francisco Iglesias 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 > > 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 > > > > 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 > > > > > > 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 > > > > > > > > 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 > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 > 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Dear 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 > 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 > > > 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 > > > > > 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 > > > > > > > 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 > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Francisco, On Thu, Jan 21, 2021 at 4:50 PM Francisco Iglesias 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 > > 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 > > > > 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 > > > > > > 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 > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > > > 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 > > > > >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 > 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 > > > 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 > > > > > 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 > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Francisco, On Tue, Jan 19, 2021 at 9:01 PM Francisco Iglesias 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 > > 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 > > > > 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 > > > > > > wrote: > > > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > > From: Bin Meng > > > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 > 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 > > > 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 > > > > > wrote: > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > > From: Bin Meng > > > > > > > > > > > > > > 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
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Francisco, On Mon, Jan 18, 2021 at 6:06 PM Francisco Iglesias 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 > > 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 > > > > wrote: > > > > > > > > > > Hi Bin, > > > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > > From: Bin Meng > > > > > > > > > > > > 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. > > > > > don't think it is fair to call them 'seriously broken' (and else we should
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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 > 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 > > > wrote: > > > > > > > > Hi Bin, > > > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > > From: Bin Meng > > > > > > > > > > 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. > > > 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,
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Francisco, On Fri, Jan 15, 2021 at 8:26 PM Francisco Iglesias 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 > > wrote: > > > > > > Hi Bin, > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > From: Bin Meng > > > > > > > > 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. > 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. > for the commands is because no request has been made for them. Also there is > one controller that has support. Definitely it's not "no request". Nearly all SPI flashes support the Fast Read (0Bh) command today, and 0Bh requires a dummy cycle. This is "seriously broken" for those case 1 type controllers because they cannot read anything from the m25p80 model at all. Unless the guest software being tested only uses Read (03h) command which is not affected. But I can't find a software that uses Read instead of Fast Read. > > The issue you pointed out that we require the total number of dummy > > bits should be multiple of 8 is true, that's why I added the > > unimplemented log message in this series (patch 2/3/4) to warn users > > if this expectation is not met. However this will not cause any issue > > when running U-Boot or Linux, because both spi-nor drivers expect the > > same assumption as we do here. > > > > See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(), > > there is a logic to calculate the dummy bytes needed for fast read > > command: > > > > /* convert the dummy cycles to the number of bytes */ > > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > > > > Note the default dummy cycles configuration for all flashes I have > > looked into as of today, meets the multiple of 8 assumption. On some > > flashes the dummy cycle number is configurable, and if it's been > > configured to be an odd value, it would not work on U-Boot/Linux in > > the first place. > > > > > > > > > > Things get complicated when interacting with different SPI or QSPI > > > > flash controllers. There are major two cases: > > > > > > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > > > > For such case, driver will calculate the correct number of dummy > > > >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Havard, On Fri, Jan 15, 2021 at 11:29 AM Havard Skinnemoen wrote: > > Hi Bin, > > On Thu, Jan 14, 2021 at 6:08 PM Bin Meng wrote: > > > > Hi Francisco, > > > > On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias > > wrote: > > > > > > Hi Bin, > > > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > > From: Bin Meng > > > > > > > > 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 issue you pointed out that we require the total number of dummy > > bits should be multiple of 8 is true, that's why I added the > > unimplemented log message in this series (patch 2/3/4) to warn users > > if this expectation is not met. However this will not cause any issue > > when running U-Boot or Linux, because both spi-nor drivers expect the > > same assumption as we do here. > > > > See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(), > > there is a logic to calculate the dummy bytes needed for fast read > > command: > > > > /* convert the dummy cycles to the number of bytes */ > > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > > > > Note the default dummy cycles configuration for all flashes I have > > looked into as of today, meets the multiple of 8 assumption. On some > > flashes the dummy cycle number is configurable, and if it's been > > configured to be an odd value, it would not work on U-Boot/Linux in > > the first place. > > > > > > > > > > Things get complicated when interacting with different SPI or QSPI > > > > flash controllers. There are major two cases: > > > > > > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > > > > For such case, driver will calculate the correct number of dummy > > > > bytes and write them into the tx fifo. Fixing the m25p80 model will > > > > fix flashes working with such controllers. > > > > > > Above can be fixed while still keeping the detailed dummy cycle > > > implementation > > > inside m25p80. Perhaps one of the following could be looked into: > > > configurating > > > the amount, letting the spi ctrl fetch the amount from m25p80 or by > > > inheriting > > > some functionality handling this in the SPI controller. Or a mixture of > > > above. > > > > Please send patches to explain this in detail how this is going to > > work. I am open to all possible solutions. > > > > > > > > > - Dummy bytes not prepared by drivers. Drivers just tell the hardware > > > > the dummy cycle configuration via some registers, and hardware will > > > > automatically generate dummy cycles for us. Fixing the m25p80 model > > > > is not enough, and we will need to fix the SPI/QSPI models for such > > > > controllers. > > > > > > > > This series fixes the mess in the m25p80 from the flash side first, > > > > > > Considering the problems solved by the solution in tree I find m25p80 > > > pretty > > > clean, at least I don't see any clearly better way for accurately > > > modeling the > > > dummy clock cycles. Counting bits instead of bytes would for example still > > > force the controllers to mark which bits to count (when transmitting one > > > dummy > > > byte from a txfifo on four lines (Quad command) it generates 2 dummy clock > > > cycles since it takes two cycles to
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
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
> wrote:
> >
> > Hi Bin,
> >
> > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
> > > From: Bin Meng
> > >
> > > 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
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
for the commands is because no request has been made for them. Also there is
one controller that has support.
>
> The issue you pointed out that we require the total number of dummy
> bits should be multiple of 8 is true, that's why I added the
> unimplemented log message in this series (patch 2/3/4) to warn users
> if this expectation is not met. However this will not cause any issue
> when running U-Boot or Linux, because both spi-nor drivers expect the
> same assumption as we do here.
>
> See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(),
> there is a logic to calculate the dummy bytes needed for fast read
> command:
>
> /* convert the dummy cycles to the number of bytes */
> op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
>
> Note the default dummy cycles configuration for all flashes I have
> looked into as of today, meets the multiple of 8 assumption. On some
> flashes the dummy cycle number is configurable, and if it's been
> configured to be an odd value, it would not work on U-Boot/Linux in
> the first place.
>
> > >
> > > Things get complicated when interacting with different SPI or QSPI
> > > flash controllers. There are major two cases:
> > >
> > > - Dummy bytes prepared by drivers, and wrote to the controller fifo.
> > > For such case, driver will calculate the correct number of dummy
> > > bytes and write them into the tx fifo. Fixing the m25p80 model will
> > > fix flashes working with such controllers.
> >
> > Above can be fixed while still keeping the detailed dummy cycle
> > implementation
> > inside m25p80. Perhaps one of the following could be looked into:
> > configurating
> > the amount, letting the spi ctrl fetch the amount from m25p80 or by
> > inheriting
> > some functionality handling this in the SPI controller. Or a mixture of
> > above.
>
> Please send patches to explain this in detail how this is going to
> work. I am open to all possible solutions.
In that case I suggest that you instead try with a device property
'model_dummy_bytes' used to select to convert the accurate dummy clock cycle
count to dummy bytes inside m25p80. Below is an example on how to modify the
decode_fast_read_cmd function (the other commands requiring dummy clock cycles
can follow a similar pattern). This way the fifo mode will be able to work the
way you desire while also keeping the current functionality intact. Suddenly
removing functionality (features) will take users by surprise.
static void decode_fast_read_cmd(Flash *s)
{
uint8_t dummy_clk_cycles = 0;
uint8_t extra_bytes;
s->needed_bytes = get_addr_length(s);
/* Obtain the number of dummy clock cycles needed */
switch
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Bin, On Thu, Jan 14, 2021 at 6:08 PM Bin Meng wrote: > > Hi Francisco, > > On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias > wrote: > > > > Hi Bin, > > > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > > From: Bin Meng > > > > > > 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 issue you pointed out that we require the total number of dummy > bits should be multiple of 8 is true, that's why I added the > unimplemented log message in this series (patch 2/3/4) to warn users > if this expectation is not met. However this will not cause any issue > when running U-Boot or Linux, because both spi-nor drivers expect the > same assumption as we do here. > > See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(), > there is a logic to calculate the dummy bytes needed for fast read > command: > > /* convert the dummy cycles to the number of bytes */ > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > > Note the default dummy cycles configuration for all flashes I have > looked into as of today, meets the multiple of 8 assumption. On some > flashes the dummy cycle number is configurable, and if it's been > configured to be an odd value, it would not work on U-Boot/Linux in > the first place. > > > > > > > Things get complicated when interacting with different SPI or QSPI > > > flash controllers. There are major two cases: > > > > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > > > For such case, driver will calculate the correct number of dummy > > > bytes and write them into the tx fifo. Fixing the m25p80 model will > > > fix flashes working with such controllers. > > > > Above can be fixed while still keeping the detailed dummy cycle > > implementation > > inside m25p80. Perhaps one of the following could be looked into: > > configurating > > the amount, letting the spi ctrl fetch the amount from m25p80 or by > > inheriting > > some functionality handling this in the SPI controller. Or a mixture of > > above. > > Please send patches to explain this in detail how this is going to > work. I am open to all possible solutions. > > > > > > - Dummy bytes not prepared by drivers. Drivers just tell the hardware > > > the dummy cycle configuration via some registers, and hardware will > > > automatically generate dummy cycles for us. Fixing the m25p80 model > > > is not enough, and we will need to fix the SPI/QSPI models for such > > > controllers. > > > > > > This series fixes the mess in the m25p80 from the flash side first, > > > > Considering the problems solved by the solution in tree I find m25p80 pretty > > clean, at least I don't see any clearly better way for accurately modeling > > the > > dummy clock cycles. Counting bits instead of bytes would for example still > > force the controllers to mark which bits to count (when transmitting one > > dummy > > byte from a txfifo on four lines (Quad command) it generates 2 dummy clock > > cycles since it takes two cycles to transfer 8 bits). > > > > SPI is a bit based protocol, not bytes. If you insist on bit modeling > with the dummy cycles then you should also suggest we change all > cycles (including command/addr/dummy/data phases) to be modeled with > bits. That way we can accurately emulate everything, for example one >
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Francisco, On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias wrote: > > Hi Bin, > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > > From: Bin Meng > > > > 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 issue you pointed out that we require the total number of dummy bits should be multiple of 8 is true, that's why I added the unimplemented log message in this series (patch 2/3/4) to warn users if this expectation is not met. However this will not cause any issue when running U-Boot or Linux, because both spi-nor drivers expect the same assumption as we do here. See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(), there is a logic to calculate the dummy bytes needed for fast read command: /* convert the dummy cycles to the number of bytes */ op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; Note the default dummy cycles configuration for all flashes I have looked into as of today, meets the multiple of 8 assumption. On some flashes the dummy cycle number is configurable, and if it's been configured to be an odd value, it would not work on U-Boot/Linux in the first place. > > > > Things get complicated when interacting with different SPI or QSPI > > flash controllers. There are major two cases: > > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > > For such case, driver will calculate the correct number of dummy > > bytes and write them into the tx fifo. Fixing the m25p80 model will > > fix flashes working with such controllers. > > Above can be fixed while still keeping the detailed dummy cycle implementation > inside m25p80. Perhaps one of the following could be looked into: > configurating > the amount, letting the spi ctrl fetch the amount from m25p80 or by inheriting > some functionality handling this in the SPI controller. Or a mixture of above. Please send patches to explain this in detail how this is going to work. I am open to all possible solutions. > > > - Dummy bytes not prepared by drivers. Drivers just tell the hardware > > the dummy cycle configuration via some registers, and hardware will > > automatically generate dummy cycles for us. Fixing the m25p80 model > > is not enough, and we will need to fix the SPI/QSPI models for such > > controllers. > > > > This series fixes the mess in the m25p80 from the flash side first, > > Considering the problems solved by the solution in tree I find m25p80 pretty > clean, at least I don't see any clearly better way for accurately modeling the > dummy clock cycles. Counting bits instead of bytes would for example still > force the controllers to mark which bits to count (when transmitting one dummy > byte from a txfifo on four lines (Quad command) it generates 2 dummy clock > cycles since it takes two cycles to transfer 8 bits). > SPI is a bit based protocol, not bytes. If you insist on bit modeling with the dummy cycles then you should also suggest we change all cycles (including command/addr/dummy/data phases) to be modeled with bits. That way we can accurately emulate everything, for example one potential problem like transferring 9 bit in the data phase. However modeling everything with bit is super inefficient. My view is that we should avoid trying to support uncommon use cases (like not multiple of 8 for dummy bits) in QEMU. Regards, Bin
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Hi Bin, On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote: > From: Bin Meng > > 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. > > Things get complicated when interacting with different SPI or QSPI > flash controllers. There are major two cases: > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > For such case, driver will calculate the correct number of dummy > bytes and write them into the tx fifo. Fixing the m25p80 model will > fix flashes working with such controllers. Above can be fixed while still keeping the detailed dummy cycle implementation inside m25p80. Perhaps one of the following could be looked into: configurating the amount, letting the spi ctrl fetch the amount from m25p80 or by inheriting some functionality handling this in the SPI controller. Or a mixture of above. > - Dummy bytes not prepared by drivers. Drivers just tell the hardware > the dummy cycle configuration via some registers, and hardware will > automatically generate dummy cycles for us. Fixing the m25p80 model > is not enough, and we will need to fix the SPI/QSPI models for such > controllers. > > This series fixes the mess in the m25p80 from the flash side first, Considering the problems solved by the solution in tree I find m25p80 pretty clean, at least I don't see any clearly better way for accurately modeling the dummy clock cycles. Counting bits instead of bytes would for example still force the controllers to mark which bits to count (when transmitting one dummy byte from a txfifo on four lines (Quad command) it generates 2 dummy clock cycles since it takes two cycles to transfer 8 bits). Best regards, Francisco Iglesias > followed by fixes to 3 known SPI controller models that fall into > the 2nd case above. > > Please note, I have no way to verify patch 7/8/9 because: > > * There is no public datasheet available for the SoC / SPI controller > * There is no QEMU docs, or details that tell people how to boot either > U-Boot or Linux kernel to verify the functionality > > These 3 patches are very likely to be wrong. Hence I would like to ask > help from the original author who wrote these SPI controller models > to help testing, or completely rewrite these 3 patches to fix things. > Thanks! > > Patch 6 is unvalidated with QEMU, mainly because there is no doc to > tell people how to boot anything to test. But I have some confidence > based on my read of the ZynqMP manual, as well as some experimental > testing on a real ZCU102 board. > > Other flash patches can be tested with the SiFive SPI series: > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=222391 > > Cherry-pick patch 16 and 17 from the series above, and switch to > different flash model to test with the following command: > > $ qemu-system-riscv64 -nographic -M sifive_u -m 2G -smp 5 -kernel u-boot > > I've picked up two for testing: > > QEMU flash: "sst25vf032b" > > U-Boot 2020.10 (Jan 14 2021 - 21:55:59 +0800) > > CPU: rv64imafdcsu > Model: SiFive HiFive Unleashed A00 > DRAM: 2 GiB > MMC: > Loading Environment from SPIFlash... SF: Detected sst25vf032b with page > size 256 Bytes, erase size 4 KiB, total 4 MiB > *** Warning - bad CRC, using default environment > > In:serial@1001 > Out: serial@1001 > Err: serial@1001 > Net: failed to get gemgxl_reset reset > > Warning: ethernet@1009 MAC addresses don't match: > Address in DT is52:54:00:12:34:56 > Address in environment is 70:b3:d5:92:f0:01 > eth0: ethernet@1009 > Hit any key to stop autoboot: 0 > => sf probe > SF: Detected
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Patchew URL: https://patchew.org/QEMU/20210114150902.11515-1-bmeng...@gmail.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210114150902.11515-1-bmeng...@gmail.com Subject: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20210114013147.92962-1-jiaxun.y...@flygoat.com -> patchew/20210114013147.92962-1-jiaxun.y...@flygoat.com * [new tag] patchew/20210114150902.11515-1-bmeng...@gmail.com -> patchew/20210114150902.11515-1-bmeng...@gmail.com Switched to a new branch 'test' b87aded hw/ssi: npcm7xx_fiu: Correct the dummy cycle emulation logic 4518be2 Revert "aspeed/smc: snoop SPI transfers to fake dummy cycles" 6a4067a Revert "aspeed/smc: Fix number of dummy cycles for FAST_READ_4 command" e5ea744 hw/ssi: xilinx_spips: Fix generic fifo dummy cycle handling 3294942 hw/block: m25p80: Support fast read for SST flashes 50a7f9f hw/block: m25p80: Fix the number of dummy bytes needed for Spansion flashes cf6f8e1 hw/block: m25p80: Fix the number of dummy bytes needed for Macronix flashes 3925fcf hw/block: m25p80: Fix the number of dummy bytes needed for Numonyx/Micron flashes 5344168 hw/block: m25p80: Fix the number of dummy bytes needed for Windbond flashes === OUTPUT BEGIN === 1/9 Checking commit 5344168de433 (hw/block: m25p80: Fix the number of dummy bytes needed for Windbond flashes) 2/9 Checking commit 3925fcf79dbc (hw/block: m25p80: Fix the number of dummy bytes needed for Numonyx/Micron flashes) 3/9 Checking commit cf6f8e145faa (hw/block: m25p80: Fix the number of dummy bytes needed for Macronix flashes) 4/9 Checking commit 50a7f9fb909b (hw/block: m25p80: Fix the number of dummy bytes needed for Spansion flashes) 5/9 Checking commit 3294942ca3a1 (hw/block: m25p80: Support fast read for SST flashes) 6/9 Checking commit e5ea74473d87 (hw/ssi: xilinx_spips: Fix generic fifo dummy cycle handling) ERROR: line over 90 characters #63: FILE: hw/ssi/xilinx_spips.c:510: +uint8_t spi_mode = ARRAY_FIELD_EX32(s->regs, GQSPI_GF_SNAPSHOT, SPI_MODE); ERROR: line over 90 characters #71: FILE: hw/ssi/xilinx_spips.c:518: +qemu_log_mask(LOG_GUEST_ERROR, "Unknown SPI MODE: 0x%x ", spi_mode); total: 2 errors, 0 warnings, 41 lines checked Patch 6/9 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 7/9 Checking commit 6a4067a6a9fc (Revert "aspeed/smc: Fix number of dummy cycles for FAST_READ_4 command") 8/9 Checking commit 4518be22e1c9 (Revert "aspeed/smc: snoop SPI transfers to fake dummy cycles") 9/9 Checking commit b87aded6dc2a (hw/ssi: npcm7xx_fiu: Correct the dummy cycle emulation logic) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210114150902.11515-1-bmeng...@gmail.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
On 1/14/21 4:08 PM, Bin Meng wrote: > From: Bin Meng > > 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). > > Things get complicated when interacting with different SPI or QSPI > flash controllers. There are major two cases: > > - Dummy bytes prepared by drivers, and wrote to the controller fifo. > For such case, driver will calculate the correct number of dummy > bytes and write them into the tx fifo. Fixing the m25p80 model will > fix flashes working with such controllers. > - Dummy bytes not prepared by drivers. Drivers just tell the hardware > the dummy cycle configuration via some registers, and hardware will > automatically generate dummy cycles for us. Fixing the m25p80 model > is not enough, and we will need to fix the SPI/QSPI models for such > controllers. > > This series fixes the mess in the m25p80 from the flash side first, > followed by fixes to 3 known SPI controller models that fall into > the 2nd case above. > > Please note, I have no way to verify patch 7/8/9 because: > > * There is no public datasheet available for the SoC / SPI controller > * There is no QEMU docs, or details that tell people how to boot either > U-Boot or Linux kernel to verify the functionality The Linux drivers are available in mainline but these branches are more up to date since not everything is merged : https://github.com/openbmc/linux u-boot : https://github.com/openbmc/u-boot/tree/v2016.07-aspeed-openbmc (ast2400/ast2500) https://github.com/openbmc/u-boot/tree/v2019.04-aspeed-openbmc (ast2600) A quick intro : https://www.qemu.org/docs/master/system/arm/aspeed.html > > These 3 patches are very likely to be wrong. Hence I would like to ask > help from the original author who wrote these SPI controller models > to help testing, or completely rewrite these 3 patches to fix things. > Thanks! A quick test shows that all Aspeed machines are broken with this patchset. Please try these command lines : wget https://openpower.xyz/job/openbmc-build/lastSuccessfulBuild/distro=ubuntu,label=builder,target=palmetto/artifact/deploy/images/palmetto/flash-palmetto wget https://openpower.xyz/job/openbmc-build/lastSuccessfulBuild/distro=ubuntu,label=builder,target=romulus/artifact/deploy/images/romulus/flash-romulus wget https://openpower.xyz/job/openbmc-build/lastSuccessfulBuild/distro=ubuntu,label=builder,target=witherspoon/artifact/deploy/images/witherspoon/obmc-phosphor-image-witherspoon.ubi.mtd qemu-system-arm -M witherspoon-bmc -nic user -drive file=obmc-phosphor-image-witherspoon.ubi.mtd,format=raw,if=mtd -nographic qemu-system-arm -M romulus-bmc -nic user -drive file=flash-romulus,format=raw,if=mtd -nographic qemu-system-arm -M palmetto-bmc -nic user -drive file=flash-palmetto,format=raw,if=mtd -nographic The Aspeed SMC model has traces to help you in the task. Thanks, C. > Patch 6 is unvalidated with QEMU, mainly because there is no doc to > tell people how to boot anything to test. But I have some confidence > based on my read of the ZynqMP manual, as well as some experimental > testing on a real ZCU102 board. > > Other flash patches can be tested with the SiFive SPI series: > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=222391 > > Cherry-pick patch 16 and 17 from the series above, and switch to > different flash model to test with the following command: > > $ qemu-system-riscv64 -nographic -M sifive_u -m 2G -smp 5 -kernel u-boot > > I've picked up two for testing: > > QEMU flash: "sst25vf032b" > > U-Boot 2020.10 (Jan 14 2021 - 21:55:59 +0800) > > CPU: rv64imafdcsu > Model: SiFive HiFive Unleashed A00 > DRAM: 2 GiB > MMC: > Loading Environment from SPIFlash... SF: Detected sst25vf032b with page > size 256 Bytes, erase size 4 KiB, total 4 MiB > *** Warning - bad CRC, using default environment > > In:serial@1001 > Out: serial@1001 > Err: serial@1001 > Net: failed to get gemgxl_reset reset > > Warning: ethernet@1009 MAC addresses don't match: > Address in DT is52:54:00:12:34:56 > Address in environment is 70:b3:d5:92:f0:01 > eth0: ethernet@1009 > Hit any
