On Sat, Jan 18, 2025 at 2:26 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> On 17/01/2025 at 19:58:39 +08, Keguang Zhang <keguang.zhang@xxxxxxxxx> wrote:
>
> > Hello Miquel,
> >
> > On Thu, Jan 16, 2025 at 2:54 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >>
> >> Hello Keguang,
> >>
> >> On 17/12/2024 at 18:16:50 +08, Keguang Zhang via B4 Relay <devnull+keguang.zhang.gmail.com@xxxxxxxxxx> 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.
Understood.
I will improve .parse_address with regmap_update_bits() to avoid this
restore logic.
>
> ...
>
> >> > +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.
Perhaps I misunderstand the usage of dma_map_resource(). dma_base is
the physical address and will be written to the DMA controller
register at last. It should be defined as the phys_addr_t type and set
to 'res->start' directly when probing. Am I right?
>
> Thanks,
> Miquèl
--
Best regards,
Keguang Zhang
