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

[Francisco Iglesias]

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?

Regarding this patch, with += 8 it looks correct to me (and will work with
above controllers as far as I can see).

Best regards,
Francisco Iglesias

> 
> Regards,
> Bin