* [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-03-13 11:23 ` Nicholas Mc Guire 0 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-03-13 11:23 UTC (permalink / raw) To: David Woodhouse Cc: Brian Norris, Aaron Sierra, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel, Nicholas Mc Guire This is only an API consolidation and should make things more readable it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability and also handles all corner-cases properly. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- Patch was compile tested with corenet64_smp_defconfig (implies CONFIG_MTD_NAND_FSL_IFC=y) Patch is against 4.0-rc3 (localversion-next is -next-20150313 drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 4c05f4f..51394e5 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) /* wait for command complete flag or timeout */ wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, - IFC_TIMEOUT_MSECS * HZ/1000); + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); /* ctrl->nand_stat will be updated from IRQ context */ if (!ctrl->nand_stat) @@ -860,7 +860,7 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv) /* wait for command complete flag or timeout */ wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, - IFC_TIMEOUT_MSECS * HZ/1000); + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) printk(KERN_ERR "fsl-ifc: Failed to Initialise SRAM\n"); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-03-13 11:23 ` Nicholas Mc Guire 0 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-03-13 11:23 UTC (permalink / raw) To: David Woodhouse Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Nicholas Mc Guire, Aaron Sierra, Brian Norris This is only an API consolidation and should make things more readable it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability and also handles all corner-cases properly. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- Patch was compile tested with corenet64_smp_defconfig (implies CONFIG_MTD_NAND_FSL_IFC=y) Patch is against 4.0-rc3 (localversion-next is -next-20150313 drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 4c05f4f..51394e5 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) /* wait for command complete flag or timeout */ wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, - IFC_TIMEOUT_MSECS * HZ/1000); + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); /* ctrl->nand_stat will be updated from IRQ context */ if (!ctrl->nand_stat) @@ -860,7 +860,7 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv) /* wait for command complete flag or timeout */ wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, - IFC_TIMEOUT_MSECS * HZ/1000); + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) printk(KERN_ERR "fsl-ifc: Failed to Initialise SRAM\n"); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-03-13 11:23 ` Nicholas Mc Guire @ 2015-04-01 13:58 ` Aaron Sierra -1 siblings, 0 replies; 14+ messages in thread From: Aaron Sierra @ 2015-04-01 13:58 UTC (permalink / raw) To: Nicholas Mc Guire Cc: David Woodhouse, Brian Norris, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel ----- Original Message ----- > From: "Nicholas Mc Guire" <hofrat@osadl.org> > Sent: Friday, March 13, 2015 6:23:47 AM > > This is only an API consolidation and should make things more readable > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > and also handles all corner-cases properly. > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > --- > > Patch was compile tested with corenet64_smp_defconfig > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > b/drivers/mtd/nand/fsl_ifc_nand.c > index 4c05f4f..51394e5 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > /* wait for command complete flag or timeout */ > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > - IFC_TIMEOUT_MSECS * HZ/1000); > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); Nicholas, Your patch makes me think that this timeout value should be calculated once during initialization and stored in a new member of struct fsl_ifc_mtd. That would improve readability AND have some positive impact on performance. -Aaron S. > > /* ctrl->nand_stat will be updated from IRQ context */ > if (!ctrl->nand_stat) > @@ -860,7 +860,7 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv) > > /* wait for command complete flag or timeout */ > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > - IFC_TIMEOUT_MSECS * HZ/1000); > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) > printk(KERN_ERR "fsl-ifc: Failed to Initialise SRAM\n"); > -- > 1.7.10.4 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-01 13:58 ` Aaron Sierra 0 siblings, 0 replies; 14+ messages in thread From: Aaron Sierra @ 2015-04-01 13:58 UTC (permalink / raw) To: Nicholas Mc Guire Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Brian Norris, David Woodhouse ----- Original Message ----- > From: "Nicholas Mc Guire" <hofrat@osadl.org> > Sent: Friday, March 13, 2015 6:23:47 AM > > This is only an API consolidation and should make things more readable > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > and also handles all corner-cases properly. > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > --- > > Patch was compile tested with corenet64_smp_defconfig > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > b/drivers/mtd/nand/fsl_ifc_nand.c > index 4c05f4f..51394e5 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > /* wait for command complete flag or timeout */ > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > - IFC_TIMEOUT_MSECS * HZ/1000); > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); Nicholas, Your patch makes me think that this timeout value should be calculated once during initialization and stored in a new member of struct fsl_ifc_mtd. That would improve readability AND have some positive impact on performance. -Aaron S. > > /* ctrl->nand_stat will be updated from IRQ context */ > if (!ctrl->nand_stat) > @@ -860,7 +860,7 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv) > > /* wait for command complete flag or timeout */ > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > - IFC_TIMEOUT_MSECS * HZ/1000); > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) > printk(KERN_ERR "fsl-ifc: Failed to Initialise SRAM\n"); > -- > 1.7.10.4 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-04-01 13:58 ` Aaron Sierra @ 2015-04-01 14:24 ` Nicholas Mc Guire -1 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-04-01 14:24 UTC (permalink / raw) To: Aaron Sierra Cc: Nicholas Mc Guire, David Woodhouse, Brian Norris, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel On Wed, 01 Apr 2015, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > This is only an API consolidation and should make things more readable > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > > and also handles all corner-cases properly. > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > --- > > > > Patch was compile tested with corenet64_smp_defconfig > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > b/drivers/mtd/nand/fsl_ifc_nand.c > > index 4c05f4f..51394e5 100644 > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > /* wait for command complete flag or timeout */ > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > - IFC_TIMEOUT_MSECS * HZ/1000); > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > Nicholas, > Your patch makes me think that this timeout value should be calculated > once during initialization and stored in a new member of > struct fsl_ifc_mtd. That would improve readability AND have some > positive impact on performance. > is it not a bit wasteful to put it into struct fsl_ifc_mtd which is used quite often in here ? #define IFC_TIMEOUT msecs_to_jiffies(500) sould do - its only referenced twice and would achieve what you suggested - initializing it in one location and improved readability wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); would there be any disadvantage of this solution? thx! hofrat ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-01 14:24 ` Nicholas Mc Guire 0 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-04-01 14:24 UTC (permalink / raw) To: Aaron Sierra Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Nicholas Mc Guire, Brian Norris, David Woodhouse On Wed, 01 Apr 2015, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > This is only an API consolidation and should make things more readable > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > > and also handles all corner-cases properly. > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > --- > > > > Patch was compile tested with corenet64_smp_defconfig > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > b/drivers/mtd/nand/fsl_ifc_nand.c > > index 4c05f4f..51394e5 100644 > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > /* wait for command complete flag or timeout */ > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > - IFC_TIMEOUT_MSECS * HZ/1000); > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > Nicholas, > Your patch makes me think that this timeout value should be calculated > once during initialization and stored in a new member of > struct fsl_ifc_mtd. That would improve readability AND have some > positive impact on performance. > is it not a bit wasteful to put it into struct fsl_ifc_mtd which is used quite often in here ? #define IFC_TIMEOUT msecs_to_jiffies(500) sould do - its only referenced twice and would achieve what you suggested - initializing it in one location and improved readability wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); would there be any disadvantage of this solution? thx! hofrat ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-04-01 14:24 ` Nicholas Mc Guire @ 2015-04-01 15:08 ` Aaron Sierra -1 siblings, 0 replies; 14+ messages in thread From: Aaron Sierra @ 2015-04-01 15:08 UTC (permalink / raw) To: Nicholas Mc Guire Cc: Nicholas Mc Guire, David Woodhouse, Brian Norris, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel ----- Original Message ----- > From: "Nicholas Mc Guire" <der.herr@hofr.at> > Sent: Wednesday, April 1, 2015 9:24:44 AM > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > ----- Original Message ----- > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > This is only an API consolidation and should make things more readable > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > readability > > > and also handles all corner-cases properly. > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > --- > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > index 4c05f4f..51394e5 100644 > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > /* wait for command complete flag or timeout */ > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > Nicholas, > > Your patch makes me think that this timeout value should be calculated > > once during initialization and stored in a new member of > > struct fsl_ifc_mtd. That would improve readability AND have some > > positive impact on performance. > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > which is used quite often in here ? Storing the timeout in the private structure would use 4 or 8 bytes, but the structure is passed around as pointer reference, so there is no added overhead there. > #define IFC_TIMEOUT msecs_to_jiffies(500) > > sould do - its only referenced twice and would achieve what you > suggested - initializing it in one location and improved readability > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > would there be any disadvantage of this solution? I don't have an issue with the compactness of your code. The issue to me is that msecs_to_jiffies() is a real function call, not a macro. That means that there would be a branch to that function followed by a non-trivial calculation *every* time fsl_ifc_run_command() is called. If the timeout value were calculated during init, then the cost of accessing it would simply a pointer dereference. -Aaron S. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-01 15:08 ` Aaron Sierra 0 siblings, 0 replies; 14+ messages in thread From: Aaron Sierra @ 2015-04-01 15:08 UTC (permalink / raw) To: Nicholas Mc Guire Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Nicholas Mc Guire, Brian Norris, David Woodhouse ----- Original Message ----- > From: "Nicholas Mc Guire" <der.herr@hofr.at> > Sent: Wednesday, April 1, 2015 9:24:44 AM > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > ----- Original Message ----- > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > This is only an API consolidation and should make things more readable > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > readability > > > and also handles all corner-cases properly. > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > --- > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > index 4c05f4f..51394e5 100644 > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > /* wait for command complete flag or timeout */ > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > Nicholas, > > Your patch makes me think that this timeout value should be calculated > > once during initialization and stored in a new member of > > struct fsl_ifc_mtd. That would improve readability AND have some > > positive impact on performance. > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > which is used quite often in here ? Storing the timeout in the private structure would use 4 or 8 bytes, but the structure is passed around as pointer reference, so there is no added overhead there. > #define IFC_TIMEOUT msecs_to_jiffies(500) > > sould do - its only referenced twice and would achieve what you > suggested - initializing it in one location and improved readability > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > would there be any disadvantage of this solution? I don't have an issue with the compactness of your code. The issue to me is that msecs_to_jiffies() is a real function call, not a macro. That means that there would be a branch to that function followed by a non-trivial calculation *every* time fsl_ifc_run_command() is called. If the timeout value were calculated during init, then the cost of accessing it would simply a pointer dereference. -Aaron S. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-04-01 15:08 ` Aaron Sierra @ 2015-04-01 15:18 ` Joe Perches -1 siblings, 0 replies; 14+ messages in thread From: Joe Perches @ 2015-04-01 15:18 UTC (permalink / raw) To: Aaron Sierra Cc: Nicholas Mc Guire, Nicholas Mc Guire, David Woodhouse, Brian Norris, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel On Wed, 2015-04-01 at 10:08 -0500, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Nicholas Mc Guire" <der.herr@hofr.at> > > Sent: Wednesday, April 1, 2015 9:24:44 AM > > > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > > > ----- Original Message ----- > > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > > > This is only an API consolidation and should make things more readable > > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > > readability > > > > and also handles all corner-cases properly. > > > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > > --- > > > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > > index 4c05f4f..51394e5 100644 > > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > > > /* wait for command complete flag or timeout */ > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > > > Nicholas, > > > Your patch makes me think that this timeout value should be calculated > > > once during initialization and stored in a new member of > > > struct fsl_ifc_mtd. That would improve readability AND have some > > > positive impact on performance. > > > > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > > which is used quite often in here ?cho > > Storing the timeout in the private structure would use 4 or 8 bytes, > but the structure is passed around as pointer reference, so there is > no added overhead there. > > > #define IFC_TIMEOUT msecs_to_jiffies(500) > > > > sould do - its only referenced twice and would achieve what you > > suggested - initializing it in one location and improved readability > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > > > would there be any disadvantage of this solution? > > I don't have an issue with the compactness of your code. The issue > to me is that msecs_to_jiffies() is a real function call, not a macro. > That means that there would be a branch to that function followed by a > non-trivial calculation *every* time fsl_ifc_run_command() is called. > > If the timeout value were calculated during init, then the cost of > accessing it would simply a pointer dereference. Nicholas, any progress on that builtin_const_p variant ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-01 15:18 ` Joe Perches 0 siblings, 0 replies; 14+ messages in thread From: Joe Perches @ 2015-04-01 15:18 UTC (permalink / raw) To: Aaron Sierra Cc: Wolfram Sang, linux-kernel, Joe Schultz, Nicholas Mc Guire, Nicholas Mc Guire, linux-mtd, Brian Norris, David Woodhouse On Wed, 2015-04-01 at 10:08 -0500, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Nicholas Mc Guire" <der.herr@hofr.at> > > Sent: Wednesday, April 1, 2015 9:24:44 AM > > > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > > > ----- Original Message ----- > > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > > > This is only an API consolidation and should make things more readable > > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > > readability > > > > and also handles all corner-cases properly. > > > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > > --- > > > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > > index 4c05f4f..51394e5 100644 > > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > > > /* wait for command complete flag or timeout */ > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > > > Nicholas, > > > Your patch makes me think that this timeout value should be calculated > > > once during initialization and stored in a new member of > > > struct fsl_ifc_mtd. That would improve readability AND have some > > > positive impact on performance. > > > > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > > which is used quite often in here ?cho > > Storing the timeout in the private structure would use 4 or 8 bytes, > but the structure is passed around as pointer reference, so there is > no added overhead there. > > > #define IFC_TIMEOUT msecs_to_jiffies(500) > > > > sould do - its only referenced twice and would achieve what you > > suggested - initializing it in one location and improved readability > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > > > would there be any disadvantage of this solution? > > I don't have an issue with the compactness of your code. The issue > to me is that msecs_to_jiffies() is a real function call, not a macro. > That means that there would be a branch to that function followed by a > non-trivial calculation *every* time fsl_ifc_run_command() is called. > > If the timeout value were calculated during init, then the cost of > accessing it would simply a pointer dereference. Nicholas, any progress on that builtin_const_p variant ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-04-01 15:18 ` Joe Perches @ 2015-04-02 0:20 ` Nicholas Mc Guire -1 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-04-02 0:20 UTC (permalink / raw) To: Joe Perches Cc: Aaron Sierra, Nicholas Mc Guire, David Woodhouse, Brian Norris, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel On Wed, 01 Apr 2015, Joe Perches wrote: > On Wed, 2015-04-01 at 10:08 -0500, Aaron Sierra wrote: > > ----- Original Message ----- > > > From: "Nicholas Mc Guire" <der.herr@hofr.at> > > > Sent: Wednesday, April 1, 2015 9:24:44 AM > > > > > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > > > > > ----- Original Message ----- > > > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > > > > > This is only an API consolidation and should make things more readable > > > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > > > readability > > > > > and also handles all corner-cases properly. > > > > > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > > > --- > > > > > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > > > index 4c05f4f..51394e5 100644 > > > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > > > > > /* wait for command complete flag or timeout */ > > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > > > > > Nicholas, > > > > Your patch makes me think that this timeout value should be calculated > > > > once during initialization and stored in a new member of > > > > struct fsl_ifc_mtd. That would improve readability AND have some > > > > positive impact on performance. > > > > > > > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > > > which is used quite often in here ?cho > > > > Storing the timeout in the private structure would use 4 or 8 bytes, > > but the structure is passed around as pointer reference, so there is > > no added overhead there. > > > > > #define IFC_TIMEOUT msecs_to_jiffies(500) > > > > > > sould do - its only referenced twice and would achieve what you > > > suggested - initializing it in one location and improved readability > > > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > > > > > would there be any disadvantage of this solution? > > > > I don't have an issue with the compactness of your code. The issue > > to me is that msecs_to_jiffies() is a real function call, not a macro. > > That means that there would be a branch to that function followed by a > > non-trivial calculation *every* time fsl_ifc_run_command() is called. > > > > If the timeout value were calculated during init, then the cost of > > accessing it would simply a pointer dereference. > > Nicholas, any progress on that builtin_const_p variant > did not get to it yet - but thats the next thing on my fun list. thx! hofrat ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-02 0:20 ` Nicholas Mc Guire 0 siblings, 0 replies; 14+ messages in thread From: Nicholas Mc Guire @ 2015-04-02 0:20 UTC (permalink / raw) To: Joe Perches Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Nicholas Mc Guire, Aaron Sierra, Brian Norris, David Woodhouse On Wed, 01 Apr 2015, Joe Perches wrote: > On Wed, 2015-04-01 at 10:08 -0500, Aaron Sierra wrote: > > ----- Original Message ----- > > > From: "Nicholas Mc Guire" <der.herr@hofr.at> > > > Sent: Wednesday, April 1, 2015 9:24:44 AM > > > > > > On Wed, 01 Apr 2015, Aaron Sierra wrote: > > > > > > > ----- Original Message ----- > > > > > From: "Nicholas Mc Guire" <hofrat@osadl.org> > > > > > Sent: Friday, March 13, 2015 6:23:47 AM > > > > > > > > > > This is only an API consolidation and should make things more readable > > > > > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps > > > > > readability > > > > > and also handles all corner-cases properly. > > > > > > > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > > > > > --- > > > > > > > > > > Patch was compile tested with corenet64_smp_defconfig > > > > > (implies CONFIG_MTD_NAND_FSL_IFC=y) > > > > > > > > > > Patch is against 4.0-rc3 (localversion-next is -next-20150313 > > > > > > > > > > drivers/mtd/nand/fsl_ifc_nand.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c > > > > > b/drivers/mtd/nand/fsl_ifc_nand.c > > > > > index 4c05f4f..51394e5 100644 > > > > > --- a/drivers/mtd/nand/fsl_ifc_nand.c > > > > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > > > > > @@ -317,7 +317,7 @@ static void fsl_ifc_run_command(struct mtd_info *mtd) > > > > > > > > > > /* wait for command complete flag or timeout */ > > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, > > > > > - IFC_TIMEOUT_MSECS * HZ/1000); > > > > > + msecs_to_jiffies(IFC_TIMEOUT_MSECS)); > > > > > > > > Nicholas, > > > > Your patch makes me think that this timeout value should be calculated > > > > once during initialization and stored in a new member of > > > > struct fsl_ifc_mtd. That would improve readability AND have some > > > > positive impact on performance. > > > > > > > > > > is it not a bit wasteful to put it into struct fsl_ifc_mtd > > > which is used quite often in here ?cho > > > > Storing the timeout in the private structure would use 4 or 8 bytes, > > but the structure is passed around as pointer reference, so there is > > no added overhead there. > > > > > #define IFC_TIMEOUT msecs_to_jiffies(500) > > > > > > sould do - its only referenced twice and would achieve what you > > > suggested - initializing it in one location and improved readability > > > > > > wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat, IFC_TIMEOUT); > > > > > > would there be any disadvantage of this solution? > > > > I don't have an issue with the compactness of your code. The issue > > to me is that msecs_to_jiffies() is a real function call, not a macro. > > That means that there would be a branch to that function followed by a > > non-trivial calculation *every* time fsl_ifc_run_command() is called. > > > > If the timeout value were calculated during init, then the cost of > > accessing it would simply a pointer dereference. > > Nicholas, any progress on that builtin_const_p variant > did not get to it yet - but thats the next thing on my fun list. thx! hofrat ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion 2015-03-13 11:23 ` Nicholas Mc Guire @ 2015-04-06 1:05 ` Brian Norris -1 siblings, 0 replies; 14+ messages in thread From: Brian Norris @ 2015-04-06 1:05 UTC (permalink / raw) To: Nicholas Mc Guire Cc: David Woodhouse, Aaron Sierra, Joe Schultz, Wolfram Sang, linux-mtd, linux-kernel On Fri, Mar 13, 2015 at 07:23:47AM -0400, Nicholas Mc Guire wrote: > This is only an API consolidation and should make things more readable > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > and also handles all corner-cases properly. > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> Pushed to l2-mtd.git. I'm not sure the objections about an extra function call are important right now; if they are, then it shouldn't be too hard to patch in a __builtin_constant_p() variant of msecs_to_jiffies(), as Joe suggested. Brian ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion @ 2015-04-06 1:05 ` Brian Norris 0 siblings, 0 replies; 14+ messages in thread From: Brian Norris @ 2015-04-06 1:05 UTC (permalink / raw) To: Nicholas Mc Guire Cc: Wolfram Sang, linux-kernel, Joe Schultz, linux-mtd, Aaron Sierra, David Woodhouse On Fri, Mar 13, 2015 at 07:23:47AM -0400, Nicholas Mc Guire wrote: > This is only an API consolidation and should make things more readable > it replaces var * HZ / 1000 by msecs_to_jiffies(var) which helps readability > and also handles all corner-cases properly. > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> Pushed to l2-mtd.git. I'm not sure the objections about an extra function call are important right now; if they are, then it shouldn't be too hard to patch in a __builtin_constant_p() variant of msecs_to_jiffies(), as Joe suggested. Brian ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-04-06 1:06 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-13 11:23 [PATCH] mtd: fsl_ifc_nand: use msecs_to_jiffies for time conversion Nicholas Mc Guire 2015-03-13 11:23 ` Nicholas Mc Guire 2015-04-01 13:58 ` Aaron Sierra 2015-04-01 13:58 ` Aaron Sierra 2015-04-01 14:24 ` Nicholas Mc Guire 2015-04-01 14:24 ` Nicholas Mc Guire 2015-04-01 15:08 ` Aaron Sierra 2015-04-01 15:08 ` Aaron Sierra 2015-04-01 15:18 ` Joe Perches 2015-04-01 15:18 ` Joe Perches 2015-04-02 0:20 ` Nicholas Mc Guire 2015-04-02 0:20 ` Nicholas Mc Guire 2015-04-06 1:05 ` Brian Norris 2015-04-06 1:05 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.