[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Bin Meng
Subject: Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands
Date: Fri, 15 Jan 2021 10:07:52 +0800

Hi Francisco,

On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias
<frasse.iglesias@gmail.com> wrote:
> Hi Bin,
> On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > The m25p80 model uses s->needed_bytes to indicate how many follow-up
> > bytes are expected to be received after it receives a command. For
> > example, depending on the address mode, either 3-byte address or
> > 4-byte address is needed.
> >
> > For fast read family commands, some dummy cycles are required after
> > sending the address bytes, and the dummy cycles need to be counted
> > in s->needed_bytes. This is where the mess began.
> >
> > As the variable name (needed_bytes) indicates, the unit is in byte.
> > It is not in bit, or cycle. However for some reason the model has
> > been using the number of dummy cycles for s->needed_bytes. The right
> > approach is to convert the number of dummy cycles to bytes based on
> > the SPI protocol, for example, 6 dummy cycles for the Fast Read Quad
> > I/O (EBh) should be converted to 3 bytes per the formula (6 * 4 / 8).
> While not being the original implementor I must assume that above solution was
> considered but not chosen by the developers due to it is inaccuracy (it
> wouldn't be possible to model exacly 6 dummy cycles, only a multiple of 8,
> meaning that if the controller is wrongly programmed to generate 7 the error
> wouldn't be caught and the controller will still be considered "correct"). Now
> that we have this detail in the implementation I'm in favor of keeping it, 
> this
> also because the detail is already in use for catching exactly above error.

I found no clue from the commit message that my proposed solution here
was ever considered, otherwise all SPI controller models supporting
software generation should have been found out seriously broken long
time ago!

The 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

    /* 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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]