| Message ID | 20230615125022.27421-1-raag.jadav@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] pinctrl: intel: refine ->irq_set_type() hook | expand |
On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote: > Refine ->irq_set_type() hook and improve its readability by: > > - Reducing scope of spinlock by moving unneeded operations out of it. > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly > into PADCFG0_RXEVCFG_* definitions. > - Utilizing temporary variables for common operations. > - Simplifying if-else-if chain. Two questions out of curiosity. Do we gain or lose bytes with this? > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 45 ++++++++++++++------------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index e8adf2580321..036eae74c479 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -55,12 +55,11 @@ > > /* Offset from pad_regs */ > #define PADCFG0 0x000 > -#define PADCFG0_RXEVCFG_SHIFT 25 > #define PADCFG0_RXEVCFG_MASK GENMASK(26, 25) > -#define PADCFG0_RXEVCFG_LEVEL 0 > -#define PADCFG0_RXEVCFG_EDGE 1 > -#define PADCFG0_RXEVCFG_DISABLED 2 > -#define PADCFG0_RXEVCFG_EDGE_BOTH 3 > +#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) > #define PADCFG0_PREGFRXSEL BIT(24) > #define PADCFG0_RXINV BIT(23) > #define PADCFG0_GPIROUTIOXAPIC BIT(20) > @@ -1127,9 +1126,9 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type) > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct intel_pinctrl *pctrl = gpiochip_get_data(gc); > unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL); > + u32 rxevcfg, rxinv, value; > unsigned long flags; > void __iomem *reg; > - u32 value; > > reg = intel_get_padcfg(pctrl, pin, PADCFG0); > if (!reg) > @@ -1145,28 +1144,32 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type) > return -EPERM; > } > > - raw_spin_lock_irqsave(&pctrl->lock, flags); > - > - intel_gpio_set_gpio_mode(reg); > - > - 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) > + rxinv = PADCFG0_RXINV; > + else > + rxinv = 0; > + > + raw_spin_lock_irqsave(&pctrl->lock, flags); > + > + intel_gpio_set_gpio_mode(reg); > + > + value = readl(reg); > + value = (value & ~PADCFG0_RXINV) | rxinv; > + value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg; Same question if we change this to be similar to the current code, i.e. value = readl(reg); value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV); value |= rxevcfg; value |= rxinv; And I would keep blank lines after readl() and before writel() since we have more than a single line in between. > writel(value, reg); > > if (type & IRQ_TYPE_EDGE_BOTH) > > base-commit: e95433c367e681dc6d4613706bd74f483a25acd8 > -- > 2.17.1 >
> On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote: > > Refine ->irq_set_type() hook and improve its readability by: > > > > - Reducing scope of spinlock by moving unneeded operations out of it. > > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly > > into PADCFG0_RXEVCFG_* definitions. > > - Utilizing temporary variables for common operations. > > - Simplifying if-else-if chain. > > Two questions out of curiosity. > Do we gain or lose bytes with this? add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33) Function old new delta intel_gpio_irq_type 317 350 +33 Total: Before=10469, After=10502, chg +0.32% > > + value = readl(reg); > > > + value = (value & ~PADCFG0_RXINV) | rxinv; > > + value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg; > > Same question if we change this to be similar to the current code, i.e. > > value = readl(reg); > > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV); > value |= rxevcfg; > value |= rxinv; > > And I would keep blank lines after readl() and before writel() since we have > more than a single line in between. add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9) Function old new delta intel_gpio_irq_type 317 308 -9 Total: Before=10469, After=10460, chg -0.09%
On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote: > > On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote: > > > Refine ->irq_set_type() hook and improve its readability by: > > > > > > - Reducing scope of spinlock by moving unneeded operations out of it. > > > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly > > > into PADCFG0_RXEVCFG_* definitions. > > > - Utilizing temporary variables for common operations. > > > - Simplifying if-else-if chain. > > > > Two questions out of curiosity. > > Do we gain or lose bytes with this? > > add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33) > Function old new delta > intel_gpio_irq_type 317 350 +33 > Total: Before=10469, After=10502, chg +0.32% > > > > + value = readl(reg); > > > > > + value = (value & ~PADCFG0_RXINV) | rxinv; > > > + value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg; > > > > Same question if we change this to be similar to the current code, i.e. > > > > value = readl(reg); > > > > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV); > > value |= rxevcfg; > > value |= rxinv; > > > > And I would keep blank lines after readl() and before writel() since we have > > more than a single line in between. > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9) > Function old new delta > intel_gpio_irq_type 317 308 -9 > Total: Before=10469, After=10460, chg -0.09% Do I understand correctly that this is your patch + suggested above?
> On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote: > > > On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote: > > > > Refine ->irq_set_type() hook and improve its readability by: > > > > > > > > - Reducing scope of spinlock by moving unneeded operations out of it. > > > > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it > directly > > > > into PADCFG0_RXEVCFG_* definitions. > > > > - Utilizing temporary variables for common operations. > > > > - Simplifying if-else-if chain. > > > > > > Two questions out of curiosity. > > > Do we gain or lose bytes with this? > > > > add/remove: 0/0 grow/shrink: 1/0 up/down: 33/0 (33) > > Function old new delta > > intel_gpio_irq_type 317 350 +33 > > Total: Before=10469, After=10502, chg +0.32% > > > > > > + value = readl(reg); > > > > > > > + value = (value & ~PADCFG0_RXINV) | rxinv; > > > > + value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg; > > > > > > Same question if we change this to be similar to the current code, i.e. > > > > > > value = readl(reg); > > > > > > value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV); > > > value |= rxevcfg; > > > value |= rxinv; > > > > > > And I would keep blank lines after readl() and before writel() since > > > we have more than a single line in between. > > > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9) > > Function old new delta > > intel_gpio_irq_type 317 308 -9 > > Total: Before=10469, After=10460, chg -0.09% > > Do I understand correctly that this is your patch + suggested above? Yes, this is tested with gcc 7.5.0 with default -O2. I see some reordering in disassembly even with this simple change, and I'm not entirely sure what kind of weird tricks gcc is pulling here.
On Thu, Jun 15, 2023 at 03:05:42PM +0000, Jadav, Raag wrote: > > On Thu, Jun 15, 2023 at 01:35:19PM +0000, Jadav, Raag wrote: ... > > > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-9 (-9) > > > Function old new delta > > > intel_gpio_irq_type 317 308 -9 > > > Total: Before=10469, After=10460, chg -0.09% > > > > Do I understand correctly that this is your patch + suggested above? > > Yes, this is tested with gcc 7.5.0 with default -O2. > I see some reordering in disassembly even with this simple change, > and I'm not entirely sure what kind of weird tricks gcc is pulling here. For me gcc (Debian 12.2.0-14) 12.2.0 gives the same, +4 bytes. I am going to apply v4 with suggested tweaks.
On Thu, Jun 15, 2023 at 06:20:22PM +0530, Raag Jadav wrote: > Refine ->irq_set_type() hook and improve its readability by: > > - Reducing scope of spinlock by moving unneeded operations out of it. > - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly > into PADCFG0_RXEVCFG_* definitions. > - Utilizing temporary variables for common operations. > - Simplifying if-else-if chain. Pushed to my review and testing queue, thanks!
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index e8adf2580321..036eae74c479 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -55,12 +55,11 @@ /* Offset from pad_regs */ #define PADCFG0 0x000 -#define PADCFG0_RXEVCFG_SHIFT 25 #define PADCFG0_RXEVCFG_MASK GENMASK(26, 25) -#define PADCFG0_RXEVCFG_LEVEL 0 -#define PADCFG0_RXEVCFG_EDGE 1 -#define PADCFG0_RXEVCFG_DISABLED 2 -#define PADCFG0_RXEVCFG_EDGE_BOTH 3 +#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) #define PADCFG0_PREGFRXSEL BIT(24) #define PADCFG0_RXINV BIT(23) #define PADCFG0_GPIROUTIOXAPIC BIT(20) @@ -1127,9 +1126,9 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type) struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct intel_pinctrl *pctrl = gpiochip_get_data(gc); unsigned int pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL); + u32 rxevcfg, rxinv, value; unsigned long flags; void __iomem *reg; - u32 value; reg = intel_get_padcfg(pctrl, pin, PADCFG0); if (!reg) @@ -1145,28 +1144,32 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type) return -EPERM; } - raw_spin_lock_irqsave(&pctrl->lock, flags); - - intel_gpio_set_gpio_mode(reg); - - 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) + rxinv = PADCFG0_RXINV; + else + rxinv = 0; + + raw_spin_lock_irqsave(&pctrl->lock, flags); + + intel_gpio_set_gpio_mode(reg); + + value = readl(reg); + + value = (value & ~PADCFG0_RXINV) | rxinv; + value = (value & ~PADCFG0_RXEVCFG_MASK) | rxevcfg; + writel(value, reg); if (type & IRQ_TYPE_EDGE_BOTH)
Refine ->irq_set_type() hook and improve its readability by: - Reducing scope of spinlock by moving unneeded operations out of it. - Dropping redundant PADCFG0_RXEVCFG_SHIFT and including it directly into PADCFG0_RXEVCFG_* definitions. - Utilizing temporary variables for common operations. - Simplifying if-else-if chain. Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 45 ++++++++++++++------------- 1 file changed, 24 insertions(+), 21 deletions(-) base-commit: e95433c367e681dc6d4613706bd74f483a25acd8