On 2018-06-07 18:13, Miquel Raynal wrote:
Hi Abhishek,
On Mon, 28 May 2018 13:04:45 +0530, Abhishek Sahu
<absahu@xxxxxxxxxxxxxx> wrote:
On 2018-05-27 19:23, Miquel Raynal wrote:
> Hi Abhishek,
> > On Fri, 25 May 2018 17:51:43 +0530, Abhishek Sahu
> <absahu@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
<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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
