Multi-parent IRQ domains

Hi everyone,

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.

The way I imagine this to work from a DT perspective is as follows:

	rtc: rtc@c2a0000 {
		compatible = "nvidia,tegra194-rtc", "nvidia,tegra20-rtc";
		reg = <0x0c2a0000 0x10000>;
		interrupt-parent = <&pmc>;
		interrupts = <73 &gic GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
		status = "disabled";
	};

	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 = <5>;
		interrupt-controller;
	};

	gpio-keys {
		compatible = "gpio-keys";

		power {
			label = "Power";
			gpios = <&gpio_aon TEGRA194_AON_GPIO(EE, 4)
					   GPIO_ACTIVE_LOW>;
			linux,input-type = <EV_KEY>;
			linux,code = <KEY_POWER>;
			debounce-interval = <10>;
			interrupt-parent = <&pmc>;
			interrupts = <29 &gpio_aon TEGRA194_AON_GPIO(EE, 4)
						   IRQ_TYPE_LEVEL_LOW 0>;
			wakeup-source;
		};
	};

The above is somewhat brittle because the PMC needs to be able to cover both GIC and GPIO interrupt specifiers, so it gets 5 interrupt cells (one for the wake event ID, one for a phandle to the parent controller and two or three for the parent specifier). It'd be nicer if we could actually have specifiers that exactly match the bindings for the respective parent controllers (so that the GPIO specifier didn't have to contain the dummy 0 at the end), but I don't see a way how that can be achieved, given how we have to "embed" the specifier in another. We could probably make it work in code, but DTC would still flag these as errors.

One thing I briefly looked into was using interrupt-map to implement this, which would give us a nice way of mapping arbitrary parent interrupts to wake events. However, since we need to program some of the PMC registers at ->irq_set_wake() time, using simply an interrupt-map wouldn't do the trick, since we wouldn't be implementing an IRQ chip and hence wouldn't be able to install any callbacks.

Perhaps a simple way to implement this would be to just blindly enable all wake events, but that's not really what we want. We really only want to enable them for devices that can be wakeup sources. Otherwise we may get wakeups for completely unrelated GPIO activity.

I tried implementing support for multiple parents within the PMC driver by looking up the parent domain based on the phandle from the specifier at IRQ domain ->alloc() and ->translate() time, but I run into deadlock when I do that, so before I go and open up a can of worms I wanted to get your input on what the best way forward would be.

Does anyone have any ideas on how to solve this? Perhaps there's a better solution to this that doesn't involve IRQ infrastructure that I'm not aware of?

Thanks,
Thierry

There is an interrupts-extended property which as a first cell takes a phandle to the parent interrupt controller, e.g:

interrupts-extended = <&gic 73 GIPC_SPI IRQ_TYPE_LEVEL_HIGH>,

			<&pmc 10>;

(provided that I understood your problem and example correctly). We have a similar use case on our STB chips where we have "main" interrupts wired to the ARM GIC and wake-up interrupts wired to a specific wake-up interrupt controller in an always-on domain. The two controllers have a different #interrupt-cells specifier.

The interrupt infrastructure in Linux already knows how to deal with that and whether you use of_get_irq() or platform_get_irq() you should obtain the correct virtual interrupt numbers that you expected, parented to the appropriate interrupt controller.

Hope this helps.

Florian

Yeah, I had looked at the interrupts-extended property as well, but while I think it might work, one issue with that is that we'd be modelling the regular IRQ and wakeup IRQ as two different IRQs. On Tegra, however, the wakeup interrupt is the same as the regular interrupt, except that the bits to enable the wake logic is in a different hardware block. So the PMC acts more like an additional mux for a subset of the interrupts in the system.

In addition to that, if I were to model the interrupts separately, I'd also have to modify existing drivers for all the wakeup-able devices to deal with two separate interrupts, which is rather undesirable.

Thierry

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.

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.

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

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.

Hmm?

Thanks,

tglx

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.

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.

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.

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

I'm not a DT wizerd, but the GPIO cannot be linked there I think.

    RTC ---------------------------> [ PMC domain 1] -----> GIC

    Button --> [GPIO domain] ------> [ PMC domain 2]

The RTC is connected to PMC domain 1 and that allocates the GIC irq.

The button is conntected to the GPIO which connect to the PMC domain
2. That PMC domain has no connection to anything. It ends there.

Let's look at the PMC level first:

The PMC has a fixed number of interrupts which are avail

domain 1:

That domain is used for interrupts which have a dedicated GIC interrupt line, e.g. the RTC

The interrupt domain needs at least:

alloc = pmc_domain1_alloc

The interrupt chip has:

irq_mask = irq_chip_mask_parent
irq_unmask = irq_chip_unmask_parent
irq_eoi = irq_chip_eoi_parent
irq_set_affinity = irq_chip_set_affinity_parent
irq_set_type = irq_chip_set_type_parent
irq_set_wake = pmc_set_wake

domain 2:

That domain is used for interrupts which are not related to the GIC directly, e.g. GPIO

The interrupt domain needs at least:

alloc = pmc_domain2_alloc

The interrupt chip has:

irq_set_wake = pmc_set_wake

Now the GPIO domain builds on top of PMC domain2

i.e. the parent of the GPIO domain is PMC domain2, which means that it's part of a hierarchy and therefore needs an alloc function in the domain ops.

The GPIO irq chip gains one extra callback:

irq_set_wake = irq_chip_set_wake_parent,

So, I don't know how GPIOs are mapped into the PMC when they are a wakeup source. It might be all of them have the ability so there is some 1:1 relation ship or if the whole GPIO -> PMC connection can be built at run time, but that's just an implementation detail.

Thanks,

tglx

Interesting, I was assuming the following:

    Button --> [PMC domain 2] --> [GPIO domain]

based on the hardware documentation that maps the wake events to specific signals on the chip.

This looks interesting. I'm going to give that a try, though it may take me a little to rework everything.

There's a fixed mapping for which signals go to which wake event. Some of those events are mapped to GPIOs, others to different signals, such as the RTC alarm. I think I should be able to build a GPIO -> PMC map at runtime using a static table.

Thanks,
Thierry

That might be, but there is no reason why we can't interpret that creatively :)

Ok, so for the RTC and the like (which are directly GIC backed) it's easy as you just say: RTC interrupt = PMC irq #N.

And for GPIO you can just have the GPIO domains all parented by the PMC domain 2 and then have the allocation function of the PMC domain do the lookup of the GPIOs which actually have a PMC connection.

If it's there then you set the hwirq in the domains irq_data to the real PMC hwirq number and for those which are not connected you set it to some invalid value. In the set_type() callback you can then error out when it's called for a GPIO which is not connected to the PMC by checking the hwirq value.

That way the GPIO code does not have any requirement to know about the PMC connections and only the PMC domain 2 needs to know. So if in the next chip the GPIO/PMC association changes, you don't have to worry about it at the GPIO (driver) level, you just hand a different GPIO/PMC map to the PMC driver via DT.

Thanks,

tglx

By recent, is it same to assume PSCI based system ? And the GIC is power managed by PSCI ?

--
Regards,
Sudeep