Re: [PATCH v6 19/30] mtd: rawnand: Define a unique reset interface configuration

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Rickard Andersson <rickaran@axis.com>,
	Michal Simek <monstr@monstr.eu>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Naga Sureshkumar Relli <nagasure@xilinx.com>
Subject: Re: [PATCH v6 19/30] mtd: rawnand: Define a unique reset interface configuration
Date: Fri, 29 May 2020 11:27:38 +0200	[thread overview]
Message-ID: <20200529112738.2f58858d@xps13> (raw)
In-Reply-To: <20200529093205.626f36d9@collabora.com>

Boris Brezillon <boris.brezillon@collabora.com> wrote on Fri, 29 May
2020 09:32:05 +0200:

> On Fri, 29 May 2020 01:16:01 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > All NAND chips will always use the same interface configuration for
> > reset and at startup: SDR mode 0. Instead of copying around the
> > interface configuration, let's just define a helper to retrieve the
> > reset interface configuration object.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c    | 10 +++-------
> >  drivers/mtd/nand/raw/nand_timings.c |  6 ++++++
> >  include/linux/mtd/rawnand.h         |  3 +++
> >  3 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 07a7d918b462..4a72ec221262 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -928,9 +928,7 @@ static int nand_reset_interface(struct nand_chip *chip, int chipnr)
> >  	 * timings to timing mode 0.
> >  	 */
> >  
> > -	onfi_fill_interface_config(chip, &chip->interface_config,
> > -				   NAND_SDR_IFACE, 0);
> > -	ret = ops->setup_interface(chip, chipnr, &chip->interface_config);
> > +	ret = ops->setup_interface(chip, chipnr, nand_get_reset_interface());
> >  	if (ret)
> >  		pr_err("Failed to configure data interface to SDR timing mode 0\n");
> >  
> > @@ -2483,7 +2481,6 @@ EXPORT_SYMBOL_GPL(nand_subop_get_data_len);
> >   */
> >  int nand_reset(struct nand_chip *chip, int chipnr)
> >  {
> > -	struct nand_interface_config saved_intf_config = chip->interface_config;
> >  	int ret;
> >  
> >  	ret = nand_reset_interface(chip, chipnr);
> > @@ -2508,11 +2505,10 @@ int nand_reset(struct nand_chip *chip, int chipnr)
> >  	 * nand_setup_interface() uses ->set/get_features() which would
> >  	 * fail anyway as the parameter page is not available yet.
> >  	 */
> > -	if (!memcmp(&chip->interface_config, &saved_intf_config,
> > -		    sizeof(saved_intf_config)))
> > +	if (!memcmp(&chip->interface_config, nand_get_reset_interface(),
> > +		    sizeof(chip->interface_config)))
> >  		return 0;
> >  
> > -	chip->interface_config = saved_intf_config;  
> 
> Are you sure you can do that now? Looks like the current interface
> config is not preserved, and you end up with mode 0 after a reset.

You're probably right, I'll move that to the patch handling
interface_config pointers.

> 
> >  	ret = nand_setup_interface(chip, chipnr);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/mtd/nand/raw/nand_timings.c b/drivers/mtd/nand/raw/nand_timings.c
> > index 6c12cd52323d..0089196b9e36 100644
> > --- a/drivers/mtd/nand/raw/nand_timings.c
> > +++ b/drivers/mtd/nand/raw/nand_timings.c
> > @@ -292,6 +292,12 @@ static const struct nand_interface_config onfi_sdr_timings[] = {
> >  	},
> >  };
> >  
> > +/* All NAND chips share the same reset data interface: SDR mode 0 */
> > +const struct nand_interface_config *nand_get_reset_interface(void)
> > +{
> > +	return &onfi_sdr_timings[0];
> > +}
> > +
> >  /**
> >   * onfi_find_closest_sdr_mode - Derive the closest ONFI SDR timing mode given a
> >   *                              set of timings
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index c2fcac38bcc2..b37eb5ee11c8 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -1202,6 +1202,9 @@ static inline struct device_node *nand_get_flash_node(struct nand_chip *chip)
> >  	return mtd_get_of_node(nand_to_mtd(chip));
> >  }
> >  
> > +/* Default/reset data interface */
> > +const struct nand_interface_config *nand_get_reset_interface(void);
> > +
> >  /*
> >   * A helper for defining older NAND chips where the second ID byte fully
> >   * defined the chip, including the geometry (chip size, eraseblock size, page  
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/