RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Enabled support to detect parameter page

[Shivamurthy Shastri (sshivamurthy)]

Hi Miquel,

> 
> Hi Shivamurthy,
> 
> "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@xxxxxxxxxx> wrote
> on
> Tue, 26 Mar 2019 10:52:00 +0000:
> 
> > Some of the SPI NAND devices has parameter page which is similar to ONFI
> > table.
> >
> > But, it may not be self sufficient to propagate all the required
> > parameters. Fixup function has been added in struct manufacturer to
> > accommodate this.
> >
> > Signed-off-by: Shivamurthy Shastri <sshivamurthy@xxxxxxxxxx>
> > ---
> >  drivers/mtd/nand/spi/core.c | 113
> +++++++++++++++++++++++++++++++++++-
> >  include/linux/mtd/spinand.h |   5 ++
> >  2 files changed, 117 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 985ad52cdaa7..40882a1d2bc1 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -574,6 +574,108 @@ 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
> > + * @spinand: the spinand device
> > + * @page: page number where parameter page tables can be found
> > + * @parameters: buffer used to store the parameter page
> 
> Does not match the prototype

I will fix this in next version.

> 
> > + * @len: length of the buffer
> > + *
> > + * Read parameter page
> > + *
> > + * Returns 0 on success, a negative error code otherwise.
> > + */
> > +static int spinand_parameter_page_read(struct nand_device *base,
> 
> Please use a spinand structure as parameter, you don't need a
> nand_device here (same for other spinand functions).

This function is helper function for generic ONFI layer.
From generic ONFI layer, I can get only nand_device.

> 
> > +				       u8 page, void *buf, unsigned int len)
> > +{
> > +	struct spinand_device *spinand = nand_to_spinand(base);
> > +	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);
> > +	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 1;
> 
> Should return 0

I will fix this in next version.

> 
> > +}
> > +
> > +static int check_version(struct nand_device *base,
> > +			 struct nand_onfi_params *p, int *onfi_version)
> > +{
> > +	/**
> 
> I don't think you need these /** here, just use /*

I will fix this in next version.

> 
> > +	 * SPI NAND do not support ONFI standard
> 
> What about:
> 
>         /*
>          * SPI NANDs do not necessarily support ONFI standard, but
>          * parameter page looks the same as an ONFI table.
>          */
> 

This looks better, I will use this.

> > +	 * But, parameter page looks same as ONFI table
> > +	 */
> > +	if (!le16_to_cpu(p->revision))
> > +		*onfi_version = 0;
> > +
> > +	return 1;
> 
> Functions should return 0 in normal state, not 1.
> 

I will fix this in next version.

> > +}
> > +
> > +static int spinand_intf_data(struct nand_device *base,
> > +			     struct nand_onfi_params *p)
> > +{
> > +	struct spinand_device *spinand = nand_to_spinand(base);
> > +
> > +	/**
> > +	 *	Manufacturers may interpret the parameter page differently
> 
>            ^^^^^ extra spaces
> 

I will fix this in next version.

> > +	 */
> 
> One-line comment should be in the form /* <comment> */
> 
> > +	if (spinand->manufacturer->ops->fixup_param_page)
> > +		spinand->manufacturer->ops->fixup_param_page(spinand,
> p);
> > +
> > +	return 1;
> 
>         return 0;

I will fix this in next version.

> 
> > +}
> > +
> > +static int spinand_param_page_detect(struct spinand_device *spinand)
> > +{
> > +	struct nand_device *base = spinand_to_nand(spinand);
> > +
> > +	base->helper.page = 0x01;
> > +	base->helper.check_revision = check_version;
> > +	base->helper.parameter_page_read =
> spinand_parameter_page_read;
> > +	base->helper.init_intf_data = spinand_intf_data;
> > +
> > +	return nand_onfi_detect(base);
> > +}
> > +
> >  static int spinand_read_page(struct spinand_device *spinand,
> >  			     const struct nand_page_io_req *req)
> >  {
> > @@ -896,7 +998,7 @@ static void spinand_manufacturer_cleanup(struct
> spinand_device *spinand)
> >  		return spinand->manufacturer->ops->cleanup(spinand);
> >  }
> >
> > -static const struct spi_mem_op *
> > +const struct spi_mem_op *
> >  spinand_select_op_variant(struct spinand_device *spinand,
> >  			  const struct spinand_op_variants *variants)
> >  {
> > @@ -1012,6 +1114,15 @@ static int spinand_detect(struct spinand_device
> *spinand)
> >  		return ret;
> >  	}
> >
> > +	if (!spinand->base.memorg.pagesize) {
> > +		ret = spinand_param_page_detect(spinand);
> > +		if (ret < 0) {
> > +			dev_err(dev, "no parameter page for %*phN\n",
> > +				SPINAND_MAX_ID_LEN, spinand->id.data);
> > +			return ret;
> > +		}
> > +	}
> > +
> 
> I think this could be added in a separate patch. Is this the only
> condition where we want to read the param page ?
> 

I will make separate patch if possible.
spinand_manufacturer_detect will not initialize memorg, I think it is 
enough to check memorg.pagesize is initialized or not.
Suggest me if you have any other thoughts.

> >  	if (nand->memorg.ntargets > 1 && !spinand->select_target) {
> >  		dev_err(dev,
> >  			"SPI NANDs with more than one die must implement
> ->select_target()\n");
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index d093d237fba8..57b3b5b075f2 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -179,6 +179,8 @@ struct spinand_manufacturer_ops {
> >  	int (*detect)(struct spinand_device *spinand);
> >  	int (*init)(struct spinand_device *spinand);
> >  	void (*cleanup)(struct spinand_device *spinand);
> > +	void (*fixup_param_page)(struct spinand_device *spinand,
> > +				 struct nand_onfi_params *p);
> 
> You could probably add this hook in a separate patch.

I will fix this in new version.

> 
> >  };
> >
> >  /**
> > @@ -429,4 +431,7 @@ int spinand_match_and_init(struct spinand_device
> *dev,
> >  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
> >  int spinand_select_target(struct spinand_device *spinand, unsigned int
> target);
> >
> > +const struct spi_mem_op *spinand_select_op_variant(struct
> spinand_device *spinand,
> > +	   const struct spinand_op_variants *variants);
> 
> What is it for?

Ah, it should be in next patch no here.
 
> 
> > +
> >  #endif /* __LINUX_MTD_SPINAND_H */
> 
> 
> 
> 
> Thanks,
> MiquÃl