Re: [PATCH RFC] spi-nor: provide a range for poll_timout
On Sun, Feb 12, 2017 at 10:59:23PM +0100, Boris Brezillon wrote: > +Mika > > On Sun, 12 Feb 2017 17:42:57 +0100 > Nicholas Mc Guirewrote: > > > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is > > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us > > to readl_poll_timeout() is set to 0. As this is never called in an atomic > > context sleeping should be no issue and there is no reasons for the > > tight-loop here. > > Hm, let's wait for Mika's feedback on this one. BTW, can you please Cc > him on you other spi-nor/intel patches and prefix your patches with the > driver name ('mtd: spi-nor: intel: ') so that we know where the changes > are made (without this prefix it looks like you're touching core files)? > will do - just saw that his email shows up in git plame which I normally also include along with what scripts/get_maintainer.pl -f shows - sorry for the ommision. thx! hofrat > > > > > Signed-off-by: Nicholas Mc Guire > > --- > > > > Problem located by experimental coccinelle script: > > ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for > > delay INTEL_SPI_TIMEOUT * 1000 > > ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for > > delay INTEL_SPI_TIMEOUT * 1000 > > > > The rational for setting the delay_us here to 40 is that > > readx_poll_timeout() > > will take delay_us >> 2 + 1 as min value and that should be at least 10us > > (see > > Documentation/timers/timers-howto.txt). Ideally the delay would be made > > even larger to keep the load on the hrtimer subsystem low as these delays > > here do not seem to be critical. Someone that knows the details of this > > device > > would need to check if a larger delay would be ok here. > > > > Patch was compile tested with: multi_v7_defconfig (implies > > CONFIG_MTD_SPI_NOR=y) > > one coccicheck finding reported and one spars finding (in separate patches) > > > > Patch is against 4.10-rc6 (localversion-next is next-20170210) > > > > drivers/mtd/spi-nor/intel-spi.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/intel-spi.c > > b/drivers/mtd/spi-nor/intel-spi.c > > index a10f602..371bcf9 100644 > > --- a/drivers/mtd/spi-nor/intel-spi.c > > +++ b/drivers/mtd/spi-nor/intel-spi.c > > @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi > > *ispi) > > u32 val; > > > > return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, > > - !(val & HSFSTS_CTL_SCIP), 0, > > + !(val & HSFSTS_CTL_SCIP), 40, > > INTEL_SPI_TIMEOUT * 1000); > > } > > > > @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi > > *ispi) > > u32 val; > > > > return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, > > - !(val & SSFSTS_CTL_SCIP), 0, > > + !(val & SSFSTS_CTL_SCIP), 40, > > INTEL_SPI_TIMEOUT * 1000); > > } > > >
Re: [PATCH RFC] spi-nor: provide a range for poll_timout
On Sun, Feb 12, 2017 at 10:59:23PM +0100, Boris Brezillon wrote: > +Mika > > On Sun, 12 Feb 2017 17:42:57 +0100 > Nicholas Mc Guire wrote: > > > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is > > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us > > to readl_poll_timeout() is set to 0. As this is never called in an atomic > > context sleeping should be no issue and there is no reasons for the > > tight-loop here. > > Hm, let's wait for Mika's feedback on this one. BTW, can you please Cc > him on you other spi-nor/intel patches and prefix your patches with the > driver name ('mtd: spi-nor: intel: ') so that we know where the changes > are made (without this prefix it looks like you're touching core files)? > will do - just saw that his email shows up in git plame which I normally also include along with what scripts/get_maintainer.pl -f shows - sorry for the ommision. thx! hofrat > > > > > Signed-off-by: Nicholas Mc Guire > > --- > > > > Problem located by experimental coccinelle script: > > ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for > > delay INTEL_SPI_TIMEOUT * 1000 > > ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for > > delay INTEL_SPI_TIMEOUT * 1000 > > > > The rational for setting the delay_us here to 40 is that > > readx_poll_timeout() > > will take delay_us >> 2 + 1 as min value and that should be at least 10us > > (see > > Documentation/timers/timers-howto.txt). Ideally the delay would be made > > even larger to keep the load on the hrtimer subsystem low as these delays > > here do not seem to be critical. Someone that knows the details of this > > device > > would need to check if a larger delay would be ok here. > > > > Patch was compile tested with: multi_v7_defconfig (implies > > CONFIG_MTD_SPI_NOR=y) > > one coccicheck finding reported and one spars finding (in separate patches) > > > > Patch is against 4.10-rc6 (localversion-next is next-20170210) > > > > drivers/mtd/spi-nor/intel-spi.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/intel-spi.c > > b/drivers/mtd/spi-nor/intel-spi.c > > index a10f602..371bcf9 100644 > > --- a/drivers/mtd/spi-nor/intel-spi.c > > +++ b/drivers/mtd/spi-nor/intel-spi.c > > @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi > > *ispi) > > u32 val; > > > > return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, > > - !(val & HSFSTS_CTL_SCIP), 0, > > + !(val & HSFSTS_CTL_SCIP), 40, > > INTEL_SPI_TIMEOUT * 1000); > > } > > > > @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi > > *ispi) > > u32 val; > > > > return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, > > - !(val & SSFSTS_CTL_SCIP), 0, > > + !(val & SSFSTS_CTL_SCIP), 40, > > INTEL_SPI_TIMEOUT * 1000); > > } > > >
Re: [PATCH RFC] spi-nor: provide a range for poll_timout
+Mika On Sun, 12 Feb 2017 17:42:57 +0100 Nicholas Mc Guirewrote: > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us > to readl_poll_timeout() is set to 0. As this is never called in an atomic > context sleeping should be no issue and there is no reasons for the > tight-loop here. Hm, let's wait for Mika's feedback on this one. BTW, can you please Cc him on you other spi-nor/intel patches and prefix your patches with the driver name ('mtd: spi-nor: intel: ') so that we know where the changes are made (without this prefix it looks like you're touching core files)? Thanks, Boris > > Signed-off-by: Nicholas Mc Guire > --- > > Problem located by experimental coccinelle script: > ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for > delay INTEL_SPI_TIMEOUT * 1000 > ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for > delay INTEL_SPI_TIMEOUT * 1000 > > The rational for setting the delay_us here to 40 is that readx_poll_timeout() > will take delay_us >> 2 + 1 as min value and that should be at least 10us (see > Documentation/timers/timers-howto.txt). Ideally the delay would be made > even larger to keep the load on the hrtimer subsystem low as these delays > here do not seem to be critical. Someone that knows the details of this device > would need to check if a larger delay would be ok here. > > Patch was compile tested with: multi_v7_defconfig (implies > CONFIG_MTD_SPI_NOR=y) > one coccicheck finding reported and one spars finding (in separate patches) > > Patch is against 4.10-rc6 (localversion-next is next-20170210) > > drivers/mtd/spi-nor/intel-spi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c > index a10f602..371bcf9 100644 > --- a/drivers/mtd/spi-nor/intel-spi.c > +++ b/drivers/mtd/spi-nor/intel-spi.c > @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, > - !(val & HSFSTS_CTL_SCIP), 0, > + !(val & HSFSTS_CTL_SCIP), 40, > INTEL_SPI_TIMEOUT * 1000); > } > > @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, > - !(val & SSFSTS_CTL_SCIP), 0, > + !(val & SSFSTS_CTL_SCIP), 40, > INTEL_SPI_TIMEOUT * 1000); > } >
Re: [PATCH RFC] spi-nor: provide a range for poll_timout
+Mika On Sun, 12 Feb 2017 17:42:57 +0100 Nicholas Mc Guire wrote: > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us > to readl_poll_timeout() is set to 0. As this is never called in an atomic > context sleeping should be no issue and there is no reasons for the > tight-loop here. Hm, let's wait for Mika's feedback on this one. BTW, can you please Cc him on you other spi-nor/intel patches and prefix your patches with the driver name ('mtd: spi-nor: intel: ') so that we know where the changes are made (without this prefix it looks like you're touching core files)? Thanks, Boris > > Signed-off-by: Nicholas Mc Guire > --- > > Problem located by experimental coccinelle script: > ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for > delay INTEL_SPI_TIMEOUT * 1000 > ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for > delay INTEL_SPI_TIMEOUT * 1000 > > The rational for setting the delay_us here to 40 is that readx_poll_timeout() > will take delay_us >> 2 + 1 as min value and that should be at least 10us (see > Documentation/timers/timers-howto.txt). Ideally the delay would be made > even larger to keep the load on the hrtimer subsystem low as these delays > here do not seem to be critical. Someone that knows the details of this device > would need to check if a larger delay would be ok here. > > Patch was compile tested with: multi_v7_defconfig (implies > CONFIG_MTD_SPI_NOR=y) > one coccicheck finding reported and one spars finding (in separate patches) > > Patch is against 4.10-rc6 (localversion-next is next-20170210) > > drivers/mtd/spi-nor/intel-spi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c > index a10f602..371bcf9 100644 > --- a/drivers/mtd/spi-nor/intel-spi.c > +++ b/drivers/mtd/spi-nor/intel-spi.c > @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, > - !(val & HSFSTS_CTL_SCIP), 0, > + !(val & HSFSTS_CTL_SCIP), 40, > INTEL_SPI_TIMEOUT * 1000); > } > > @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, > - !(val & SSFSTS_CTL_SCIP), 0, > + !(val & SSFSTS_CTL_SCIP), 40, > INTEL_SPI_TIMEOUT * 1000); > } >
[PATCH RFC] spi-nor: provide a range for poll_timout
The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us to readl_poll_timeout() is set to 0. As this is never called in an atomic context sleeping should be no issue and there is no reasons for the tight-loop here. Signed-off-by: Nicholas Mc Guire--- Problem located by experimental coccinelle script: ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000 ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000 The rational for setting the delay_us here to 40 is that readx_poll_timeout() will take delay_us >> 2 + 1 as min value and that should be at least 10us (see Documentation/timers/timers-howto.txt). Ideally the delay would be made even larger to keep the load on the hrtimer subsystem low as these delays here do not seem to be critical. Someone that knows the details of this device would need to check if a larger delay would be ok here. Patch was compile tested with: multi_v7_defconfig (implies CONFIG_MTD_SPI_NOR=y) one coccicheck finding reported and one spars finding (in separate patches) Patch is against 4.10-rc6 (localversion-next is next-20170210) drivers/mtd/spi-nor/intel-spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c index a10f602..371bcf9 100644 --- a/drivers/mtd/spi-nor/intel-spi.c +++ b/drivers/mtd/spi-nor/intel-spi.c @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi) u32 val; return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, - !(val & HSFSTS_CTL_SCIP), 0, + !(val & HSFSTS_CTL_SCIP), 40, INTEL_SPI_TIMEOUT * 1000); } @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi) u32 val; return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, - !(val & SSFSTS_CTL_SCIP), 0, + !(val & SSFSTS_CTL_SCIP), 40, INTEL_SPI_TIMEOUT * 1000); } -- 2.1.4
[PATCH RFC] spi-nor: provide a range for poll_timout
The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us to readl_poll_timeout() is set to 0. As this is never called in an atomic context sleeping should be no issue and there is no reasons for the tight-loop here. Signed-off-by: Nicholas Mc Guire --- Problem located by experimental coccinelle script: ./drivers/mtd/spi-nor/intel-spi.c:265:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000 ./drivers/mtd/spi-nor/intel-spi.c:274:8-26: WARNING: usleep_range min=0 for delay INTEL_SPI_TIMEOUT * 1000 The rational for setting the delay_us here to 40 is that readx_poll_timeout() will take delay_us >> 2 + 1 as min value and that should be at least 10us (see Documentation/timers/timers-howto.txt). Ideally the delay would be made even larger to keep the load on the hrtimer subsystem low as these delays here do not seem to be critical. Someone that knows the details of this device would need to check if a larger delay would be ok here. Patch was compile tested with: multi_v7_defconfig (implies CONFIG_MTD_SPI_NOR=y) one coccicheck finding reported and one spars finding (in separate patches) Patch is against 4.10-rc6 (localversion-next is next-20170210) drivers/mtd/spi-nor/intel-spi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c index a10f602..371bcf9 100644 --- a/drivers/mtd/spi-nor/intel-spi.c +++ b/drivers/mtd/spi-nor/intel-spi.c @@ -263,7 +263,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi) u32 val; return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, - !(val & HSFSTS_CTL_SCIP), 0, + !(val & HSFSTS_CTL_SCIP), 40, INTEL_SPI_TIMEOUT * 1000); } @@ -272,7 +272,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi) u32 val; return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, - !(val & SSFSTS_CTL_SCIP), 0, + !(val & SSFSTS_CTL_SCIP), 40, INTEL_SPI_TIMEOUT * 1000); } -- 2.1.4