Re: RFC: qcom spmi/ssbi-gpio hierarchical irqchip problems

[Brian Masney <masneyb@xxxxxxxxxxxxx>]

Hi all,

On Mon, Nov 26, 2018 at 09:41:38AM +0100, Linus Walleij wrote:
> Hi folks,
> 
> I see some people are working on the Qualcomm PMIC GPIOs,
> so wanted to bring to your attention one of the long overdue
> TODO things with the SPMI/SSBI GPIO.
> 
> I discussed it with Bjorn in the past and ran into it when hashing
> out some GPIO lines on the APQ8060 DragonBoard but it has
> since been forgotten.
> 
> The problem is that these drivers should have hierarchical
> irqchips.
> 
> For example (arch/arm/boot/dts/qcom-msm8660.dtsi):
> 
>                 qcom,ssbi@500000 {
>                         compatible = "qcom,ssbi";
>                         reg = <0x500000 0x1000>;
>                         qcom,controller-type = "pmic-arbiter";
> 
>                         pm8058: pmic@0 {
>                                 compatible = "qcom,pm8058";
>                                 interrupt-parent = <&tlmm>;
>                                 interrupts = <88 8>;
>                                 #interrupt-cells = <2>;
>                                 interrupt-controller;
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
> 
>                                 pm8058_gpio: gpio@150 {
>                                         compatible = "qcom,pm8058-gpio",
>                                                      "qcom,ssbi-gpio";
>                                         reg = <0x150>;
>                                         interrupt-parent = <&pm8058>;
>                                         interrupts = <192 IRQ_TYPE_NONE>,
>                                                      <193 IRQ_TYPE_NONE>,
>                                                      <194 IRQ_TYPE_NONE>,
>                                                      <195 IRQ_TYPE_NONE>,
>                                                      <196 IRQ_TYPE_NONE>,
> (...)
> 
> As you can see qcom,pm8058-gpio clearly specifies that
> it is consuming IRQs 192, 193.... from its parent PMIC
> interrupt controller.
> 
> (This is not a problem just with old boards like mine,
> but all later platforms using PMIC GPIO IRQs as well.)
> 
> This means the GPIO driver (ssbi, spmi) should be
> hierarchical and pick one of those IRQs.
> 
> However device tree consumers does not specify that
> it picks IRQs from the GPIO chip (in this case the
> pm8058_gpio node) but instead pick them directly from
> pm8058.
> 
> For example:
> 
>                 /* GPIO controlled ethernet power regulator */
>                 dragon_veth: xc622a331mrg {
>                         compatible = "regulator-fixed";
>                         regulator-name = "XC6222A331MR-G";
>                         regulator-min-microvolt = <3300000>;
>                         regulator-max-microvolt = <3300000>;
>                         vin-supply = <&vph>;
>                         gpio = <&pm8058_gpio 40 GPIO_ACTIVE_HIGH>;
>                         enable-active-high;
> 
> So this consumer is picking GPIO 40 on the GPIO and
> proceeds to pick an IRQ from the underlying PMIC.
> 
> We are lucky that the
> ethernet driver calls gpio[d]_to_irq() on the GPIO line, resulting
> in this function getting called in the driver:
> 
> static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> {
>         struct pm8xxx_gpio *pctrl = gpiochip_get_data(chip);
>         struct pm8xxx_pin_data *pin = pctrl->desc.pins[offset].drv_data;
> 
>         return pin->irq;
> }
> 
> Which does the translation to the parent IRQ and it "works" for
> this case where you specify GPIO offset but not IRQ.
> 
> The problem is that all irqchips in a device tree should
> be usable straight off, without using any gpio[d]_to_irq() calls:
> they are orthogonal abstractions and should not depend
> on each other like this. It doesn't work at all with these
> drivers that don't have proper (hierarchical) irqchips.
> 
> Device tree nodes that use the irqchip directly does not work,
> so for example this consumer is out of luck:
> 
> ak8975@c {
>          compatible = "asahi-kasei,ak8975";
>          reg = <0x0c>;
>          /* FIXME: GPIO33 has interrupt 224 on the PM8058 */
>          interrupt-parent = <&pm8058>;
>          interrupts = <224 IRQ_TYPE_EDGE_RISING>;
>          (...)
> };
> 
> Since we are unable to pick an interrupt directly from the GPIO
> block, we need to refer to the parent to talk directly to the
> irqchip in the PMIC.
> 
> If we had implemented a proper hierarchical irqchip in
> the SSBI/SSPI-gpio drivers we could pick the IRQ just fine
> and things would get orderly in the device tree and
> /proc/interrupts.
> 
> This is not the only driver with this problem, but definately
> one of the most used drivers with this problem.
> 
> It's on my TODO as well and I bet on Bjorn's too, it's just
> that maybe we don't use it so much, so if any of you folks
> are actively using PMIC GPIO IRQs and can easily test
> these, could you please look into this?

I started to work this evening on converting pinctrl-spmi-gpio.c to
hierarchical irqs using pinctrl-stm32.c as a reference. I'll be
able to test that this is working properly on the Nexus 5 by using
gpio-keys with the volume up/down buttons on the phone. I'll need to
remove the call to gpiod_to_irq() from gpio_keys.c in my local tree..

stm32_pctl_probe() only sets up the hierarchical irqs if
interrupt-parent is defined on that node in device tree.
qcom-pm8941.dtsi currently doesn't have this property in the current
mainline and it is also not present in the downstream Android sources
for the Nexus 5. Does anyone know what I should put for the parent
interrupt controller? I don't have access to the documentation for
this particular hardware. For your convenience, here is the relevant
device tree node from qcom-pm8941.dtsi:

pm8941_gpios: gpios@c000 {
        compatible = "qcom,pm8941-gpio", "qcom,spmi-gpio";
        reg = <0xc000>;
        gpio-controller;
        gpio-ranges = <&pm8941_gpios 0 0 36>;
        #gpio-cells = <2>;
        interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
                     <0 0xc1 0 IRQ_TYPE_NONE>,
                     <0 0xc2 0 IRQ_TYPE_NONE>,
                     <0 0xc3 0 IRQ_TYPE_NONE>,
                     <0 0xc4 0 IRQ_TYPE_NONE>,
                     <0 0xc5 0 IRQ_TYPE_NONE>,
                     <0 0xc6 0 IRQ_TYPE_NONE>,
                     <0 0xc7 0 IRQ_TYPE_NONE>,
                     <0 0xc8 0 IRQ_TYPE_NONE>,
                     <0 0xc9 0 IRQ_TYPE_NONE>,
                     <0 0xca 0 IRQ_TYPE_NONE>,
                     <0 0xcb 0 IRQ_TYPE_NONE>,
                     <0 0xcc 0 IRQ_TYPE_NONE>,
                     <0 0xcd 0 IRQ_TYPE_NONE>,
                     <0 0xce 0 IRQ_TYPE_NONE>,
                     <0 0xcf 0 IRQ_TYPE_NONE>,
                     <0 0xd0 0 IRQ_TYPE_NONE>,
                     <0 0xd1 0 IRQ_TYPE_NONE>,
                     <0 0xd2 0 IRQ_TYPE_NONE>,
                     <0 0xd3 0 IRQ_TYPE_NONE>,
                     <0 0xd4 0 IRQ_TYPE_NONE>,
                     <0 0xd5 0 IRQ_TYPE_NONE>,
                     <0 0xd6 0 IRQ_TYPE_NONE>,
                     <0 0xd7 0 IRQ_TYPE_NONE>,
                     <0 0xd8 0 IRQ_TYPE_NONE>,
                     <0 0xd9 0 IRQ_TYPE_NONE>,
                     <0 0xda 0 IRQ_TYPE_NONE>,
                     <0 0xdb 0 IRQ_TYPE_NONE>,
                     <0 0xdc 0 IRQ_TYPE_NONE>,
                     <0 0xdd 0 IRQ_TYPE_NONE>,
                     <0 0xde 0 IRQ_TYPE_NONE>,
                     <0 0xdf 0 IRQ_TYPE_NONE>,
                     <0 0xe0 0 IRQ_TYPE_NONE>,
                     <0 0xe1 0 IRQ_TYPE_NONE>,
                     <0 0xe2 0 IRQ_TYPE_NONE>,
                     <0 0xe3 0 IRQ_TYPE_NONE>;

        boost_bypass_n_pin: boost-bypass {
                pins = "gpio21";
                function = "normal";
        };
};

Thanks,

Brian