* [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
@ 2023-06-15 12:50 Raag Jadav
2023-06-15 13:14 ` Andy Shevchenko
2023-06-15 15:28 ` Andy Shevchenko
0 siblings, 2 replies; 7+ messages in thread
From: Raag Jadav @ 2023-06-15 12:50 UTC (permalink / raw)
To: linus.walleij, mika.westerberg, andriy.shevchenko
Cc: linux-gpio, linux-kernel, mallikarjunappa.sangannavar, pandith.n,
Raag Jadav
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(-)
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)
base-commit: e95433c367e681dc6d4613706bd74f483a25acd8
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 12:50 [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook Raag Jadav
@ 2023-06-15 13:14 ` Andy Shevchenko
2023-06-15 13:35 ` Jadav, Raag
2023-06-15 15:28 ` Andy Shevchenko
1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2023-06-15 13:14 UTC (permalink / raw)
To: Raag Jadav
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
mallikarjunappa.sangannavar, pandith.n
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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 13:14 ` Andy Shevchenko
@ 2023-06-15 13:35 ` Jadav, Raag
2023-06-15 14:09 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Jadav, Raag @ 2023-06-15 13:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
Sangannavar, Mallikarjunappa, N, Pandith
> 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%
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 13:35 ` Jadav, Raag
@ 2023-06-15 14:09 ` Andy Shevchenko
2023-06-15 15:05 ` Jadav, Raag
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2023-06-15 14:09 UTC (permalink / raw)
To: Jadav, Raag
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
Sangannavar, Mallikarjunappa, N, Pandith
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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 14:09 ` Andy Shevchenko
@ 2023-06-15 15:05 ` Jadav, Raag
2023-06-15 15:16 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Jadav, Raag @ 2023-06-15 15:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
Sangannavar, Mallikarjunappa, N, Pandith
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 15:05 ` Jadav, Raag
@ 2023-06-15 15:16 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2023-06-15 15:16 UTC (permalink / raw)
To: Jadav, Raag
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
Sangannavar, Mallikarjunappa, N, Pandith
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook
2023-06-15 12:50 [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook Raag Jadav
2023-06-15 13:14 ` Andy Shevchenko
@ 2023-06-15 15:28 ` Andy Shevchenko
1 sibling, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2023-06-15 15:28 UTC (permalink / raw)
To: Raag Jadav
Cc: linus.walleij, mika.westerberg, linux-gpio, linux-kernel,
mallikarjunappa.sangannavar, pandith.n
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!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-15 15:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 12:50 [PATCH v4] pinctrl: intel: refine ->irq_set_type() hook Raag Jadav
2023-06-15 13:14 ` Andy Shevchenko
2023-06-15 13:35 ` Jadav, Raag
2023-06-15 14:09 ` Andy Shevchenko
2023-06-15 15:05 ` Jadav, Raag
2023-06-15 15:16 ` Andy Shevchenko
2023-06-15 15:28 ` Andy Shevchenko
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.