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

[Francisco Iglesias]

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

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

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

Best regards,
Francisco Iglesias

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