patches for the synopsys gpio controller, rount 3

patches for the synopsys gpio controller, rount 3

[Sebastian Andrzej Siewior]

Hi,

my queue, updated with latest comments I recevied.
#1 fixes a list corruption bug
#4 enabled mixed edge/level user

the dts changes are not part of the series as I believe that they have
been already picked up by the maintainer(s).

Sebastian

[PATCH 1/5] gpio: dwapb: drop irq_setup_generic_chip()

[Sebastian Andrzej Siewior]

The driver calls irq_alloc_domain_generic_chips() which creates a gc and
adds it to gc_list. The driver later then calls irq_setup_generic_chip()
which also initializes the gc and adds it to the gc_list() and this
corrupts the list. Enable LIST_DEBUG and you see the kernel complain.
This isn't required, irq_alloc_domain_generic_chips() did the init.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ed5711f..4d25a06b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -260,9 +260,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	ct->regs.ack = GPIO_PORTA_EOI;
 	ct->regs.mask = GPIO_INTMASK;
 
-	irq_setup_generic_chip(irq_gc, IRQ_MSK(port->bgc.gc.ngpio),
-			IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
-
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
 
-- 
1.9.1

Re: [PATCH 1/5] gpio: dwapb: drop irq_setup_generic_chip()

[delicious quinoa]

On Mon, Apr 7, 2014 at 5:13 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> The driver calls irq_alloc_domain_generic_chips() which creates a gc and
> adds it to gc_list. The driver later then calls irq_setup_generic_chip()
> which also initializes the gc and adds it to the gc_list() and this
> corrupts the list. Enable LIST_DEBUG and you see the kernel complain.
> This isn't required, irq_alloc_domain_generic_chips() did the init.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/gpio/gpio-dwapb.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index ed5711f..4d25a06b 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -260,9 +260,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>         ct->regs.ack = GPIO_PORTA_EOI;
>         ct->regs.mask = GPIO_INTMASK;
>
> -       irq_setup_generic_chip(irq_gc, IRQ_MSK(port->bgc.gc.ngpio),
> -                       IRQ_GC_INIT_NESTED_LOCK, IRQ_NOREQUEST, 0);
> -
>         irq_set_chained_handler(irq, dwapb_irq_handler);
>         irq_set_handler_data(irq, gpio);
>
> --
> 1.9.1


I've tested this patch.  It does fix a problem that was making the
kernel hang while going down for a reboot.

Alan

Re: [PATCH 1/5] gpio: dwapb: drop irq_setup_generic_chip()

[Linus Walleij]

On Mon, Apr 7, 2014 at 12:13 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:

> The driver calls irq_alloc_domain_generic_chips() which creates a gc and
> adds it to gc_list. The driver later then calls irq_setup_generic_chip()
> which also initializes the gc and adds it to the gc_list() and this
> corrupts the list. Enable LIST_DEBUG and you see the kernel complain.
> This isn't required, irq_alloc_domain_generic_chips() did the init.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Patch applied, adding Alan Tull's Tested-by.

Yours,
Linus Walleij

[PATCH 2/5] gpio: dwapb: use irq_linear_revmap() for the faster lookup

[Sebastian Andrzej Siewior]

According to irq_linear_revmap() comment, it is slightly faster compared
to irq_find_mapping() since we don't use a radix tree but a linear
mapping.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4d25a06b..541b893 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -92,7 +92,7 @@ static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
 
 	while (irq_status) {
 		int hwirq = fls(irq_status) - 1;
-		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+		int gpio_irq = irq_linear_revmap(gpio->domain, hwirq);
 
 		generic_handle_irq(gpio_irq);
 		irq_status &= ~BIT(hwirq);
-- 
1.9.1

[PATCH 3/5] gpio: dwapb: use irq_gc_lock() for locking instead bc's lock

[Sebastian Andrzej Siewior]

The generic irq chip uses irq_gc_lock() for locking so the
enable/startup and type callbacks should use the same lock. None of
those registers (polarity, mask, enable) are used by the gpio part.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 541b893..752ccb1 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -20,7 +20,6 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
 
 #define GPIO_SWPORTA_DR		0x00
 #define GPIO_SWPORTA_DDR	0x04
@@ -110,30 +109,26 @@ static void dwapb_irq_enable(struct irq_data *d)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
 	val |= BIT(d->hwirq);
 	writel(val, gpio->regs + GPIO_INTEN);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 }
 
 static void dwapb_irq_disable(struct irq_data *d)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
 	val &= ~BIT(d->hwirq);
 	writel(val, gpio->regs + GPIO_INTEN);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 }
 
 static int dwapb_irq_reqres(struct irq_data *d)
@@ -163,15 +158,14 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	struct bgpio_chip *bgc = &gpio->ports[0].bgc;
 	int bit = d->hwirq;
-	unsigned long level, polarity, flags;
+	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
 		return -EINVAL;
 
-	spin_lock_irqsave(&bgc->lock, flags);
+	irq_gc_lock(igc);
 	level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
 	polarity = readl(gpio->regs + GPIO_INT_POLARITY);
 
@@ -200,7 +194,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
-	spin_unlock_irqrestore(&bgc->lock, flags);
+	irq_gc_unlock(igc);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 4/5] gpio: dwapb: use a second irq chip

[Sebastian Andrzej Siewior]

Right new have one irq chip running always in level mode. It would nicer
to have two irq chips where one is handling level type interrupts and
the other one is doing edge interrupts. So we can have at runtime two users
where one is using edge and the other level.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 752ccb1..13f1ad0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -192,6 +192,8 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
+	irq_setup_alt_chip(d, type);
+
 	writel(level, gpio->regs + GPIO_INTTYPE_LEVEL);
 	writel(polarity, gpio->regs + GPIO_INT_POLARITY);
 	irq_gc_unlock(igc);
@@ -221,7 +223,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	if (!gpio->domain)
 		return;
 
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 1,
+	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
 					     "gpio-dwapb", handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
@@ -242,7 +244,20 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->reg_base = gpio->regs;
 	irq_gc->private = gpio;
 
-	ct = irq_gc->chip_types;
+	ct = &irq_gc->chip_types[0];
+	ct->chip.irq_ack = irq_gc_ack_set_bit;
+	ct->chip.irq_mask = irq_gc_mask_set_bit;
+	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
+	ct->chip.irq_set_type = dwapb_irq_set_type;
+	ct->chip.irq_enable = dwapb_irq_enable;
+	ct->chip.irq_disable = dwapb_irq_disable;
+	ct->chip.irq_request_resources = dwapb_irq_reqres;
+	ct->chip.irq_release_resources = dwapb_irq_relres;
+	ct->regs.ack = GPIO_PORTA_EOI;
+	ct->regs.mask = GPIO_INTMASK;
+	ct->type = IRQ_TYPE_LEVEL_MASK;
+
+	ct = &irq_gc->chip_types[1];
 	ct->chip.irq_ack = irq_gc_ack_set_bit;
 	ct->chip.irq_mask = irq_gc_mask_set_bit;
 	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
@@ -253,6 +268,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	ct->chip.irq_release_resources = dwapb_irq_relres;
 	ct->regs.ack = GPIO_PORTA_EOI;
 	ct->regs.mask = GPIO_INTMASK;
+	ct->type = IRQ_TYPE_EDGE_BOTH;
+	ct->handler = handle_edge_irq;
 
 	irq_set_chained_handler(irq, dwapb_irq_handler);
 	irq_set_handler_data(irq, gpio);
-- 
1.9.1

[PATCH 5/5] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

d->mask contains exact the same information as BIT(bit) so we could save
a few cycles here.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/gpio/gpio-dwapb.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 13f1ad0..d85fc2e 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val |= BIT(d->hwirq);
+	val |= d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -126,7 +126,7 @@ static void dwapb_irq_disable(struct irq_data *d)
 
 	irq_gc_lock(igc);
 	val = readl(gpio->regs + GPIO_INTEN);
-	val &= ~BIT(d->hwirq);
+	val &= ~d->mask;
 	writel(val, gpio->regs + GPIO_INTEN);
 	irq_gc_unlock(igc);
 }
@@ -158,7 +158,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
-	int bit = d->hwirq;
+	u32 mask = d->mask;
 	unsigned long level, polarity;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
@@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_BOTH:
-		level |= BIT(bit);
-		dwapb_toggle_trigger(gpio, bit);
+		level |= mask;
+		dwapb_toggle_trigger(gpio, d->hwirq);
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		level |= BIT(bit);
-		polarity |= BIT(bit);
+		level |= mask;
+		polarity |= mask;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		level |= BIT(bit);
-		polarity &= ~BIT(bit);
+		level |= mask;
+		polarity &= ~mask;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		level &= ~BIT(bit);
-		polarity |= BIT(bit);
+		level &= ~mask;
+		polarity |= mask;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		level &= ~BIT(bit);
-		polarity &= ~BIT(bit);
+		level &= ~mask;
+		polarity &= ~mask;
 		break;
 	}
 
-- 
1.9.1

Re: [PATCH 5/5] gpio: dwapb: use d->mask instead od BIT(bit)

[Gerhard Sittig]

On Mon, 2014-04-07 at 12:13 +0200, Sebastian Andrzej Siewior wrote:
> 
> d->mask contains exact the same information as BIT(bit) so we could save
> a few cycles here.

ISTR that the benefit of saving cycles was questioned in previous
review comments.  On ARM, the shift "comes for free".

I'm not saying that the patch is doing something wrong.  But I
suggest to rephrase the commit message (and put the version
number into the subject prefix, should you have to resend).

Reducing the number of variables involved, or hiding details
behind common abstractions, or eliminating redundancy, all of
those benefits are as valuable.  It's just that this patch does
not save any computation time.

> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
>  
>  	irq_gc_lock(igc);
>  	val = readl(gpio->regs + GPIO_INTEN);
> -	val |= BIT(d->hwirq);
> +	val |= d->mask;

these are equally costly or cheap, nothing saved here

>  	struct dwapb_gpio *gpio = igc->private;
> -	int bit = d->hwirq;
> +	u32 mask = d->mask;
>  	unsigned long level, polarity;
>  
>  	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> @@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  
>  	switch (type) {
>  	case IRQ_TYPE_EDGE_BOTH:
> -		level |= BIT(bit);
> -		dwapb_toggle_trigger(gpio, bit);
> +		level |= mask;
> +		dwapb_toggle_trigger(gpio, d->hwirq);

these introduce another pointer dereference, unless 'bit' was
assigned from a pointer dereference (as is shown above), so
nothing was gained


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 5/5] gpio: dwapb: use d->mask instead od BIT(bit)

[Sebastian Andrzej Siewior]

On 04/07/2014 02:26 PM, Gerhard Sittig wrote:
> On Mon, 2014-04-07 at 12:13 +0200, Sebastian Andrzej Siewior wrote:
>>
>> d->mask contains exact the same information as BIT(bit) so we could save
>> a few cycles here.
> 
> ISTR that the benefit of saving cycles was questioned in previous
> review comments.  On ARM, the shift "comes for free".

I can't recall that some pointed this out. However:
- you load one variable in both cases. Not performing the shift means
  there is at least one instruction less to be performed.
- that gpio controller is generic IP core from Synopsys. Every can buy
  it and but into their IP core so it is not limited to ARM.

>> --- a/drivers/gpio/gpio-dwapb.c
>> +++ b/drivers/gpio/gpio-dwapb.c
>> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
>>  
>>  	irq_gc_lock(igc);
>>  	val = readl(gpio->regs + GPIO_INTEN);
>> -	val |= BIT(d->hwirq);
>> +	val |= d->mask;
> 
> these are equally costly or cheap, nothing saved here

I still thing not performing an instruction is more efficient than
performing one.

>>  	struct dwapb_gpio *gpio = igc->private;
>> -	int bit = d->hwirq;
>> +	u32 mask = d->mask;
>>  	unsigned long level, polarity;
>>  
>>  	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
>> @@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>>  
>>  	switch (type) {
>>  	case IRQ_TYPE_EDGE_BOTH:
>> -		level |= BIT(bit);
>> -		dwapb_toggle_trigger(gpio, bit);
>> +		level |= mask;
>> +		dwapb_toggle_trigger(gpio, d->hwirq);
> 
> these introduce another pointer dereference, unless 'bit' was
> assigned from a pointer dereference (as is shown above), so
> nothing was gained

dwapb_toggle_trigger() is a bit special and it needs both. However,
size on ARM says

   text    data     bss     dec     hex filename
   3264      96       0    3360     d20 drivers/gpio/gpio-dwapb.o.before
   3224      96       0    3320     cf8 drivers/gpio/gpio-dwapb.o.after

that with the patch the code is smaller by 40 bytes. Does 40 bytes
smaller code quality for "safe a few cycles" statement?

> virtually yours
> Gerhard Sittig

Sebastian

Re: [PATCH 5/5] gpio: dwapb: use d->mask instead od BIT(bit)

[Gerhard Sittig]

[ ignore this if you are busy :) ]

On Mon, 2014-04-07 at 20:26 +0200, Sebastian Andrzej Siewior wrote:
> 
> On 04/07/2014 02:26 PM, Gerhard Sittig wrote:
> > On Mon, 2014-04-07 at 12:13 +0200, Sebastian Andrzej Siewior wrote:
> >>
> >> d->mask contains exact the same information as BIT(bit) so we could save
> >> a few cycles here.
> > 
> > ISTR that the benefit of saving cycles was questioned in previous
> > review comments.  On ARM, the shift "comes for free".
> 
> I can't recall that some pointed this out.

http://article.gmane.org/gmane.linux.kernel.gpio/2410 raises a
concern about the cost of dereferencing pointers

That shifts might not involve additional cost appears to not have
been stated explicitly in earlier feedback, or I have missed it
in the search, doesn't matter much.  Might as well have confused
this submission with another one.

> However:
> - you load one variable in both cases. Not performing the shift means
>   there is at least one instruction less to be performed.
> - that gpio controller is generic IP core from Synopsys. Every can buy
>   it and but into their IP core so it is not limited to ARM.

You assume that the shift is done in an individual instruction.
That does not necessarily apply to the ARM architecture, which
has a barrel shifter and can fold shifts into other instructions
"for free".

This IP core has "APB" in its name, which is a memory bus that
I've never seen in use outside of the ARM ecosphere.  Whatever
that may be worth, anyway.  We are not talking about the kind of
GPIO block that resides in FPGAs, we are talking about an IP
block that is "on the CPU side" of SoCs.

But see below, I do not want to block anything, feel free to
ignore my concerns as they are not strong here.

> >> --- a/drivers/gpio/gpio-dwapb.c
> >> +++ b/drivers/gpio/gpio-dwapb.c
> >> @@ -113,7 +113,7 @@ static void dwapb_irq_enable(struct irq_data *d)
> >>  
> >>  	irq_gc_lock(igc);
> >>  	val = readl(gpio->regs + GPIO_INTEN);
> >> -	val |= BIT(d->hwirq);
> >> +	val |= d->mask;
> > 
> > these are equally costly or cheap, nothing saved here
> 
> I still thing not performing an instruction is more efficient than
> performing one.
> 
> >>  	struct dwapb_gpio *gpio = igc->private;
> >> -	int bit = d->hwirq;
> >> +	u32 mask = d->mask;
> >>  	unsigned long level, polarity;
> >>  
> >>  	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
> >> @@ -171,24 +171,24 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
> >>  
> >>  	switch (type) {
> >>  	case IRQ_TYPE_EDGE_BOTH:
> >> -		level |= BIT(bit);
> >> -		dwapb_toggle_trigger(gpio, bit);
> >> +		level |= mask;
> >> +		dwapb_toggle_trigger(gpio, d->hwirq);
> > 
> > these introduce another pointer dereference, unless 'bit' was
> > assigned from a pointer dereference (as is shown above), so
> > nothing was gained
> 
> dwapb_toggle_trigger() is a bit special and it needs both. However,
> size on ARM says
> 
>    text    data     bss     dec     hex filename
>    3264      96       0    3360     d20 drivers/gpio/gpio-dwapb.o.before
>    3224      96       0    3320     cf8 drivers/gpio/gpio-dwapb.o.after
> 
> that with the patch the code is smaller by 40 bytes. Does 40 bytes
> smaller code quality for "safe a few cycles" statement?

I would not necessarily jump to the conclusion that code of
smaller size translates into fewer instructions.  It may be an
educated guess here in this specific situation (given the very
nature of the change), but does not apply in general.  Actually
you can reduce code size by adding instruction overhead, and
speed up code by unrolling it.  There always is the compromise
between space and time.

It's hard to tell without further analysis of the generated code
whether this specific reduction in size comes from eliminated
shifts, or from the re-use of already pre-loaded variables.  I'd
rather assume the latter, and suggested such a potential cause in
my earlier reply.

Anyway, it's not that I would have strong feelings about this
change.  It's just that I wanted to check whether the motivation
and the description are correct, and observed effects are not
assigned to unrelated components.  Never did I question the
benefit of cleaning up redundancy, I was questioning whether the
commit message appropriately reflects what is observed.

You have presented numbers that were not available before.  The
assumption that your change reduces code size is supported.  The
explanation is an educated guess, concerns are small.  Nevermind.


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