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

[Brian Masney <masneyb@xxxxxxxxxxxxx>]

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?

Hi Linus,

Thanks for the detailed write up about the issue. I can take this task
on. I actually encountered this issue with the asahi-kasei,ak8963 driver
on the Nexus 5 but I never dug into it further.

It looks like pinctrl-msm8x74.c may have the same issue as well and that
may be contributing to some of the issues that I'm encountering with the
display on the Nexus 5.

Brian