RE: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Naga Sureshkumar Relli]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Sent: Monday, April 29, 2019 5:54 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: Helmut Grohne <helmut.grohne@xxxxxxxxxx>; bbrezillon@xxxxxxxxxx; richard@xxxxxx;
> dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> nagasureshkumarrelli@xxxxxxxxx
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> Hi Naga,
> 
> > > > +	u32 addrs;
> > > > +	u32 naddrs;
> > > > +	u32 addr5;
> > > > +	u32 addr6;
> > >
> > > Why are addr5 and addr6 u32? You only ever store u8 values in here.
> > > How about merging them into an u16 addr56? Doing so would make the
> > > access in pl353_nand_exec_op_cmd simpler and move a little complexity into
> pl353_nfc_parse_instructions.
> > Will try this. But I don't see any complex logic involved using addr5 and addr6.
> 
> This is a relic of a too quick copy from marvell_nand.c. Please match the structure with your
> controller memory layout instead.
Ok, I will re check this.
> 
> > > > +	const struct nand_op_instr *data_instr; };
> > > > +
> > > > +/**
> > > > + * struct pl353_nand_controller - Defines the NAND flash controller driver
> > > > + *				  instance
> > > > + * @chip:		NAND chip information structure
> > > > + * @dev:		Parent device (used to print error messages)
> > > > + * @regs:		Virtual address of the NAND flash device
> > > > + * @buf_addr:		Virtual address of the NAND flash device for
> > > > + *			data read/writes
> > > > + * @addr_cycles:	Address cycles
> > > > + * @mclk:		Memory controller clock
> > > > + * @buswidth:		Bus width 8 or 16
> > > > + */
> > > > +struct pl353_nand_controller {
> > > > +	struct nand_controller controller;
> > > > +	struct nand_chip chip;
> > > > +	struct device *dev;
> > > > +	void __iomem *regs;
> > > > +	void __iomem *buf_addr;
> > >
> > > I find the use of buf_addr unfortunate. It is consumed by two
> > > functions pl353_nand_read_data_op and pl353_nand_write_data_op. All
> > > other functions update its value. Semantically, its value is regs +
> > > some flags. For the updaters that means a complex logic where they
> > > first have to subtract reg, then change flags and add reg again. To
> > > make matters worse, this computation involves __force casts between
> > > long and __iomem (which yielded complaints in earlier reviews).  I
> > > think it would simplify the code if you replaced buf_addr with something like addr_flags
> and then compute buf_addr as regs + addr_flags in those two consumers. What do you think?
> > This definitely simplifies the driver logic, we have to do that.
> > I tried it previously, regarding __force and __iomem casting changes,
> > but the driver functionality was broken With this update.
> > Let me update it and check it again.
> > But just wanted to know, do you see issues with these __force and __iomem castings?
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't constant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
> 
> What Helmut proposes is, I think, saving the clock frequency to avoid requesting it everytime
> you want to use it (if this is done many times).
Ok. got it.
> 
> [...]
> 
> > > > +
> > > > +	oobregion->offset = (section * chip->ecc.bytes) + 2;
> > > > +	oobregion->length = 50;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct mtd_ooblayout_ops pl353_ecc_ooblayout64_ops = {
> > > > +	.ecc = pl353_ecc_ooblayout64_ecc,
> > > > +	.free = pl353_ecc_ooblayout64_free, };
> > > > +
> > > > +/* Generic flash bbt decriptors */ static u8 bbt_pattern[] = {
> > > > +'B', 'b', 't', '0' }; static u8 mirror_pattern[] = { '1', 't',
> > > > +'b', 'B' };
> > > > +
> > > > +static struct nand_bbt_descr bbt_main_descr = {
> > > > +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE |
> > > NAND_BBT_WRITE
> > > > +		| NAND_BBT_2BIT | NAND_BBT_VERSION |
> NAND_BBT_PERCHIP,
> > > > +	.offs = 4,
> > > > +	.len = 4,
> > > > +	.veroffs = 20,
> > > > +	.maxblocks = 4,
> > > > +	.pattern = bbt_pattern
> > >
> > > I have a general question concerning the nand framework here. The
> > > pattern member in struct nand_bbt_descr is not declared const.
> > > Therefore, bbt_pattern cannot be const here. As far as I looked, all
> > > accesses of pattern use it with memcmp or as source for memcpy. Also
> > > the diskonchip.c driver assigns a string constant here. This
> > > suggests, that pattern should be declared const or that diskonchip.c is doing it wrong.
> > May be Miquel can comment on this.
> 
> I did not check diskonchip.c's implementation but turning it into const seems reasonable to me.
> However in the scope of this driver, Naga, you can keep it as it is right now.
Ok.
> 
> > >
> > > > +};
> > > > +
> > > > +static struct nand_bbt_descr bbt_mirror_descr = {
> > > > +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE |
> > > NAND_BBT_WRITE
> > > > +		| NAND_BBT_2BIT | NAND_BBT_VERSION |
> NAND_BBT_PERCHIP,
> > > > +	.offs = 4,
> > > > +	.len = 4,
> > > > +	.veroffs = 20,
> > > > +	.maxblocks = 4,
> > > > +	.pattern = mirror_pattern
> > > > +};
> > > > +
> > > > +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> > > > +					bool force_8bit)
> > > > +{
> > > > +	int ret;
> > > > +	struct pl353_nand_controller *xnfc =
> > > > +		container_of(chip, struct pl353_nand_controller, chip);
> > > > +
> > > > +	if (xnfc->buswidth == 8)
> > >
> > > This buswidth member is never assigned anywhere. Thus the value is
> > > always 0 and this comparison always fails.
> > No, in the probe we should update this, by reading it from dts.
> > Unfortunately, the assignment was removed, during checkpatch clean up(its my editor issue).
> > I will update it.
> 
> This answer is scary, there are probably many other places where you deleted useful code then?
Sorry for that. But I won't repeat this.
> 
> 
> 
> [...]
> 
> > > > +/* NAND framework ->exec_op() hooks and related helpers */
> static
> > > > +void pl353_nfc_parse_instructions(struct nand_chip *chip,
> > > > +					 const struct nand_subop *subop,
> > > > +					 struct pl353_nfc_op *nfc_op) {
> > > > +	const struct nand_op_instr *instr = NULL;
> > > > +	unsigned int op_id, offset, naddrs;
> > > > +	int i;
> > > > +	const u8 *addrs;
> > > > +
> > > > +	memset(nfc_op, 0, sizeof(struct pl353_nfc_op));
> > > > +	for (op_id = 0; op_id < subop->ninstrs; op_id++) {
> > > > +		instr = &subop->instrs[op_id];
> > > > +
> > > > +		switch (instr->type) {
> > > > +		case NAND_OP_CMD_INSTR:
> > > > +			if (op_id)
> > > > +				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> > > > +			else
> > > > +				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> > > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_ADDR_INSTR:
> > > > +			offset = nand_subop_get_addr_start_off(subop, op_id);
> > > > +			naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
> > > > +			addrs = &instr->ctx.addr.addrs[offset];
> > > > +			nfc_op->addrs = instr->ctx.addr.addrs[offset];
> > > > +			for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> > > > +				nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you really meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for
> loop?
> >
> > >
> > > > +						 (8 * i);
> > > > +			}
> > > > +
> > > > +			if (naddrs >= 5)
> > > > +				nfc_op->addr5 = addrs[4];
> > > > +			if (naddrs >= 6)
> > > > +				nfc_op->addr6 = addrs[5];
> 
> Also, you probably don't need addr5 and addr6 here, they were needed in marvell_nand.c
> because addresses are stored in three different registers and are limited to 6 cycles, but this is
> probably not the case in your driver.
Ok. I will cross check it once and update it next version.
> 
> > > > +			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
> > > > +								     op_id);
> > > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_DATA_IN_INSTR:
> > > > +			nfc_op->data_instr = instr;
> > > > +			nfc_op->data_instr_idx = op_id;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_DATA_OUT_INSTR:
> > > > +			nfc_op->data_instr = instr;
> > > > +			nfc_op->data_instr_idx = op_id;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_WAITRDY_INSTR:
> > > > +			nfc_op->rdy_timeout_ms = instr->ctx.waitrdy.timeout_ms;
> > > > +			nfc_op->rdy_delay_ns = instr->delay_ns;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static void cond_delay(unsigned int ns) {
> > > > +	if (!ns)
> > > > +		return;
> > > > +
> > > > +	if (ns < 10000)
> > > > +		ndelay(ns);
> > > > +	else
> > > > +		udelay(DIV_ROUND_UP(ns, 1000)); }
> > >
> 
> [...]
> 
> > > > +
> > > > +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> > > > +	if (!nfc_op.data_instr) {
> > > > +		if (nfc_op.rdy_timeout_ms) {
> > > > +			if (pl353_wait_for_dev_ready(chip))
> > > > +				return -ETIMEDOUT;
> > > > +		}
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> > > > +	if (!reading) {
> > > > +		len = nand_subop_get_data_len(subop, op_id);
> > > > +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> > > > +					 len, instr->ctx.data.force_8bit);
> > > > +		if (nfc_op.rdy_timeout_ms) {
> > > > +			if (pl353_wait_for_dev_ready(chip))
> > > > +				return -ETIMEDOUT;
> > > > +		}
> > > > +
> > > > +		cond_delay(nfc_op.rdy_delay_ns);
> > > > +	}
> > > > +
> > > > +	if (reading) {
> > >
> > > You could use an else branch instead of inverting the condition here.
> > > When Miquel complained about this in v13, you said you'd change it, but you didn't.
> > Sorry for that. I will change it.
> 
> Yes please, be careful and not in a hurry when re-posting, it already takes quite some time to
> review the entire driver.
Sure Miquel and thanks for the review.

Thanks,
Naga Sureshkumar Relli
> 
> Thanks,
> MiquÃl