| Message ID | 1437056014-16928-1-git-send-email-alexander.stein@systec-electronic.com |
|---|---|
| State | Superseded |
| Headers | show |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/16/2015 10:13 AM, Alexander Stein wrote: > Access is only serialized for each NOR device in spi_nor_lock_and_prep(), > but not for the QSPI device. Hi Alexander, Are both devices that you're testing with in the below scenario QSPI, or is it a combination of QSPI + some other MTD device? Just want to understand how it was tested as I've got hardware here that may let me help out with that... Best regards, - -Cory > We must ensure only one NOR device is accessed as each call to > fsl_qspi_set_base_addr (in fsl_qspi_prep) will affect the effective flash > address. > This can simply be achieved by using a mutex in prepare/unprepare > callbacks. > > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> > --- > This problem can simply be trigged by doing a hexdump on /dev/mtdblock0 > (256 kiB) and /dev/mtdblock3 (64MiB), in my case. > > $ hexdump -C /dev/mtdblock3 # doing a long taking access. > $ hexdump -C /dev/mtdblock0 # in the meanwhile will show data from mtdblock3 > > drivers/mtd/spi-nor/fsl-quadspi.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c > index 5d2df50..a48495a 100644 > --- a/drivers/mtd/spi-nor/fsl-quadspi.c > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c > @@ -226,6 +226,7 @@ struct fsl_qspi { > u32 memmap_phy; > struct clk *clk, *clk_en; > struct device *dev; > + struct mutex lock; /* to serialize access to different mtd partitions */ > struct completion c; > struct fsl_qspi_devtype_data *devtype_data; > u32 nor_size; > @@ -771,6 +772,8 @@ static int fsl_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops) > return ret; > } > > + mutex_lock(&q->lock); > + > fsl_qspi_set_base_addr(q, nor); > return 0; > } > @@ -779,6 +782,8 @@ static void fsl_qspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops) > { > struct fsl_qspi *q = nor->priv; > > + mutex_unlock(&q->lock); > + > clk_disable(q->clk); > clk_disable(q->clk_en); > } > @@ -856,6 +861,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) > q->dev = dev; > q->devtype_data = (struct fsl_qspi_devtype_data *)of_id->data; > platform_set_drvdata(pdev, q); > + mutex_init(&q->lock); > > ret = fsl_qspi_nor_setup(q); > if (ret) > - -- Cory Tusar Principal PID 1 Solutions, Inc. "There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies, and the other way is to make it so complicated that there are no obvious deficiencies." --Sir Charles Anthony Richard Hoare -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlWn6foACgkQHT1tsfGwHJ8pmwCfQsbV/yEhq19HPdcbshjeWnrx Pw0AoJxcek3AjjtPZHSSDEvo81NwaV4H =Rwdj -----END PGP SIGNATURE-----
Hello Cory, On Thursday 16 July 2015 13:29:30, Cory Tusar wrote: > On 07/16/2015 10:13 AM, Alexander Stein wrote: > > Access is only serialized for each NOR device in spi_nor_lock_and_prep(), > > but not for the QSPI device. > > Are both devices that you're testing with in the below scenario QSPI, or > is it a combination of QSPI + some other MTD device? On my board a s70fl01gs is connected to QSPI bank A. As this is a dual-die chip it uses both chip-selcts on bank A. So in the end it's 2 QSPI devices. When accessing different flashs on those 2 chip-selects i get the following error message: > [ 233.157410] fsl-quadspi 1550000.quadspi: cmd 0x05 timeout, addr@00000000, > FR:0x08000000, SR:0x00003c00 [ 233.166731] error -110 reading SR > [ 233.170097] end_request: I/O error, dev mtdblock3, sector 72 > [ 233.175740] Buffer I/O error on device mtdblock3, logical block 9 This seems to be caused by simultaneously access to different chips. Best regards, Alexander
Hi Alexander, I think it is the same patch http://patchwork.ozlabs.org/patch/429631/ On Mon, Jul 20, 2015 at 1:50 AM, Alexander Stein <alexander.stein@systec-electronic.com> wrote: > Hello Cory, > > On Thursday 16 July 2015 13:29:30, Cory Tusar wrote: >> On 07/16/2015 10:13 AM, Alexander Stein wrote: >> > Access is only serialized for each NOR device in spi_nor_lock_and_prep(), >> > but not for the QSPI device. >> >> Are both devices that you're testing with in the below scenario QSPI, or >> is it a combination of QSPI + some other MTD device? > > On my board a s70fl01gs is connected to QSPI bank A. As this is a dual-die > chip it uses both chip-selcts on bank A. So in the end it's 2 QSPI devices. > When accessing different flashs on those 2 chip-selects i get the following > error message: >> [ 233.157410] fsl-quadspi 1550000.quadspi: cmd 0x05 timeout, addr@00000000, >> FR:0x08000000, SR:0x00003c00 [ 233.166731] error -110 reading SR >> [ 233.170097] end_request: I/O error, dev mtdblock3, sector 72 >> [ 233.175740] Buffer I/O error on device mtdblock3, logical block 9 > > This seems to be caused by simultaneously access to different chips. > > Best regards, > Alexander > -- > Dipl.-Inf. Alexander Stein > SYS TEC electronic GmbH > alexander.stein@systec-electronic.com > > Legal and Commercial Address: > Am Windrad 2 > 08468 Heinsdorfergrund > Germany > > Office: +49 (0) 3765 38600-11xx > Fax: +49 (0) 0) 3765 38600-41xx > > Managing Directors: > Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt; > Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp > Commercial Registry: > Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010 > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Han, On Thursday 23 July 2015 10:31:05, Han Xu wrote: > I think it is the same patch > > http://patchwork.ozlabs.org/patch/429631/ Yep. They are more or less the same. I didn't know about this. I lack the include though, which seems the right thing to do. But I see a problem in your case: > @@ -751,6 +753,8 @@ static int fsl_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops) > struct fsl_qspi *q = nor->priv; > int ret; > > + mutex_lock(&q->lock); > + > ret = clk_enable(q->clk_en); > if (ret) > return ret; I see a problem here. If one of those clk_enable fails, you return with the mutex still being locked. That's why I lock/unlock the mutex the last/first in prepare/unprepare. Best regards, Alexander
The link is the first version, please check the one pushed in l2-mtd repo. Thanks. On Thu, Jul 23, 2015 at 10:41 AM, Alexander Stein <alexander.stein@systec-electronic.com> wrote: > Hi Han, > > On Thursday 23 July 2015 10:31:05, Han Xu wrote: >> I think it is the same patch >> >> http://patchwork.ozlabs.org/patch/429631/ > > Yep. They are more or less the same. I didn't know about this. > I lack the include though, which seems the right thing to do. > > But I see a problem in your case: >> @@ -751,6 +753,8 @@ static int fsl_qspi_prep(struct spi_nor *nor, enum > spi_nor_ops ops) >> struct fsl_qspi *q = nor->priv; >> int ret; >> >> + mutex_lock(&q->lock); >> + >> ret = clk_enable(q->clk_en); >> if (ret) >> return ret; > > I see a problem here. If one of those clk_enable fails, you return with the > mutex still being locked. That's why I lock/unlock the mutex the last/first in > prepare/unprepare. > > Best regards, > Alexander > -- > Dipl.-Inf. Alexander Stein > SYS TEC electronic GmbH > alexander.stein@systec-electronic.com > > Legal and Commercial Address: > Am Windrad 2 > 08468 Heinsdorfergrund > Germany > > Office: +49 (0) 3765 38600-0 > Fax: +49 (0) 3765 38600-4100 > > Managing Directors: > Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt; > Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp > Commercial Registry: > Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010 >
On Thursday 23 July 2015 10:48:33, Han Xu wrote: > The link is the first version, please check the one pushed in l2-mtd > repo. Thanks. Thanks for the hint. Didn't know about that repo. Found the commit at http://git.infradead.org/l2-mtd.git/commit/392d39cfcaa9bcfa202fb0fd3bc65f3c20de318f Brian: When does this hit mainline? v4.3? Best regards, Alexander
On Thu, Jul 23, 2015 at 06:00:43PM +0200, Alexander Stein wrote: > On Thursday 23 July 2015 10:48:33, Han Xu wrote: > > The link is the first version, please check the one pushed in l2-mtd > > repo. Thanks. > > Thanks for the hint. Didn't know about that repo. It's included in linux-next.git [1], and it's listed in MAINTAINERS and on the linux-mtd website [2]. There's a full explanation there. > Found the commit at http://git.infradead.org/l2-mtd.git/commit/392d39cfcaa9bcfa202fb0fd3bc65f3c20de318f > > Brian: When does this hit mainline? v4.3? Yes, it'll go out for 4.3-rc1. Brian [1] https://www.kernel.org/doc/man-pages/linux-next.html [2] http://linux-mtd.infradead.org/source.html
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c index 5d2df50..a48495a 100644 --- a/drivers/mtd/spi-nor/fsl-quadspi.c +++ b/drivers/mtd/spi-nor/fsl-quadspi.c @@ -226,6 +226,7 @@ struct fsl_qspi { u32 memmap_phy; struct clk *clk, *clk_en; struct device *dev; + struct mutex lock; /* to serialize access to different mtd partitions */ struct completion c; struct fsl_qspi_devtype_data *devtype_data; u32 nor_size; @@ -771,6 +772,8 @@ static int fsl_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops) return ret; } + mutex_lock(&q->lock); + fsl_qspi_set_base_addr(q, nor); return 0; } @@ -779,6 +782,8 @@ static void fsl_qspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops) { struct fsl_qspi *q = nor->priv; + mutex_unlock(&q->lock); + clk_disable(q->clk); clk_disable(q->clk_en); } @@ -856,6 +861,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) q->dev = dev; q->devtype_data = (struct fsl_qspi_devtype_data *)of_id->data; platform_set_drvdata(pdev, q); + mutex_init(&q->lock); ret = fsl_qspi_nor_setup(q); if (ret)
Access is only serialized for each NOR device in spi_nor_lock_and_prep(), but not for the QSPI device. We must ensure only one NOR device is accessed as each call to fsl_qspi_set_base_addr (in fsl_qspi_prep) will affect the effective flash address. This can simply be achieved by using a mutex in prepare/unprepare callbacks. Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> --- This problem can simply be trigged by doing a hexdump on /dev/mtdblock0 (256 kiB) and /dev/mtdblock3 (64MiB), in my case. $ hexdump -C /dev/mtdblock3 # doing a long taking access. $ hexdump -C /dev/mtdblock0 # in the meanwhile will show data from mtdblock3 drivers/mtd/spi-nor/fsl-quadspi.c | 6 ++++++ 1 file changed, 6 insertions(+)