Re: [PATCH v4 7/8] mtd: spi-nor: Enhance locking to support reads while writes

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Tudor,

tudor.ambarus@linaro.org wrote on Mon, 27 Mar 2023 10:29:03 +0100:

> On 3/24/23 17:41, Miquel Raynal wrote:
> > Hi Tudor,
> >   
> 
> Hi!
> 
> > tudor.ambarus@linaro.org wrote on Fri, 17 Mar 2023 05:59:08 +0000:
> >   
> >> Hi, Miquel,
> >>
> >> I find the overall idea good.  
> > 
> > Thanks a lot for the detailed review!
> >   
> >> On 2/1/23 11:36, Miquel Raynal wrote:  
> >>> On devices featuring several banks, the Read While Write (RWW) feature
> >>> is here to improve the overall performance when performing parallel
> >>> reads and writes at different locations (different banks). The following
> >>> constraints have to be taken into account:
> >>> 1#: A single operation can be performed in a given bank.
> >>> 2#: Only a single program or erase operation can happen on the entire
> >>>     chip (common hardware limitation to limit costs)
> >>> 3#: Reads must remain serialized even though reads on different banks
> >>>     might occur at the same time.    
> >>
> >> 3# is unclear if one limits just at reading the commit message. Are the
> >> reads serialized per bank or per flash?  
> > 
> > Per flash.
> >   
> >> After reading the code, it looks like all the reads are serialized per
> >> flash regardless if it reads registers or memory. I assume you meant
> >> that crossing a bank boundary with a single read is fine.  
> > 
> > Yes, I will update that item to clarify.  
> 
> thanks!
> 
> >   
> >> But can you
> >> really read from bank 1 and bank 3 at the same time? The code doesn't
> >> take this into consideration.  
> > 
> > Yes this is taken into account and supported, a read can cross a bank
> > boundary.  
> 
> No, I meant that you can't do a read from bank 1 and while the first
> read is in progress, to start a second read from the 3rd bank and
> process both reads in parallel, reading from both banks at the same
> time. At least not with the current code, because you set
> rww.{ongoing_io, ongoing_rd} to true and the second read will wait.
> Cross boundary reads on successive banks should work with current code,
> yes. So what does the hw support?

Ok, sorry for the confusion. So, I think I remember a discussion where
I was told that this was not supported even though it would not be
extremely complex to support at a physical level ("just" by increasing
the current source). But IIRC right now this is not supported. Anyhow,
the main target of the RWW is to perform a read during a while, this is
very handy for performing eg. system updates besides reducing the
overall latency, but I don't think we want to bring even more
parallelism between reads. Actually the current implementation would
not work and a whole mtd I/O scheduler would be needed for that, which
is yet another task.


[...]

> >>> @@ -1087,7 +1157,81 @@ static void spi_nor_unprep(struct spi_nor *nor)
> >>>  		nor->controller_ops->unprepare(nor);
> >>>  }
> >>>  
> >>> +static void spi_nor_offset_to_banks(struct spi_nor *nor, loff_t start, size_t len,    
> >>
> >> pass directly the bank_size instead of the pointer to nor, you'll avoid
> >> the double dereference.  
> > 
> > Done
> >   
> >>  
> >>> +				    unsigned int *first, unsigned int *last)    
> >>
> >> unsigned long long *first, *last ?  
> > 
> > Actually I want these to remain unsigned int, the ULL suffix just mean
> > the input might be a 64-bit value, but it is quite common to treat the
> > output as 32-bit. Here we do not expect values greater than 4.  
> 
> Ok. Then maybe we should match how we define nbanks in NOR. Was it a u8?

Why not.

> 
> >   
> >>> +{
> >>> +	*first = DIV_ROUND_DOWN_ULL(start, nor->params->bank_size);
> >>> +	*last = DIV_ROUND_DOWN_ULL(start + len - 1, nor->params->bank_size);
> >>> +}
> >>> +

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel