[PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

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      ...

Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 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);
+
+	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,
 	},
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On 26/11/2023 10:46, Youngmin Nam wrote:
> 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

Does this command succeed on your board?

>     # 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:

...

On which board did you test it?


> +	if (parent)
> +		return parent->chip->irq_set_affinity(parent, dest, force);
> +

I think there is a  helper for it: irq_chip_set_affinity_parent().


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
> On 26/11/2023 10:46, Youngmin Nam wrote:
> > 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
> 
> Does this command succeed on your board?
> 
Yes.

> >     # 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:
> 
> ...
> 
> On which board did you test it?
> 
> 
I tested on S5E9945 ERD(Exynos Reference Development) board.

> > +	if (parent)
> > +		return parent->chip->irq_set_affinity(parent, dest, force);
> > +
> 
> I think there is a  helper for it: irq_chip_set_affinity_parent().
> 
> 

The irq_chip_set_affinity_parent() requires parent_data of irq_data.
But when I tested as below, exynos's irqd->parent_data was null.
So we should use irqchip's affinity function instead of the helper function.

--- 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!!

> Best regards,
> Krzysztof
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On 28/11/2023 02:01, Youngmin Nam wrote:
> On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
>> On 26/11/2023 10:46, Youngmin Nam wrote:
>>> 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
>>
>> Does this command succeed on your board?
>>
> Yes.

Hm, fails all the time one mine.

> 
>>>     # 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:
>>
>> ...
>>
>> On which board did you test it?
>>
>>
> I tested on S5E9945 ERD(Exynos Reference Development) board.

There is no such board upstream. How can we reproduce this issue? I am
afraid we cannot test neither the bug nor the fix.

> 
>>> +	if (parent)
>>> +		return parent->chip->irq_set_affinity(parent, dest, force);
>>> +
>>
>> I think there is a  helper for it: irq_chip_set_affinity_parent().
>>
>>
> 
> The irq_chip_set_affinity_parent() requires parent_data of irq_data.

Hm, so now I wonder why do we not have parent_data...

> But when I tested as below, exynos's irqd->parent_data was null.
> So we should use irqchip's affinity function instead of the helper function.
> 



Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Sam Protsenko]

On Tue, Nov 28, 2023 at 1:29 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/11/2023 02:01, Youngmin Nam wrote:
> > On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
> >> On 26/11/2023 10:46, Youngmin Nam wrote:
> >>> 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
> >>
> >> Does this command succeed on your board?
> >>
> > Yes.
>
> Hm, fails all the time one mine.
>

I tried to test this patch on E850-96, and an attempt to write into
smp_affinity (for some GPIO irq) also fails for me:

    # echo f0 > smp_affinity
    -bash: echo: write error: Input/output error

When I add some pr_err() to exynos_irq_set_affinity(), I can't see
those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
get called at all. So the error probably happens before
.irq_set_affinity callback gets called.

Youngmin, can you please try and test this patch on E850-96? This
board is already supported in upstream kernel. For example you can use
"Volume Up" interrupt for the test, which is GPIO irq.

> >
> >>>     # 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:
> >>
> >> ...
> >>
> >> On which board did you test it?
> >>
> >>
> > I tested on S5E9945 ERD(Exynos Reference Development) board.
>
> There is no such board upstream. How can we reproduce this issue? I am
> afraid we cannot test neither the bug nor the fix.
>
> >
> >>> +   if (parent)
> >>> +           return parent->chip->irq_set_affinity(parent, dest, force);
> >>> +
> >>
> >> I think there is a  helper for it: irq_chip_set_affinity_parent().
> >>
> >>
> >
> > The irq_chip_set_affinity_parent() requires parent_data of irq_data.
>
> Hm, so now I wonder why do we not have parent_data...
>
> > But when I tested as below, exynos's irqd->parent_data was null.
> > So we should use irqchip's affinity function instead of the helper function.
> >
>
>
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]

On Tue, Nov 28, 2023 at 03:35:53PM -0600, Sam Protsenko wrote:
> On Tue, Nov 28, 2023 at 1:29 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 28/11/2023 02:01, Youngmin Nam wrote:
> > > On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
> > >> On 26/11/2023 10:46, Youngmin Nam wrote:
> > >>> 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
> > >>
> > >> Does this command succeed on your board?
> > >>
> > > Yes.
> >
> > Hm, fails all the time one mine.
> >
> 
> I tried to test this patch on E850-96, and an attempt to write into
> smp_affinity (for some GPIO irq) also fails for me:
> 
>     # echo f0 > smp_affinity
>     -bash: echo: write error: Input/output error
> 
> When I add some pr_err() to exynos_irq_set_affinity(), I can't see
> those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
> get called at all. So the error probably happens before
> .irq_set_affinity callback gets called.
> 
> Youngmin, can you please try and test this patch on E850-96? This
> board is already supported in upstream kernel. For example you can use
> "Volume Up" interrupt for the test, which is GPIO irq.
> 

I intened this affinity setting would work only on *Non* Wakeup External Interrupt.
The "Volume Up" on E850-96 board is connected with "gpa0-7" and
that is Wakeup External interrupt so that we can't test the callback.

I couldn't find out a pin for the test on E850-96 board yet.
We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
on E850-96 board.

Do you have any idea ?

Thanks

> > >
> > >>>     # 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:
> > >>
> > >> ...
> > >>
> > >> On which board did you test it?
> > >>
> > >>
> > > I tested on S5E9945 ERD(Exynos Reference Development) board.
> >
> > There is no such board upstream. How can we reproduce this issue? I am
> > afraid we cannot test neither the bug nor the fix.
> >
> > >
> > >>> +   if (parent)
> > >>> +           return parent->chip->irq_set_affinity(parent, dest, force);
> > >>> +
> > >>
> > >> I think there is a  helper for it: irq_chip_set_affinity_parent().
> > >>
> > >>
> > >
> > > The irq_chip_set_affinity_parent() requires parent_data of irq_data.
> >
> > Hm, so now I wonder why do we not have parent_data...
> >
> > > But when I tested as below, exynos's irqd->parent_data was null.
> > > So we should use irqchip's affinity function instead of the helper function.
> > >
> >
> >
> >
> > Best regards,
> > Krzysztof
> >
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On 29/11/2023 08:07, Youngmin Nam wrote:
> On Tue, Nov 28, 2023 at 03:35:53PM -0600, Sam Protsenko wrote:
>> On Tue, Nov 28, 2023 at 1:29 AM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 28/11/2023 02:01, Youngmin Nam wrote:
>>>> On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
>>>>> On 26/11/2023 10:46, Youngmin Nam wrote:
>>>>>> 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
>>>>>
>>>>> Does this command succeed on your board?
>>>>>
>>>> Yes.
>>>
>>> Hm, fails all the time one mine.
>>>
>>
>> I tried to test this patch on E850-96, and an attempt to write into
>> smp_affinity (for some GPIO irq) also fails for me:
>>
>>     # echo f0 > smp_affinity
>>     -bash: echo: write error: Input/output error
>>
>> When I add some pr_err() to exynos_irq_set_affinity(), I can't see
>> those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
>> get called at all. So the error probably happens before
>> .irq_set_affinity callback gets called.
>>
>> Youngmin, can you please try and test this patch on E850-96? This
>> board is already supported in upstream kernel. For example you can use
>> "Volume Up" interrupt for the test, which is GPIO irq.
>>
> 
> I intened this affinity setting would work only on *Non* Wakeup External Interrupt.
> The "Volume Up" on E850-96 board is connected with "gpa0-7" and
> that is Wakeup External interrupt so that we can't test the callback.
> 
> I couldn't find out a pin for the test on E850-96 board yet.
> We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
> on E850-96 board.
> 
> Do you have any idea ?

Please test on any upstream platform or upstream your existing platform.
I hesitate to take this change because I don't trust Samsung that this
was tested on mainline kernel. OK, for sure 100% it was not tested on
mainline, but I am afraid that differences were far beyond just missing
platforms. Therefore the issue might or might not exist at all. Maybe
issue is caused by other Samsung non-upstreamed code.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Wed, Nov 29, 2023 at 09:00:04AM +0100, Krzysztof Kozlowski wrote:
> On 29/11/2023 08:07, Youngmin Nam wrote:
> > On Tue, Nov 28, 2023 at 03:35:53PM -0600, Sam Protsenko wrote:
> >> On Tue, Nov 28, 2023 at 1:29 AM Krzysztof Kozlowski
> >> <krzysztof.kozlowski@linaro.org> wrote:
> >>>
> >>> On 28/11/2023 02:01, Youngmin Nam wrote:
> >>>> On Mon, Nov 27, 2023 at 10:54:56AM +0100, Krzysztof Kozlowski wrote:
> >>>>> On 26/11/2023 10:46, Youngmin Nam wrote:
> >>>>>> 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
> >>>>>
> >>>>> Does this command succeed on your board?
> >>>>>
> >>>> Yes.
> >>>
> >>> Hm, fails all the time one mine.
> >>>
> >>
> >> I tried to test this patch on E850-96, and an attempt to write into
> >> smp_affinity (for some GPIO irq) also fails for me:
> >>
> >>     # echo f0 > smp_affinity
> >>     -bash: echo: write error: Input/output error
> >>
> >> When I add some pr_err() to exynos_irq_set_affinity(), I can't see
> >> those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
> >> get called at all. So the error probably happens before
> >> .irq_set_affinity callback gets called.
> >>
> >> Youngmin, can you please try and test this patch on E850-96? This
> >> board is already supported in upstream kernel. For example you can use
> >> "Volume Up" interrupt for the test, which is GPIO irq.
> >>
> > 
> > I intened this affinity setting would work only on *Non* Wakeup External Interrupt.
> > The "Volume Up" on E850-96 board is connected with "gpa0-7" and
> > that is Wakeup External interrupt so that we can't test the callback.
> > 
> > I couldn't find out a pin for the test on E850-96 board yet.
> > We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
> > on E850-96 board.
> > 
> > Do you have any idea ?
> 
> Please test on any upstream platform or upstream your existing platform.
> I hesitate to take this change because I don't trust Samsung that this
> was tested on mainline kernel. OK, for sure 100% it was not tested on
> mainline, but I am afraid that differences were far beyond just missing
> platforms. Therefore the issue might or might not exist at all. Maybe
> issue is caused by other Samsung non-upstreamed code.
> 
> Best regards,
> Krzysztof

Sure. Let me find how to test on upstreamed device like E850-96 board.
> 
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On 29/11/2023 09:46, Youngmin Nam wrote:
>>> I couldn't find out a pin for the test on E850-96 board yet.
>>> We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
>>> on E850-96 board.
>>>
>>> Do you have any idea ?
>>
>> Please test on any upstream platform or upstream your existing platform.
>> I hesitate to take this change because I don't trust Samsung that this
>> was tested on mainline kernel. OK, for sure 100% it was not tested on
>> mainline, but I am afraid that differences were far beyond just missing
>> platforms. Therefore the issue might or might not exist at all. Maybe
>> issue is caused by other Samsung non-upstreamed code.
>>
>> Best regards,
>> Krzysztof
> 
> Sure. Let me find how to test on upstreamed device like E850-96 board.

There are many reasons why companies using Linux for their products
should be involved in upstreaming their devices.

The one visible from this conversation: Whatever technical debt you
have, it will be only growing because upstream might not even take
simple patches from you, until you start contributing with the rest.
Samsung's out-of-tree kernels are so far away from the upstream, that
basically we might feel that contributions from Samsung are not
addressing real problems. This will affect your Android trees due to GKI.

That's one more argument to talk to with your managers why staying away
from the upstream is not the best idea.

Second argument is look at your competitor: Qualcomm, one of the most
active upstreamers of SoC code doing awesome job.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Wed, Nov 29, 2023 at 09:39:45AM +0100, Krzysztof Kozlowski wrote:
> On 29/11/2023 09:46, Youngmin Nam wrote:
> >>> I couldn't find out a pin for the test on E850-96 board yet.
> >>> We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
> >>> on E850-96 board.
> >>>
> >>> Do you have any idea ?
> >>
> >> Please test on any upstream platform or upstream your existing platform.
> >> I hesitate to take this change because I don't trust Samsung that this
> >> was tested on mainline kernel. OK, for sure 100% it was not tested on
> >> mainline, but I am afraid that differences were far beyond just missing
> >> platforms. Therefore the issue might or might not exist at all. Maybe
> >> issue is caused by other Samsung non-upstreamed code.
> >>
> >> Best regards,
> >> Krzysztof
> > 
> > Sure. Let me find how to test on upstreamed device like E850-96 board.
> 
> There are many reasons why companies using Linux for their products
> should be involved in upstreaming their devices.
> 
> The one visible from this conversation: Whatever technical debt you
> have, it will be only growing because upstream might not even take
> simple patches from you, until you start contributing with the rest.
> Samsung's out-of-tree kernels are so far away from the upstream, that
> basically we might feel that contributions from Samsung are not
> addressing real problems. This will affect your Android trees due to GKI.
> 
> That's one more argument to talk to with your managers why staying away
> from the upstream is not the best idea.
> 
> Second argument is look at your competitor: Qualcomm, one of the most
> active upstreamers of SoC code doing awesome job.
> 
> Best regards,
> Krzysztof

Thank you for your opinion.
By the way, this patch is not related with GKI and I just thought this work
would be helpful to all exynos platform.

But it seems that changing affinity of any irqs including gpio is not allowed
on exynos platform. So we need to debug by adding some logs.

> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Sam Protsenko]

On Wed, Nov 29, 2023 at 12:32 AM Youngmin Nam <youngmin.nam@samsung.com> wrote:
> > I tried to test this patch on E850-96, and an attempt to write into
> > smp_affinity (for some GPIO irq) also fails for me:
> >
> >     # echo f0 > smp_affinity
> >     -bash: echo: write error: Input/output error
> >
> > When I add some pr_err() to exynos_irq_set_affinity(), I can't see
> > those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
> > get called at all. So the error probably happens before
> > .irq_set_affinity callback gets called.
> >
> > Youngmin, can you please try and test this patch on E850-96? This
> > board is already supported in upstream kernel. For example you can use
> > "Volume Up" interrupt for the test, which is GPIO irq.
> >
>
> I intened this affinity setting would work only on *Non* Wakeup External Interrupt.
> The "Volume Up" on E850-96 board is connected with "gpa0-7" and
> that is Wakeup External interrupt so that we can't test the callback.
>

Oh no. It was really silly mistake on my part :) But please check my
answer below for good news.

> I couldn't find out a pin for the test on E850-96 board yet.
> We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
> on E850-96 board.
>
> Do you have any idea ?
>

Yep, just managed to successfully test your patch on E850-96. My
testing procedure below might appear messy, but I didn't want to do
any extra soldering :)

Used GPG1[0] pin for testing. As you can see from schematics [1],
GPG1[0] is connected to R196 resistor (which is not installed on the
board).

I've modified E850-96 dts file like this:

8<---------------------------------------------------------------------------->8
        gpio-keys {
                compatible = "gpio-keys";
                pinctrl-names = "default";
-               pinctrl-0 = <&key_voldown_pins &key_volup_pins>;
+               pinctrl-0 = <&key_voldown_pins &key_volup_pins &key_test_pins>;

                ...

+               test-key {
+                       label = "Test Key";
+                       linux,code = <KEY_RIGHTCTRL>;
+                       gpios = <&gpg1 0 GPIO_ACTIVE_LOW>;
+               };
        };

...

+&pinctrl_peri {
+       key_test_pins: key-test-pins {
+               samsung,pins = "gpg1-0";
+               samsung,pin-function = <EXYNOS_PIN_FUNC_EINT>;
+               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
+               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
+       };
+};
8<---------------------------------------------------------------------------->8

It appeared in /proc/interrupts as follows:

    87:          2          0          0          0          0
 0          0          0      gpg1   0 Edge      Test Key

Then I touched R196 resistor pads with my finger (emulating key press)
and looked again at /proc/interrupts:

    87:        444          0          0          0          0
 0          0          0      gpg1   0 Edge      Test Key

Then I reconfigured smp_affinity like so:

    # cat /proc/irq/87/smp_affinity
    ff
    # echo f0 > /proc/irq/87/smp_affinity
    # cat /proc/irq/87/smp_affinity
    f0

Then I messed with R196 resistor pads with my finger again, and
re-checked /proc/interrupts:

               CPU0       CPU1       CPU2       CPU3       CPU4
CPU5       CPU6       CPU7
     87:        444          0          0          0        234
  0          0          0      gpg1   0 Edge      Test Key

And without this patch that procedure above of course doesn't work.

So as far as I'm concerned, feel free to add:

Tested-by: Sam Protsenko <semen.protsenko@linaro.org>

[1] https://www.96boards.org/documentation/consumer/e850-96b/hardware-docs/files/e850-96b-schematics.pdf

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

On Wed, Nov 29, 2023 at 12:48:56PM -0600, Sam Protsenko wrote:
> On Wed, Nov 29, 2023 at 12:32 AM Youngmin Nam <youngmin.nam@samsung.com> wrote:
> > > I tried to test this patch on E850-96, and an attempt to write into
> > > smp_affinity (for some GPIO irq) also fails for me:
> > >
> > >     # echo f0 > smp_affinity
> > >     -bash: echo: write error: Input/output error
> > >
> > > When I add some pr_err() to exynos_irq_set_affinity(), I can't see
> > > those printed in dmesg. So I guess exynos_irq_set_affinity() doesn't
> > > get called at all. So the error probably happens before
> > > .irq_set_affinity callback gets called.
> > >
> > > Youngmin, can you please try and test this patch on E850-96? This
> > > board is already supported in upstream kernel. For example you can use
> > > "Volume Up" interrupt for the test, which is GPIO irq.
> > >
> >
> > I intened this affinity setting would work only on *Non* Wakeup External Interrupt.
> > The "Volume Up" on E850-96 board is connected with "gpa0-7" and
> > that is Wakeup External interrupt so that we can't test the callback.
> >
> 
> Oh no. It was really silly mistake on my part :) But please check my
> answer below for good news.
> 
> > I couldn't find out a pin for the test on E850-96 board yet.
> > We can test if there is a usage of *Non" Wake up External Interrupt of GPIO
> > on E850-96 board.
> >
> > Do you have any idea ?
> >
> 
> Yep, just managed to successfully test your patch on E850-96. My
> testing procedure below might appear messy, but I didn't want to do
> any extra soldering :)
> 
> Used GPG1[0] pin for testing. As you can see from schematics [1],
> GPG1[0] is connected to R196 resistor (which is not installed on the
> board).
> 
> I've modified E850-96 dts file like this:
> 
> 8<---------------------------------------------------------------------------->8
>         gpio-keys {
>                 compatible = "gpio-keys";
>                 pinctrl-names = "default";
> -               pinctrl-0 = <&key_voldown_pins &key_volup_pins>;
> +               pinctrl-0 = <&key_voldown_pins &key_volup_pins &key_test_pins>;
> 
>                 ...
> 
> +               test-key {
> +                       label = "Test Key";
> +                       linux,code = <KEY_RIGHTCTRL>;
> +                       gpios = <&gpg1 0 GPIO_ACTIVE_LOW>;
> +               };
>         };
> 
> ...
> 
> +&pinctrl_peri {
> +       key_test_pins: key-test-pins {
> +               samsung,pins = "gpg1-0";
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_EINT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +       };
> +};
> 8<---------------------------------------------------------------------------->8
> 
> It appeared in /proc/interrupts as follows:
> 
>     87:          2          0          0          0          0
>  0          0          0      gpg1   0 Edge      Test Key
> 
> Then I touched R196 resistor pads with my finger (emulating key press)
> and looked again at /proc/interrupts:
> 
>     87:        444          0          0          0          0
>  0          0          0      gpg1   0 Edge      Test Key
> 
> Then I reconfigured smp_affinity like so:
> 
>     # cat /proc/irq/87/smp_affinity
>     ff
>     # echo f0 > /proc/irq/87/smp_affinity
>     # cat /proc/irq/87/smp_affinity
>     f0
> 
> Then I messed with R196 resistor pads with my finger again, and
> re-checked /proc/interrupts:
> 
>                CPU0       CPU1       CPU2       CPU3       CPU4
> CPU5       CPU6       CPU7
>      87:        444          0          0          0        234
>   0          0          0      gpg1   0 Edge      Test Key
> 
> And without this patch that procedure above of course doesn't work.
> 
> So as far as I'm concerned, feel free to add:
> 
> Tested-by: Sam Protsenko <semen.protsenko@linaro.org>
> 
> [1] https://protect2.fireeye.com/v1/url?k=d3eebafe-8c7293d4-d3ef31b1-000babe598f7-06c0db00473e1bca&q=1&e=855a917d-d0f9-49cd-8e05-7cccf44d06a8&u=https%3A%2F%2Fwww.96boards.org%2Fdocumentation%2Fconsumer%2Fe850-96b%2Fhardware-docs%2Ffiles%2Fe850-96b-schematics.pdf
> 
> Thanks!
> 
Thanks for your test Sam. It's really great work.
I confirmed the patch did work by following your test sequence as below.

* Before
     CPU0       CPU1       CPU2       CPU3       CPU4 ...
87:    40          0          0          0          0 ... gpg1   0 Edge   Test Key

* After
87:    40          0          0          0         22 ... gpg1   0 Edge   Test Key

Krzysztof,
Sam and I tested this patch on e850-95 board.
Let me update commit message with test result and will update patchset.

Thanks.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On 30/11/2023 08:30, Youngmin Nam wrote:
> Thanks for your test Sam. It's really great work.
> I confirmed the patch did work by following your test sequence as below.
> 
> * Before
>      CPU0       CPU1       CPU2       CPU3       CPU4 ...
> 87:    40          0          0          0          0 ... gpg1   0 Edge   Test Key
> 
> * After
> 87:    40          0          0          0         22 ... gpg1   0 Edge   Test Key
> 
> Krzysztof,
> Sam and I tested this patch on e850-95 board.
> Let me update commit message with test result and will update patchset.


Be sure you always run checkpatch on every patch you send.

[Checkpatch]
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
description?)

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt

[Krzysztof Kozlowski]

On Sun, 26 Nov 2023 18:46:18 +0900, Youngmin Nam wrote:
> 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    ...
> 
> [...]

Applied, thanks!

[1/1] pinctrl: samsung: add irq_set_affinity() for non wake up external gpio interrupt
      https://git.kernel.org/pinctrl/samsung/c/b77f5ef8ebe4d8ee3a712a216415d7f4d4d0acf2

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel