Re: [PATCH V2 14/14] irqchip/gic: Add support for tegra AGIC interrupt controller

[Marc Zyngier]

On 20/04/16 12:03, Jon Hunter wrote:
> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
> 
> The Tegra AGIC (Audio GIC) is part of the Audio Processing Engine (APE) on
> Tegra210 and can route interrupts to either the GIC for the CPU subsystem
> or the Audio DSP (ADSP) within the APE. The AGIC uses CPU interface 0 to
> route interrupts to the CPU GIC and CPU interface 1 to route interrupts to
> the ADSP.
> 
> The APE is located within its own power domain on the chip and so the
> AGIC needs to manage both the power domain and its clocks. Commit
> afbbd2338176 ("irqchip/gic: Document optional Clock and Power Domain
> properties") has already added clock and power-domain properties to the
> GIC binding and so we can make use of these for the AGIC.
> 
> With the AGIC being located in a different power domain to the main CPU
> cluster this means that:
> 1. The interrupt controller cannot be registered via IRQCHIP_DECLARE()
>    because it needs to be registered as a platform device so that the
>    generic PM domain core will ensure that the power domain is available
>    before probing.
> 2. The interrupt controller cannot be suspended/restored based upon
>    changes in the CPU power state and needs to use runtime-pm instead.
> 
> The GIC platform driver has been implemented by making the following
> changes to the core GIC driver:
> 1. Remove the dependency on CONFIG_CPU_PM from PM specific variables and
>    functions so that they can be used by the platform driver even when
>    CONFIG_CPU_PM is not selected.
> 2. Move the code that maps the GIC registers and parses the device-tree
>    blob into a new function called gic_of_setup() that can be used by
>    both the platform driver as well as the existing driver.
> 3. Add and register platform driver for the GIC. The platform driver
>    uses the PM_CLK framework for managing the clocks used by the GIC
>    and so select CONFIG_PM_CLK.
> 
> Finally, a couple other notes on the implementation are:
> 1. Currently the GIC platform driver only supports non-root GICs and
>    assumes that the GIC has a parent interrupt. It is assumed that
>    root interrupt controllers need to be initialised early.
> 2. There is no specific suspend handling for GICs registered as platform
>    devices. Non-wakeup interrupts will be disabled by the kernel during
>    late suspend, however, this alone will not power down the GIC if
>    interrupts have been requested and not freed. Therefore, requestors
>    of non-wakeup interrupts will need to free them on entering suspend
>    in order to power-down the GIC.
> 
> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> ---
> 
> Please note that so far I have not added all the clock names for the
> various GIC versions (as described by the DT documentation). I thought
> we could add these as they are needed.

So while the code looks OK, I'd like to stop adding more code to this
poor GIC driver, because it is starting to look like a huge mess.

What is preventing us to move this to a separate irq-gic-pm.c file?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...