Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <CGME20231119085440epcas2p375fa3b2999e1a3ceeff9949136db7e28@epcas2p3.samsung.com>
 <20231119092909.3018578-1-youngmin.nam@samsung.com>
In-Reply-To: <20231119092909.3018578-1-youngmin.nam@samsung.com>
From:   Sam Protsenko <semen.protsenko@linaro.org>
Date:   Tue, 21 Nov 2023 12:33:51 -0600
Message-ID: <CAPLW+4kO4wYP=5Sx7dPXU17b_CHBJKN_93GhWtZ60vKgNRTKwQ@mail.gmail.com>
Subject: Re: [PATCH] pinctrl: samsung: add irq_set_affinity() for non wake up
 external gpio interrupt
To:     Youngmin Nam <youngmin.nam@samsung.com>
Cc:     tomasz.figa@gmail.com, krzysztof.kozlowski@linaro.org,
        s.nawrocki@samsung.com, alim.akhtar@samsung.com,
        linus.walleij@linaro.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Nov 19, 2023 at 2:54=E2=80=AFAM 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 pinct=
rl
> driver data.
>
> Before applying this patch, we couldn't change irq affinity of gpio inter=
rupt.
> * 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 a=
s 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/s=
amsung/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 =3D irq_data_get_irq_chip_data(irqd=
);
> +       struct samsung_pinctrl_drv_data *d =3D bank->drvdata;
> +       struct irq_data *parent =3D 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 =3D irq_data_get_irq_chip_data(irqd=
);
> @@ -212,6 +225,7 @@ static const struct exynos_irq_chip exynos_gpio_irq_c=
hip __initconst =3D {
>                 .irq_mask =3D exynos_irq_mask,
>                 .irq_ack =3D exynos_irq_ack,
>                 .irq_set_type =3D exynos_irq_set_type,
> +               .irq_set_affinity =3D 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 =3D exynos_irq_request_resources,
>                 .irq_release_resources =3D exynos_irq_release_resources,
>         },
> --
> 2.39.2
>