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

[Francisco Iglesias]

Hi bin,

On [2020 Dec 12] Sat 16:16:59, Bin Meng wrote:
> 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.


The xilinx_spips supports above way of transferring a command so you can look
into that one for an example of how to handle a command pushed into a txfifo
with regards to the dummy clock cycles. Not all controllers support generating
dummy clock cycles, meaning that not all do the dummy byte -> dummy clock cycle
conversion. The controllers that do not do this currently does not support
issuing commands requiring them towards m25p80 (as FAST_READ, DOR, QOR etc..).

Best regards,
Francisco Iglesias


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