lkml.org 
[lkml]   [2019]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
    Hi Mason,

    masonccyang@mxic.com.tw wrote on Fri, 10 May 2019 16:51:20 +0800:

    > Hi Miquel,
    >
    >
    > > > Add a driver for Macronix NAND read retry.
    > >
    > > "Add support for Macronix NAND read retry."? This is not a "new driver".
    > >
    > > >
    > > > Macronix NAND supports specfical read for data recovery and enabled
    > >
    > >
    > > Macronix NANDs support specific read operation for data recovery,
    > > which can be enabled with a SET_FEATURE.
    > >
    > > > Driver check byte 167 of Vendor Blocks in ONFI parameter page table
    > >
    > > checks
    > >
    > > > to see if this high reliability function is support or not.
    > >
    > > high-reliability function? not sure it is English
    > > anyway.
    > >
    > > supported
    > >
    >
    > okay, thanks for your review, will patch it to:
    > ------------------------------------------------------------------------
    > Add support for Macronix NAND read retry.
    >
    > Macronix NANDs support specific read operation for data recovery,
    > which can be enabled with a SET_FEATURE.
    > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
    > to see if this high-reliability function is supported.
    > -------------------------------------------------------------------------

    Fine by me.

    [...]

    > > > + int ret;
    > > > +
    > > > + if (mode > MACRONIX_READ_RETRY_MODE)
    > > > + mode = MACRONIX_READ_RETRY_MODE;
    > > > +
    > > > + feature[0] = mode;
    > > > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY,
    > feature);
    > >
    > > Don't you miss to select/deselect the target?
    >
    > nand_select_target() and nand_deselect_target() are already in
    > nand_do_read_ops().

    Right

    >
    > >
    > > > + if (ret)
    > > > + pr_err("set feature failed to read retry moded:%d\n", mode);
    > >
    > > "Failed to set read retry mode: %d\n"
    > >
    > > I think you can abort the operation with a negative return code in this
    > > case.
    > >
    >
    > After set feature operation, this NAND device need a get feature command

    You need to add a comment for this.

    > or
    > SW reset command to exit read retry mode.
    > Therefore, set features command followed by get feature command is needed.

    In this case you must check first that both set and get are supported.

    >
    > > > +
    > > > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY,
    > feature);
    > >
    > > If the operation succeeded but the controller cannot get the feature
    > > you don't want to abort the operation. You should check if get_features
    > > is supported, if yes you can rely on the below test.
    > >
    >
    > I thought it has been check in macronix_nand_onfi_init() and set by
    > set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);

    You only checked that the operation can be done, you cannot know in
    advance if it will actually succeed.

    >
    > right ?
    >
    > > > + if (ret || feature[0] != mode)
    > > > + pr_err("get feature failed to read retry moded:%d(%d)\n",
    > > > + mode, feature[0]);
    > >
    > > "Failed to verify read retry mode..."
    > >
    > > Also return something negative here.
    > >
    > > > +
    > > > + return ret;
    > >
    > > And if all went right, return 0 at the end.
    > >
    > > > +}
    > > > +
    > > > +static void macronix_nand_onfi_init(struct nand_chip *chip)
    > > > +{
    > > > + struct nand_parameters *p = &chip->parameters;
    > > > +
    > > > + if (p->onfi) {
    > > > + struct nand_onfi_vendor_macronix *mxic =
    > > > + (void *)p->onfi->vendor;
    > >
    > > Please put everything on the same line
    >
    > It will over 80 char.

    I know, that's fine here.

    >
    > >
    > > > +
    > > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
    > > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
    > >
    > > Why +1 here, I am missing something?
    >
    >
    > Without + 1, read retry mode is up to 4 rather than 5.
    > But this NAND device support read retry mode up to 5.
    >

    If there are 5 modes, you need to set 5 to chip->read_retries, not 6.

    If only 4 modes are used, there is probably a bug in the core that
    must be fixed, please do not workaround it in the driver!


    Thanks,
    Miquèl

    \
     
     \ /
      Last update: 2019-05-10 11:13    [W:2.859 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site