qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Francisco Iglesias
Subject: Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes
Date: Tue, 15 Dec 2020 17:40:52 +0100
User-agent: Mutt/1.10.1 (2018-07-13)

Hello Bin,

On [2020 Dec 12] Sat 17:44:27, Bin Meng wrote:
> Hi Francisco,
> 
> On Sat, Dec 12, 2020 at 5:24 PM Francisco Iglesias
> <frasse.iglesias@gmail.com> wrote:
> >
> > 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..).
> 
> No, I don't think inspecting tx fifo to decode the SPI command, and to
> insert dummy cycles when seeing FAST_READ is the correct way for these
> SPI controllers like imx_spi. The real hardware does not do this and
> we should not make them behave like xilinx_spips.

Above is not correct, the xilinx_spips does not insert dummy clock cycles, it
converts the dummy bytes in the txfifo into the correct amount of dummy clock
cycles needed to be generated for the dummy byte based on the command and state
of the controller. For example if the command (as DOR) uses 2 lines when
transmitting the dummy byte it will issue 4 dummy clock cycles, if the command
uses 4 lines (example QIOR) it converts the dummy bytes into 2 dummy clock
cycles each.

How the hardware really works and how QEMU models it is not necessarly the
same, for the FAST_READ command the hardware will generate 8 dummy cycles of
the dummy byte (probably by shifting out the byte), currently the only way to
model this in QEMU is by making the controller convert the dummy
byte into 8 byte writes into m25p80 (there's no command in m25p80 modeling
this differently). If you would like to change this I think it is better if you
post a patch series demonstrating a solution that is better and also solves all
problems currently solved by the current one. Example problems (but not all)
are the ones mentioned above.

Thanks,
Francisco Iglesias


> 
> Regards,
> Bin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]