Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes

[Bin Meng]

Hi Francisco,

On Sat, Dec 12, 2020 at 12:11 AM Francisco Iglesias
<frasse.iglesias@gmail.com> wrote:
>
> Hello Bin,
>
> On [2020 Dec 11] Fri 23:29:16, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Fri, Dec 11, 2020 at 11:16 PM Francisco Iglesias
> > <frasse.iglesias@gmail.com> wrote:
> > >
> > > Hello Bin,
> > >
> > > On [2020 Dec 11] Fri 14:07:21, Bin Meng wrote:
> > > > Hi Francisco,
> > > >
> > > > On Fri, Dec 4, 2020 at 7:28 PM Francisco Iglesias
> > > > <frasse.iglesias@gmail.com> wrote:
> > > > >
> > > > > Hello Bin,
> > > > >
> > > > > On [2020 Dec 04] Fri 18:52:50, Bin Meng wrote:
> > > > > > Hi Francisco,
> > > > > >
> > > > > > On Fri, Dec 4, 2020 at 6:46 PM Francisco Iglesias
> > > > > > <frasse.iglesias@gmail.com> wrote:
> > > > > > >
> > > > > > > Hello Bin,
> > > > > > >
> > > > > > > On [2020 Dec 04] Fri 15:52:12, Bin Meng wrote:
> > > > > > > > Hi Francisco,
> > > > > > > >
> > > > > > > > On Thu, Dec 3, 2020 at 4:38 PM Francisco Iglesias
> > > > > > > > <frasse.iglesias@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Bin and Alistair,
> > > > > > > > >
> > > > > > > > > On [2020 Dec 02] Wed 11:40:11, Alistair Francis wrote:
> > > > > > > > > > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng 
> > > > > > > > > > <bmeng.cn@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Bin Meng <bin.meng@windriver.com>
> > > > > > > > > > >
> > > > > > > > > > > SST flashes require a dummy byte after the address bits.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > > > > > > >
> > > > > > > > > > I couldn't find a datasheet that says this... But the 
> > > > > > > > > > actual code
> > > > > > > > > > change looks fine, so:
> > > > > > > > > >
> > > > > > > > > > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > > > > >
> > > > > > > > > > Alistair
> > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > >  hw/block/m25p80.c | 3 +++
> > > > > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > > > > > > > > > index 483925f..9b36762 100644
> > > > > > > > > > > --- a/hw/block/m25p80.c
> > > > > > > > > > > +++ b/hw/block/m25p80.c
> > > > > > > > > > > @@ -825,6 +825,9 @@ static void 
> > > > > > > > > > > decode_fast_read_cmd(Flash *s)
> > > > > > > > > > >      s->needed_bytes = get_addr_length(s);
> > > > > > > > > > >      switch (get_man(s)) {
> > > > > > > > > > >      /* Dummy cycles - modeled with bytes writes instead 
> > > > > > > > > > > of bits */
> > > > > > > > > > > +    case MAN_SST:
> > > > > > > > > > > +        s->needed_bytes += 1;
> > > > > > > > >
> > > > > > > > > 1 dummy clk cycle is modelled as 1 byte write (see the 
> > > > > > > > > comment above), so 1
> > > > > > > > > dummy byte (8 dummy clk cycles) will need +8 above.
> > > > > > > >
> > > > > > > > I think you were confused by the WINBOND codes. The comments are
> > > > > > > > correct. It is modeled with bytes instead of bits, so we should 
> > > > > > > > +=1.
> > > > > > >
> > > > > > > What the comment says is (perhaps not superclear) that 1 dummy 
> > > > > > > clock cycle
> > > > > > > is modeled as one 1 byte write into the flash (meaining that 8 
> > > > > > > byte writes
> > > > > > > are needed for 1 dummy byte). Perhaps it is easier to understand
> > > > > > > looking into how the controllers issue the command towards the 
> > > > > > > flash model
> > > > > > > (for example the xilinx_spips), the start of the FAST_READ cmd is 
> > > > > > > issued
> > > > > > > as writing the following into the flash: 1 byte (cmd), 3 bytes 
> > > > > > > (address),
> > > > > > > 8 bytes (8 dummy cycles -> 1 dummy byte).
> > > > > > >
> > > > > >
> > > > > > My interpretation of the comments are opposite: one cycle is a bit,
> > > > > > but we are not using bits, instead we are using bytes.
> > > > >
> > > > > Yes, the mentioning of 'bits' in the comment makes it not very clear 
> > > > > at first read.
> > > > > Maybe just bellow would have been better:
> > > > >
> > > > > /* Dummy clock cycles - modeled with bytes writes */
> > > > >
> > > > > >
> > > > > > Testing shows that +=1 is the correct way with the imx_spi 
> > > > > > controller,
> > > > > > and with my SiFive SPI model in my local tree (not upstreamed yet)
> > > > >
> > > > > Perhaps an option could be to look into how the aspeed_smc, 
> > > > > xilinx_spips or the
> > > > > npcm7xx_fiu generate dummy clock cycles and see if a similar solution 
> > > > > to one of
> > > > > those could work aswell for the imx_spi?
> > > > >
> > > >
> > > > Thanks for pointing this out. So there is some inconsistency among
> > > > different SPI controller modeling.
> > >
> > > I'm not sure I understand you correctly but the controllers supporting
> > > commands with dummy clock cycles can only do it following the modeled
> > > approach, so I would rather say it is pretty consistent across the
> > > controllers (not all controllers support these commands though).
> >
> > I mean there are 2 approaches to emulate the dummy cycles for
>
> There is currently only 1 way of modeling dummy clock cycles. All commands 
> that
> require / support them in m25p80 goes with that approach. An the controllers
> that support dummy clock cycles uses that approach.

No, there are 2 ways. One way is how aspeed_smc, xilinx_spips and
npcm7xx do for dummy cycles. For these controllers, there are hardware
registers for dummy cycles, and software does not need to write
anything into the tx fifo. These models emulate one dummy cycle by
transferring one byte one the SPI line so we see there are actually a
number of (bit * 8) bytes needed in decode_fast_read_cmd(). The other
way is how imx_spi, mss-spi, pl022, stm32f2xx_spi and xilinx_spi. For
these controllers, they just transfer whatever is written by guest
software to tx fifo without any special awareness of dummy cycles.

>
> > different SPI controller models, yet we only have one m25p80 flash
> > model to work with both of them. Some controllers may choose 1 byte to
> > emulate 1 dummy clock cycle, but some others choose 1 bit to emulate 1
> > dummy cycle. This is inconsistent.
> >
> > >
> > > >
> > > > Or maybe fixing aspeed_smc, xilinx_spips and npcm7xx_fiu to work like
> > > > imx_spi?
> > >
> > > For me I would say no to above (it makes more sense to let new controllers
> > > implement the currently modeled approach).
> >
> > Yes, we can certainly make them consistent. But the question is which
> > one is the correct one? I tried to search in the doc but in vain.
>
> I think it is better to look into the code instead of doc/comments since it
> might be outdated/misinterpreted.
>

But the codes have 2 different approaches and I don't know which one
was the original intended behavior.

> >
> > >
> > > > Which one is the expected behavior for dummy cycles?
> > >
> > > Dummy clock cycles are modeled as 1 byte written to the flash per dummy 
> > > clock
> > > cycle (expected behavior).
> > >
> >
> > Like I mentioned before, 8 bytes to emulate 8 dummy clock cycles does
> > not work with current imx_spi model.
>
> I've understood this and that is why I'm trying to explain that it needs to
> convert the dummy byte to 8 writes (1 for each clock cycle). At the moment the
> imx_spi does not support dummy clock cycle generation as far as I can see.
>
> This patch works for the imx_xpi with the SST flash connected because it's
> hardcoding the flash to use 1 dummy clock cycle (speaking m25p80.c language)
> and then issues 1 byte write (for the dummy byte) from the model (matching 
> it).

Please see above, the imx_spi does not hard coding 1 dummy clock cycle
to be used.

> If you switch to another flash, for example Windbond, the FAST_READ command
> won't work for the imx_spi anymore. And if you use an SST flash (using this
> patch) with another controller that has implemented dummy cycle generation
> the FAST_READ command won't work.

Dividing the Winbound dummy cycles by 8 should work with imx_spi. But
like I mentioned, we should firstly take a consistent approach to
handle dummy cycles in the SPI models.

Regards,
Bin