Re: [PATCH v3 15/16] mtd: rawnand: qcom: helper function for raw read

[Abhishek Sahu <absahu@codeaurora.org>]

On 2018-06-07 18:13, Miquel Raynal wrote:
> Hi Abhishek,
> 
> On Mon, 28 May 2018 13:04:45 +0530, Abhishek Sahu
> <absahu@codeaurora.org> wrote:
> 
>> On 2018-05-27 19:23, Miquel Raynal wrote:
>> > Hi Abhishek,
>> > > On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
>> > <absahu@codeaurora.org> wrote:
>> > >> This patch does minor code reorganization for raw reads.
>> >> Currently the raw read is required for complete page but for
>> >> subsequent patches related with erased codeword bit flips
>> >> detection, only few CW should be read. So, this patch adds
>> >> helper function and introduces the read CW bitmask which
>> >> specifies which CW reads are required in complete page.
>> >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> >> ---

  <snip>

>> >> +	for (i = start_step; i < last_step; i++) {
>> > > This comment applies for both patches 15 and 16:
>> > > I would really prefer having a qcom_nandc_read_cw_raw() that reads only
>> > one CW. From qcom_nandc_read_page_raw() you would loop over all the CW
>> > calling qcom_nandc_read_cw_raw() helper (it's raw reads, we don't care
>> > about performances)
>> 
>>   Doing that way will degrade performances hugely.
>> 
>>   Currently once we formed the descriptor, the DMA will take care
>>   of complete page data transfer from NAND device to buffer and will
>>   generate single interrupt.
>> 
>>   Now it will form one CW descriptor and wait for it to be finished.
>>   In background, the data transfer from NAND device will be also
>>   split and for every CW, it will give the PAGE_READ command again,
>>   which is again time consuming.
>> 
>>   Data transfer degradation is ok but it will increase CPU time
>>   and number of interrupts which will impact other peripherals
>>   performance that time.
>> 
>>   Most of the NAND parts has 4K page size i.e 8 CWs.
>> 
>> > and from ->read_page_raw() you would check
>> > CW with uncorrectable errors for being blank with that helper. You
>> > would avoid the not-so-nice logic where you read all the CW between the
>> > first bad one and the last bad one.
>> >
>>   The reading b/w first CW and last CW is only from NAND device to 
>> NAND
>>   HW buffers. The NAND controller has 2 HW buffers which is used to
>>   optimize the traffic throughput between the NAND device and
>>   system memory,in both directions. Each buffer is 544B in size: 512B
>>   for data + 32B spare bytes. Throughput optimization is achieved by
>>   executing internal data transfers (i.e. between NANDc buffers and
>>   system memory) simultaneously with NAND device operations.
>> 
>>   Making separate function won't help in improving performance for
>>   this case either since once every thing is set for reading page
>>   (descriptor formation, issue the PAGE_READ, Data transfer from
>>   Flash array to data register in NAND device), the read time from
>>   device to NAND HW buffer is very less. Again, we did optimization
>>   in which the copying from NAND HW buffer to actual buffer is being
>>   done only for those CW's only.
>> 
>>   Again, in this case CPU time will be more.
>> 
> 
> 
> I understand the point and thanks for detailing it. But raw access
> happen either during debug (we don't care about CPU time) or when there
> is an uncorrectable error, which is very unlikely to happen very often
> when using eg. UBI/UBIFS. So I'm still convinced it is better to have a
> _simple_ and straightforward code for this path than something way
> harder to understand and much faster.
> 
> You can add a comment to explain what would be the fastest way and
> why though.
> 

  Thanks Miquel. I will do the changes to make function for
  single codeword raw read.

  Regards,
  Abhishek