Hi Abhishek,
On Wed, 25 Apr 2018 12:02:29 +0530, Abhishek Sahu
<absahu@xxxxxxxxxxxxxx> wrote:
> On 2018-04-23 12:28, Miquel Raynal wrote:
> > Hi Abhishek,
> > > On Mon, 23 Apr 2018 11:58:42 +0530, Abhishek Sahu
> > <absahu@xxxxxxxxxxxxxx> wrote:
> > >> On 2018-04-22 21:49, Miquel Raynal wrote:
> >> > Hi Abhishek,
> >> > > On Thu, 12 Apr 2018 12:36:42 +0530, Abhishek Sahu
> >> > <absahu@xxxxxxxxxxxxxx> wrote:
> >> > >> On 2018-04-10 15:14, Miquel Raynal wrote:
> >> >> > Hi Abhishek,
> >> >> > > On Wed, 4 Apr 2018 18:12:24 +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.
> >> >> > > I am not sure this is the right approach for subpage reads. If the
> >> >> > controller supports it, you should just enable it in chip->options.
> >> >> >
> >> >> Thanks Miquel.
> >> >> >> It is internal to this file only. This patch makes one static helper
> >> >> function which provides the support to read subpages.
> >> > > Drivers should expose raw helpers, why keeping this helper static then?
> >> > >> >> >> >> Signed-off-by: Abhishek Sahu <absahu@xxxxxxxxxxxxxx>
> >> >> >> ---
> >> >> >> drivers/mtd/nand/qcom_nandc.c | 186 >> +++++++++++++++++++++++++-----------------
> >> >> >> 1 file changed, 110 insertions(+), 76 deletions(-)
> >> >> >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c
> >> >> >> index 40c790e..f5d1fa4 100644
> >> >> >> --- a/drivers/mtd/nand/qcom_nandc.c
> >> >> >> +++ b/drivers/mtd/nand/qcom_nandc.c
> >> >> >> @@ -1590,6 +1590,114 @@ static int check_flash_errors(struct >> qcom_nand_host *host, int cw_cnt)
> >> >> >> }
> >> >> >> >> /*
> >> >> >> + * Helper to perform the page raw read operation. The read_cw_mask >> will be
> >> >> >> + * used to specify the codewords for which the data should be read. >> The
> >> >> >> + * single page contains multiple CW. Sometime, only few CW data is >> required
> >> >> >> + * in complete page. Also, start address will be determined with
> >> >> >> + * this CW mask to skip unnecessary data copy from NAND flash device. >> Then,
> >> >> >> + * actual data copy from NAND controller to data buffer will be done >> only
> >> >> >> + * for the CWs which have the mask set.
> >> >> >> + */
> >> >> >> +static int
> >> >> >> +nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> >> >> >> + u8 *data_buf, u8 *oob_buf,
> >> >> >> + int page, unsigned long read_cw_mask)
> >> >> >> +{
>
> <snip>
>
> >> >> >> - free_descs(nandc);
> >> >> >> -
> >> >> >> - if (!ret)
> >> >> >> - ret = check_flash_errors(host, ecc->steps);
> >> >> >> -
> >> >> >> - return 0;
> >> >> >> + return nandc_read_page_raw(mtd, chip, buf, chip->oob_poi, page,
> >> >> >> + BIT(chip->ecc.steps) - 1);
> >> >> > > I don't understand this. chip->ecc.steps is constant, right? So you
> >> >> > always ask for the same subpage?
> >> >> >> We need to do raw read for subpages in which we got uncorrectable
> >> >> error in next patch for erased page bitflip detection. This patch >> does
> >> >> reorganization of raw read and moves common code in helper function
> >> >> nandc_read_page_raw.
> >> >> >> For nomral raw read, all the subpages will be read so
> >> >> BIT(chip->ecc.steps) - 1 is used for qcom_nandc_read_page_raw.
> >> >> >> While for erased page raw read, only those sub pages will be
> >> >> read for which the controller gives the uncorrectable error.
> >> > > Still not okay: the driver should expose a way to do raw reads no
> >> > matter the length and the start and you should use that in a generic
> >> > way.
> >> >
> >> Thanks Miquel.
> >> I will explore regarding that.
> >> Looks like, we can some helper like read_subpage.
> > > Of course, when you implement raw accessors you can have static helpers
> > to clarify the code.
> >
> Hi Miquel,
>
> I checked regarding generic function.
>
> Normally the other NAND controller stores the data in main area
> and ecc bytes in oob area. So if page size is 2048+64 then 2048
> data bytes will go in main area followed by ECC bytes
>
> Main | OOB
> ------------------------------------
> D1 D2...........D2048|E1..........E64
>
> The QCOM nand contoller follows different layout in which this
> 2048+64 is internally divided in 528 bytes codeword so it will
> come to 4 codewords. Each codeword contains 516 bytes followed
> by ECC parity bytes. Also in at 0x1f0 offset in each CW, the bad
> block marker is stored. so each CW contains badblock marker also.
> The avilable OOB bytes are 16 which is also protected by ECC.
>
> 516 * 4 = 2048 data bytes + 16 available oob bytes.
>
> So for QCOM layout, it will be something lile
>
>
> 0 D1.........D495 B1 D496....D516 E1...E7 U1..U4
> 0x210 D517......D1012 B2 D1016..D1032 E8...E14 U5..U8
> 0x420 D1033.....D1528 B3 D1529..D1548 E15..E21 U9..U12
> 0x630 D1549.....D2044 B4 D2045..D2048 O1...O16 E22 E28 U13..U16
>
> Where D - Data bytes
> B - BBM bytes
> E - ECC parity bytes
> U - Unused bytes
> O - Avilable OOB bytes
>
> Now, the raw read expect data to be returned in buffer in the
> form of
>
> D1.....D2048 B1 E1...E7 U1..U4 B2 E8...E14 U5..U8 B3 E15..E21
> U9..U12 B4 O1...O16 E22 E28 U13..U16
>
> Now, the puporose of that helper function is read selected
> codewords. This codeword is QCOM specific and other controller
> normally doesn't follow this kind of layout.
>
> Now If we make generic fuction for subpgage raw read with any
> start and length then that function we can't use for erased page
> bitflip detection.
>
> If we give the start as 0 with length 528 then our requirement is
> to get 516 data bytes in data buffer and 12 bytes in OOB buffer
> but according to standard operations, it will be interpreted as
> all data bytes from D1..D528
>
> so that helper function is to read selected Codewords from
> QCOM NAND controller. This Codeword is different from subpages
Thanks for the clarification, I understand now. Maybe you can add a
comment about this particular layout and why you need this helper to
check for erased pages.
BTW are these BBM preserved somehow?
Regards,
Miquèl
>
> Regarding more info for layout, you can refer comment in
> https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/qcom_nandc.c#L2191
>
> Thanks,
> Abhishek
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
--
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
