Re: [PATCH v2 4/8] mtd: rawnand: loongson: Add nand chip select support

[Binbin Zhou <zhoubb.aaron@gmail.com>]

Hi:

On Mon, Aug 11, 2025 at 2:40 PM Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Binbin,
>
> On Mon, Aug 11, 2025 at 2:03 PM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
> >
> > The page address register describes the page address of the starting
> > address for NAND read/write/erase operations.
> >
> > According to the manual, it consists of two parts:
> >         {chip select, page number}
> >
> > The `chip select` is fixed at 2 bits, and the `page number` is
> > determined based on the actual capacity of the single-chip memory.
> > Therefore we need to determine the `chip select` bits base on the `page
> > number`.
> >
> > For example, for a 1GB capacity chip (2K page size), it has 1M pages.
> > Thus, [19:0] is used to represent the page number, and [21:20]
> > represents the chip select.
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> >  .../mtd/nand/raw/loongson-nand-controller.c   | 133 ++++++++++++++----
> >  1 file changed, 103 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/loongson-nand-controller.c b/drivers/mtd/nand/raw/loongson-nand-controller.c
> > index 97cd566420a8..5a51c7d299cc 100644
> > --- a/drivers/mtd/nand/raw/loongson-nand-controller.c
> > +++ b/drivers/mtd/nand/raw/loongson-nand-controller.c
> > @@ -82,6 +82,7 @@ struct loongson_nand_data {
> >         unsigned int op_scope_field;
> >         unsigned int hold_cycle;
> >         unsigned int wait_cycle;
> > +       unsigned int nand_cs;
> >         void (*set_addr)(struct loongson_nand_host *host, struct loongson_nand_op *op);
> >  };
> >
> > @@ -90,6 +91,7 @@ struct loongson_nand_host {
> >         struct nand_chip chip;
> >         struct nand_controller controller;
> >         const struct loongson_nand_data *data;
> > +       unsigned int addr_cs_field;
> >         void __iomem *reg_base;
> >         struct regmap *regmap;
> >         /* DMA Engine stuff */
> > @@ -215,6 +217,26 @@ static int loongson_nand_parse_instructions(struct nand_chip *chip, const struct
> >         return 0;
> >  }
> >
> > +static void loongson_nand_set_addr_cs(struct loongson_nand_host *host)
> > +{
> > +       struct nand_chip *chip = &host->chip;
> > +       struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > +       if (!host->data->nand_cs)
> > +               return;
> > +
> > +       /*
> > +        * The Manufacturer/Chip ID read operation precedes attach_chip, at which point
> > +        * information such as NAND chip selection and capacity is unknown. As a
> > +        * workaround, we use 128MB cellsize (2KB pagesize) as a fallback.
> > +        */
> > +       if (!mtd->writesize)
> > +               host->addr_cs_field = GENMASK(17, 16);
> > +
> > +       regmap_update_bits(host->regmap, LOONGSON_NAND_ADDR2, host->addr_cs_field,
> > +                          host->data->nand_cs << __ffs(host->addr_cs_field));
> > +}
> > +
> >  static void ls1b_nand_set_addr(struct loongson_nand_host *host, struct loongson_nand_op *op)
> >  {
> >         struct nand_chip *chip = &host->chip;
> > @@ -263,6 +285,8 @@ static void ls1c_nand_set_addr(struct loongson_nand_host *host, struct loongson_
> >                         regmap_update_bits(host->regmap, LOONGSON_NAND_ADDR2, mask, val);
> >                 }
> >         }
> > +
> > +       loongson_nand_set_addr_cs(host);
> >  }
> >
> >  static void loongson_nand_trigger_op(struct loongson_nand_host *host, struct loongson_nand_op *op)
> > @@ -603,42 +627,89 @@ static int loongson_nand_exec_op(struct nand_chip *chip, const struct nand_opera
> >         return nand_op_parser_exec_op(chip, &loongson_nand_op_parser, op, check_only);
> >  }
> >
> > -static int loongson_nand_attach_chip(struct nand_chip *chip)
> > +static int loongson_nand_get_chip_capacity(struct nand_chip *chip)
> >  {
> >         struct loongson_nand_host *host = nand_get_controller_data(chip);
> >         u64 chipsize = nanddev_target_size(&chip->base);
> > -       int cell_size = 0;
> > +       struct mtd_info *mtd = nand_to_mtd(chip);
> >
> > -       switch (chipsize) {
> > -       case SZ_128M:
> > -               cell_size = 0x0;
> > -               break;
> > -       case SZ_256M:
> > -               cell_size = 0x1;
> > -               break;
> > -       case SZ_512M:
> > -               cell_size = 0x2;
> > -               break;
> > -       case SZ_1G:
> > -               cell_size = 0x3;
> > -               break;
> > -       case SZ_2G:
> > -               cell_size = 0x4;
> > -               break;
> > -       case SZ_4G:
> > -               cell_size = 0x5;
> > -               break;
> > -       case SZ_8G:
> > -               cell_size = 0x6;
> > -               break;
> > -       case SZ_16G:
> > -               cell_size = 0x7;
> > -               break;
> > -       default:
> > -               dev_err(host->dev, "unsupported chip size: %llu MB\n", chipsize);
> > -               return -EINVAL;
> > +       if (mtd->writesize == SZ_4K && chipsize == SZ_2G) {
> > +               host->addr_cs_field = GENMASK(20, 19);
> > +               return 0x4;
> >         }
> I think you want to sort this part by the return value, but "return
> 0x4" has already break it.
>
> So I think it is better to use a big switch-case for writesize, and
> sort the big switch-case by writesize.

How about:

        switch (mtd->writesize) {
        case SZ_512:
                switch (chipsize) {
                case SZ_8M:
                        host->addr_cs_field = GENMASK(15, 14);
                        return 0x9;
                case SZ_16M:
                        host->addr_cs_field = GENMASK(16, 15);
                        return 0xa;
                case SZ_32M:
                        host->addr_cs_field = GENMASK(17, 16);
                        return 0xb;
                case SZ_64M:
                        host->addr_cs_field = GENMASK(18, 17);
                        return 0xc;
                case SZ_128M:
                        host->addr_cs_field = GENMASK(19, 18);
                        return 0xd;
                }
                break;
        case SZ_2K:
                switch (chipsize) {
                case SZ_128M:
                        host->addr_cs_field = GENMASK(17, 16);
                        return 0;
                case SZ_256M:
                        host->addr_cs_field = GENMASK(18, 17);
                        return 0x1;
                case SZ_512M:
                        host->addr_cs_field = GENMASK(19, 18);
                        return 0x2;
                case SZ_1G:
                        host->addr_cs_field = GENMASK(20, 19);
                        return 0x3;
                }
                break;
        case SZ_4K:
                if (chipsize == SZ_2G) {
                        host->addr_cs_field = GENMASK(20, 19);
                        return 0x4;
                }
                break;
        case SZ_8K:
                switch (chipsize) {
                case SZ_4G:
                        host->addr_cs_field = GENMASK(20, 19);
                        return 0x5;
                case SZ_8G:
                        host->addr_cs_field = GENMASK(21, 20);
                        return 0x6;
                case SZ_16G:
                        host->addr_cs_field = GENMASK(22, 21);
                        return 0x7;
                }
                break;
        default:
                break;
        }

        dev_err(host->dev, "Unsupported chip size: %llu MB with page
size %u B\n",
                chipsize, mtd->writesize);
>
> Huacai
>
> >
> > +       if (mtd->writesize == SZ_2K) {
> > +               switch (chipsize) {
> > +               case SZ_128M:
> > +                       host->addr_cs_field = GENMASK(17, 16);
> > +                       return 0;
> > +               case SZ_256M:
> > +                       host->addr_cs_field = GENMASK(18, 17);
> > +                       return 0x1;
> > +               case SZ_512M:
> > +                       host->addr_cs_field = GENMASK(19, 18);
> > +                       return 0x2;
> > +               case SZ_1G:
> > +                       host->addr_cs_field = GENMASK(20, 19);
> > +                       return 0x3;
> > +               default:
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +       if (mtd->writesize == SZ_8K) {
> > +               switch (chipsize) {
> > +               case SZ_4G:
> > +                       host->addr_cs_field = GENMASK(20, 19);
> > +                       return 0x5;
> > +               case SZ_8G:
> > +                       host->addr_cs_field = GENMASK(21, 20);
> > +                       return 0x6;
> > +               case SZ_16G:
> > +                       host->addr_cs_field = GENMASK(22, 21);
> > +                       return 0x7;
> > +               default:
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +       if (mtd->writesize == SZ_512) {
> > +               switch (chipsize) {
> > +               case SZ_8M:
> > +                       host->addr_cs_field = GENMASK(15, 14);
> > +                       return 0x9;
> > +               case SZ_16M:
> > +                       host->addr_cs_field = GENMASK(16, 15);
> > +                       return 0xa;
> > +               case SZ_32M:
> > +                       host->addr_cs_field = GENMASK(17, 16);
> > +                       return 0xb;
> > +               case SZ_64M:
> > +                       host->addr_cs_field = GENMASK(18, 17);
> > +                       return 0xc;
> > +               case SZ_128M:
> > +                       host->addr_cs_field = GENMASK(19, 18);
> > +                       return 0xd;
> > +               default:
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +err:
> > +       dev_err(host->dev, "Unsupported chip size: %llu MB with page size %u B\n",
> > +               chipsize, mtd->writesize);
> > +       return -EINVAL;
> > +}
> > +
> > +static int loongson_nand_attach_chip(struct nand_chip *chip)
> > +{
> > +       struct loongson_nand_host *host = nand_get_controller_data(chip);
> > +       int cell_size = 0;
> > +
> > +       cell_size = loongson_nand_get_chip_capacity(chip);
> > +       if (cell_size < 0)
> > +               return cell_size;
> > +
> >         switch (chip->ecc.engine_type) {
> >         case NAND_ECC_ENGINE_TYPE_NONE:
> >                 break;
> > @@ -818,6 +889,7 @@ static const struct loongson_nand_data ls1b_nand_data = {
> >         .status_field = GENMASK(15, 8),
> >         .hold_cycle = 0x2,
> >         .wait_cycle = 0xc,
> > +       .nand_cs = 0x0,
> >         .set_addr = ls1b_nand_set_addr,
> >  };
> >
> > @@ -828,6 +900,7 @@ static const struct loongson_nand_data ls1c_nand_data = {
> >         .op_scope_field = GENMASK(29, 16),
> >         .hold_cycle = 0x2,
> >         .wait_cycle = 0xc,
> > +       .nand_cs = 0x0,
> >         .set_addr = ls1c_nand_set_addr,
> >  };
> >
> > --
> > 2.47.3
> >
> >

-- 
Thanks.
Binbin