Hi Arseniy,
avkrasnov@xxxxxxxxxxxxxx wrote on Tue, 4 Jul 2023 12:23:03 +0300:
> On 04.07.2023 11:36, Miquel Raynal wrote:
> > Hi Arseniy,
> >
> > AVKrasnov@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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
