On Thu, Nov 23, 2023 at 10:00 PM Youngmin Nam <youngmin.nam@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> > > +
> > > + 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
> > >
> >
