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@xxxxxxxxxxxxx>
> Sent: Friday, January 7, 2022 5:16 PM
> To: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> Cc: Chanho Park <chanho61.park@xxxxxxxxxxx>; linux-samsung-
> soc@xxxxxxxxxxxxxxx; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>;
> Tomasz Figa <tomasz.figa@xxxxxxxxx>
> 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@xxxxxxxxxxxxx> 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