[PATCH v3 1/2] mtd: fsl-quadspi: Call fsl_qspi_set_base_addr after nor_size is set

[Huang Shijie]

On Fri, Jan 09, 2015 at 12:26:54PM -0800, Brian Norris wrote:

I planned to review this patch yesterday. But I was blocked by something.

> On Wed, Jan 07, 2015 at 08:32:06AM -0200, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam at freescale.com>
> > 
> > fsl_qspi_set_base_addr() uses nor_size information, but it is called prior to
> > the initialization of nor_size.
> > 
> > Fix it by calling fsl_qspi_set_base_addr() after nor_size is configured.
> 
> This patch doesn't look correct either. But then, the original driver
> seems confusing too.
Do you have a datasheet for this controller?
The controller has two buses: bus A and bus B.
We can attach two flashes to each bus, just like

      ______
     |      |  bus A
     |      | ----->  (flash 1 and flash 2)
     |      |
     |      |  bus B     
     |      | ----->  (flash 3 and flash 4)
     |______|
           

 These four flashes are mmapped to the four contiguous memory spaces for
 this controller, assume it case 1: 
      (flash 1)  ====> [0   ~ 16M)
      (flash 2)  ====> [16M ~ 32M)
      (flash 3)  ====> [32M ~ 48M)
      (flash 4)  ====> [48M ~ 64M)

 But most of the time, we only attach one flash to each bus, so the memory space we use like 
 this, assume it the case 2:
      (flash 1)  ====> [0   ~ 16M)
      (flash 2)  ====> not exist
      (flash 3)  ====> [32M ~ 48M)
      (flash 4)  ====> not exist.


 That's why the "fsl,qspi-has-second-chip" is needed here. 
 If we remove this property,
         (1) it means case 1 if set 4 child node for the quadspi driver.
         (2) it means case 2 if set 2 child node for the quadspi driver.

 I thinks we should add the above description for the driver's document
 file. Brian, Do you think it is okay?

> 
> Huang, is this driver assuming that all flashes on this controller will
> be the same size? Or maybe I'm not understanding your MMIO layout. But I
Yes. We always attach the same size NOR to the quadspi controller.

> notice that 'nor_size' is shared between all NOR instances on this
> controller. And for that matter, I don't see any locking. Are you sure
> this driver is safe for multiple flash instances?
It is not safe now. The current quadspi driver is just a basic driver
which can pass the generic tests. I ever was planed to some locks for the
multiple flashes access. I think we still need an extra patch for the
multiple flashes case. 

> 
> > Signed-off-by: Fabio Estevam <fabio.estevam at freescale.com>
> > ---
> > Changes since v2:
> > - Newly introduced in this version
> > 
> >  drivers/mtd/spi-nor/fsl-quadspi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 39763b9..20cffd2 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -897,9 +897,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >  		if (ret < 0)
> >  			goto map_failed;
> >  
> > -		/* set the chip address for READID */
> > -		fsl_qspi_set_base_addr(q, nor);
> > -
> >  		ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
> >  		if (ret)
> >  			goto map_failed;
> > @@ -917,6 +914,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >  			fsl_qspi_set_map_addr(q);
> >  		}
> >  
> > +		/* set the chip address for READID */
> 
> OK, so this is to get READID correct... but you're doing this after
> READID. So is this for configuring the *next* flash? I'm confused.

Yes. This patch is not correct. As Brian said, you move the
fsl_qspi_set_base_addr() after the spi_nor_scan. So the spi_nor_scan
will fail.
> 
> > +		fsl_qspi_set_base_addr(q, nor);
> > +
> >  		/*
> >  		 * The TX FIFO is 64 bytes in the Vybrid, but the Page Program
> >  		 * may writes 265 bytes per time. The write is working in the
> 
> I don't think I want to take any of these patches until I get a clearer
> picture of who/what/how you're testing these, and I get an ack from
> Huang (or someone else who is going to be the de factor / official
> maintianer of this driver, since I note that Huang is no longer with
> Freescale).
In actually, the one who will maintain this driver is blocked by some
NAND issues. Before he can take over this driver, i can help to maintain
this driver during this time gap.


thanks
Huang Shijie