mbox series

[v3,0/3] Minor improvements for Intel pinctrl

Message ID 20230613085054.10976-1-raag.jadav@intel.com
Headers show
Series Minor improvements for Intel pinctrl | expand

Message

Raag Jadav June 13, 2023, 8:50 a.m. UTC 
This series implements minor improvements for Intel pinctrl driver.

The optimizations are as tested with gcc 7.5.0 with default -O2.

Changes since v2:
- Drop redundant patches.
- Update commit message.

Changes since v1:
- Update commit message.
- Specify compiler options used to measure optimizations.
- Drop redundant comments.

Raag Jadav (3):
  pinctrl: intel: refine ->set_mux() hook
  pinctrl: intel: refine ->irq_set_type() hook
  pinctrl: intel: simplify exit path of ->gpio_request_enable() hook

 drivers/pinctrl/intel/pinctrl-intel.c | 48 +++++++++++++--------------
 1 file changed, 24 insertions(+), 24 deletions(-)

Comments

Andy Shevchenko June 14, 2023, 4:22 p.m. UTC | #1 
On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> Utilize a temporary variable for common shift operation
> in ->irq_set_type() hook and improve readability.
> While at it, simplify if-else-if chain and save a few bytes.
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> Function                                     old     new   delta
> intel_gpio_irq_type                          317     301     -16
> Total: Before=10469, After=10453, chg -0.15%

...

>  	value = readl(reg);
> -
>  	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
>  
>  	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> -		value |= PADCFG0_RXEVCFG_EDGE_BOTH << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
>  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> -		value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> -		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
>  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> -		if (type & IRQ_TYPE_LEVEL_LOW)
> -			value |= PADCFG0_RXINV;
> +		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
>  	} else {
> -		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
> +		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
>  	}
>  
> +	if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
> +		value |= PADCFG0_RXINV;
> +
> +	value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
>  	writel(value, reg);

Looking at this I realized that entire temporary variable assignments can be
done outside of spin lock. You probably would need another one for keeping
rxinv value.

Will it give us any memory reduction in comparison to the current code?
Raag Jadav June 15, 2023, 9:48 a.m. UTC | #2 
> On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> > Utilize a temporary variable for common shift operation in
> > ->irq_set_type() hook and improve readability.
> > While at it, simplify if-else-if chain and save a few bytes.
> >
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> > Function                                     old     new   delta
> > intel_gpio_irq_type                          317     301     -16
> > Total: Before=10469, After=10453, chg -0.15%
> 
> ...
> 
> >  	value = readl(reg);
> > -
> >  	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> >
> >  	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > -		value |= PADCFG0_RXEVCFG_EDGE_BOTH <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> >  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> > -		value |= PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > -		value |= PADCFG0_RXINV;
> > +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
> >  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> > -		value |= PADCFG0_RXEVCFG_EDGE <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
> >  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> > -		if (type & IRQ_TYPE_LEVEL_LOW)
> > -			value |= PADCFG0_RXINV;
> > +		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> >  	} else {
> > -		value |= PADCFG0_RXEVCFG_DISABLED <<
> PADCFG0_RXEVCFG_SHIFT;
> > +		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> >  	}
> >
> > +	if (type == IRQ_TYPE_EDGE_FALLING || type ==
> IRQ_TYPE_LEVEL_LOW)
> > +		value |= PADCFG0_RXINV;
> > +
> > +	value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> >  	writel(value, reg);
> 
> Looking at this I realized that entire temporary variable assignments can be
> done outside of spin lock. You probably would need another one for keeping
> rxinv value.

Something like this?

        u32 value, rxevcfg;
        u32 rxinv = 0;

        if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
                rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
        } else if (type & IRQ_TYPE_EDGE_FALLING) {
                rxevcfg = PADCFG0_RXEVCFG_EDGE;
        } else if (type & IRQ_TYPE_EDGE_RISING) {
                rxevcfg = PADCFG0_RXEVCFG_EDGE;
        } else if (type & IRQ_TYPE_LEVEL_MASK) {
                rxevcfg = PADCFG0_RXEVCFG_LEVEL;
        } else {
                rxevcfg = PADCFG0_RXEVCFG_DISABLED;
        }

        if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
                rxinv = PADCFG0_RXINV;

        raw_spin_lock_irqsave(&pctrl->lock, flags);

        intel_gpio_set_gpio_mode(reg);

        value = readl(reg);

        value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
        value |= rxinv;
        value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;

        writel(value, reg);

> Will it give us any memory reduction in comparison to the current code?

add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
Function                                     old     new   delta
intel_gpio_irq_type                          317     321      +4
Total: Before=10469, After=10473, chg +0.04%

Unfortunately gcc doesn't seem to consider this as best of the sequence,
and I'm not entirely sure why.
Mika Westerberg June 15, 2023, 9:55 a.m. UTC | #3 
On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> > > Utilize a temporary variable for common shift operation in
> > > ->irq_set_type() hook and improve readability.
> > > While at it, simplify if-else-if chain and save a few bytes.
> > >
> > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
> > > Function                                     old     new   delta
> > > intel_gpio_irq_type                          317     301     -16
> > > Total: Before=10469, After=10453, chg -0.15%
> > 
> > ...
> > 
> > >  	value = readl(reg);
> > > -
> > >  	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > >
> > >  	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > > -		value |= PADCFG0_RXEVCFG_EDGE_BOTH <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > +		rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > >  	} else if (type & IRQ_TYPE_EDGE_FALLING) {
> > > -		value |= PADCFG0_RXEVCFG_EDGE <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > -		value |= PADCFG0_RXINV;
> > > +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > >  	} else if (type & IRQ_TYPE_EDGE_RISING) {
> > > -		value |= PADCFG0_RXEVCFG_EDGE <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > +		rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > >  	} else if (type & IRQ_TYPE_LEVEL_MASK) {
> > > -		if (type & IRQ_TYPE_LEVEL_LOW)
> > > -			value |= PADCFG0_RXINV;
> > > +		rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > >  	} else {
> > > -		value |= PADCFG0_RXEVCFG_DISABLED <<
> > PADCFG0_RXEVCFG_SHIFT;
> > > +		rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > >  	}
> > >
> > > +	if (type == IRQ_TYPE_EDGE_FALLING || type ==
> > IRQ_TYPE_LEVEL_LOW)
> > > +		value |= PADCFG0_RXINV;
> > > +
> > > +	value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> > >  	writel(value, reg);
> > 
> > Looking at this I realized that entire temporary variable assignments can be
> > done outside of spin lock. You probably would need another one for keeping
> > rxinv value.
> 
> Something like this?
> 
>         u32 value, rxevcfg;
>         u32 rxinv = 0;
> 
>         if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
>                 rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
>         } else if (type & IRQ_TYPE_EDGE_FALLING) {
>                 rxevcfg = PADCFG0_RXEVCFG_EDGE;
>         } else if (type & IRQ_TYPE_EDGE_RISING) {
>                 rxevcfg = PADCFG0_RXEVCFG_EDGE;
>         } else if (type & IRQ_TYPE_LEVEL_MASK) {
>                 rxevcfg = PADCFG0_RXEVCFG_LEVEL;
>         } else {
>                 rxevcfg = PADCFG0_RXEVCFG_DISABLED;
>         }
> 
>         if (type == IRQ_TYPE_EDGE_FALLING || type == IRQ_TYPE_LEVEL_LOW)
>                 rxinv = PADCFG0_RXINV;
> 
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
> 
>         intel_gpio_set_gpio_mode(reg);
> 
>         value = readl(reg);
> 
>         value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
>         value |= rxinv;
>         value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> 
>         writel(value, reg);

This one looks better.

> > Will it give us any memory reduction in comparison to the current code?
> 
> add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
> Function                                     old     new   delta
> intel_gpio_irq_type                          317     321      +4
> Total: Before=10469, After=10473, chg +0.04%
> 
> Unfortunately gcc doesn't seem to consider this as best of the sequence,
> and I'm not entirely sure why.

It's fine as is, readability counts more than few bytes here.
Raag Jadav June 15, 2023, 11:08 a.m. UTC | #4 
> On Thu, Jun 15, 2023 at 12:55:17PM +0300,
> mika.westerberg@linux.intel.com wrote:
> > On Thu, Jun 15, 2023 at 09:48:12AM +0000, Jadav, Raag wrote:
> > > > On Tue, Jun 13, 2023 at 02:20:53PM +0530, Raag Jadav wrote:
> 
> ...
> 
> > > > Looking at this I realized that entire temporary variable
> > > > assignments can be done outside of spin lock. You probably would
> > > > need another one for keeping rxinv value.
> > >
> > > Something like this?
> 
> Almost, see below.
> 
> > >         u32 value, rxevcfg;
> > >         u32 rxinv = 0;
> 
> No assignment here.
> 
>          u32 rxinv, rxevcfg;
>          u32 value;
> 
> > >         if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
> > >                 rxevcfg = PADCFG0_RXEVCFG_EDGE_BOTH;
> > >         } else if (type & IRQ_TYPE_EDGE_FALLING) {
> > >                 rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > >         } else if (type & IRQ_TYPE_EDGE_RISING) {
> > >                 rxevcfg = PADCFG0_RXEVCFG_EDGE;
> > >         } else if (type & IRQ_TYPE_LEVEL_MASK) {
> > >                 rxevcfg = PADCFG0_RXEVCFG_LEVEL;
> > >         } else {
> > >                 rxevcfg = PADCFG0_RXEVCFG_DISABLED;
> > >         }
> 
> Now, if it's fully included in the diff (even with --patience parameter), then
> you may drop {}.
> 
> > >         if (type == IRQ_TYPE_EDGE_FALLING || type ==
> IRQ_TYPE_LEVEL_LOW)
> > >                 rxinv = PADCFG0_RXINV;
> 
> 		else
> 			rxinv = 0;
> 
> > >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > >         intel_gpio_set_gpio_mode(reg);
> > >
> > >         value = readl(reg);
> > >
> > >         value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> > >         value |= rxinv;
> > >         value |= rxevcfg << PADCFG0_RXEVCFG_SHIFT;
> 
> And I would rewrite these to the standard patterns:
> 
>          value = (value & ~PADCFG0_RXINV) | rxinv;
>          value = (value & ~PADCFG0_RXEVCFG_MASK) | (rxevcfg <<
> PADCFG0_RXEVCFG_SHIFT);
> 
> And looking at this, perhaps do shift also outside the lock:
> 
>          } else {
>                  rxevcfg = PADCFG0_RXEVCFG_DISABLED;
>          }
>          rxevcfg <<= PADCFG0_RXEVCFG_SHIFT;
> 
> But, taking into account scope of the _RXEVCFG_*, I would add shift directly
> to the definitions and kill that SHIFT entirely:
> 
> #define PADCFG0_RXEVCFG_LEVEL           (0 << 25)
> #define PADCFG0_RXEVCFG_EDGE            (1 << 25)
> #define PADCFG0_RXEVCFG_DISABLED        (2 << 25)
> #define PADCFG0_RXEVCFG_EDGE_BOTH       (3 << 25)
> 
> 	 ...
> 
>          value = (value & ~PADCFG0_RXINV) | rxinv;
>          value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg;
> 
> Try that one and look if it looks better. It might even save bytes after all.

Should I add all of this in original patch or send this as a separate patch
on top this series?