On Wed, Mar 19, 2025 at 12:15 AM Miquel Raynal
<miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hello Keguang,
>
> I guess I am mostly fine with the driver, it's probably time to merge
> it. Just a few final changes below, I plan on merging it at -rc1.
>
Great!
I will fix the following lines ASAP.
Thanks!
> > + case NAND_CMD_READSTART:
> > + if (!op->is_read)
> > + return -EOPNOTSUPP;
> > + op->cmd_reg = LS1X_NAND_CMD_READ;
> > + break;
> > + case NAND_CMD_RNDOUT:
> > + op->is_change_column = true;
> > + break;
> > + case NAND_CMD_RNDOUTSTART:
> > + if (!op->is_change_column)
> > + return -EOPNOTSUPP;
> > + op->cmd_reg = LS1X_NAND_CMD_READ;
> > + break;
> > + default:
> > + dev_err(host->dev, "unsupported opcode: %u\n", opcode);
>
> No error message in the normal path. This should be a debug log at
> most. This function is called in the check_only path.
>
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int ls1x_nand_read_id_type_exec(struct nand_chip *chip, const struct nand_subop *subop)
> > +{
> > + struct ls1x_nand_host *host = nand_get_controller_data(chip);
> > + struct ls1x_nand_op op = {};
> > + int i, ret;
> > + union {
> > + char ids[5];
> > + struct {
> > + int idl;
> > + char idh;
> > + };
> > + } nand_id;
> > +
> > + ret = ls1x_nand_misc_type_exec(chip, subop, &op);
> > + if (ret) {
> > + dev_err(host->dev, "failed to read ID! %d\n", ret);
>
> No print here, it's not useful.
>
> > + return ret;
> > + }
> > +
> > + nand_id.idl = readl(host->reg_base + LS1X_NAND_IDL);
> > + nand_id.idh = readb(host->reg_base + LS1X_NAND_IDH_STATUS);
> > +
> > + for (i = 0; i < min(sizeof(nand_id.ids), op.orig_len); i++)
> > + op.buf[i] = nand_id.ids[sizeof(nand_id.ids) - 1 - i];
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int ls1x_nand_is_valid_cmd(struct device *dev, u8 opcode)
> > +{
> > + if (opcode == NAND_CMD_STATUS || opcode == NAND_CMD_RESET || opcode == NAND_CMD_READID)
> > + return 0;
> > +
> > + dev_err(dev, "unsupported opcode: %x", opcode);
>
> Ditto
>
> > +
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int ls1x_nand_is_valid_cmd_seq(struct device *dev, u8 opcode1, u8 opcode2)
> > +{
> > + if (opcode1 == NAND_CMD_RNDOUT && opcode2 == NAND_CMD_RNDOUTSTART)
> > + return 0;
> > +
> > + if (opcode1 == NAND_CMD_READ0 && opcode2 == NAND_CMD_READSTART)
> > + return 0;
> > +
> > + if (opcode1 == NAND_CMD_ERASE1 && opcode2 == NAND_CMD_ERASE2)
> > + return 0;
> > +
> > + if (opcode1 == NAND_CMD_SEQIN && opcode2 == NAND_CMD_PAGEPROG)
> > + return 0;
> > +
> > + dev_err(dev, "unsupported opcode sequence: %x %x", opcode1,
> > opcode2);
>
> Ditto
>
> > +
> > + return -EOPNOTSUPP;
> > +}
>
> ...
>
> > +static int ls1x_nand_attach_chip(struct nand_chip *chip)
> > +{
> > + struct ls1x_nand_host *host = nand_get_controller_data(chip);
> > + u64 chipsize = nanddev_target_size(&chip->base);
> > + int cell_size = 0;
> > +
> > + 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 -EOPNOTSUPP;
>
> EINVAL
>
> > + }
> > +
> > + switch (chip->ecc.engine_type) {
> > + case NAND_ECC_ENGINE_TYPE_NONE:
> > + dev_info(host->dev, "No ECC\n");
>
> Please drop
>
> > + break;
> > + case NAND_ECC_ENGINE_TYPE_SOFT:
> > + dev_info(host->dev, "using SW ECC\n");
>
> Drop
>
> > + break;
> > + default:
> > + dev_err(host->dev, "ECC mode %d not supported\n",
> > chip->ecc.engine_type);
>
> Drop
>
> > + return -EINVAL;
> > + }
> > +
> > + /* set cell size */
> > + regmap_update_bits(host->regmap, LS1X_NAND_PARAM, LS1X_NAND_CELL_SIZE_MASK,
> > + FIELD_PREP(LS1X_NAND_CELL_SIZE_MASK, cell_size));
> > +
> > + regmap_update_bits(host->regmap, LS1X_NAND_TIMING, LS1X_NAND_HOLD_CYCLE_MASK,
> > + FIELD_PREP(LS1X_NAND_HOLD_CYCLE_MASK, host->data->hold_cycle));
> > +
> > + regmap_update_bits(host->regmap, LS1X_NAND_TIMING, LS1X_NAND_WAIT_CYCLE_MASK,
> > + FIELD_PREP(LS1X_NAND_WAIT_CYCLE_MASK, host->data->wait_cycle));
> > +
> > + chip->ecc.read_page_raw = nand_monolithic_read_page_raw;
> > + chip->ecc.write_page_raw = nand_monolithic_write_page_raw;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct nand_controller_ops ls1x_nand_controller_ops = {
> > + .exec_op = ls1x_nand_exec_op,
> > + .attach_chip = ls1x_nand_attach_chip,
> > +};
> > +
> > +static void ls1x_nand_controller_cleanup(struct ls1x_nand_host *host)
> > +{
> > + if (host->dma_chan)
> > + dma_release_channel(host->dma_chan);
> > +}
> > +
> > +static int ls1x_nand_controller_init(struct ls1x_nand_host *host)
> > +{
> > + struct device *dev = host->dev;
> > + struct dma_chan *chan;
> > + struct dma_slave_config cfg = {};
> > + int ret;
> > +
> > + host->regmap = devm_regmap_init_mmio(dev, host->reg_base, &ls1x_nand_regmap_config);
> > + if (IS_ERR(host->regmap))
> > + return dev_err_probe(dev, PTR_ERR(host->regmap), "failed to init regmap\n");
> > +
> > + chan = dma_request_chan(dev, "rxtx");
> > + if (IS_ERR(chan))
> > + return dev_err_probe(dev, PTR_ERR(chan), "failed to request DMA channel\n");
> > + host->dma_chan = chan;
> > +
> > + cfg.src_addr = host->dma_base;
> > + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + cfg.dst_addr = host->dma_base;
> > + cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + ret = dmaengine_slave_config(host->dma_chan, &cfg);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to config DMA channel\n");
> > +
> > + init_completion(&host->dma_complete);
> > +
> > + dev_dbg(dev, "got %s for %s access\n",
> > dma_chan_name(host->dma_chan), dev_name(dev));
>
> You can drop this one as well
>
> > +
> > + return 0;
> > +}
>
> Thanks,
> Miquèl
--
Best regards,
Keguang Zhang
