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 <[email protected]>
Reviewed-by: Sam Protsenko <[email protected]>
---
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
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
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
On Tue, Nov 28, 2023 at 1:29 AM Krzysztof Kozlowski
<[email protected]> 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
>
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
>> <[email protected]> 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
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
On Wed, Nov 29, 2023 at 12:32 AM Youngmin Nam <[email protected]> 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 <[email protected]>
[1] https://www.96boards.org/documentation/consumer/e850-96b/hardware-docs/files/e850-96b-schematics.pdf
Thanks!
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 <[email protected]>
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