Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

Hi Chanho and Sam,

I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
node with: wakeup-interrupt-controller. This is an interrupt muxing
several external wakeup interrupts, e.g. EINT16 - EINT31.

For Exynos5433 this looks like:
https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857

Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
you should see in dmesg error log like:
    "irq number for muxed EINTs not found"

Can you check that your wakeup-interrupt-controller is properly defined
in DTSI? If yes, I will need to include such differences in the dtschema.

[1] https://github.com/krzk/linux/tree/n/dt-bindings-samsung-pinctrl-schema

Best regards,
Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

On 30/12/2021 20:34, Krzysztof Kozlowski wrote:
> Hi Chanho and Sam,
> 
> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
> node with: wakeup-interrupt-controller. This is an interrupt muxing
> several external wakeup interrupts, e.g. EINT16 - EINT31.
> 
> For Exynos5433 this looks like:
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
> 
> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
> you should see in dmesg error log like:
>     "irq number for muxed EINTs not found"
> 
> Can you check that your wakeup-interrupt-controller is properly defined
> in DTSI? If yes, I will need to include such differences in the dtschema.
> 

Exynos850 DTSI additionally defines 32 interrupts for ALIVE pinctrl and
8 for CMGP. This looks suspicious - driver does not support multiple
interupts and how would they even work? What would be the source? It
seems that Exynos850 should move these interrupts to wakeup pinctrl
banks (actually - they are defined there!).

I'll send a patch for this.

> [1] https://github.com/krzk/linux/tree/n/dt-bindings-samsung-pinctrl-schema
> 
> Best regards,
> Krzysztof
> 


Best regards,
Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Sam Protsenko]

On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> Hi Chanho and Sam,
>
> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
> node with: wakeup-interrupt-controller. This is an interrupt muxing
> several external wakeup interrupts, e.g. EINT16 - EINT31.
>
> For Exynos5433 this looks like:
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
>
> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
> you should see in dmesg error log like:
>     "irq number for muxed EINTs not found"
>
> Can you check that your wakeup-interrupt-controller is properly defined
> in DTSI? If yes, I will need to include such differences in the dtschema.
>

In case of Exynos850, no muxed interrupts exist for wakeup GPIO
domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
wake-up capable, and they have dedicated interrupt for each particular
GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
file, in next nodes:
  - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
  - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)

All mentioned interrupts are wakeup interrupts, and there are no muxed
ones. So it seems like it's not possible to specify "interrupts"
property in pinctrl nodes with wakeup-interrupt-controller. The PM is
not enabled in Exynos850 platform yet, so I can't really test if
interrupts I mentioned are able to wake up the system.

After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
gpm7 nodes to Exynos850"), I can't see this error message anymore:

    samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not found

That's because exynos_eint_wkup_init() function exits in this check:

    if (!muxed_banks) {
        of_node_put(wkup_np);
        return 0;
    }

But I actually can see another error message, printed in
exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
because those nodes don't have "interrupts" property now -- you
removed those in your patch):

    samsung-pinctrl 11850000.pinctrl: irq number not available
    samsung-pinctrl 11c30000.pinctrl: irq number not available

which in turn leads to exynos_eint_gpio_init() function to exit with
-EINVAL code in the very beginning, and I'm not sure if it's ok? As I
said, those errors only appear after your patch ("arm64: dts: exynos:
drop incorrectly placed wakeup interrupts in Exynos850").

It raises next questions, which I'm trying to think over right now.
Krzysztof, please let me know if you already have answers to those:

1. Regarding "wakeup-interrupt-controller" node (and
exynos_eint_wkup_init() function): is it ok to not have "interrupts"
property in there? Would corresponding interrupts specified in child
nodes (gpa0..gpa4) function as wake-up interrupts in this case? Or
pinctrl driver should be reworked somehow?

2. Regarding missing interrupts in pinctrl nodes (and corresponding
error in exynos_eint_gpio_init() function): should it be reworked in
some way for Exynos850? Error message seems invalid in Exynos850 case,
and I'm not even sure if it's ok exynos_eint_gpio_init() fails. Should
it be modified to work that error around, in case of Exynos850?

All other pinctrl nodes have a muxed interrupt (except pinctrl_aud,
but that's probably fine).

Thanks!

> [1] https://github.com/krzk/linux/tree/n/dt-bindings-samsung-pinctrl-schema
>
> Best regards,
> Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

On 03/01/2022 21:59, Sam Protsenko wrote:
> On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> Hi Chanho and Sam,
>>
>> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
>> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
>> node with: wakeup-interrupt-controller. This is an interrupt muxing
>> several external wakeup interrupts, e.g. EINT16 - EINT31.
>>
>> For Exynos5433 this looks like:
>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
>>
>> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
>> you should see in dmesg error log like:
>>     "irq number for muxed EINTs not found"
>>
>> Can you check that your wakeup-interrupt-controller is properly defined
>> in DTSI? If yes, I will need to include such differences in the dtschema.
>>
> 
> In case of Exynos850, no muxed interrupts exist for wakeup GPIO
> domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
> wake-up capable, and they have dedicated interrupt for each particular
> GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
> file, in next nodes:
>   - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
>   - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)
> 
> All mentioned interrupts are wakeup interrupts, and there are no muxed
> ones. So it seems like it's not possible to specify "interrupts"
> property in pinctrl nodes with wakeup-interrupt-controller. The PM is
> not enabled in Exynos850 platform yet, so I can't really test if
> interrupts I mentioned are able to wake up the system.

Thanks for confirming, I'll adjust the schema.

> 
> After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
> gpm7 nodes to Exynos850"), I can't see this error message anymore:
> 
>     samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not found
> 
> That's because exynos_eint_wkup_init() function exits in this check:
> 
>     if (!muxed_banks) {
>         of_node_put(wkup_np);
>         return 0;
>     }
> 
> But I actually can see another error message, printed in
> exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
> because those nodes don't have "interrupts" property now -- you
> removed those in your patch):
> 
>     samsung-pinctrl 11850000.pinctrl: irq number not available
>     samsung-pinctrl 11c30000.pinctrl: irq number not available
> 
> which in turn leads to exynos_eint_gpio_init() function to exit with
> -EINVAL code in the very beginning, and I'm not sure if it's ok? As I
> said, those errors only appear after your patch ("arm64: dts: exynos:
> drop incorrectly placed wakeup interrupts in Exynos850").

Yeah, I replied to this next to my patch. I think my patch was not
correct and you need one - exactly one - interrupt for regular GPIO
interrupts.

> 
> It raises next questions, which I'm trying to think over right now.
> Krzysztof, please let me know if you already have answers to those:
> 
> 1. Regarding "wakeup-interrupt-controller" node (and
> exynos_eint_wkup_init() function): is it ok to not have "interrupts"
> property in there? Would corresponding interrupts specified in child
> nodes (gpa0..gpa4) function as wake-up interrupts in this case? Or
> pinctrl driver should be reworked somehow?

Yes, it should be fine. The message should be changed from error to info
or even debug, maybe depending on SoC-type (so define in struct
samsung_pin_ctrl whether exynos_eint_wkup_init expects muxed wake-ip
interrupts).

> 
> 2. Regarding missing interrupts in pinctrl nodes (and corresponding
> error in exynos_eint_gpio_init() function): should it be reworked in
> some way for Exynos850? Error message seems invalid in Exynos850 case,
> and I'm not even sure if it's ok exynos_eint_gpio_init() fails. Should
> it be modified to work that error around, in case of Exynos850?
> 
> All other pinctrl nodes have a muxed interrupt (except pinctrl_aud,
> but that's probably fine).

The error message is valid - correctly points to wrong configuration.
All pinctrl nodes should have one interrupt, if they have GPIOs capable
of interrupt as a function (usually 0xf as GPIO CON register). Why
pinctrl_aud does not have it? Maybe the function EXT_INT (0xf) is not
available for its pins?

Best regards,
Krzysztof

RE: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Chanho Park]

Hi,
Sorry for late response due to my vacation.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Sent: Friday, January 7, 2022 5:16 PM
> To: Sam Protsenko <semen.protsenko@linaro.org>
> Cc: Chanho Park <chanho61.park@samsung.com>; linux-samsung-
> soc@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Sylwester Nawrocki <s.nawrocki@samsung.com>;
> Tomasz Figa <tomasz.figa@gmail.com>
> Subject: Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt
> 
> On 03/01/2022 21:59, Sam Protsenko wrote:
> > On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> Hi Chanho and Sam,
> >>
> >> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
> >> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
> >> node with: wakeup-interrupt-controller. This is an interrupt muxing
> >> several external wakeup interrupts, e.g. EINT16 - EINT31.
> >>
> >> For Exynos5433 this looks like:
> >> https://protect2.fireeye.com/v1/url?k=5b66d98c-04fde0da-5b6752c3-0cc4
> >> 7a31ce52-358bc1856a87fe6d&q=1&e=c9523e36-5b45-4a15-9b11-877e07a0ebba&
> >> u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Farch%2
> >> Farm64%2Fboot%2Fdts%2Fexynos%2Fexynos5433.dtsi%23L857
> >>
> >> Missing muxed interrupt for Exynos850 and Autov9 might be fine,
> >> although you should see in dmesg error log like:
> >>     "irq number for muxed EINTs not found"
> >>
> >> Can you check that your wakeup-interrupt-controller is properly
> >> defined in DTSI? If yes, I will need to include such differences in the
> dtschema.
> >>
> >
> > In case of Exynos850, no muxed interrupts exist for wakeup GPIO
> > domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
> > wake-up capable, and they have dedicated interrupt for each particular
> > GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
> > file, in next nodes:
> >   - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
> >   - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)
> >
> > All mentioned interrupts are wakeup interrupts, and there are no muxed
> > ones. So it seems like it's not possible to specify "interrupts"
> > property in pinctrl nodes with wakeup-interrupt-controller. The PM is
> > not enabled in Exynos850 platform yet, so I can't really test if
> > interrupts I mentioned are able to wake up the system.
> 
> Thanks for confirming, I'll adjust the schema.
> 
> >
> > After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
> > gpm7 nodes to Exynos850"), I can't see this error message anymore:
> >
> >     samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not
> > found
> >
> > That's because exynos_eint_wkup_init() function exits in this check:
> >
> >     if (!muxed_banks) {
> >         of_node_put(wkup_np);
> >         return 0;
> >     }
> >
> > But I actually can see another error message, printed in
> > exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
> > because those nodes don't have "interrupts" property now -- you
> > removed those in your patch):
> >
> >     samsung-pinctrl 11850000.pinctrl: irq number not available
> >     samsung-pinctrl 11c30000.pinctrl: irq number not available
> >
> > which in turn leads to exynos_eint_gpio_init() function to exit with
> > -EINVAL code in the very beginning, and I'm not sure if it's ok? As I
> > said, those errors only appear after your patch ("arm64: dts: exynos:
> > drop incorrectly placed wakeup interrupts in Exynos850").
> 
> Yeah, I replied to this next to my patch. I think my patch was not correct
> and you need one - exactly one - interrupt for regular GPIO interrupts.
> 
> >
> > It raises next questions, which I'm trying to think over right now.
> > Krzysztof, please let me know if you already have answers to those:
> >
> > 1. Regarding "wakeup-interrupt-controller" node (and
> > exynos_eint_wkup_init() function): is it ok to not have "interrupts"
> > property in there? Would corresponding interrupts specified in child
> > nodes (gpa0..gpa4) function as wake-up interrupts in this case? Or
> > pinctrl driver should be reworked somehow?
> 
> Yes, it should be fine. The message should be changed from error to info
> or even debug, maybe depending on SoC-type (so define in struct
> samsung_pin_ctrl whether exynos_eint_wkup_init expects muxed wake-ip
> interrupts).
> 
> >
> > 2. Regarding missing interrupts in pinctrl nodes (and corresponding
> > error in exynos_eint_gpio_init() function): should it be reworked in
> > some way for Exynos850? Error message seems invalid in Exynos850 case,
> > and I'm not even sure if it's ok exynos_eint_gpio_init() fails. Should
> > it be modified to work that error around, in case of Exynos850?
> >
> > All other pinctrl nodes have a muxed interrupt (except pinctrl_aud,
> > but that's probably fine).
> 
> The error message is valid - correctly points to wrong configuration.
> All pinctrl nodes should have one interrupt, if they have GPIOs capable of
> interrupt as a function (usually 0xf as GPIO CON register). Why
> pinctrl_aud does not have it? Maybe the function EXT_INT (0xf) is not
> available for its pins?

Regarding pinctrl_aud, the interrupt number is not defined in interrupt source table because the line is not connected to CPU's GIC. It is directed to the GIC of dedicated audio subsystem which name is ABOX. So, we cannot handle the interrupt of pinctrl_aud even though GPBx_CON registers have EXT_INT(0xf) setting.

Best Regards,
Chanho Park

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

On 10/01/2022 08:54, Chanho Park wrote:
>>
>> The error message is valid - correctly points to wrong configuration.
>> All pinctrl nodes should have one interrupt, if they have GPIOs capable of
>> interrupt as a function (usually 0xf as GPIO CON register). Why
>> pinctrl_aud does not have it? Maybe the function EXT_INT (0xf) is not
>> available for its pins?
> 
> Regarding pinctrl_aud, the interrupt number is not defined in interrupt source table because the line is not connected to CPU's GIC. It is directed to the GIC of dedicated audio subsystem which name is ABOX. So, we cannot handle the interrupt of pinctrl_aud even though GPBx_CON registers have EXT_INT(0xf) setting.

Thanks for checking. I will need to include this in the dtschema.


Best regards,
Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Sam Protsenko]

On Fri, 7 Jan 2022 at 10:16, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 03/01/2022 21:59, Sam Protsenko wrote:
> > On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> Hi Chanho and Sam,
> >>
> >> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
> >> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
> >> node with: wakeup-interrupt-controller. This is an interrupt muxing
> >> several external wakeup interrupts, e.g. EINT16 - EINT31.
> >>
> >> For Exynos5433 this looks like:
> >> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
> >>
> >> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
> >> you should see in dmesg error log like:
> >>     "irq number for muxed EINTs not found"
> >>
> >> Can you check that your wakeup-interrupt-controller is properly defined
> >> in DTSI? If yes, I will need to include such differences in the dtschema.
> >>
> >
> > In case of Exynos850, no muxed interrupts exist for wakeup GPIO
> > domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
> > wake-up capable, and they have dedicated interrupt for each particular
> > GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
> > file, in next nodes:
> >   - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
> >   - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)
> >
> > All mentioned interrupts are wakeup interrupts, and there are no muxed
> > ones. So it seems like it's not possible to specify "interrupts"
> > property in pinctrl nodes with wakeup-interrupt-controller. The PM is
> > not enabled in Exynos850 platform yet, so I can't really test if
> > interrupts I mentioned are able to wake up the system.
>
> Thanks for confirming, I'll adjust the schema.
>
> >
> > After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
> > gpm7 nodes to Exynos850"), I can't see this error message anymore:
> >
> >     samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not found
> >
> > That's because exynos_eint_wkup_init() function exits in this check:
> >
> >     if (!muxed_banks) {
> >         of_node_put(wkup_np);
> >         return 0;
> >     }
> >
> > But I actually can see another error message, printed in
> > exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
> > because those nodes don't have "interrupts" property now -- you
> > removed those in your patch):
> >
> >     samsung-pinctrl 11850000.pinctrl: irq number not available
> >     samsung-pinctrl 11c30000.pinctrl: irq number not available
> >
> > which in turn leads to exynos_eint_gpio_init() function to exit with
> > -EINVAL code in the very beginning, and I'm not sure if it's ok? As I
> > said, those errors only appear after your patch ("arm64: dts: exynos:
> > drop incorrectly placed wakeup interrupts in Exynos850").
>
> Yeah, I replied to this next to my patch. I think my patch was not
> correct and you need one - exactly one - interrupt for regular GPIO
> interrupts.
>

I just need to remove ".eint_gpio_init" in exynos850_pin_ctrl[] for
pinctrl_alive and pinctrl_gpmc. Those already have ".eint_wkup_init",
which is enough to handle all interrupts (per-pin). GPIO_ALIVE and
GPIO_GPMC lack EINT capabilities: judging from TRM, there are no EINT
interrupts (like EINT_SVC, which is accessed in EINT ISR), and there
are no EINT interrupts wired to GIC (like INTREQ__GPIO_ALIVE or
INTREQ__GPIO_GPMC). With removed ".eint_gpio_init", I can see in
"/proc/interrupts" that corresponding interrupts are still handled
properly (because of .eint_wkup_init), and the error message is gone.
Will send the patch soon -- please add it to the beginning of your
series along with my other patch I already submitted.

> >
> > It raises next questions, which I'm trying to think over right now.
> > Krzysztof, please let me know if you already have answers to those:
> >
> > 1. Regarding "wakeup-interrupt-controller" node (and
> > exynos_eint_wkup_init() function): is it ok to not have "interrupts"
> > property in there? Would corresponding interrupts specified in child
> > nodes (gpa0..gpa4) function as wake-up interrupts in this case? Or
> > pinctrl driver should be reworked somehow?
>
> Yes, it should be fine. The message should be changed from error to info
> or even debug, maybe depending on SoC-type (so define in struct
> samsung_pin_ctrl whether exynos_eint_wkup_init expects muxed wake-ip
> interrupts).
>
> >
> > 2. Regarding missing interrupts in pinctrl nodes (and corresponding
> > error in exynos_eint_gpio_init() function): should it be reworked in
> > some way for Exynos850? Error message seems invalid in Exynos850 case,
> > and I'm not even sure if it's ok exynos_eint_gpio_init() fails. Should
> > it be modified to work that error around, in case of Exynos850?
> >
> > All other pinctrl nodes have a muxed interrupt (except pinctrl_aud,
> > but that's probably fine).
>
> The error message is valid - correctly points to wrong configuration.
> All pinctrl nodes should have one interrupt, if they have GPIOs capable
> of interrupt as a function (usually 0xf as GPIO CON register). Why
> pinctrl_aud does not have it? Maybe the function EXT_INT (0xf) is not
> available for its pins?
>
> Best regards,
> Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

On 14/01/2022 21:32, Sam Protsenko wrote:
> On Fri, 7 Jan 2022 at 10:16, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 03/01/2022 21:59, Sam Protsenko wrote:
>>> On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>
>>>> Hi Chanho and Sam,
>>>>
>>>> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
>>>> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
>>>> node with: wakeup-interrupt-controller. This is an interrupt muxing
>>>> several external wakeup interrupts, e.g. EINT16 - EINT31.
>>>>
>>>> For Exynos5433 this looks like:
>>>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
>>>>
>>>> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
>>>> you should see in dmesg error log like:
>>>>     "irq number for muxed EINTs not found"
>>>>
>>>> Can you check that your wakeup-interrupt-controller is properly defined
>>>> in DTSI? If yes, I will need to include such differences in the dtschema.
>>>>
>>>
>>> In case of Exynos850, no muxed interrupts exist for wakeup GPIO
>>> domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
>>> wake-up capable, and they have dedicated interrupt for each particular
>>> GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
>>> file, in next nodes:
>>>   - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
>>>   - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)
>>>
>>> All mentioned interrupts are wakeup interrupts, and there are no muxed
>>> ones. So it seems like it's not possible to specify "interrupts"
>>> property in pinctrl nodes with wakeup-interrupt-controller. The PM is
>>> not enabled in Exynos850 platform yet, so I can't really test if
>>> interrupts I mentioned are able to wake up the system.
>>
>> Thanks for confirming, I'll adjust the schema.
>>
>>>
>>> After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
>>> gpm7 nodes to Exynos850"), I can't see this error message anymore:
>>>
>>>     samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not found
>>>
>>> That's because exynos_eint_wkup_init() function exits in this check:
>>>
>>>     if (!muxed_banks) {
>>>         of_node_put(wkup_np);
>>>         return 0;
>>>     }
>>>
>>> But I actually can see another error message, printed in
>>> exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
>>> because those nodes don't have "interrupts" property now -- you
>>> removed those in your patch):
>>>
>>>     samsung-pinctrl 11850000.pinctrl: irq number not available
>>>     samsung-pinctrl 11c30000.pinctrl: irq number not available
>>>
>>> which in turn leads to exynos_eint_gpio_init() function to exit with
>>> -EINVAL code in the very beginning, and I'm not sure if it's ok? As I
>>> said, those errors only appear after your patch ("arm64: dts: exynos:
>>> drop incorrectly placed wakeup interrupts in Exynos850").
>>
>> Yeah, I replied to this next to my patch. I think my patch was not
>> correct and you need one - exactly one - interrupt for regular GPIO
>> interrupts.
>>
> 
> I just need to remove ".eint_gpio_init" in exynos850_pin_ctrl[] for
> pinctrl_alive and pinctrl_gpmc. Those already have ".eint_wkup_init",
> which is enough to handle all interrupts (per-pin). GPIO_ALIVE and
> GPIO_GPMC lack EINT capabilities: judging from TRM, there are no EINT
> interrupts (like EINT_SVC, which is accessed in EINT ISR), and there
> are no EINT interrupts wired to GIC (like INTREQ__GPIO_ALIVE or
> INTREQ__GPIO_GPMC). With removed ".eint_gpio_init", I can see in
> "/proc/interrupts" that corresponding interrupts are still handled
> properly (because of .eint_wkup_init), and the error message is gone.

This would mean that my dts patch removing all interrupts for alive and
cmgp was correct:
https://lore.kernel.org/linux-samsung-soc/66754058-187e-ffd5-71ba-4720101f5d98@canonical.com/T/#mf0b06ebdac554d57d8230dc546c3d57d59d7bd6b
Was it?

> Will send the patch soon -- please add it to the beginning of your
> series along with my other patch I already submitted.

Sure.





Best regards,
Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Sam Protsenko]

On Sat, 15 Jan 2022 at 17:46, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 14/01/2022 21:32, Sam Protsenko wrote:
> > On Fri, 7 Jan 2022 at 10:16, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 03/01/2022 21:59, Sam Protsenko wrote:
> >>> On Thu, 30 Dec 2021 at 21:34, Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@canonical.com> wrote:
> >>>>
> >>>> Hi Chanho and Sam,
> >>>>
> >>>> I am slowly finishing dtschema for Samsung pinctrl drivers [1] and I
> >>>> noticed that Exynos850 and Auto v9 do not define interrupt in pinctrl
> >>>> node with: wakeup-interrupt-controller. This is an interrupt muxing
> >>>> several external wakeup interrupts, e.g. EINT16 - EINT31.
> >>>>
> >>>> For Exynos5433 this looks like:
> >>>> https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/exynos/exynos5433.dtsi#L857
> >>>>
> >>>> Missing muxed interrupt for Exynos850 and Autov9 might be fine, although
> >>>> you should see in dmesg error log like:
> >>>>     "irq number for muxed EINTs not found"
> >>>>
> >>>> Can you check that your wakeup-interrupt-controller is properly defined
> >>>> in DTSI? If yes, I will need to include such differences in the dtschema.
> >>>>
> >>>
> >>> In case of Exynos850, no muxed interrupts exist for wakeup GPIO
> >>> domains. Basically, "pinctrl_alive" and "pinctrl_cmgp" domains are
> >>> wake-up capable, and they have dedicated interrupt for each particular
> >>> GPIO pin. All those interrupts are defined in exynos850-pinctrl.dtsi
> >>> file, in next nodes:
> >>>   - pinctrl_alive: gpa0..gpa4 (interrupt numbers 1..36)
> >>>   - pinctrl_cmgp: gpm0..gpm7 (interrupt numbers 39..46)
> >>>
> >>> All mentioned interrupts are wakeup interrupts, and there are no muxed
> >>> ones. So it seems like it's not possible to specify "interrupts"
> >>> property in pinctrl nodes with wakeup-interrupt-controller. The PM is
> >>> not enabled in Exynos850 platform yet, so I can't really test if
> >>> interrupts I mentioned are able to wake up the system.
> >>
> >> Thanks for confirming, I'll adjust the schema.
> >>
> >>>
> >>> After adding this patch ("arm64: dts: exynos: Add missing gpm6 and
> >>> gpm7 nodes to Exynos850"), I can't see this error message anymore:
> >>>
> >>>     samsung-pinctrl 11c30000.pinctrl: irq number for muxed EINTs not found
> >>>
> >>> That's because exynos_eint_wkup_init() function exits in this check:
> >>>
> >>>     if (!muxed_banks) {
> >>>         of_node_put(wkup_np);
> >>>         return 0;
> >>>     }
> >>>
> >>> But I actually can see another error message, printed in
> >>> exynos_eint_gpio_init() function (for wake-up capable pinctrl nodes,
> >>> because those nodes don't have "interrupts" property now -- you
> >>> removed those in your patch):
> >>>
> >>>     samsung-pinctrl 11850000.pinctrl: irq number not available
> >>>     samsung-pinctrl 11c30000.pinctrl: irq number not available
> >>>
> >>> which in turn leads to exynos_eint_gpio_init() function to exit with
> >>> -EINVAL code in the very beginning, and I'm not sure if it's ok? As I
> >>> said, those errors only appear after your patch ("arm64: dts: exynos:
> >>> drop incorrectly placed wakeup interrupts in Exynos850").
> >>
> >> Yeah, I replied to this next to my patch. I think my patch was not
> >> correct and you need one - exactly one - interrupt for regular GPIO
> >> interrupts.
> >>
> >
> > I just need to remove ".eint_gpio_init" in exynos850_pin_ctrl[] for
> > pinctrl_alive and pinctrl_gpmc. Those already have ".eint_wkup_init",
> > which is enough to handle all interrupts (per-pin). GPIO_ALIVE and
> > GPIO_GPMC lack EINT capabilities: judging from TRM, there are no EINT
> > interrupts (like EINT_SVC, which is accessed in EINT ISR), and there
> > are no EINT interrupts wired to GIC (like INTREQ__GPIO_ALIVE or
> > INTREQ__GPIO_GPMC). With removed ".eint_gpio_init", I can see in
> > "/proc/interrupts" that corresponding interrupts are still handled
> > properly (because of .eint_wkup_init), and the error message is gone.
>
> This would mean that my dts patch removing all interrupts for alive and
> cmgp was correct:
> https://lore.kernel.org/linux-samsung-soc/66754058-187e-ffd5-71ba-4720101f5d98@canonical.com/T/#mf0b06ebdac554d57d8230dc546c3d57d59d7bd6b
> Was it?
>

Yep. But patches [1,2] I sent recently should be probably applied
before yours -- they belong together. Please take those in your patch
series (when sending the next version).

Thanks!

[1] https://lkml.org/lkml/2022/1/14/861
[2] https://lkml.org/lkml/2022/1/3/680

> > Will send the patch soon -- please add it to the beginning of your
> > series along with my other patch I already submitted.
>
> Sure.
>
>
>
>
>
> Best regards,
> Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Krzysztof Kozlowski]

On 15/01/2022 21:38, Sam Protsenko wrote:
> On Sat, 15 Jan 2022 at 17:46, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> This would mean that my dts patch removing all interrupts for alive and
>> cmgp was correct:
>> https://lore.kernel.org/linux-samsung-soc/66754058-187e-ffd5-71ba-4720101f5d98@canonical.com/T/#mf0b06ebdac554d57d8230dc546c3d57d59d7bd6b
>> Was it?
>>
> 
> Yep. But patches [1,2] I sent recently should be probably applied
> before yours -- they belong together. Please take those in your patch
> series (when sending the next version).
> 
> Thanks!
> 
> [1] https://lkml.org/lkml/2022/1/14/861
> [2] https://lkml.org/lkml/2022/1/3/680
> 
>>> Will send the patch soon -- please add it to the beginning of your
>>> series along with my other patch I already submitted.

DTS and driver changes never go via same tree, so if you say there is
such dependency, then my patch should wait till another release so your
driver change will be in mainline.

What is actually the dependency here between my patch removing incorrect
interrupts and yours:
1. removing EINT for these GPIO banks from pinctrl driver,
2. adding missing GPIO banks?


Best regards,
Krzysztof

Re: Exynos850 and ExynosAuto v9 pinctrl wakeup muxed interrupt

[Sam Protsenko]

On Sun, 16 Jan 2022 at 19:15, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 15/01/2022 21:38, Sam Protsenko wrote:
> > On Sat, 15 Jan 2022 at 17:46, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> This would mean that my dts patch removing all interrupts for alive and
> >> cmgp was correct:
> >> https://lore.kernel.org/linux-samsung-soc/66754058-187e-ffd5-71ba-4720101f5d98@canonical.com/T/#mf0b06ebdac554d57d8230dc546c3d57d59d7bd6b
> >> Was it?
> >>
> >
> > Yep. But patches [1,2] I sent recently should be probably applied
> > before yours -- they belong together. Please take those in your patch
> > series (when sending the next version).
> >
> > Thanks!
> >
> > [1] https://lkml.org/lkml/2022/1/14/861
> > [2] https://lkml.org/lkml/2022/1/3/680
> >
> >>> Will send the patch soon -- please add it to the beginning of your
> >>> series along with my other patch I already submitted.
>
> DTS and driver changes never go via same tree, so if you say there is
> such dependency, then my patch should wait till another release so your
> driver change will be in mainline.
>
> What is actually the dependency here between my patch removing incorrect
> interrupts and yours:
> 1. removing EINT for these GPIO banks from pinctrl driver,
> 2. adding missing GPIO banks?
>

No dependency really. I just assumed those can go through one tree,
and it would be nice to avoid error messages to appear between
commits. But those errors are actually already there anyway (just
masked by the code you remove), and nothing is broken functionally in
your commit. So if those patches go thru different trees -- no harm in
changing the order. I'm more concerned about time needed to get those
in mainline. So please send all the patches together. Sorry for
confusion :)

Thanks!

>
> Best regards,
> Krzysztof