[PATCH] mtd: spi-nor: fix the wrong dummy value

[PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

The dummy cycles is actually 8 for SPI fast/dual/quad read.

This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c  |    3 +++
 drivers/mtd/spi-nor/spi-nor.c |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 1557d8f..112ca8b 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	int dummy = nor->read_dummy;
 	int ret;
 
+	/* convert the dummy cycles to the number of byte */
+	dummy >>= 3;
+
 	/* Wait till previous write/erase is done. */
 	ret = nor->wait_till_ready(nor);
 	if (ret)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index cd4b277..b88cc7e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -77,7 +77,7 @@ static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
 	case SPI_NOR_FAST:
 	case SPI_NOR_DUAL:
 	case SPI_NOR_QUAD:
-		return 1;
+		return 8;
 	case SPI_NOR_NORMAL:
 		return 0;
 	}
-- 
1.7.2.rc3

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Gerhard Sittig]

On Wed, 2014-04-16 at 16:18 +0800, Huang Shijie wrote:
> 
> The dummy cycles is actually 8 for SPI fast/dual/quad read.
> 
> This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/devices/m25p80.c  |    3 +++
>  drivers/mtd/spi-nor/spi-nor.c |    2 +-
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 1557d8f..112ca8b 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>  	int dummy = nor->read_dummy;
>  	int ret;
>  
> +	/* convert the dummy cycles to the number of byte */
> +	dummy >>= 3;
> +

"dummy /= 8" to match the comment / commit message, and for
better mental association with a byte's width and the below
return value?

is the number of bits guaranteed to always be a multiple of 8
bits?  I guess it is for m25p80, but thought I'd ask (have seen
"odd" numbers like 6 bits in quad spi implementations -- those
are impossible to run with "traditional" SPI controllers)

>  	/* Wait till previous write/erase is done. */
>  	ret = nor->wait_till_ready(nor);
>  	if (ret)
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index cd4b277..b88cc7e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -77,7 +77,7 @@ static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
>  	case SPI_NOR_FAST:
>  	case SPI_NOR_DUAL:
>  	case SPI_NOR_QUAD:
> -		return 1;
> +		return 8;
>  	case SPI_NOR_NORMAL:
>  		return 0;
>  	}


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Wednesday, April 16, 2014 at 10:18:19 AM, Huang Shijie wrote:
> The dummy cycles is actually 8 for SPI fast/dual/quad read.
> 
> This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Inspecting this patch, I see the code will behave identically with/without this 
patch. It is thus unclear to me from the commit message, why this change is 
necessary.

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

On Wed, Apr 16, 2014 at 10:08:49PM +0200, Gerhard Sittig wrote:
> On Wed, 2014-04-16 at 16:18 +0800, Huang Shijie wrote:
> > 
> > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > 
> > This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/devices/m25p80.c  |    3 +++
> >  drivers/mtd/spi-nor/spi-nor.c |    2 +-
> >  2 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 1557d8f..112ca8b 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
> >  	int dummy = nor->read_dummy;
> >  	int ret;
> >  
> > +	/* convert the dummy cycles to the number of byte */
> > +	dummy >>= 3;
> > +
> 
> "dummy /= 8" to match the comment / commit message, and for
> better mental association with a byte's width and the below
> return value?

I first version is "dummy /= 8". but i think the "dummy >>= 3" is more
faster.


> 
> is the number of bits guaranteed to always be a multiple of 8
> bits?  I guess it is for m25p80, but thought I'd ask (have seen
> "odd" numbers like 6 bits in quad spi implementations -- those
> are impossible to run with "traditional" SPI controllers)

that's why i add this patch.

In the DDR quad read, the dummy can be 3, 6 or 7 cycles.

Currently the m25p80 only support the 8 cycles dummy, and
since the m25p80 can not support the DDR QUAD READ (is this true?).

The code is ok.

Please see my other patch for the DDR QUAD read .

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

On Thu, Apr 17, 2014 at 01:40:29AM +0200, Marek Vasut wrote:
> On Wednesday, April 16, 2014 at 10:18:19 AM, Huang Shijie wrote:
> > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > 
> > This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> 
> Inspecting this patch, I see the code will behave identically with/without this 
> patch. It is thus unclear to me from the commit message, why this change is 
> necessary.
> 
firstly, in theory, the dummy cycles should be 8, not 1.
secondly, the DDR QUAD READ may use 4 dummy cycles.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Thursday, April 17, 2014 at 06:59:43 AM, Huang Shijie wrote:
> On Wed, Apr 16, 2014 at 10:08:49PM +0200, Gerhard Sittig wrote:
> > On Wed, 2014-04-16 at 16:18 +0800, Huang Shijie wrote:
> > > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > > 
> > > This patch fixes the wrong dummy value for both the spi-nor.c and
> > > m25p80.c.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > ---
> > > 
> > >  drivers/mtd/devices/m25p80.c  |    3 +++
> > >  drivers/mtd/spi-nor/spi-nor.c |    2 +-
> > >  2 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c
> > > b/drivers/mtd/devices/m25p80.c index 1557d8f..112ca8b 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t
> > > from, size_t len,
> > > 
> > >  	int dummy = nor->read_dummy;
> > >  	int ret;
> > > 
> > > +	/* convert the dummy cycles to the number of byte */
> > > +	dummy >>= 3;
> > > +
> > 
> > "dummy /= 8" to match the comment / commit message, and for
> > better mental association with a byte's width and the below
> > return value?
> 
> I first version is "dummy /= 8". but i think the "dummy >>= 3" is more
> faster.

You can prove that by running objdump on the object file and inspecting the 
generated code. My expectation would be that GCC would optimize this as needed.
[...]

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Thursday, April 17, 2014 at 07:01:25 AM, Huang Shijie wrote:
> On Thu, Apr 17, 2014 at 01:40:29AM +0200, Marek Vasut wrote:
> > On Wednesday, April 16, 2014 at 10:18:19 AM, Huang Shijie wrote:
> > > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > > 
> > > This patch fixes the wrong dummy value for both the spi-nor.c and
> > > m25p80.c.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > 
> > Inspecting this patch, I see the code will behave identically
> > with/without this patch. It is thus unclear to me from the commit
> > message, why this change is necessary.
> 
> firstly, in theory, the dummy cycles should be 8, not 1.
> secondly, the DDR QUAD READ may use 4 dummy cycles.

Right, it took me a bit of reading into the thread until I understood the 
intention of the patch. If in doubt, try reading the commit message a day
later and you'll see that it might be insufficient. Basically, try looking
at the commit message from the receiving party's side ;-)

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

On Thu, Apr 17, 2014 at 01:32:52PM +0200, Marek Vasut wrote:
> On Thursday, April 17, 2014 at 07:01:25 AM, Huang Shijie wrote:
> > On Thu, Apr 17, 2014 at 01:40:29AM +0200, Marek Vasut wrote:
> > > On Wednesday, April 16, 2014 at 10:18:19 AM, Huang Shijie wrote:
> > > > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > > > 
> > > > This patch fixes the wrong dummy value for both the spi-nor.c and
> > > > m25p80.c.
> > > > 
> > > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > 
> > > Inspecting this patch, I see the code will behave identically
> > > with/without this patch. It is thus unclear to me from the commit
> > > message, why this change is necessary.
> > 
> > firstly, in theory, the dummy cycles should be 8, not 1.
> > secondly, the DDR QUAD READ may use 4 dummy cycles.
> 
> Right, it took me a bit of reading into the thread until I understood the 
> intention of the patch. If in doubt, try reading the commit message a day
> later and you'll see that it might be insufficient. Basically, try looking
> at the commit message from the receiving party's side ;-)
my fault.

I will update the commit message in the next version.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

On Wed, Apr 16, 2014 at 10:08:49PM +0200, Gerhard Sittig wrote:
> On Wed, 2014-04-16 at 16:18 +0800, Huang Shijie wrote:
> > 
> > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > 
> > This patch fixes the wrong dummy value for both the spi-nor.c and m25p80.c.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/devices/m25p80.c  |    3 +++
> >  drivers/mtd/spi-nor/spi-nor.c |    2 +-
> >  2 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 1557d8f..112ca8b 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
> >  	int dummy = nor->read_dummy;
> >  	int ret;
> >  
> > +	/* convert the dummy cycles to the number of byte */
> > +	dummy >>= 3;
> > +
> 
> "dummy /= 8" to match the comment / commit message, and for
> better mental association with a byte's width and the below
> return value?


The disassemble code for "int dummy = 8; dummy /= 8;" is:
--------------------------------------------------
    83a6:	2308      	movs	r3, #8
    83a8:	607b      	str	r3, [r7, #4]
    83aa:	687b      	ldr	r3, [r7, #4]
    83ac:	1dda      	adds	r2, r3, #7
    83ae:	2b00      	cmp	r3, #0
    83b0:	bfb4      	ite	lt
    83b2:	4613      	movlt	r3, r2
    83b4:	461b      	movge	r3, r3
    83b6:	10db      	asrs	r3, r3, #3
    83b8:	607b      	str	r3, [r7, #4]
--------------------------------------------------

The disassemble code for "int dummy = 8; dummy >>= 3;" is:
--------------------------------------------------
    83a6:	2308      	movs	r3, #8
    83a8:	607b      	str	r3, [r7, #4]
    83aa:	687b      	ldr	r3, [r7, #4]
    83ac:	10db      	asrs	r3, r3, #3
    83ae:	607b      	str	r3, [r7, #4]
--------------------------------------------------

Obviously, the "dummy >>= 3" is faster then "dummy /= 8".

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Thursday, April 17, 2014 at 03:41:29 PM, Huang Shijie wrote:
> On Wed, Apr 16, 2014 at 10:08:49PM +0200, Gerhard Sittig wrote:
> > On Wed, 2014-04-16 at 16:18 +0800, Huang Shijie wrote:
> > > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > > 
> > > This patch fixes the wrong dummy value for both the spi-nor.c and
> > > m25p80.c.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > ---
> > > 
> > >  drivers/mtd/devices/m25p80.c  |    3 +++
> > >  drivers/mtd/spi-nor/spi-nor.c |    2 +-
> > >  2 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c
> > > b/drivers/mtd/devices/m25p80.c index 1557d8f..112ca8b 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -131,6 +131,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t
> > > from, size_t len,
> > > 
> > >  	int dummy = nor->read_dummy;
> > >  	int ret;
> > > 
> > > +	/* convert the dummy cycles to the number of byte */
> > > +	dummy >>= 3;
> > > +
> > 
> > "dummy /= 8" to match the comment / commit message, and for
> > better mental association with a byte's width and the below
> > return value?
> 
> The disassemble code for "int dummy = 8; dummy /= 8;" is:
> --------------------------------------------------
>     83a6:	2308      	movs	r3, #8
>     83a8:	607b      	str	r3, [r7, #4]
>     83aa:	687b      	ldr	r3, [r7, #4]
>     83ac:	1dda      	adds	r2, r3, #7
>     83ae:	2b00      	cmp	r3, #0
>     83b0:	bfb4      	ite	lt
>     83b2:	4613      	movlt	r3, r2
>     83b4:	461b      	movge	r3, r3
>     83b6:	10db      	asrs	r3, r3, #3
>     83b8:	607b      	str	r3, [r7, #4]
> --------------------------------------------------
> 
> The disassemble code for "int dummy = 8; dummy >>= 3;" is:
> --------------------------------------------------
>     83a6:	2308      	movs	r3, #8
>     83a8:	607b      	str	r3, [r7, #4]
>     83aa:	687b      	ldr	r3, [r7, #4]
>     83ac:	10db      	asrs	r3, r3, #3
>     83ae:	607b      	str	r3, [r7, #4]
> --------------------------------------------------
> 
> Obviously, the "dummy >>= 3" is faster then "dummy /= 8".

Which GCC compiler is that please ? I'm surprised GCC doesn't optimize it.

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Thursday, April 17, 2014 at 02:59:21 PM, Huang Shijie wrote:
> On Thu, Apr 17, 2014 at 01:32:52PM +0200, Marek Vasut wrote:
> > On Thursday, April 17, 2014 at 07:01:25 AM, Huang Shijie wrote:
> > > On Thu, Apr 17, 2014 at 01:40:29AM +0200, Marek Vasut wrote:
> > > > On Wednesday, April 16, 2014 at 10:18:19 AM, Huang Shijie wrote:
> > > > > The dummy cycles is actually 8 for SPI fast/dual/quad read.
> > > > > 
> > > > > This patch fixes the wrong dummy value for both the spi-nor.c and
> > > > > m25p80.c.
> > > > > 
> > > > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > > 
> > > > Inspecting this patch, I see the code will behave identically
> > > > with/without this patch. It is thus unclear to me from the commit
> > > > message, why this change is necessary.
> > > 
> > > firstly, in theory, the dummy cycles should be 8, not 1.
> > > secondly, the DDR QUAD READ may use 4 dummy cycles.
> > 
> > Right, it took me a bit of reading into the thread until I understood the
> > intention of the patch. If in doubt, try reading the commit message a day
> > later and you'll see that it might be insufficient. Basically, try
> > looking at the commit message from the receiving party's side ;-)
> 
> my fault.
> 
> I will update the commit message in the next version.

Thanks!

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Gerhard Sittig]

On Thu, 2014-04-17 at 21:41 +0800, Huang Shijie wrote:
> 
> The disassemble code for "int dummy = 8; dummy /= 8;" is:
> --------------------------------------------------
>     83a6:	2308      	movs	r3, #8
>     83a8:	607b      	str	r3, [r7, #4]
>     83aa:	687b      	ldr	r3, [r7, #4]
>     83ac:	1dda      	adds	r2, r3, #7
>     83ae:	2b00      	cmp	r3, #0
>     83b0:	bfb4      	ite	lt
>     83b2:	4613      	movlt	r3, r2
>     83b4:	461b      	movge	r3, r3
>     83b6:	10db      	asrs	r3, r3, #3
>     83b8:	607b      	str	r3, [r7, #4]
> --------------------------------------------------
> 
> The disassemble code for "int dummy = 8; dummy >>= 3;" is:
> --------------------------------------------------
>     83a6:	2308      	movs	r3, #8
>     83a8:	607b      	str	r3, [r7, #4]
>     83aa:	687b      	ldr	r3, [r7, #4]
>     83ac:	10db      	asrs	r3, r3, #3
>     83ae:	607b      	str	r3, [r7, #4]
> --------------------------------------------------
> 
> Obviously, the "dummy >>= 3" is faster then "dummy /= 8".

That is because of signedness.  Both forms of "/= 8" and ">>= 3"
should be identical to the compiler, and generate the same
output.  Compilers know that division by powers of two can be
done with a shift.

Signedness apparently makes a difference.  If you know that the
number of clocks always is non-negative, use appropriate data
types.  Or let the compiler carry out the correct instructions
for the very data type that was declared.  Pick one, don't
violate abstractions.

Counter example, matching the expectation:

	unsigned int u_div(unsigned int v) {
		return v / 8;
	}

	unsigned int u_shift(unsigned int v) {
		return v >> 3;
	}

	0000001c <u_div>:
	  1c:   e1a001a0        lsr     r0, r0, #3
	  20:   e12fff1e        bx      lr

	00000024 <u_shift>:
	  24:   e1a001a0        lsr     r0, r0, #3
	  28:   e12fff1e        bx      lr


Anyway, source code should be written for humans, as it gets read
more often than written, and maintenance is hard enough already.
Try to come up with a text search pattern to catch both the 3 and
8 values at the same time.  Or try to easily see how they are the
same when there is no comment.  Is the code path so hot that
single instructions count so badly, that the downsides should be
considered acceptable?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

On Thu, Apr 17, 2014 at 05:55:07PM +0200, Gerhard Sittig wrote:
> On Thu, 2014-04-17 at 21:41 +0800, Huang Shijie wrote:
> > 
> > The disassemble code for "int dummy = 8; dummy /= 8;" is:
> > --------------------------------------------------
> >     83a6:	2308      	movs	r3, #8
> >     83a8:	607b      	str	r3, [r7, #4]
> >     83aa:	687b      	ldr	r3, [r7, #4]
> >     83ac:	1dda      	adds	r2, r3, #7
> >     83ae:	2b00      	cmp	r3, #0
> >     83b0:	bfb4      	ite	lt
> >     83b2:	4613      	movlt	r3, r2
> >     83b4:	461b      	movge	r3, r3
> >     83b6:	10db      	asrs	r3, r3, #3
> >     83b8:	607b      	str	r3, [r7, #4]
> > --------------------------------------------------
> > 
> > The disassemble code for "int dummy = 8; dummy >>= 3;" is:
> > --------------------------------------------------
> >     83a6:	2308      	movs	r3, #8
> >     83a8:	607b      	str	r3, [r7, #4]
> >     83aa:	687b      	ldr	r3, [r7, #4]
> >     83ac:	10db      	asrs	r3, r3, #3
> >     83ae:	607b      	str	r3, [r7, #4]
> > --------------------------------------------------
> > 
> > Obviously, the "dummy >>= 3" is faster then "dummy /= 8".
> 
> That is because of signedness.  Both forms of "/= 8" and ">>= 3"
> should be identical to the compiler, and generate the same
> output.  Compilers know that division by powers of two can be
> done with a shift.
> 
> Signedness apparently makes a difference.  If you know that the
> number of clocks always is non-negative, use appropriate data
> types.  Or let the compiler carry out the correct instructions
> for the very data type that was declared.  Pick one, don't
> violate abstractions.
> 
> Counter example, matching the expectation:
> 
> 	unsigned int u_div(unsigned int v) {
> 		return v / 8;
> 	}
> 
> 	unsigned int u_shift(unsigned int v) {
> 		return v >> 3;
> 	}
> 
> 	0000001c <u_div>:
> 	  1c:   e1a001a0        lsr     r0, r0, #3
> 	  20:   e12fff1e        bx      lr
> 
> 	00000024 <u_shift>:
> 	  24:   e1a001a0        lsr     r0, r0, #3
> 	  28:   e12fff1e        bx      lr
> 
> 
> Anyway, source code should be written for humans, as it gets read
> more often than written, and maintenance is hard enough already.
> Try to come up with a text search pattern to catch both the 3 and
> 8 values at the same time.  Or try to easily see how they are the
> same when there is no comment.  Is the code path so hot that
> single instructions count so badly, that the downsides should be
> considered acceptable?
okay.

I will change to "dummy /= 8" in the next version. 

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Thursday, April 17, 2014 at 05:57:59 PM, Huang Shijie wrote:
> On Thu, Apr 17, 2014 at 05:55:07PM +0200, Gerhard Sittig wrote:
> > On Thu, 2014-04-17 at 21:41 +0800, Huang Shijie wrote:
> > > The disassemble code for "int dummy = 8; dummy /= 8;" is:
> > > --------------------------------------------------
> > > 
> > >     83a6:	2308      	movs	r3, #8
> > >     83a8:	607b      	str	r3, [r7, #4]
> > >     83aa:	687b      	ldr	r3, [r7, #4]
> > >     83ac:	1dda      	adds	r2, r3, #7
> > >     83ae:	2b00      	cmp	r3, #0
> > >     83b0:	bfb4      	ite	lt
> > >     83b2:	4613      	movlt	r3, r2
> > >     83b4:	461b      	movge	r3, r3
> > >     83b6:	10db      	asrs	r3, r3, #3
> > >     83b8:	607b      	str	r3, [r7, #4]
> > > 
> > > --------------------------------------------------
> > > 
> > > The disassemble code for "int dummy = 8; dummy >>= 3;" is:
> > > --------------------------------------------------
> > > 
> > >     83a6:	2308      	movs	r3, #8
> > >     83a8:	607b      	str	r3, [r7, #4]
> > >     83aa:	687b      	ldr	r3, [r7, #4]
> > >     83ac:	10db      	asrs	r3, r3, #3
> > >     83ae:	607b      	str	r3, [r7, #4]
> > > 
> > > --------------------------------------------------
> > > 
> > > Obviously, the "dummy >>= 3" is faster then "dummy /= 8".
> > 
> > That is because of signedness.  Both forms of "/= 8" and ">>= 3"
> > should be identical to the compiler, and generate the same
> > output.  Compilers know that division by powers of two can be
> > done with a shift.
> > 
> > Signedness apparently makes a difference.  If you know that the
> > number of clocks always is non-negative, use appropriate data
> > types.  Or let the compiler carry out the correct instructions
> > for the very data type that was declared.  Pick one, don't
> > violate abstractions.
> > 
> > Counter example, matching the expectation:
> > 	unsigned int u_div(unsigned int v) {
> > 	
> > 		return v / 8;
> > 	
> > 	}
> > 	
> > 	unsigned int u_shift(unsigned int v) {
> > 	
> > 		return v >> 3;
> > 	
> > 	}
> > 	
> > 	0000001c <u_div>:
> > 	  1c:   e1a001a0        lsr     r0, r0, #3
> > 	  20:   e12fff1e        bx      lr
> > 	
> > 	00000024 <u_shift>:
> > 	  24:   e1a001a0        lsr     r0, r0, #3
> > 	  28:   e12fff1e        bx      lr
> > 
> > Anyway, source code should be written for humans, as it gets read
> > more often than written, and maintenance is hard enough already.
> > Try to come up with a text search pattern to catch both the 3 and
> > 8 values at the same time.  Or try to easily see how they are the
> > same when there is no comment.  Is the code path so hot that
> > single instructions count so badly, that the downsides should be
> > considered acceptable?
> 
> okay.
> 
> I will change to "dummy /= 8" in the next version.

My impression is that you also need to change the signedness of the data type.