lkml.org 
[lkml]   [2018]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 8/9] mtd: nand: qcom: helper function for raw read
    Hi Abhishek,

    On Wed, 25 Apr 2018 12:02:29 +0530, Abhishek Sahu
    <absahu@codeaurora.org> wrote:

    > On 2018-04-23 12:28, Miquel Raynal wrote:
    > > Hi Abhishek,
    > > > On Mon, 23 Apr 2018 11:58:42 +0530, Abhishek Sahu
    > > <absahu@codeaurora.org> wrote:
    > > >> On 2018-04-22 21:49, Miquel Raynal wrote:
    > >> > Hi Abhishek,
    > >> > > On Thu, 12 Apr 2018 12:36:42 +0530, Abhishek Sahu
    > >> > <absahu@codeaurora.org> wrote:
    > >> > >> On 2018-04-10 15:14, Miquel Raynal wrote:
    > >> >> > Hi Abhishek,
    > >> >> > > On Wed, 4 Apr 2018 18:12:24 +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.
    > >> >> > > 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@codeaurora.org>
    > >> >> >> ---
    > >> >> >> 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

    \
     
     \ /
      Last update: 2018-04-25 15:00    [W:3.132 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site