Re: [EXT] Re: [PATCH 4/8] mtd: spinand: enabled parameter page support

[Boris Brezillon]

On Mon, 19 Aug 2019 11:21:18 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Boris,
> 
> Another question for you :)
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@xxxxxxxxxx> wrote on
> Mon, 19 Aug 2019 08:51:52 +0000:
> 
> > Hi Miquel,
> >   
> > > 
> > > Hi Shiva,
> > > 
> > > shiva.linuxworks@xxxxxxxxx wrote on Mon, 22 Jul 2019 07:56:17 +0200:
> > > 
> > > "mtd: spinand: enable parameter page support"
> > >     
> > > > From: Shivamurthy Shastri <sshivamurthy@xxxxxxxxxx>
> > > >
> > > > Some of the SPI NAND devices has parameter page, which is similar to    
> > >                  -             have a    
> > > > ONFI table.    
> > >   regular raw NAND ONFI tables.
> > >     
> > > >
> > > > But, it may not be self sufficient to propagate all the required    
> > >   As it may not be    
> > > > parameters. Fixup function has been added in struct manufacturer to    
> > >             , a fixup        is being added in the manufacturer structure    
> > > > accommodate this.    
> > > 
> > > The fixup function sentence should be dropped from the commit message,
> > > see below.    
> > 
> > Okay, I will create separate patch for fixup function.
> >   
> > >     
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <sshivamurthy@xxxxxxxxxx>
> > > > ---
> > > >  drivers/mtd/nand/spi/core.c | 134    
> > > ++++++++++++++++++++++++++++++++++++    
> > > >  include/linux/mtd/spinand.h |   3 +
> > > >  2 files changed, 137 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > > index 89f6beefb01c..7ae76dab9141 100644
> > > > --- a/drivers/mtd/nand/spi/core.c
> > > > +++ b/drivers/mtd/nand/spi/core.c
> > > > @@ -400,6 +400,131 @@ static int spinand_lock_block(struct    
> > > spinand_device *spinand, u8 lock)    
> > > >  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> > > >  }
> > > >
> > > > +/**
> > > > + * spinand_read_param_page_op - Read parameter page operation    
> > > 
> > > Again, the name in the doc does not fit the function you describe
> > >     
> > > > + * @spinand: the spinand    
> > >                     SPI-NAND chip
> > > 
> > > Shiva, there are way too much typos and shortcuts in your series.
> > > Please be more careful otherwise we can't focus on the technical
> > > aspects. I am not a native English speaker at all but please, plain
> > > English is not C code. We talk SPI-NAND and not spinand, we say
> > > structure and not struct, acronyms are uppercase, etc.
> > >     
> > 
> > Sorry for the inconvenience caused, I will take care from next time.
> >   
> > > > + * @page: page number where parameter page tables can be found    
> > >                               ^ the    
> > > > + * @buf: buffer used to store the parameter page
> > > > + * @len: length of the buffer
> > > > + *
> > > > + * Read parameter page    
> > >           the    
> > > > + *
> > > > + * Returns 0 on success, a negative error code otherwise.
> > > > + */
> > > > +static int spinand_parameter_page_read(struct spinand_device *spinand,
> > > > +				       u8 page, void *buf, unsigned int len)
> > > > +{
> > > > +	struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> > > > +	struct spi_mem_op pread_cache_op =
> > > > +    
> > > 	SPINAND_PAGE_READ_FROM_CACHE_OP(false,    
> > > > +								0,
> > > > +								1,
> > > > +								buf,
> > > > +								len);    
> > > 
> > > That's ok if you cross the 80 characters boundary here. You may put "0,
> > > 1," on the first line and "buf, len);" on the second.
> > >     
> > > > +	u8 feature;
> > > > +	u8 status;
> > > > +	int ret;
> > > > +
> > > > +	if (len && !buf)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* CFG_OTP_ENABLE is used to enable parameter page access */
> > > > +	feature |= CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_wait(spinand, &status);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = spinand_read_reg_op(spinand, REG_CFG,
> > > > +				  &feature);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	feature &= ~CFG_OTP_ENABLE;
> > > > +
> > > > +	spinand_write_reg_op(spinand, REG_CFG, feature);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +    
> > > Add the kernel doc please
> > > 
> > > Change the below function so that it returns 1 if the page was
> > > detected, 0 if it did not, an negative error code otherwise.
> > >     
> > > > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > > > +{
> > > > +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> > > > +	struct nand_memory_organization *memorg;
> > > > +	struct nand_onfi_params *p;
> > > > +	struct nand_device *base = spinand_to_nand(spinand);
> > > > +	int i, ret;
> > > > +
> > > > +	memorg = nanddev_get_memorg(base);
> > > > +
> > > > +	/* Allocate buffer to hold parameter page */
> > > > +	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > > +	if (!p)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) *    
> > > 3);    
> > > > +	if (ret) {
> > > > +		ret = 0;    
> > > 
> > > No, you should return the error in case of error. You will later handle
> > > the fact that there is no parameter page.    
> > 
> > okay.
> >   
> > >     
> > > > +		goto free_param_page;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < 3; i++) {
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==    
> > >                                                            ^
> > > If you force the parameter page to be 254 bytes long it means you limit
> > > yourself to ONFI standard. That's not a problem, but then you should
> > > mention it in the function name.    
> > 
> > okay, I will mention in kernel doc.
> >   
> > >     
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			if (i)
> > > > +				memcpy(p, &p[i], sizeof(*p));
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (i == 3) {
> > > > +		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > > +
> > > > +		pr_warn("Could not find a valid ONFI parameter page, trying    
> > > bit-wise majority to recover it\n");    
> > > > +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > > > +				       sizeof(*p));
> > > > +
> > > > +		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > > > +				le16_to_cpu(p->crc)) {
> > > > +			pr_err("ONFI parameter recovery failed,    
> > > aborting\n");    
> > > > +			goto free_param_page;
> > > > +		}
> > > > +	}    
> > > 
> > > The whole for-loop and the if (i==3) condition is exactly the same as
> > > for raw NANDs and must be extracted in a generic function:
> > > 1/ extract the function from nand/raw/nand_onfi.c and put it in
> > > nand/onfi.c.
> > > 2/ then use it in this patch.

Sounds like a good idea indeed.
  
> > 
> > I have done this intentionally, because in raw NAND case there is function
> > "nand_read_data_op" called inside for-loop. I don't think just for if (i == 3) 
> > it is necessary to create new function.

Well, reading more than just a single param page in a single step
shouldn't hurt. You can let the caller choose the number of page it
wants to read (can be more than 3) and pass this information to the
helper function. You should probably call nand_bit_wise_majority() from
this helper too.

> > 
> > Let me know if you have different opinion.  
> 
> I don't have a strong opinion on that. Boris what do you think? Shall
> we duplicate the code? It's not just about the if condition, it's the
> whole for loop which is very similar.

I think Miquel is right, we shouldn't duplicate this logic.