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

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

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@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.

Signed-off-by: Fabio Estevam <fabio.estevam@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 */
+		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
-- 
1.9.1

[PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When removing the fsl-quadspi module and running 'cat /proc/mtd' afterwards,
we see garbage data like:

$ rmmod  fsl-quadspi
$ cat /proc/mtd
dev:    size   erasesize  name
mtd0: 00000000 00000000 "(null)"
mtd0: 00000000 00000000 "(null)"
mtd0: 00000000 00000000 "(null)"
...
mtd0: a22296c6c756e28 00000000 "(null)"
mtd0: a22296c6c756e28 3064746d "(null)"

The reason for this is due to the wrong mtd index used in
mtd_device_unregister() in the remove function. This index mismatch is caused
by the usage of the 'qspi-has-second-chip' property in the probe function.

Such property is not really necessary and we can simplify the index logic
calculation by avoiding some 'holes' in the mtd indexing.

Thanks to Brian Norris for his suggestion.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Do not change FSL_QSPI_MAX_CHIP
- Fix fsl_qspi_set_base_addr() calculation

 .../devicetree/bindings/mtd/fsl-quadspi.txt        |  9 ---------
 drivers/mtd/spi-nor/fsl-quadspi.c                  | 22 +++++++---------------
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
index 823d134..b8e96ec 100644
--- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
@@ -9,15 +9,6 @@ Required properties:
   - clocks : The clocks needed by the QuadSPI controller
   - clock-names : the name of the clocks
 
-Optional properties:
-  - fsl,qspi-has-second-chip: The controller has two buses, bus A and bus B.
-                              Each bus can be connected with two NOR flashes.
-			      Most of the time, each bus only has one NOR flash
-			      connected, this is the default case.
-			      But if there are two NOR flashes connected to the
-			      bus, you should enable this property.
-			      (Please check the board's schematic.)
-
 Example:
 
 qspi0: quadspi@40044000 {
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 20cffd2..ddb955a 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -661,7 +661,7 @@ MODULE_DEVICE_TABLE(of, fsl_qspi_dt_ids);
 
 static void fsl_qspi_set_base_addr(struct fsl_qspi *q, struct spi_nor *nor)
 {
-	q->chip_base_addr = q->nor_size * (nor - q->nor);
+	q->chip_base_addr = q->nor_size * q->nor_num;
 }
 
 static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
@@ -782,8 +782,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct spi_nor *nor;
 	struct mtd_info *mtd;
-	int ret, i = 0;
-	bool has_second_chip = false;
+	int ret, i;
 	const struct of_device_id *of_id =
 			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
 
@@ -791,8 +790,8 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	if (!q)
 		return -ENOMEM;
 
-	q->nor_num = of_get_child_count(dev->of_node);
-	if (!q->nor_num || q->nor_num > FSL_QSPI_MAX_CHIP)
+	ret = of_get_child_count(dev->of_node);
+	if (!ret || ret > FSL_QSPI_MAX_CHIP)
 		return -ENODEV;
 
 	/* find the resources */
@@ -859,19 +858,12 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	if (ret)
 		goto irq_failed;
 
-	if (of_get_property(np, "fsl,qspi-has-second-chip", NULL))
-		has_second_chip = true;
-
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
 		char modalias[40];
 
-		/* skip the holes */
-		if (!has_second_chip)
-			i *= 2;
-
-		nor = &q->nor[i];
-		mtd = &q->mtd[i];
+		nor = &q->nor[q->nor_num];
+		mtd = &q->mtd[q->nor_num];
 
 		nor->mtd = mtd;
 		nor->dev = dev;
@@ -929,7 +921,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		if (nor->page_size > q->devtype_data->txfifo)
 			nor->page_size = q->devtype_data->txfifo;
 
-		i++;
+		q->nor_num++;
 	}
 
 	/* finish the rest init. */
-- 
1.9.1

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Brian Norris]

OK, I still don't think you've got things right here.

Are you testing this with a two-flash system? What sort of tests?

On Wed, Jan 07, 2015 at 08:32:07AM -0200, Fabio Estevam wrote:
...
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 20cffd2..ddb955a 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -661,7 +661,7 @@ MODULE_DEVICE_TABLE(of, fsl_qspi_dt_ids);
>  
>  static void fsl_qspi_set_base_addr(struct fsl_qspi *q, struct spi_nor *nor)
>  {
> -	q->chip_base_addr = q->nor_size * (nor - q->nor);
> +	q->chip_base_addr = q->nor_size * q->nor_num;

This function is called from the 'prepare' function, so it may be called
for *different* NOR devices, and it's called for each flash transaction,
not just at boot time. Now, you're making the base address into a fixed
constant for all NOR flashes attached to this controller. That's wrong.

So you need an index associated with each NOR, not just the "total
number of NOR flash" (i.e., q->nor_size). That's currently satisfied by
the pointer subtraction on the ->nor[] array.

>  }
>  
>  static int fsl_qspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
...

Brian

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

Hi Brian,

On Fri, Jan 9, 2015 at 6:17 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> OK, I still don't think you've got things right here.
>
> Are you testing this with a two-flash system? What sort of tests?

Yes, there are two qspi flashes on imx6sx-sdb board.

The test I do are:

- module unload/load - When I was adding dts support for imx6sx-sdb I
would like to be able to load/unload the qspi driver. This is not
possible currently, and this was the motivation for this series.

- Read the two flashes.

root@freescale /$ dd if=/dev/mtd0 of=/dev/null bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0MB) copied, 0.047851 seconds, 20.9MB/s

root@freescale /$ dd if=/dev/mtd1 of=/dev/null bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0MB) copied, 0.052515 seconds, 19.0MB/s

Not sure what is the correct way to fix this though.

Regards,

Fabio Estevam

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Brian Norris]

Hi Fabio,

On Tue, Jan 13, 2015 at 01:35:02PM -0200, Fabio Estevam wrote:
> On Fri, Jan 9, 2015 at 6:17 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > OK, I still don't think you've got things right here.
> >
> > Are you testing this with a two-flash system? What sort of tests?
> 
> Yes, there are two qspi flashes on imx6sx-sdb board.
> 
> The test I do are:
> 
> - module unload/load - When I was adding dts support for imx6sx-sdb I
> would like to be able to load/unload the qspi driver. This is not
> possible currently, and this was the motivation for this series.

Right. So that's a good test.

> - Read the two flashes.

Are you doing any verification to make sure you're reading the *correct*
data? I'd imagine from some what I see in your patches, that you might
actually be reading from the wrong flash.

> root@freescale /$ dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> 1+0 records in
> 1+0 records out
> 1048576 bytes (1.0MB) copied, 0.047851 seconds, 20.9MB/s
> 
> root@freescale /$ dd if=/dev/mtd1 of=/dev/null bs=1M count=1
> 1+0 records in
> 1+0 records out
> 1048576 bytes (1.0MB) copied, 0.052515 seconds, 19.0MB/s
> 
> Not sure what is the correct way to fix this though.

Brian

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

Hi Brian,

On Tue, Jan 13, 2015 at 4:51 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

>> - Read the two flashes.
>
> Are you doing any verification to make sure you're reading the *correct*
> data? I'd imagine from some what I see in your patches, that you might
> actually be reading from the wrong flash.

Yes, you are right. Just confirmed that with this v3 applied I erased
/dev/mtd0, but that also incorrectly erased /dev/mtd1.

Now I came back to the original v1 patch: isn't it the simpler
approach for fixing the module load/unload crash problem?

It only keeps the mtd unregistration index in sync with registration
and doesn't touch other areas of the driver.

IMHO it is an improvement over the current situation.

I agree that this driver needs more rework, but I am not able to put
it on such good state.

Please let me know your opinion.

Thanks,

Fabio Estevam

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Brian Norris]

Hi Fabio,

On Tue, Jan 13, 2015 at 07:45:22PM -0200, Fabio Estevam wrote:
> On Tue, Jan 13, 2015 at 4:51 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> >> - Read the two flashes.
> >
> > Are you doing any verification to make sure you're reading the *correct*
> > data? I'd imagine from some what I see in your patches, that you might
> > actually be reading from the wrong flash.
> 
> Yes, you are right. Just confirmed that with this v3 applied I erased
> /dev/mtd0, but that also incorrectly erased /dev/mtd1.
> 
> Now I came back to the original v1 patch: isn't it the simpler
> approach for fixing the module load/unload crash problem?
> 
> It only keeps the mtd unregistration index in sync with registration
> and doesn't touch other areas of the driver.
> 
> IMHO it is an improvement over the current situation.

Right, I thought your original patch was an improvement, but it did
still leave some of the error path broken. And as Huang mentioned,
multiple devices were never actually *properly* supported, so there were
problems there.

> I agree that this driver needs more rework, but I am not able to put
> it on such good state.

I might be OK with taking v1 if we can do the following:

(1) Identify who will take responsibility for testing and improving this
driver. We might even add a MAINTAINERS entry

(2) Get an 'ack' from said person

(3) Document the known issues with this driver; add some TODO/FIXME
comments, and maybe disable potentially broken features until they get
fixed (e.g., only probe up to 1 flash, leaving the second flash
disabled)

Huang mentioned that Frank may be interested in (1).

Brian

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

Hi Brian,

On Tue, Jan 13, 2015 at 7:58 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

> Right, I thought your original patch was an improvement, but it did
> still leave some of the error path broken. And as Huang mentioned,
> multiple devices were never actually *properly* supported, so there were
> problems there.
>
>> I agree that this driver needs more rework, but I am not able to put
>> it on such good state.
>
> I might be OK with taking v1 if we can do the following:

Sounds like a good deal, thanks.

>
> (1) Identify who will take responsibility for testing and improving this
> driver. We might even add a MAINTAINERS entry

Frank or Allen, would any of you be interested in becoming the
maintainer of fsl-quadspi?

> (2) Get an 'ack' from said person

I will resend v1 with everyone on Cc.

> (3) Document the known issues with this driver; add some TODO/FIXME
> comments, and maybe disable potentially broken features until they get
> fixed (e.g., only probe up to 1 flash, leaving the second flash
> disabled)

Frank/Allen/Huang, could you please prepare such patch?

Thanks,

Fabio Estevam

RE: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Frank.Li]

> Hi Fabio,
> 
> On Tue, Jan 13, 2015 at 07:45:22PM -0200, Fabio Estevam wrote:
> > On Tue, Jan 13, 2015 at 4:51 PM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> >
> > >> - Read the two flashes.
> > >
> > > Are you doing any verification to make sure you're reading the
> > > *correct* data? I'd imagine from some what I see in your patches,
> > > that you might actually be reading from the wrong flash.
> >
> > Yes, you are right. Just confirmed that with this v3 applied I erased
> > /dev/mtd0, but that also incorrectly erased /dev/mtd1.
> >
> > Now I came back to the original v1 patch: isn't it the simpler
> > approach for fixing the module load/unload crash problem?
> >
> > It only keeps the mtd unregistration index in sync with registration
> > and doesn't touch other areas of the driver.
> >
> > IMHO it is an improvement over the current situation.
> 
> Right, I thought your original patch was an improvement, but it did still leave
> some of the error path broken. And as Huang mentioned, multiple devices
> were never actually *properly* supported, so there were problems there.
> 
> > I agree that this driver needs more rework, but I am not able to put
> > it on such good state.
> 
> I might be OK with taking v1 if we can do the following:
> 
> (1) Identify who will take responsibility for testing and improving this driver.
> We might even add a MAINTAINERS entry

Our QSPI driver owner Xu han (han.xu@freescale.com) will take responsibility to test and improve
This driver.  I will help him to be familiar with linux kernel community work. 

Best regards
Frank Li

> 
> (2) Get an 'ack' from said person
> 
> (3) Document the known issues with this driver; add some TODO/FIXME
> comments, and maybe disable potentially broken features until they get
> fixed (e.g., only probe up to 1 flash, leaving the second flash
> disabled)
> 
> Huang mentioned that Frank may be interested in (1).
> 
> Brian

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Huang Shijie]

On Tue, Jan 13, 2015 at 10:05:29PM +0000, Frank.Li@freescale.com wrote:
> 
> > Hi Fabio,
> > 
> > On Tue, Jan 13, 2015 at 07:45:22PM -0200, Fabio Estevam wrote:
> > > On Tue, Jan 13, 2015 at 4:51 PM, Brian Norris
> > > <computersforpeace@gmail.com> wrote:
> > >
> > > >> - Read the two flashes.
> > > >
> > > > Are you doing any verification to make sure you're reading the
> > > > *correct* data? I'd imagine from some what I see in your patches,
> > > > that you might actually be reading from the wrong flash.
> > >
> > > Yes, you are right. Just confirmed that with this v3 applied I erased
> > > /dev/mtd0, but that also incorrectly erased /dev/mtd1.
> > >
> > > Now I came back to the original v1 patch: isn't it the simpler
> > > approach for fixing the module load/unload crash problem?
> > >
> > > It only keeps the mtd unregistration index in sync with registration
> > > and doesn't touch other areas of the driver.
> > >
> > > IMHO it is an improvement over the current situation.
> > 
> > Right, I thought your original patch was an improvement, but it did still leave
> > some of the error path broken. And as Huang mentioned, multiple devices
> > were never actually *properly* supported, so there were problems there.
> > 
> > > I agree that this driver needs more rework, but I am not able to put
> > > it on such good state.
> > 
> > I might be OK with taking v1 if we can do the following:
> > 
> > (1) Identify who will take responsibility for testing and improving this driver.
> > We might even add a MAINTAINERS entry
> 
> Our QSPI driver owner Xu han (han.xu@freescale.com) will take responsibility to test and improve
> This driver.  I will help him to be familiar with linux kernel community work. 
Good news. 

Huang Shijie

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

Hi Xu Han,

On Tue, Jan 13, 2015 at 8:05 PM, Frank.Li@freescale.com
<Frank.Li@freescale.com> wrote:

> Our QSPI driver owner Xu han (han.xu@freescale.com) will take responsibility to test and improve
> This driver.  I will help him to be familiar with linux kernel community work.

That's great!

Could you please send a patch adding yourself as the maintainer for
the fsl-quadspi driver?

Regards,

Fabio Estevam

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Brian Norris]

On Wed, Jan 14, 2015 at 06:26:38PM -0200, Fabio Estevam wrote:
> On Tue, Jan 13, 2015 at 8:05 PM, Frank.Li@freescale.com
> <Frank.Li@freescale.com> wrote:
> 
> > Our QSPI driver owner Xu han (han.xu@freescale.com) will take responsibility to test and improve
> > This driver.  I will help him to be familiar with linux kernel community work.
> 
> That's great!

Yes, it'll be great to be sure there is at least one "owner" who gets
CC'd on these types of patches.

> Could you please send a patch adding yourself as the maintainer for
> the fsl-quadspi driver?

I'd be OK with multiple developers as reviewers if more desire to do so,
given that Xu sounds to be new to our development model. Mainly I'm
interested in at least one developer who has access to and knowledge of
the hardware, and who can test, review, and ack/nak patches. I'll still
handle merging the patches, of course.

Brian

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Han Xu]

On Wed, Jan 14, 2015 at 4:53 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Jan 14, 2015 at 06:26:38PM -0200, Fabio Estevam wrote:
>> On Tue, Jan 13, 2015 at 8:05 PM, Frank.Li@freescale.com
>> <Frank.Li@freescale.com> wrote:
>>
>> > Our QSPI driver owner Xu han (han.xu@freescale.com) will take responsibility to test and improve
>> > This driver.  I will help him to be familiar with linux kernel community work.
>>
>> That's great!
>
> Yes, it'll be great to be sure there is at least one "owner" who gets
> CC'd on these types of patches.
>
>> Could you please send a patch adding yourself as the maintainer for
>> the fsl-quadspi driver?
>
> I'd be OK with multiple developers as reviewers if more desire to do so,
> given that Xu sounds to be new to our development model. Mainly I'm

OK for me. I would love to maintain freescale qspi module and continue
improve it.

> interested in at least one developer who has access to and knowledge of
> the hardware, and who can test, review, and ack/nak patches. I'll still
> handle merging the patches, of course.
>
> Brian
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Han Xu]

On Tue, Jan 13, 2015 at 3:58 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi Fabio,
>
> On Tue, Jan 13, 2015 at 07:45:22PM -0200, Fabio Estevam wrote:
>> On Tue, Jan 13, 2015 at 4:51 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>
>> >> - Read the two flashes.
>> >
>> > Are you doing any verification to make sure you're reading the *correct*
>> > data? I'd imagine from some what I see in your patches, that you might
>> > actually be reading from the wrong flash.
>>
>> Yes, you are right. Just confirmed that with this v3 applied I erased
>> /dev/mtd0, but that also incorrectly erased /dev/mtd1.
>>
>> Now I came back to the original v1 patch: isn't it the simpler
>> approach for fixing the module load/unload crash problem?
>>
>> It only keeps the mtd unregistration index in sync with registration
>> and doesn't touch other areas of the driver.
>>
>> IMHO it is an improvement over the current situation.
>
> Right, I thought your original patch was an improvement, but it did
> still leave some of the error path broken. And as Huang mentioned,
> multiple devices were never actually *properly* supported, so there were
> problems there.
>
>> I agree that this driver needs more rework, but I am not able to put
>> it on such good state.
>
> I might be OK with taking v1 if we can do the following:
>
> (1) Identify who will take responsibility for testing and improving this
> driver. We might even add a MAINTAINERS entry

I will send a patch to add myself as freescale qspi maintainer, but
don't know the
patch should be send to whom and cc to which maillist?
>
> (2) Get an 'ack' from said person
>
> (3) Document the known issues with this driver; add some TODO/FIXME
> comments, and maybe disable potentially broken features until they get
> fixed (e.g., only probe up to 1 flash, leaving the second flash
> disabled)
>
> Huang mentioned that Frank may be interested in (1).
>
> Brian
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 2/2] mtd: fsl-quadspi: Fix module unbound

[Fabio Estevam]

On Thu, Jan 15, 2015 at 2:34 PM, Han Xu <xhnjupt@gmail.com> wrote:

> I will send a patch to add myself as freescale qspi maintainer, but
> don't know the
> patch should be send to whom and cc to which maillist?

You should send it to Brian with linux-mtd list on Cc.

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

[Brian Norris]

On Wed, Jan 07, 2015 at 08:32:06AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@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.

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

> Signed-off-by: Fabio Estevam <fabio.estevam@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.

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

BTW, do we want a MAINTAINERS entry for this driver?

Brian

Re: [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@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@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