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

[Miquel Raynal]

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

> + * @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).

> +				       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

> +}
> +
> +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 /*

> +	 * 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.
         */

> +	 * 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.

> +}
> +
> +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

> +	 */

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;

> +}
> +
> +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 ?

>  	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.

>  };
>  
>  /**
> @@ -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?

> +
>  #endif /* __LINUX_MTD_SPINAND_H */




Thanks,
MiquÃl