| Message ID | 20230628092937.538683-3-AVKrasnov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | support 512B ECC step size for Meson NAND | expand |
Hi Arseniy, AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > Meson NAND supports both 512B and 1024B ECC step size. > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > index 345212e8c691..6cc4f63b86c8 100644 > --- a/drivers/mtd/nand/raw/meson_nand.c > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { > struct meson_nand_ecc { > u32 bch; > u32 strength; > + u32 size; > }; > > struct meson_nfc_data { > @@ -190,7 +191,8 @@ struct meson_nfc { > }; > > enum { > - NFC_ECC_BCH8_1K = 2, > + NFC_ECC_BCH8_512 = 1, > + NFC_ECC_BCH8_1K, > NFC_ECC_BCH24_1K, > NFC_ECC_BCH30_1K, > NFC_ECC_BCH40_1K, > @@ -198,15 +200,16 @@ enum { > NFC_ECC_BCH60_1K, > }; > > -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > > static struct meson_nand_ecc meson_ecc[] = { > - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), > - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), > - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), > - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), > - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), > - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), > + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), > + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), > + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), > + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), > + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), > + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), > + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), > }; > > static int meson_nand_calc_ecc_bytes(int step_size, int strength) > @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) > > NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > - meson_nand_calc_ecc_bytes, 1024, 8); > + > +static const int axg_stepinfo_strengths[] = { 8 }; > +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > + .stepsize = 1024, > + .strengths = axg_stepinfo_strengths, > + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > +}; > + > +static const struct nand_ecc_step_info axg_stepinfo_512 = { > + .stepsize = 512, > + .strengths = axg_stepinfo_strengths, > + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > +}; > + > +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > + > +static const struct nand_ecc_caps meson_axg_ecc_caps = { > + .stepinfos = axg_stepinfo, > + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > +}; > > static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > { > @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) > return -EINVAL; > > for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > - if (meson_ecc[i].strength == nand->ecc.strength) { > + if (meson_ecc[i].strength == nand->ecc.strength && > + meson_ecc[i].size == nand->ecc.size) { > meson_chip->bch_mode = meson_ecc[i].bch; > return 0; > } > @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > struct meson_nfc *nfc = nand_get_controller_data(nand); > struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > struct mtd_info *mtd = nand_to_mtd(nand); > - int nsectors = mtd->writesize / 1024; > + int nsectors = mtd->writesize / 512; This cannot be unconditional, right? > int raw_writesize; > int ret; > Thanks, Miquèl
On 04.07.2023 11:36, Miquel Raynal wrote: > Hi Arseniy, > > AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > >> Meson NAND supports both 512B and 1024B ECC step size. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- >> 1 file changed, 35 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >> index 345212e8c691..6cc4f63b86c8 100644 >> --- a/drivers/mtd/nand/raw/meson_nand.c >> +++ b/drivers/mtd/nand/raw/meson_nand.c >> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { >> struct meson_nand_ecc { >> u32 bch; >> u32 strength; >> + u32 size; >> }; >> >> struct meson_nfc_data { >> @@ -190,7 +191,8 @@ struct meson_nfc { >> }; >> >> enum { >> - NFC_ECC_BCH8_1K = 2, >> + NFC_ECC_BCH8_512 = 1, >> + NFC_ECC_BCH8_1K, >> NFC_ECC_BCH24_1K, >> NFC_ECC_BCH30_1K, >> NFC_ECC_BCH40_1K, >> @@ -198,15 +200,16 @@ enum { >> NFC_ECC_BCH60_1K, >> }; >> >> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >> >> static struct meson_nand_ecc meson_ecc[] = { >> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), >> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), >> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), >> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), >> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), >> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), >> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), >> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), >> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), >> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), >> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), >> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), >> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), >> }; >> >> static int meson_nand_calc_ecc_bytes(int step_size, int strength) >> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) >> >> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, >> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); >> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, >> - meson_nand_calc_ecc_bytes, 1024, 8); >> + >> +static const int axg_stepinfo_strengths[] = { 8 }; >> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >> + .stepsize = 1024, >> + .strengths = axg_stepinfo_strengths, >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >> +}; >> + >> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >> + .stepsize = 512, >> + .strengths = axg_stepinfo_strengths, >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >> +}; >> + >> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >> + >> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >> + .stepinfos = axg_stepinfo, >> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >> +}; >> >> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) >> { >> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) >> return -EINVAL; >> >> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >> - if (meson_ecc[i].strength == nand->ecc.strength) { >> + if (meson_ecc[i].strength == nand->ecc.strength && >> + meson_ecc[i].size == nand->ecc.size) { >> meson_chip->bch_mode = meson_ecc[i].bch; >> return 0; >> } >> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >> struct meson_nfc *nfc = nand_get_controller_data(nand); >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >> struct mtd_info *mtd = nand_to_mtd(nand); >> - int nsectors = mtd->writesize / 1024; >> + int nsectors = mtd->writesize / 512; > > This cannot be unconditional, right? Hello Miquel! Yes, this code looks strange. 'nsectors' is used to calculate space in OOB that could be used by ECC engine (this value will be passed as 'oobavail' to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case for ECC, e.g. minimal number of bytes for ECC engine (and at the same time maximum number of free bytes). For Meson, if ECC step size is 512, then we have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free bytes in OOB). I think this code could be reworked in the following way: if ECC step size is already known here (from DTS), calculate 'nsectors' using given value (div by 512 for example). Otherwise calculate 'nsectors' in the current manner: int nsectors = mtd->writesize / 1024; Moreover 1024 is default ECC step size for this driver, so default behaviour will be preserved. Thanks, Arseniy > >> int raw_writesize; >> int ret; >> > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > On 04.07.2023 11:36, Miquel Raynal wrote: > > Hi Arseniy, > > > > AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > > > >> Meson NAND supports both 512B and 1024B ECC step size. > >> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- > >> 1 file changed, 35 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >> index 345212e8c691..6cc4f63b86c8 100644 > >> --- a/drivers/mtd/nand/raw/meson_nand.c > >> +++ b/drivers/mtd/nand/raw/meson_nand.c > >> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { > >> struct meson_nand_ecc { > >> u32 bch; > >> u32 strength; > >> + u32 size; > >> }; > >> > >> struct meson_nfc_data { > >> @@ -190,7 +191,8 @@ struct meson_nfc { > >> }; > >> > >> enum { > >> - NFC_ECC_BCH8_1K = 2, > >> + NFC_ECC_BCH8_512 = 1, > >> + NFC_ECC_BCH8_1K, > >> NFC_ECC_BCH24_1K, > >> NFC_ECC_BCH30_1K, > >> NFC_ECC_BCH40_1K, > >> @@ -198,15 +200,16 @@ enum { > >> NFC_ECC_BCH60_1K, > >> }; > >> > >> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > >> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > >> > >> static struct meson_nand_ecc meson_ecc[] = { > >> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), > >> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), > >> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), > >> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), > >> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), > >> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), > >> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), > >> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), > >> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), > >> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), > >> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), > >> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), > >> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), > >> }; > >> > >> static int meson_nand_calc_ecc_bytes(int step_size, int strength) > >> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) > >> > >> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > >> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > >> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > >> - meson_nand_calc_ecc_bytes, 1024, 8); > >> + > >> +static const int axg_stepinfo_strengths[] = { 8 }; > >> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > >> + .stepsize = 1024, > >> + .strengths = axg_stepinfo_strengths, > >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >> +}; > >> + > >> +static const struct nand_ecc_step_info axg_stepinfo_512 = { > >> + .stepsize = 512, > >> + .strengths = axg_stepinfo_strengths, > >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >> +}; > >> + > >> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > >> + > >> +static const struct nand_ecc_caps meson_axg_ecc_caps = { > >> + .stepinfos = axg_stepinfo, > >> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > >> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > >> +}; > >> > >> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > >> { > >> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) > >> return -EINVAL; > >> > >> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > >> - if (meson_ecc[i].strength == nand->ecc.strength) { > >> + if (meson_ecc[i].strength == nand->ecc.strength && > >> + meson_ecc[i].size == nand->ecc.size) { > >> meson_chip->bch_mode = meson_ecc[i].bch; > >> return 0; > >> } > >> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >> struct meson_nfc *nfc = nand_get_controller_data(nand); > >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >> struct mtd_info *mtd = nand_to_mtd(nand); > >> - int nsectors = mtd->writesize / 1024; > >> + int nsectors = mtd->writesize / 512; > > > > This cannot be unconditional, right? > > Hello Miquel! > > Yes, this code looks strange. 'nsectors' is used to calculate space in OOB > that could be used by ECC engine (this value will be passed as 'oobavail' > to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case > for ECC, e.g. minimal number of bytes for ECC engine (and at the same time > maximum number of free bytes). For Meson, if ECC step size is 512, then we > have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free > bytes in OOB). > > I think this code could be reworked in the following way: > > if ECC step size is already known here (from DTS), calculate 'nsectors' using > given value (div by 512 for example). Otherwise calculate 'nsectors' in the > current manner: It will always be known when these function are run. There is no guessing here. > > int nsectors = mtd->writesize / 1024; > > Moreover 1024 is default ECC step size for this driver, so default behaviour > will be preserved. Yes, otherwise you would break existing users. > > Thanks, Arseniy > > > > >> int raw_writesize; > >> int ret; > >> > > > > > > Thanks, > > Miquèl Thanks, Miquèl
On 04.07.2023 12:41, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > >> On 04.07.2023 11:36, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: >>> >>>> Meson NAND supports both 512B and 1024B ECC step size. >>>> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- >>>> 1 file changed, 35 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>> index 345212e8c691..6cc4f63b86c8 100644 >>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { >>>> struct meson_nand_ecc { >>>> u32 bch; >>>> u32 strength; >>>> + u32 size; >>>> }; >>>> >>>> struct meson_nfc_data { >>>> @@ -190,7 +191,8 @@ struct meson_nfc { >>>> }; >>>> >>>> enum { >>>> - NFC_ECC_BCH8_1K = 2, >>>> + NFC_ECC_BCH8_512 = 1, >>>> + NFC_ECC_BCH8_1K, >>>> NFC_ECC_BCH24_1K, >>>> NFC_ECC_BCH30_1K, >>>> NFC_ECC_BCH40_1K, >>>> @@ -198,15 +200,16 @@ enum { >>>> NFC_ECC_BCH60_1K, >>>> }; >>>> >>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >>>> >>>> static struct meson_nand_ecc meson_ecc[] = { >>>> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), >>>> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), >>>> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), >>>> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), >>>> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), >>>> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), >>>> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), >>>> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), >>>> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), >>>> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), >>>> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), >>>> }; >>>> >>>> static int meson_nand_calc_ecc_bytes(int step_size, int strength) >>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) >>>> >>>> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, >>>> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); >>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, >>>> - meson_nand_calc_ecc_bytes, 1024, 8); >>>> + >>>> +static const int axg_stepinfo_strengths[] = { 8 }; >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >>>> + .stepsize = 1024, >>>> + .strengths = axg_stepinfo_strengths, >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>> +}; >>>> + >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >>>> + .stepsize = 512, >>>> + .strengths = axg_stepinfo_strengths, >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>> +}; >>>> + >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >>>> + >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >>>> + .stepinfos = axg_stepinfo, >>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >>>> +}; >>>> >>>> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) >>>> { >>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) >>>> return -EINVAL; >>>> >>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >>>> - if (meson_ecc[i].strength == nand->ecc.strength) { >>>> + if (meson_ecc[i].strength == nand->ecc.strength && >>>> + meson_ecc[i].size == nand->ecc.size) { >>>> meson_chip->bch_mode = meson_ecc[i].bch; >>>> return 0; >>>> } >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>> struct meson_nfc *nfc = nand_get_controller_data(nand); >>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>> - int nsectors = mtd->writesize / 1024; >>>> + int nsectors = mtd->writesize / 512; >>> >>> This cannot be unconditional, right? >> >> Hello Miquel! >> >> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB >> that could be used by ECC engine (this value will be passed as 'oobavail' >> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case >> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time >> maximum number of free bytes). For Meson, if ECC step size is 512, then we >> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free >> bytes in OOB). >> >> I think this code could be reworked in the following way: >> >> if ECC step size is already known here (from DTS), calculate 'nsectors' using >> given value (div by 512 for example). Otherwise calculate 'nsectors' in the >> current manner: > > It will always be known when these function are run. There is no > guessing here. Hm I checked, that but if step size is not set in DTS, here it will be 0, then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' and 'oobavail'... Anyway, I'll do the following thing: int nsectors; if (nand->ecc.size) nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC else nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC Thanks, Arseniy > >> >> int nsectors = mtd->writesize / 1024; >> >> Moreover 1024 is default ECC step size for this driver, so default behaviour >> will be preserved. > > Yes, otherwise you would break existing users. > >> >> Thanks, Arseniy >> >>> >>>> int raw_writesize; >>>> int ret; >>>> >>> >>> >>> Thanks, >>> Miquèl > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300: > On 04.07.2023 12:41, Miquel Raynal wrote: > > Hi Arseniy, > > > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > > > >> On 04.07.2023 11:36, Miquel Raynal wrote: > >>> Hi Arseniy, > >>> > >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > >>> > >>>> Meson NAND supports both 512B and 1024B ECC step size. > >>>> > >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >>>> --- > >>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- > >>>> 1 file changed, 35 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > >>>> index 345212e8c691..6cc4f63b86c8 100644 > >>>> --- a/drivers/mtd/nand/raw/meson_nand.c > >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c > >>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { > >>>> struct meson_nand_ecc { > >>>> u32 bch; > >>>> u32 strength; > >>>> + u32 size; > >>>> }; > >>>> > >>>> struct meson_nfc_data { > >>>> @@ -190,7 +191,8 @@ struct meson_nfc { > >>>> }; > >>>> > >>>> enum { > >>>> - NFC_ECC_BCH8_1K = 2, > >>>> + NFC_ECC_BCH8_512 = 1, > >>>> + NFC_ECC_BCH8_1K, > >>>> NFC_ECC_BCH24_1K, > >>>> NFC_ECC_BCH30_1K, > >>>> NFC_ECC_BCH40_1K, > >>>> @@ -198,15 +200,16 @@ enum { > >>>> NFC_ECC_BCH60_1K, > >>>> }; > >>>> > >>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > >>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > >>>> > >>>> static struct meson_nand_ecc meson_ecc[] = { > >>>> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), > >>>> }; > >>>> > >>>> static int meson_nand_calc_ecc_bytes(int step_size, int strength) > >>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) > >>>> > >>>> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > >>>> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > >>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > >>>> - meson_nand_calc_ecc_bytes, 1024, 8); > >>>> + > >>>> +static const int axg_stepinfo_strengths[] = { 8 }; > >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > >>>> + .stepsize = 1024, > >>>> + .strengths = axg_stepinfo_strengths, > >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { > >>>> + .stepsize = 512, > >>>> + .strengths = axg_stepinfo_strengths, > >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > >>>> + > >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { > >>>> + .stepinfos = axg_stepinfo, > >>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > >>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > >>>> +}; > >>>> > >>>> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > >>>> { > >>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) > >>>> return -EINVAL; > >>>> > >>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > >>>> - if (meson_ecc[i].strength == nand->ecc.strength) { > >>>> + if (meson_ecc[i].strength == nand->ecc.strength && > >>>> + meson_ecc[i].size == nand->ecc.size) { > >>>> meson_chip->bch_mode = meson_ecc[i].bch; > >>>> return 0; > >>>> } > >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >>>> struct meson_nfc *nfc = nand_get_controller_data(nand); > >>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >>>> struct mtd_info *mtd = nand_to_mtd(nand); > >>>> - int nsectors = mtd->writesize / 1024; > >>>> + int nsectors = mtd->writesize / 512; > >>> > >>> This cannot be unconditional, right? > >> > >> Hello Miquel! > >> > >> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB > >> that could be used by ECC engine (this value will be passed as 'oobavail' > >> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case > >> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time > >> maximum number of free bytes). For Meson, if ECC step size is 512, then we > >> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free > >> bytes in OOB). > >> > >> I think this code could be reworked in the following way: > >> > >> if ECC step size is already known here (from DTS), calculate 'nsectors' using > >> given value (div by 512 for example). Otherwise calculate 'nsectors' in the > >> current manner: > > > > It will always be known when these function are run. There is no > > guessing here. > > Hm I checked, that but if step size is not set in DTS, here it will be 0, > then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' > and 'oobavail'... > > Anyway, I'll do the following thing: > > int nsectors; > > if (nand->ecc.size) > nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC You should set nand->ecc.size in ->attach_chip() instead. > else > nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC > > Thanks, Arseniy > > > > >> > >> int nsectors = mtd->writesize / 1024; > >> > >> Moreover 1024 is default ECC step size for this driver, so default behaviour > >> will be preserved. > > > > Yes, otherwise you would break existing users. > > > >> > >> Thanks, Arseniy > >> > >>> > >>>> int raw_writesize; > >>>> int ret; > >>>> > >>> > >>> > >>> Thanks, > >>> Miquèl > > > > > > Thanks, > > Miquèl Thanks, Miquèl
On 04.07.2023 12:56, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300: > >> On 04.07.2023 12:41, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: >>> >>>> On 04.07.2023 11:36, Miquel Raynal wrote: >>>>> Hi Arseniy, >>>>> >>>>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: >>>>> >>>>>> Meson NAND supports both 512B and 1024B ECC step size. >>>>>> >>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>> --- >>>>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- >>>>>> 1 file changed, 35 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>>>> index 345212e8c691..6cc4f63b86c8 100644 >>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { >>>>>> struct meson_nand_ecc { >>>>>> u32 bch; >>>>>> u32 strength; >>>>>> + u32 size; >>>>>> }; >>>>>> >>>>>> struct meson_nfc_data { >>>>>> @@ -190,7 +191,8 @@ struct meson_nfc { >>>>>> }; >>>>>> >>>>>> enum { >>>>>> - NFC_ECC_BCH8_1K = 2, >>>>>> + NFC_ECC_BCH8_512 = 1, >>>>>> + NFC_ECC_BCH8_1K, >>>>>> NFC_ECC_BCH24_1K, >>>>>> NFC_ECC_BCH30_1K, >>>>>> NFC_ECC_BCH40_1K, >>>>>> @@ -198,15 +200,16 @@ enum { >>>>>> NFC_ECC_BCH60_1K, >>>>>> }; >>>>>> >>>>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >>>>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >>>>>> >>>>>> static struct meson_nand_ecc meson_ecc[] = { >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), >>>>>> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), >>>>>> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), >>>>>> }; >>>>>> >>>>>> static int meson_nand_calc_ecc_bytes(int step_size, int strength) >>>>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) >>>>>> >>>>>> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, >>>>>> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); >>>>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, >>>>>> - meson_nand_calc_ecc_bytes, 1024, 8); >>>>>> + >>>>>> +static const int axg_stepinfo_strengths[] = { 8 }; >>>>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >>>>>> + .stepsize = 1024, >>>>>> + .strengths = axg_stepinfo_strengths, >>>>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>>>> +}; >>>>>> + >>>>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >>>>>> + .stepsize = 512, >>>>>> + .strengths = axg_stepinfo_strengths, >>>>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>>>> +}; >>>>>> + >>>>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >>>>>> + >>>>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >>>>>> + .stepinfos = axg_stepinfo, >>>>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >>>>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >>>>>> +}; >>>>>> >>>>>> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) >>>>>> { >>>>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) >>>>>> return -EINVAL; >>>>>> >>>>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >>>>>> - if (meson_ecc[i].strength == nand->ecc.strength) { >>>>>> + if (meson_ecc[i].strength == nand->ecc.strength && >>>>>> + meson_ecc[i].size == nand->ecc.size) { >>>>>> meson_chip->bch_mode = meson_ecc[i].bch; >>>>>> return 0; >>>>>> } >>>>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>>>> struct meson_nfc *nfc = nand_get_controller_data(nand); >>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>>> - int nsectors = mtd->writesize / 1024; >>>>>> + int nsectors = mtd->writesize / 512; >>>>> >>>>> This cannot be unconditional, right? >>>> >>>> Hello Miquel! >>>> >>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB >>>> that could be used by ECC engine (this value will be passed as 'oobavail' >>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case >>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time >>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we >>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free >>>> bytes in OOB). >>>> >>>> I think this code could be reworked in the following way: >>>> >>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using >>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the >>>> current manner: >>> >>> It will always be known when these function are run. There is no >>> guessing here. >> >> Hm I checked, that but if step size is not set in DTS, here it will be 0, >> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' >> and 'oobavail'... >> >> Anyway, I'll do the following thing: >> >> int nsectors; >> >> if (nand->ecc.size) >> nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC > > You should set nand->ecc.size in ->attach_chip() instead. Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach callback it will be already known (DT part was processed in 'rawnand_dt_init()'). If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it according provided ecc caps. What do You mean for "You should set ..." ? Thanks, Arseniy > >> else >> nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC >> >> Thanks, Arseniy >> >>> >>>> >>>> int nsectors = mtd->writesize / 1024; >>>> >>>> Moreover 1024 is default ECC step size for this driver, so default behaviour >>>> will be preserved. >>> >>> Yes, otherwise you would break existing users. >>> >>>> >>>> Thanks, Arseniy >>>> >>>>> >>>>>> int raw_writesize; >>>>>> int ret; >>>>>> >>>>> >>>>> >>>>> Thanks, >>>>> Miquèl >>> >>> >>> Thanks, >>> Miquèl > > > Thanks, > Miquèl
Hi Arseniy, > >>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB > >>>> that could be used by ECC engine (this value will be passed as 'oobavail' > >>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case > >>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time > >>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we > >>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free > >>>> bytes in OOB). > >>>> > >>>> I think this code could be reworked in the following way: > >>>> > >>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using > >>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the > >>>> current manner: > >>> > >>> It will always be known when these function are run. There is no > >>> guessing here. > >> > >> Hm I checked, that but if step size is not set in DTS, here it will be 0, > >> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' > >> and 'oobavail'... > >> > >> Anyway, I'll do the following thing: > >> > >> int nsectors; > >> > >> if (nand->ecc.size) > >> nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC > > > > You should set nand->ecc.size in ->attach_chip() instead. > > Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach > callback it will be already known (DT part was processed in 'rawnand_dt_init()'). > If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it > according provided ecc caps. What do You mean for "You should set ..." ? The current approach is wrong, it decides the number of ECC chunks (called nsectors in the driver) and then asks the core to decide the number of ECC chunks to use. Just provide mtd->oobsize - 2 as last parameter and then rely on the core's logic to find the right ECC step-size/strength? There is no point in requesting a particular step size without a specific strength, or? So I believe you should provide both in the DTS if you want particular parameters to be applied, otherwise you can let the core decide what is best. Thanks, Miquèl
On 04.07.2023 16:41, Miquel Raynal wrote: > Hi Arseniy, > >>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB >>>>>> that could be used by ECC engine (this value will be passed as 'oobavail' >>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case >>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time >>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we >>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free >>>>>> bytes in OOB). >>>>>> >>>>>> I think this code could be reworked in the following way: >>>>>> >>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using >>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the >>>>>> current manner: >>>>> >>>>> It will always be known when these function are run. There is no >>>>> guessing here. >>>> >>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, >>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' >>>> and 'oobavail'... >>>> >>>> Anyway, I'll do the following thing: >>>> >>>> int nsectors; >>>> >>>> if (nand->ecc.size) >>>> nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC >>> >>> You should set nand->ecc.size in ->attach_chip() instead. >> >> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach >> callback it will be already known (DT part was processed in 'rawnand_dt_init()'). >> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it >> according provided ecc caps. What do You mean for "You should set ..." ? > > The current approach is wrong, it decides the number of ECC chunks > (called nsectors in the driver) and then asks the core to decide the > number of ECC chunks to use. Yes! I was also confused about that. > > Just provide mtd->oobsize - 2 as last parameter and then rely on the > core's logic to find the right ECC step-size/strength? > > There is no point in requesting a particular step size without a > specific strength, or? So I believe you should provide both in the DTS > if you want particular parameters to be applied, otherwise you can let > the core decide what is best. So I think this could be a separated patch as it doesn't rely on 512 step size ECC support for Meson and may be it should be "Fix" tagged. Thanks, Arseniy > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 18:07:04 +0300: > On 04.07.2023 16:41, Miquel Raynal wrote: > > Hi Arseniy, > > > >>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB > >>>>>> that could be used by ECC engine (this value will be passed as 'oobavail' > >>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case > >>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time > >>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we > >>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free > >>>>>> bytes in OOB). > >>>>>> > >>>>>> I think this code could be reworked in the following way: > >>>>>> > >>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using > >>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the > >>>>>> current manner: > >>>>> > >>>>> It will always be known when these function are run. There is no > >>>>> guessing here. > >>>> > >>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, > >>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' > >>>> and 'oobavail'... > >>>> > >>>> Anyway, I'll do the following thing: > >>>> > >>>> int nsectors; > >>>> > >>>> if (nand->ecc.size) > >>>> nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC > >>> > >>> You should set nand->ecc.size in ->attach_chip() instead. > >> > >> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach > >> callback it will be already known (DT part was processed in 'rawnand_dt_init()'). > >> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it > >> according provided ecc caps. What do You mean for "You should set ..." ? > > > > The current approach is wrong, it decides the number of ECC chunks > > (called nsectors in the driver) and then asks the core to decide the > > number of ECC chunks to use. > > Yes! I was also confused about that. > > > > > Just provide mtd->oobsize - 2 as last parameter and then rely on the > > core's logic to find the right ECC step-size/strength? > > > > There is no point in requesting a particular step size without a > > specific strength, or? So I believe you should provide both in the DTS > > if you want particular parameters to be applied, otherwise you can let > > the core decide what is best. > > So I think this could be a separated patch as it doesn't rely on 512 step size ECC > support for Meson and may be it should be "Fix" tagged. Yup! Thanks for cleaning so thoroughly this driver :) Cheers, Miquèl
On 04.07.2023 18:32, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 18:07:04 +0300: > >> On 04.07.2023 16:41, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>>>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB >>>>>>>> that could be used by ECC engine (this value will be passed as 'oobavail' >>>>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case >>>>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time >>>>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we >>>>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free >>>>>>>> bytes in OOB). >>>>>>>> >>>>>>>> I think this code could be reworked in the following way: >>>>>>>> >>>>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using >>>>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the >>>>>>>> current manner: >>>>>>> >>>>>>> It will always be known when these function are run. There is no >>>>>>> guessing here. >>>>>> >>>>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, >>>>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps' >>>>>> and 'oobavail'... >>>>>> >>>>>> Anyway, I'll do the following thing: >>>>>> >>>>>> int nsectors; >>>>>> >>>>>> if (nand->ecc.size) >>>>>> nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC >>>>> >>>>> You should set nand->ecc.size in ->attach_chip() instead. >>>> >>>> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach >>>> callback it will be already known (DT part was processed in 'rawnand_dt_init()'). >>>> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it >>>> according provided ecc caps. What do You mean for "You should set ..." ? >>> >>> The current approach is wrong, it decides the number of ECC chunks >>> (called nsectors in the driver) and then asks the core to decide the >>> number of ECC chunks to use. >> >> Yes! I was also confused about that. >> >>> >>> Just provide mtd->oobsize - 2 as last parameter and then rely on the >>> core's logic to find the right ECC step-size/strength? >>> >>> There is no point in requesting a particular step size without a >>> specific strength, or? So I believe you should provide both in the DTS >>> if you want particular parameters to be applied, otherwise you can let >>> the core decide what is best. >> >> So I think this could be a separated patch as it doesn't rely on 512 step size ECC >> support for Meson and may be it should be "Fix" tagged. > > Yup! Thanks for cleaning so thoroughly this driver :) Thanks again for review and details! :) Thanks, Arseniy > > Cheers, > Miquèl
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index 345212e8c691..6cc4f63b86c8 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { struct meson_nand_ecc { u32 bch; u32 strength; + u32 size; }; struct meson_nfc_data { @@ -190,7 +191,8 @@ struct meson_nfc { }; enum { - NFC_ECC_BCH8_1K = 2, + NFC_ECC_BCH8_512 = 1, + NFC_ECC_BCH8_1K, NFC_ECC_BCH24_1K, NFC_ECC_BCH30_1K, NFC_ECC_BCH40_1K, @@ -198,15 +200,16 @@ enum { NFC_ECC_BCH60_1K, }; -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } static struct meson_nand_ecc meson_ecc[] = { - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), }; static int meson_nand_calc_ecc_bytes(int step_size, int strength) @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength) NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, - meson_nand_calc_ecc_bytes, 1024, 8); + +static const int axg_stepinfo_strengths[] = { 8 }; +static const struct nand_ecc_step_info axg_stepinfo_1024 = { + .stepsize = 1024, + .strengths = axg_stepinfo_strengths, + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) +}; + +static const struct nand_ecc_step_info axg_stepinfo_512 = { + .stepsize = 512, + .strengths = axg_stepinfo_strengths, + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) +}; + +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; + +static const struct nand_ecc_caps meson_axg_ecc_caps = { + .stepinfos = axg_stepinfo, + .nstepinfos = ARRAY_SIZE(axg_stepinfo), + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, +}; static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) { @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand) return -EINVAL; for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { - if (meson_ecc[i].strength == nand->ecc.strength) { + if (meson_ecc[i].strength == nand->ecc.strength && + meson_ecc[i].size == nand->ecc.size) { meson_chip->bch_mode = meson_ecc[i].bch; return 0; } @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) struct meson_nfc *nfc = nand_get_controller_data(nand); struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); struct mtd_info *mtd = nand_to_mtd(nand); - int nsectors = mtd->writesize / 1024; + int nsectors = mtd->writesize / 512; int raw_writesize; int ret;
Meson NAND supports both 512B and 1024B ECC step size. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 12 deletions(-)