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.igles...@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.igles...@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.igles...@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.igles...@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.igles...@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...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > From: Bin Meng <bin.m...@windriver.com> > > > > > > > > > > > > > > > > > > > > > > > > SST flashes require a dummy byte after the address bits. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Bin Meng <bin.m...@windriver.com> > > > > > > > > > > > > > > > > > > > > > > I couldn't find a datasheet that says this... But the > > > > > > > > > > > actual code > > > > > > > > > > > change looks fine, so: > > > > > > > > > > > > > > > > > > > > > > Acked-by: Alistair Francis <alistair.fran...@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