On Fri, Sep 14, 2018 at 12:31:18PM +0200, Thomas Gleixner wrote:
> On Thu, 13 Sep 2018, Thierry Reding wrote:
> > I've been trying to implement a feature on recent Tegra chips that's
> > called "wake events". These wake events are implemented as part of the
> > power management controller (PMC) and they control which events can be
> > used to wake the system from suspend. Events include things like an
> > alarm interrupt from an RTC, or the GPIO interrupt hooked up to a
> > power button for example.
> > 
> > We already have a driver for a couple of other things that the PMC does,
> > so I thought it'd be natural to implement this functionality as an IRQ
> > chip in the PMC driver. The way that this would work is that for all the
> > devices that are wakeup sources, we'd end up specifying the PMC as the
> > interrupt parent and the PMC would then itself have the main GIC as its
> > parent. That way, the hierarchical IRQ domain infrastructure would take
> > care of pretty much everything.
> > 
> > Unfortunately I've run into some issues here. One problem that I'm
> > facing is that PMC could have more than one parent. For example, the RTC
> > alarm interrupt goes straight to the GIC, but the power button GPIO goes
> > through the GPIO controller first and then to the GIC. This doesn't seem
> > to be something that's currently possible.
> 
> So what you are saying is that the RTC is directly wired up to the GIC and
> the GPIO interrupts are demultiplxed by a single GIC interrupt first, but
> at the same time have the requirement to talk to the PMC.

Yes, that's correct. Technically the GPIO controllers can have multiple
parent interrupts at the GIC, but I don't think that's relevant to the
discussion. Just mentioning it for completeness.

> Lets look at the current implementation without taking PMC into account.
> 
> - RTC gets its interrupt directly from the GIC domain
> 
> - GPIO gets its interrupt from the GPIO domain. The GPIO domain does not
>   have a parent domain. It is stand alone because it sets up a demultiplex
>   interrupt from the GIC domain.
> 
>   So there is absolutely no irqdomain relationship between a single GPIO
>   and the GIC. The indirection through the demultiplex interrupt does not
>   create one, really.
> 
> Now, you need the PMC for both, the GPIOs and the RTC. What you can do here
> is to provide two irq domains in PMC. One which has GIC as its parent and
> one which has no parent. Surely they need to share some resources, but
> that should be a solvable problem.

I think I have this working to some degree, finally. GPIO is still
proving difficult, but RTC seems to be working fine. I've currently
solved this by making the PMC an interrupt controller and then have
an interrupt-map property in its device tree node that lists those
wake events that we're interested in. It looks something like this:

	pmc: pmc@c360000 {
		compatible = "nvidia,tegra194-pmc";
		reg = <0x0c360000 0x10000>,
		      <0x0c370000 0x10000>,
		      <0x0c380000 0x10000>,
		      <0x0c390000 0x10000>,
		      <0x0c3a0000 0x10000>;
		reg-names = "pmc", "wake", "aotag", "scratch", "misc";

		#interrupt-cells = <1>;
		interrupt-controller;

		interrupt-map = /*<29 &gpio_aon TEGRA194_AON_GPIO(EE, 4)
					      IRQ_TYPE_LEVEL_HIGH>,*/
				<73 &gic GIC_SPI 10
					 IRQ_TYPE_LEVEL_HIGH>;
	};

Note that I've commented out the GPIO wake event (this is for the power
button) because that currently crashes in the GPIO driver, probably
because I misunderstood how to properly implement this.

> Now you connect RTC to the GIC parented one and let the PMC irq chip just
> hand through, except for the set_wake() functionality.

I've solved this in the following way:

	for (i = 0; i < pmc->num_wake_parents; i++) {
		struct tegra_wake_parent *parent;
		struct irq_chip *chip;

		parent = &pmc->wake_parents[i];

		chip = &parent->chip;
		chip->name = dev_name(pmc->dev);
		chip->irq_eoi = irq_chip_eoi_parent;
		chip->irq_mask = irq_chip_mask_parent;
		chip->irq_unmask = irq_chip_unmask_parent;
		chip->irq_retrigger = irq_chip_retrigger_hierarchy;
		chip->irq_set_wake = tegra_pmc_irq_set_wake;
		chip->irq_set_type = irq_chip_set_type_parent;
		chip->irq_set_affinity = irq_chip_set_affinity_parent;

		parent->domain = irq_domain_add_hierarchy(parent->domain, 0,
							  parent->num_events,
							  pmc->dev->of_node,
							  &tegra_pmc_irq_domain_ops,
							  pmc);
		if (!parent->domain) {
			dev_err(pmc->dev, "failed to allocate domain\n");
			return -ENOMEM;
		}
	}

The pmc->num_wake_parents is the number of unique wake parents that we
have in the interrupt-map property.

I think this is what you were suggesting, we override ->irq_set_wake()
and pass through everything else.

> The GPIO domain is connected to the parentless PMC domain and the irq chip
> merily provides the set_wake() stuff and everything else is a noop.
> 
> So GPIO ends up as a hierarchy which is still not connected to the GIC
> directly.

When I use the above code on the PMC/GPIO domain, then I see crashes
because the GPIO controller doesn't implement things like the ->alloc()
callback for its IRQ domain. But perhaps this is what I misunderstand.
Are you saying that for the case of GPIO I can just *not* pass through
all other operations and just let them be NULL? So that the only
callback will be ->irq_set_wake()? What I don't quite understand is how
the IRQ code would know how to properly set up the GPIO interrupt in
that case. For example, if I have this:

	gpio_aon: gpio@c2f0000 {
		...
	};

	pmc: pmc@c360000 {
		...
	};

	gpio-keys {
		...

		power {
			...
			gpios = <&gpio_aon TEGRA194_AON_GPIO(EE, 4)
					   GPIO_ACTIVE_LOW>;
			interrupt-parent = <&pmc>;
			interrupts = <29>;
			...
		};
	};

In the above, gpio-keys will only set up a handler for the PMC interrupt
and will therefore not process any interrupts from the GPIO line. If we
don't set up a parent/child relationship between PMC and GPIO, how does
the IRQ code know how to translate the PMC/29 interrupt to the one from
the GPIO controller? The PMC controller itself doesn't generate any
interrupts for wake events, so there's no interrupt handler from which
we could do any demultiplexing.

Thierry