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

[Francisco Iglesias <frasse.iglesias@gmail.com>]

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).

Best regards,
Francisco Iglesias

> 
> > An option could be to fall
> > through to the Windbond case below instead (since it seems to operate
> > likewise).
> >
> > Best regards,
> > Francisco Iglesias
> >
> >
> > > > +        break;
> > > >      case MAN_WINBOND:
> > > >          s->needed_bytes += 8;
> 
> Actually this is wrong. This should be corrected to +=1. I will
> prepare a patch for it.
> 
> > > >          break;
> > > > --
> 
> Regards,
> Bin