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

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:

quoted

Hi Tudor,
  

Hi!

quoted

tudor.ambarus@linaro.org wrote on Fri, 17 Mar 2023 05:59:08 +0000:
  

quoted

Hi, Miquel,

I find the overall idea good.  

Thanks a lot for the detailed review!
  

quoted

On 2/1/23 11:36, Miquel Raynal wrote:  

quoted

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.
  

quoted

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!

quoted

quoted

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.


[...]

quoted

quoted

quoted

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

quoted

quoted

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

quoted

quoted

quoted

+{
+	*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