Re: [PATCH v11 2/2] mtd: rawnand: Add Loongson-1 NAND Controller Driver

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 17/01/2025 at 19:58:39 +08, Keguang Zhang <keguang.zhang@gmail.com> wrote:

> Hello Miquel,
>
> On Thu, Jan 16, 2025 at 2:54 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> Hello Keguang,
>>
>> On 17/12/2024 at 18:16:50 +08, Keguang Zhang via B4 Relay <devnull+keguang.zhang.gmail.com@kernel.org> wrote:
>>
>> > +static int ls1x_nand_op_cmd_mapping(struct nand_chip *chip, struct ls1x_nand_op *op, u8 opcode)
>> > +{
>> > +     struct ls1x_nand_host *host = nand_get_controller_data(chip);
>> > +     int ret = 0;
>>
>> This return code is unused.
>>
>> > +
>> > +     op->row_start = chip->page_shift + 1;
>> > +
>> > +     /* The controller abstracts the following NAND operations. */
>> > +     switch (opcode) {
>> > +     case NAND_CMD_STATUS:
>> > +             op->cmd_reg = LS1X_NAND_CMD_STATUS;
>> > +             break;
>> > +     case NAND_CMD_RESET:
>> > +             op->cmd_reg = LS1X_NAND_CMD_RESET;
>> > +             break;
>> > +     case NAND_CMD_READID:
>> > +             op->is_readid = true;
>> > +             op->cmd_reg = LS1X_NAND_CMD_READID;
>> > +             break;
>> > +     case NAND_CMD_ERASE1:
>> > +             op->is_erase = true;
>> > +             op->addrs_offset = 2;
>> > +             break;
>> > +     case NAND_CMD_ERASE2:
>> > +             if (!op->is_erase)
>> > +                     return -EOPNOTSUPP;
>> > +             /* During erasing, row_start differs from the default value. */
>>
>> ...
>>
>> > +static void ls1x_nand_trigger_op(struct ls1x_nand_host *host, struct ls1x_nand_op *op)
>> > +{
>> > +     struct nand_chip *chip = &host->chip;
>> > +     struct mtd_info *mtd = nand_to_mtd(chip);
>> > +     int col0 = op->addrs[0];
>> > +     short col;
>> > +
>> > +     /* restore row address for column change */
>> > +     if (op->is_change_column) {
>> > +             op->addr2_reg = readl(host->reg_base + LS1X_NAND_ADDR2);
>> > +             op->addr1_reg = readl(host->reg_base + LS1X_NAND_ADDR1);
>> > +             op->addr1_reg &= ~(mtd->writesize - 1);
>> > +     }
>>
>> This looks very suspicious. You should not have to do that and to be
>> honest, I don't undertand what this means.
>>
> The Loongson-1 NAND controller requires a full address (column address
> + row address).
> However, nand_change_read_column_op() function only provides the
> column address. Therefore, the row address must be restored.
> The above logic retrieves the row address from the addr1_reg in order
> to restore the row address.

If it needs the full offset, it's probably not a change column
command.

What you do here is very risky and clearly not future proof, I'd prefer
to avoid it. If anything happens in the core between the read0 and the
column change, your logic breaks, and there are chances that this will
happen at some point.

Are you sure you implemented it correctly? What if you provide 0 as page
offset? If there is no change column possible, maybe the best thing is
to not support it.

...

>> > +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;
>>
>> Don't you need a dma_addr_t here instead? You shall remap the resource.
>>
> Sorry, I don't quite understand.
> 'dma_base' is already of type dma_addr_t.

I didn't identify where the dma_base was remapped, but if that's already
done then we're good.

Thanks,
Miquèl