Re: [LINUX PATCH v11 3/3] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

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

Hi Naga,

> > > +/**
> > > + * pl353_nand_read_data_op - read chip data into buffer
> > > + * @chip:	Pointer to the NAND chip info structure
> > > + * @in:		Pointer to the buffer to store read data
> > > + * @len:	Number of bytes to read
> > > + * Return:	Always return zero
> > > + */
> > > +static int pl353_nand_read_data_op(struct nand_chip *chip,
> > > +				   u8 *in,
> > > +				   unsigned int len)
> > > +{
> > > +	int i;
> > > +	struct pl353_nand_info *xnfc =
> > > +		container_of(chip, struct pl353_nand_info, chip);
> > > +
> > > +	if (IS_ALIGNED((uint32_t)in, sizeof(uint32_t)) &&
> > > +	    IS_ALIGNED(len, sizeof(uint32_t))) {
> > > +		u32 *ptr = (u32 *)in;
> > > +
> > > +		len /= 4;
> > > +		for (i = 0; i < len; i++)
> > > +			ptr[i] = readl(xnfc->nandaddr);
> > > +	} else {
> > > +		for (i = 0; i < len; i++)
> > > +			in[i] = readb(xnfc->nandaddr);
> > > +	}  
> > 
> > What about reading 0-3 bytes with readb if the driver is not aligned, then doing aligned
> > access with readl until readb must be used again for the last 0-3 bytes?  
> The else case is handling that right?

No it's not. The else case reads byte per byte, always.

[...]

> > > +static int pl353_nand_calculate_hwecc(struct mtd_info *mtd,
> > > +				      const u8 *data, u8 *ecc)
> > > +{
> > > +	u32 ecc_value;
> > > +	u8 ecc_reg, ecc_byte, ecc_status;
> > > +	unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
> > > +
> > > +	/* Wait till the ECC operation is complete or timeout */
> > > +	do {
> > > +		if (pl353_smc_ecc_is_busy())
> > > +			cpu_relax();
> > > +		else
> > > +			break;
> > > +	} while (!time_after_eq(jiffies, timeout));
> > > +
> > > +	if (time_after_eq(jiffies, timeout)) {
> > > +		pr_err("%s timed out\n", __func__);
> > > +		return -ETIMEDOUT;
> > > +	}  
> > 
> > All of this should be done by the function calling nand_calculate_hwecc(), not here.  
> Ok, I will update it.
> > 
> > Plus, it should deserve a function on its own.  
> Ok, will add new one.
> >   
> > > +
> > > +	for (ecc_reg = 0; ecc_reg < 4; ecc_reg++) {
> > > +		/* Read ECC value for each block */  
> > 
> > So you assume here that there are 4, and only 4 "blocks" (I prefer "ECC chunks" or
> > "subpages"). Is it always the case? Or is it just how it works in your situation? I would rather
> > prefer a to define this value anyway.  
> Sure, I will define a macro as ECC chunks to represent 4.
> And there are always 4 ECC registers as per the controller. And there is no assumption here.

What about smaller or larger NAND chips? Maybe there are some
limitations in what chips are actually supported, then they should be
rejected.


[...]

> > > +static void pl353_prepare_cmd(struct mtd_info *mtd, struct
nand_chip *chip,
> > > +			      int page, int column, int start_cmd, int end_cmd,
> > > +			      bool read)
> > > +{
> > > +	unsigned long data_phase_addr;
> > > +	u32 end_cmd_valid = 0;
> > > +	unsigned long cmd_phase_addr = 0, cmd_data = 0;
> > > +
> > > +	struct pl353_nand_info *xnfc =
> > > +		container_of(chip, struct pl353_nand_info, chip);
> > > +
> > > +	end_cmd_valid = read ? 1 : 0;
> > > +
> > > +	cmd_phase_addr = (unsigned long __force)xnfc->nand_base +  
> > 
> > do you really need this cast?  
> As I said above, during command and data phases, we have to update the AXI Read/Write addresses with cycles, start command, end command
> And some extra info. Hence I am converting it to a value and then adding the above values and then again converting back as an
> Address.
> 
> >   
> > > +			 ((xnfc->addr_cycles
> > > +			 << ADDR_CYCLES_SHIFT) |
> > > +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > > +			 (COMMAND_PHASE) |
> > > +			 (end_cmd << END_CMD_SHIFT) |
> > > +			 (start_cmd << START_CMD_SHIFT));  
> > 
> > So the number of address cycles changes the address where you should write the address
> > cycles? that's weird, no?  
> I agree, but the implementation of PL353 is like that.
> As I pointed the spec above, for every transfer we have to frame AXI Write address and Read address.
> 
> >   
> > > +
> > > +	/* Get the data phase address */
> > > +	data_phase_addr = (unsigned long __force)xnfc->nand_base +
> > > +			  ((0x0 << CLEAR_CS_SHIFT) |
> > > +			  (0 << END_CMD_VALID_SHIFT) |
> > > +			  (DATA_PHASE) |
> > > +			  (end_cmd << END_CMD_SHIFT) |
> > > +			  (0x0 << ECC_LAST_SHIFT));
> > > +  
> > Same question here?
> >   
> > > +	xnfc->nandaddr = (void __iomem * __force)data_phase_addr;  
> > 
> > This should be done only once in the probe  
> No, as explained above, once we frame the Axi Write address/Read address with valid data, then we
> Are converting back as memory address with the casting.
> Do you see any issues with that?
> Let me know, is there an alternative to achieve the same. 
> All is about, constructing AXI Write address and Read address during command and data phases.

Ok, I'll ask Boris to also review your next version, once -rc1 is out.

Thanks,
Miquèl