| Message ID | 20181011074217.10149-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 940ec770c295682993d1cccce3081fd7c74fece8 |
| Headers | show |
| Series | [FIX] spi: bcm-qspi: switch back to reading flash using smaller chunks | expand |
On Thu, 11 Oct 2018 09:42:17 +0200 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Fixing/optimizing bcm_qspi_bspi_read() performance introduced two > changes: > 1) It added a loop to read all requested data using multiple BSPI ops. > 2) It bumped max size of a single BSPI block request from 256 to 512 B. > > The later change resulted in occasional BSPI timeouts causing a > regression. > > For some unknown reason hardware doesn't always handle reads as expected > when using 512 B chunks. In such cases it may happen that BSPI returns > amount of requested bytes without the last 1-3 ones. It provides the > remaining bytes later but doesn't raise an interrupt until another LR > start. > > Switching back to 256 B reads fixes that problem and regression. Kamal, can you help Rafal with this issue? I mean, even if it seems to solve the problem, switching back to 256bytes is not helping us understanding what's going on here. > > Fixes: 345309fa7c0c ("spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance") > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > Cc: stable@vger.kernel.org # 4.11+ > --- > This fixes a regression I reported 2,5 months ago in the e-mail thread: > bcm-qspi: unstable bspi mode since the 345309fa7c0c ("spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance") > --- > drivers/spi/spi-bcm-qspi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c > index eb3d67f01e8c..584bcb018a62 100644 > --- a/drivers/spi/spi-bcm-qspi.c > +++ b/drivers/spi/spi-bcm-qspi.c > @@ -89,7 +89,7 @@ > #define BSPI_BPP_MODE_SELECT_MASK BIT(8) > #define BSPI_BPP_ADDR_SELECT_MASK BIT(16) > > -#define BSPI_READ_LENGTH 512 > +#define BSPI_READ_LENGTH 256 > > /* MSPI register offsets */ > #define MSPI_SPCR0_LSB 0x000
Rafal, "switching back to 256bytes is not helping us understanding what's going on here.. " The read take a different path and use hw acceleration. . It is to do with the BSPI_RAF buffer size. I think it's safe to keep it at 256 and will not impact performance either. Kamal On Fri, Oct 12, 2018 at 5:30 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > On Thu, 11 Oct 2018 09:42:17 +0200 > Rafał Miłecki <zajec5@gmail.com> wrote: > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > Fixing/optimizing bcm_qspi_bspi_read() performance introduced two > > changes: > > 1) It added a loop to read all requested data using multiple BSPI ops. > > 2) It bumped max size of a single BSPI block request from 256 to 512 B. > > > > The later change resulted in occasional BSPI timeouts causing a > > regression. > > > > For some unknown reason hardware doesn't always handle reads as expected > > when using 512 B chunks. In such cases it may happen that BSPI returns > > amount of requested bytes without the last 1-3 ones. It provides the > > remaining bytes later but doesn't raise an interrupt until another LR > > start. > > > > Switching back to 256 B reads fixes that problem and regression. > > Kamal, can you help Rafal with this issue? I mean, even if it seems to > solve the problem, switching back to 256bytes is not helping us > understanding what's going on here. > > > > > Fixes: 345309fa7c0c ("spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance") > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > Cc: stable@vger.kernel.org # 4.11+ > > --- > > This fixes a regression I reported 2,5 months ago in the e-mail thread: > > bcm-qspi: unstable bspi mode since the 345309fa7c0c ("spi: bcm-qspi: Fix bcm_qspi_bspi_read() performance") > > --- > > drivers/spi/spi-bcm-qspi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c > > index eb3d67f01e8c..584bcb018a62 100644 > > --- a/drivers/spi/spi-bcm-qspi.c > > +++ b/drivers/spi/spi-bcm-qspi.c > > @@ -89,7 +89,7 @@ > > #define BSPI_BPP_MODE_SELECT_MASK BIT(8) > > #define BSPI_BPP_ADDR_SELECT_MASK BIT(16) > > > > -#define BSPI_READ_LENGTH 512 > > +#define BSPI_READ_LENGTH 256 > > > > /* MSPI register offsets */ > > #define MSPI_SPCR0_LSB 0x000 >
Hi Kamal, On Fri, 12 Oct 2018 18:06:06 -0400 Kamal Dasu <kdasu.kdev@gmail.com> wrote: > Rafal, > > "switching back to 256bytes is not helping us understanding what's > going on here.. " > > The read take a different path and use hw acceleration. . It is to do > with the BSPI_RAF buffer size. I think it's safe to keep it at 256 > and will not impact performance either. So you don't care about what the actual problem is, or do you confirm that the IP does not support 512B chunk reads and switching to 512B was buggy in the first place? Boris
"So you don't care about what the actual problem is or do you confirm that the IP does not support 512B chunk reads and switching to 512B was buggy in the first place" I will look into it for sure with the specific SoC being used. Meanwhile I think changing back to 256 would be right thing to do. Kamal On Sat, Oct 13, 2018 at 1:45 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > Hi Kamal, > > On Fri, 12 Oct 2018 18:06:06 -0400 > Kamal Dasu <kdasu.kdev@gmail.com> wrote: > > > Rafal, > > > > "switching back to 256bytes is not helping us understanding what's > > going on here.. " > > > > The read take a different path and use hw acceleration. . It is to do > > with the BSPI_RAF buffer size. I think it's safe to keep it at 256 > > and will not impact performance either. > > So you don't care about what the actual problem is, or do you confirm > that the IP does not support 512B chunk reads and switching to 512B was > buggy in the first place? > > Boris
On Sat, 13 Oct 2018 at 22:29, Kamal Dasu <kdasu.kdev@gmail.com> wrote: > "So you don't care about what the actual problem is or do you confirm > that the IP does not support 512B chunk reads and switching to 512B > was > buggy in the first place" > > I will look into it for sure with the specific SoC being used. > Meanwhile I think changing back to 256 would be right thing to do. Let me know if you need testing of any patches (debugging / fixing / whatever)!
Hi Kamal, On Sat, 13 Oct 2018 16:28:59 -0400 Kamal Dasu <kdasu.kdev@gmail.com> wrote: > "So you don't care about what the actual problem is or do you confirm > that the IP does not support 512B chunk reads and switching to 512B > was > buggy in the first place" > > I will look into it for sure with the specific SoC being used. > Meanwhile I think changing back to 256 would be right thing to do. Okay, can you maybe add your A-b/R-b then? Thanks, Boris
diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c index eb3d67f01e8c..584bcb018a62 100644 --- a/drivers/spi/spi-bcm-qspi.c +++ b/drivers/spi/spi-bcm-qspi.c @@ -89,7 +89,7 @@ #define BSPI_BPP_MODE_SELECT_MASK BIT(8) #define BSPI_BPP_ADDR_SELECT_MASK BIT(16) -#define BSPI_READ_LENGTH 512 +#define BSPI_READ_LENGTH 256 /* MSPI register offsets */ #define MSPI_SPCR0_LSB 0x000