| Message ID | 20231119092909.3018578-1-youngmin.nam@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt | expand |
On Sun, Nov 19, 2023 at 2:54 AM Youngmin Nam <youngmin.nam@samsung.com> wrote: > > To support affinity setting for non wake up external gpio interrupt, > we add a new irq_set_affinity callback using irq number which is in pinctrl > driver data. > > Before applying this patch, we couldn't change irq affinity of gpio interrupt. > * before > erd9945:/proc/irq/418 # cat smp_affinity > 3ff > erd9945:/proc/irq/418 # echo 00f > smp_affinity > erd9945:/proc/irq/418 # cat smp_affinity > 3ff > erd9945:/proc/irq/418 # cat /proc/interrupts > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > 418: 3631 0 0 0 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > > After applying this patch, we can change irq affinity of gpio interrupt as below. > * after > erd9945:/proc/irq/418 # cat smp_affinity > 3ff > erd9945:/proc/irq/418 # echo 00f > smp_affinity > erd9945:/proc/irq/418 # cat smp_affinity > 00f > erd9945:/proc/irq/418 # cat /proc/interrupts > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > 418: 3893 201 181 188 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > Suggest formatting the commit message as follows, to make it more readable: 8<-------------------------------------------------------------------------->8 To support affinity setting for non wake up external gpio interrupt, add irq_set_affinity callback using irq number from pinctrl driver data. Before this patch, changing the irq affinity of gpio interrupt is not possible: # cat /proc/irq/418/smp_affinity 3ff # echo 00f > /proc/irq/418/smp_affinity # cat /proc/irq/418/smp_affinity 3ff # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 ... 418: 3631 0 0 0 ... With this patch applied, it's possible to change irq affinity of gpio interrupt: # cat /proc/irq/418/smp_affinity 3ff # echo 00f > /proc/irq/418/smp_affinity # cat /proc/irq/418/smp_affinity 00f # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 ... 418: 3893 201 181 188 ... 8<-------------------------------------------------------------------------->8 > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com> > --- > drivers/pinctrl/samsung/pinctrl-exynos.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > index 6b58ec84e34b..5d7b788282e9 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -147,6 +147,19 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) > return 0; > } > > +static int exynos_irq_set_affinity(struct irq_data *irqd, > + const struct cpumask *dest, bool force) > +{ > + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > + struct samsung_pinctrl_drv_data *d = bank->drvdata; > + struct irq_data *parent = irq_get_irq_data(d->irq); I'm probably missing something, but: why not just use "irqd" parameter and avoid declaring "bank" and "d"? Is "d->irq" somehow different from "irqd"? > + > + if (parent) > + return parent->chip->irq_set_affinity(parent, dest, force); > + Why not use irq_chip_set_affinity_parent() API? > + return -EINVAL; Maybe use something like this instead: if (!irqd->parent_data) return -EINVAL; return irq_chip_set_affinity_parent(irqd, dest, force); Can you please test if this code works? > +} > + > static int exynos_irq_request_resources(struct irq_data *irqd) > { > struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > @@ -212,6 +225,7 @@ static const struct exynos_irq_chip exynos_gpio_irq_chip __initconst = { > .irq_mask = exynos_irq_mask, > .irq_ack = exynos_irq_ack, > .irq_set_type = exynos_irq_set_type, > + .irq_set_affinity = exynos_irq_set_affinity, What happens if we just assign irq_chip_set_affinity_parent() here? Would it work, or Exynos case is more complicated than this? > .irq_request_resources = exynos_irq_request_resources, > .irq_release_resources = exynos_irq_release_resources, > }, > -- > 2.39.2 >
On Tue, Nov 21, 2023 at 12:33:51PM -0600, Sam Protsenko wrote: > On Sun, Nov 19, 2023 at 2:54 AM Youngmin Nam <youngmin.nam@samsung.com> wrote: > > > > To support affinity setting for non wake up external gpio interrupt, > > we add a new irq_set_affinity callback using irq number which is in pinctrl > > driver data. > > > > Before applying this patch, we couldn't change irq affinity of gpio interrupt. > > * before > > erd9945:/proc/irq/418 # cat smp_affinity > > 3ff > > erd9945:/proc/irq/418 # echo 00f > smp_affinity > > erd9945:/proc/irq/418 # cat smp_affinity > > 3ff > > erd9945:/proc/irq/418 # cat /proc/interrupts > > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > > 418: 3631 0 0 0 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > > > > After applying this patch, we can change irq affinity of gpio interrupt as below. > > * after > > erd9945:/proc/irq/418 # cat smp_affinity > > 3ff > > erd9945:/proc/irq/418 # echo 00f > smp_affinity > > erd9945:/proc/irq/418 # cat smp_affinity > > 00f > > erd9945:/proc/irq/418 # cat /proc/interrupts > > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > > 418: 3893 201 181 188 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > > > > Suggest formatting the commit message as follows, to make it more readable: > > 8<-------------------------------------------------------------------------->8 > To support affinity setting for non wake up external gpio interrupt, > add irq_set_affinity callback using irq number from pinctrl driver > data. > > Before this patch, changing the irq affinity of gpio interrupt is not > possible: > > # cat /proc/irq/418/smp_affinity > 3ff > # echo 00f > /proc/irq/418/smp_affinity > # cat /proc/irq/418/smp_affinity > 3ff > # cat /proc/interrupts > CPU0 CPU1 CPU2 CPU3 ... > 418: 3631 0 0 0 ... > > With this patch applied, it's possible to change irq affinity of gpio > interrupt: > > # cat /proc/irq/418/smp_affinity > 3ff > # echo 00f > /proc/irq/418/smp_affinity > # cat /proc/irq/418/smp_affinity > 00f > # cat /proc/interrupts > CPU0 CPU1 CPU2 CPU3 ... > 418: 3893 201 181 188 ... > 8<-------------------------------------------------------------------------->8 > Thanks for your suggestion. I'll modify it. > > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com> > > --- > > drivers/pinctrl/samsung/pinctrl-exynos.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > > index 6b58ec84e34b..5d7b788282e9 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > > @@ -147,6 +147,19 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) > > return 0; > > } > > > > +static int exynos_irq_set_affinity(struct irq_data *irqd, > > + const struct cpumask *dest, bool force) > > +{ > > + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > > + struct samsung_pinctrl_drv_data *d = bank->drvdata; > > + struct irq_data *parent = irq_get_irq_data(d->irq); > > I'm probably missing something, but: why not just use "irqd" parameter > and avoid declaring "bank" and "d"? Is "d->irq" somehow different from > "irqd"? > Yes, irqd->irq is different from d->irq as below. [ 188.230707] irqd->irq is 417 [ 188.230837] d->irq is 133 We have to use d->irq(133) instead of irqd->irq(417) because d->irq has GICv3 as a IRQ chip. To use set_affinity() call back of GICv3, d->irq is needed. IRQ HWIRQ Type Affinity IRQ_DESC CPU0 CPU1 CPU2 CPU3 ... Chip Name 133 603 Level 0x3ff 0xffffff883b25d800 52260 0 0 0 ... GICv3 11030000.pinctrl 417 0 Edge 0xffffffff 0xffffff883b68a800 52259 0 0 0 ... gpg2 19100000.drmdecon erd9945: # cat /proc/interrupts | grep gpg2 417: 9250 48 45 45 ... gpg2 0 Edge 19100000.drmdecon erd9945: # cat /proc/interrupts | grep 11030000 133: 9250 48 45 45 ... GICv3 603 Level 11030000.pinctrl > > + > > + if (parent) > > + return parent->chip->irq_set_affinity(parent, dest, force); > > + > > Why not use irq_chip_set_affinity_parent() API? > > > + return -EINVAL; > > Maybe use something like this instead: > > if (!irqd->parent_data) > return -EINVAL; > > return irq_chip_set_affinity_parent(irqd, dest, force); > > Can you please test if this code works? > I tested as you suggested as below. diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index bf8dd5e3c3d2..593320b408ce 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -153,14 +153,12 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) static int exynos_irq_set_affinity(struct irq_data *irqd, const struct cpumask *dest, bool force) { - struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); - struct samsung_pinctrl_drv_data *d = bank->drvdata; - struct irq_data *parent = irq_get_irq_data(d->irq); - - if (parent) - return parent->chip->irq_set_affinity(parent, dest, force); + if (!irqd->parent_data) { + pr_err("irqd->parent_data is null!!\n"); + return -EINVAL; + } - return -EINVAL; + return irq_chip_set_affinity_parent(irqd, dest, force); } [ 149.658395] irqd->parent_data is null!! Currently, irqd->paranet_data is null. > > +} > > + > > static int exynos_irq_request_resources(struct irq_data *irqd) > > { > > struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > > @@ -212,6 +225,7 @@ static const struct exynos_irq_chip exynos_gpio_irq_chip __initconst = { > > .irq_mask = exynos_irq_mask, > > .irq_ack = exynos_irq_ack, > > .irq_set_type = exynos_irq_set_type, > > + .irq_set_affinity = exynos_irq_set_affinity, > > What happens if we just assign irq_chip_set_affinity_parent() here? > Would it work, or Exynos case is more complicated than this? > Yes, I couldn't find how to use irq_chip_set_affinity_parent() directly yet. > > .irq_request_resources = exynos_irq_request_resources, > > .irq_release_resources = exynos_irq_release_resources, > > }, > > -- > > 2.39.2 > > >
On Thu, Nov 23, 2023 at 10:00 PM Youngmin Nam <youngmin.nam@samsung.com> wrote: > > On Tue, Nov 21, 2023 at 12:33:51PM -0600, Sam Protsenko wrote: > > On Sun, Nov 19, 2023 at 2:54 AM Youngmin Nam <youngmin.nam@samsung.com> wrote: > > > > > > To support affinity setting for non wake up external gpio interrupt, > > > we add a new irq_set_affinity callback using irq number which is in pinctrl > > > driver data. > > > > > > Before applying this patch, we couldn't change irq affinity of gpio interrupt. > > > * before > > > erd9945:/proc/irq/418 # cat smp_affinity > > > 3ff > > > erd9945:/proc/irq/418 # echo 00f > smp_affinity > > > erd9945:/proc/irq/418 # cat smp_affinity > > > 3ff > > > erd9945:/proc/irq/418 # cat /proc/interrupts > > > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > > > 418: 3631 0 0 0 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > > > > > > After applying this patch, we can change irq affinity of gpio interrupt as below. > > > * after > > > erd9945:/proc/irq/418 # cat smp_affinity > > > 3ff > > > erd9945:/proc/irq/418 # echo 00f > smp_affinity > > > erd9945:/proc/irq/418 # cat smp_affinity > > > 00f > > > erd9945:/proc/irq/418 # cat /proc/interrupts > > > CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 > > > 418: 3893 201 181 188 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon > > > > > > > Suggest formatting the commit message as follows, to make it more readable: > > > > 8<-------------------------------------------------------------------------->8 > > To support affinity setting for non wake up external gpio interrupt, > > add irq_set_affinity callback using irq number from pinctrl driver > > data. > > > > Before this patch, changing the irq affinity of gpio interrupt is not > > possible: > > > > # cat /proc/irq/418/smp_affinity > > 3ff > > # echo 00f > /proc/irq/418/smp_affinity > > # cat /proc/irq/418/smp_affinity > > 3ff > > # cat /proc/interrupts > > CPU0 CPU1 CPU2 CPU3 ... > > 418: 3631 0 0 0 ... > > > > With this patch applied, it's possible to change irq affinity of gpio > > interrupt: > > > > # cat /proc/irq/418/smp_affinity > > 3ff > > # echo 00f > /proc/irq/418/smp_affinity > > # cat /proc/irq/418/smp_affinity > > 00f > > # cat /proc/interrupts > > CPU0 CPU1 CPU2 CPU3 ... > > 418: 3893 201 181 188 ... > > 8<-------------------------------------------------------------------------->8 > > > > Thanks for your suggestion. I'll modify it. > > > > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com> > > > --- > > > drivers/pinctrl/samsung/pinctrl-exynos.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > > > index 6b58ec84e34b..5d7b788282e9 100644 > > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > > > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > > > @@ -147,6 +147,19 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) > > > return 0; > > > } > > > > > > +static int exynos_irq_set_affinity(struct irq_data *irqd, > > > + const struct cpumask *dest, bool force) > > > +{ > > > + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > > > + struct samsung_pinctrl_drv_data *d = bank->drvdata; > > > + struct irq_data *parent = irq_get_irq_data(d->irq); > > > > I'm probably missing something, but: why not just use "irqd" parameter > > and avoid declaring "bank" and "d"? Is "d->irq" somehow different from > > "irqd"? > > > > Yes, irqd->irq is different from d->irq as below. > > [ 188.230707] irqd->irq is 417 > [ 188.230837] d->irq is 133 > > We have to use d->irq(133) instead of irqd->irq(417) because d->irq has GICv3 as a IRQ chip. > To use set_affinity() call back of GICv3, d->irq is needed. > > IRQ HWIRQ Type Affinity IRQ_DESC CPU0 CPU1 CPU2 CPU3 ... Chip Name > 133 603 Level 0x3ff 0xffffff883b25d800 52260 0 0 0 ... GICv3 11030000.pinctrl > 417 0 Edge 0xffffffff 0xffffff883b68a800 52259 0 0 0 ... gpg2 19100000.drmdecon > > erd9945: # cat /proc/interrupts | grep gpg2 > 417: 9250 48 45 45 ... gpg2 0 Edge 19100000.drmdecon > > erd9945: # cat /proc/interrupts | grep 11030000 > 133: 9250 48 45 45 ... GICv3 603 Level 11030000.pinctrl > Thanks for the explanation! Apart from my suggestion for the commit message: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > > > + > > > + if (parent) > > > + return parent->chip->irq_set_affinity(parent, dest, force); > > > + > > > > Why not use irq_chip_set_affinity_parent() API? > > > > > + return -EINVAL; > > > > Maybe use something like this instead: > > > > if (!irqd->parent_data) > > return -EINVAL; > > > > return irq_chip_set_affinity_parent(irqd, dest, force); > > > > Can you please test if this code works? > > > > I tested as you suggested as below. > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > index bf8dd5e3c3d2..593320b408ce 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -153,14 +153,12 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) > static int exynos_irq_set_affinity(struct irq_data *irqd, > const struct cpumask *dest, bool force) > { > - struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > - struct samsung_pinctrl_drv_data *d = bank->drvdata; > - struct irq_data *parent = irq_get_irq_data(d->irq); > - > - if (parent) > - return parent->chip->irq_set_affinity(parent, dest, force); > + if (!irqd->parent_data) { > + pr_err("irqd->parent_data is null!!\n"); > + return -EINVAL; > + } > > - return -EINVAL; > + return irq_chip_set_affinity_parent(irqd, dest, force); > } > > [ 149.658395] irqd->parent_data is null!! > > Currently, irqd->paranet_data is null. > > > > +} > > > + > > > static int exynos_irq_request_resources(struct irq_data *irqd) > > > { > > > struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); > > > @@ -212,6 +225,7 @@ static const struct exynos_irq_chip exynos_gpio_irq_chip __initconst = { > > > .irq_mask = exynos_irq_mask, > > > .irq_ack = exynos_irq_ack, > > > .irq_set_type = exynos_irq_set_type, > > > + .irq_set_affinity = exynos_irq_set_affinity, > > > > What happens if we just assign irq_chip_set_affinity_parent() here? > > Would it work, or Exynos case is more complicated than this? > > > > Yes, I couldn't find how to use irq_chip_set_affinity_parent() directly yet. > > > > .irq_request_resources = exynos_irq_request_resources, > > > .irq_release_resources = exynos_irq_release_resources, > > > }, > > > -- > > > 2.39.2 > > > > >
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index 6b58ec84e34b..5d7b788282e9 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -147,6 +147,19 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) return 0; } +static int exynos_irq_set_affinity(struct irq_data *irqd, + const struct cpumask *dest, bool force) +{ + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); + struct samsung_pinctrl_drv_data *d = bank->drvdata; + struct irq_data *parent = irq_get_irq_data(d->irq); + + if (parent) + return parent->chip->irq_set_affinity(parent, dest, force); + + return -EINVAL; +} + static int exynos_irq_request_resources(struct irq_data *irqd) { struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); @@ -212,6 +225,7 @@ static const struct exynos_irq_chip exynos_gpio_irq_chip __initconst = { .irq_mask = exynos_irq_mask, .irq_ack = exynos_irq_ack, .irq_set_type = exynos_irq_set_type, + .irq_set_affinity = exynos_irq_set_affinity, .irq_request_resources = exynos_irq_request_resources, .irq_release_resources = exynos_irq_release_resources, },
To support affinity setting for non wake up external gpio interrupt, we add a new irq_set_affinity callback using irq number which is in pinctrl driver data. Before applying this patch, we couldn't change irq affinity of gpio interrupt. * before erd9945:/proc/irq/418 # cat smp_affinity 3ff erd9945:/proc/irq/418 # echo 00f > smp_affinity erd9945:/proc/irq/418 # cat smp_affinity 3ff erd9945:/proc/irq/418 # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 418: 3631 0 0 0 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon After applying this patch, we can change irq affinity of gpio interrupt as below. * after erd9945:/proc/irq/418 # cat smp_affinity 3ff erd9945:/proc/irq/418 # echo 00f > smp_affinity erd9945:/proc/irq/418 # cat smp_affinity 00f erd9945:/proc/irq/418 # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7 CPU8 CPU9 418: 3893 201 181 188 0 0 0 0 0 0 gpg2 0 Edge 19100000.drmdecon Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com> --- drivers/pinctrl/samsung/pinctrl-exynos.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)