[PATCH 0/2] Add bcm2835aux interrupt controller

[PATCH 0/2] Add bcm2835aux interrupt controller

[Phil Elwell]

Devices in the AUX block share a common interrupt line, with a register
indicating which devices have active IRQs. Expose this as a nested
interrupt controller to avoid IRQ sharing problems (easily observed if
UART1 and SPI1/2 are enabled simultaneously).

The interrupt functionality could arguably be forked off as a separate
irqchip driver, but the clock driver has already claimed the AUX_IRQ
register so some driver and DT surgery would still be required.
Eric Anholt thought that including it here is reasonable, but I'm
prepared to split it out if this is considered too hacky.

Phil Elwell (2):
  clk: bcm2835: Add AUX interrupt controller
  ARM: dts: bcm283x: Enable AUX interrupt controller

 arch/arm/boot/dts/bcm283x.dtsi    |  12 +++-
 drivers/clk/bcm/clk-bcm2835-aux.c | 120 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 3 deletions(-)

-- 
1.9.1

Re: [PATCH 0/2] Add bcm2835aux interrupt controller

[Stefan Wahren]

Hi Phil,

> Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> hat am 7. Juni 2017 um 13:11 geschrieben:
> 
> 
> Devices in the AUX block share a common interrupt line, with a register
> indicating which devices have active IRQs. Expose this as a nested
> interrupt controller to avoid IRQ sharing problems (easily observed if
> UART1 and SPI1/2 are enabled simultaneously).
> 
> The interrupt functionality could arguably be forked off as a separate
> irqchip driver, but the clock driver has already claimed the AUX_IRQ
> register so some driver and DT surgery would still be required.
> Eric Anholt thought that including it here is reasonable, but I'm
> prepared to split it out if this is considered too hacky.

in order to give the maintainers (e.g. irqchip) a chance, they should be included into CC.

> 
> Phil Elwell (2):
>   clk: bcm2835: Add AUX interrupt controller

Either way the dt-binding must be updated as a separate patch. So Rob can review it.

Thanks for submitting this upstream.

Stefan

>   ARM: dts: bcm283x: Enable AUX interrupt controller
> 
>  arch/arm/boot/dts/bcm283x.dtsi    |  12 +++-
>  drivers/clk/bcm/clk-bcm2835-aux.c | 120 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 129 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Add bcm2835aux interrupt controller

[Phil Elwell]

On 07/06/2017 21:37, Stefan Wahren wrote:
> Hi Phil,
>
>> Phil Elwell <phil@raspberrypi.org> hat am 7. Juni 2017 um 13:11 geschrieben:
>>
>>
>> Devices in the AUX block share a common interrupt line, with a register
>> indicating which devices have active IRQs. Expose this as a nested
>> interrupt controller to avoid IRQ sharing problems (easily observed if
>> UART1 and SPI1/2 are enabled simultaneously).
>>
>> The interrupt functionality could arguably be forked off as a separate
>> irqchip driver, but the clock driver has already claimed the AUX_IRQ
>> register so some driver and DT surgery would still be required.
>> Eric Anholt thought that including it here is reasonable, but I'm
>> prepared to split it out if this is considered too hacky.
>
> in order to give the maintainers (e.g. irqchip) a chance, they should be included into CC.

Will do.

>> Phil Elwell (2):
>>   clk: bcm2835: Add AUX interrupt controller
>
> Either way the dt-binding must be updated as a separate patch. So Rob can review it.

Thanks, Stefan - I'll add that to v2.

Phil

Re: [PATCH 0/2] Add bcm2835aux interrupt controller

[Florian Fainelli]

On 06/07/2017 04:11 AM, Phil Elwell wrote:
> Devices in the AUX block share a common interrupt line, with a register
> indicating which devices have active IRQs. Expose this as a nested
> interrupt controller to avoid IRQ sharing problems (easily observed if
> UART1 and SPI1/2 are enabled simultaneously).
> 
> The interrupt functionality could arguably be forked off as a separate
> irqchip driver, but the clock driver has already claimed the AUX_IRQ
> register so some driver and DT surgery would still be required.
> Eric Anholt thought that including it here is reasonable, but I'm
> prepared to split it out if this is considered too hacky.

You probably remember your fix to the irqchip drive being flamed because
the irqchip driver was re-purposed as an ARM SMP secondary core bringup
method, maybe we can avoid doing the same mistake and having this a
separate interrupt controller be under drivers/irqchip/*?

Even if the clock driver already claims the AUX_IRQ register space, we
can still have an irqchip ioremap() the two register offsets that it
cares about (AUXIRQ, AUXENB) and just manage that 8 bytes worth of
register space. We just need to make sure that the clock driver really
does not touch those (why would it) and that there won't be any
conflicting request_mem_region() against the same register range.

Thanks!

> 
> Phil Elwell (2):
>   clk: bcm2835: Add AUX interrupt controller
>   ARM: dts: bcm283x: Enable AUX interrupt controller
> 
>  arch/arm/boot/dts/bcm283x.dtsi    |  12 +++-
>  drivers/clk/bcm/clk-bcm2835-aux.c | 120 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 129 insertions(+), 3 deletions(-)
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Add bcm2835aux interrupt controller

[Phil Elwell]

On 07/06/2017 21:58, Florian Fainelli wrote:
> On 06/07/2017 04:11 AM, Phil Elwell wrote:
>> Devices in the AUX block share a common interrupt line, with a register
>> indicating which devices have active IRQs. Expose this as a nested
>> interrupt controller to avoid IRQ sharing problems (easily observed if
>> UART1 and SPI1/2 are enabled simultaneously).
>>
>> The interrupt functionality could arguably be forked off as a separate
>> irqchip driver, but the clock driver has already claimed the AUX_IRQ
>> register so some driver and DT surgery would still be required.
>> Eric Anholt thought that including it here is reasonable, but I'm
>> prepared to split it out if this is considered too hacky.
> 
> You probably remember your fix to the irqchip drive being flamed because
> the irqchip driver was re-purposed as an ARM SMP secondary core bringup
> method, maybe we can avoid doing the same mistake and having this a
> separate interrupt controller be under drivers/irqchip/*?
> 
> Even if the clock driver already claims the AUX_IRQ register space, we
> can still have an irqchip ioremap() the two register offsets that it
> cares about (AUXIRQ, AUXENB) and just manage that 8 bytes worth of
> register space. We just need to make sure that the clock driver really
> does not touch those (why would it) and that there won't be any
> conflicting request_mem_region() against the same register range.
> 
> Thanks!

The "clock" driver (it's really just a block enabler) and IRQ functionality
each use one of two adjacent word registers. Is it not simpler, clearer and
more accurate to represent those as separate nodes in the device tree,
rather than continuing to allow the clock node to claim the neighbouring
register and expecting the IRQ driver to use other mechanisms to locate
(phandle node reference? hard-coded base address?) and use the register that
rightfully belongs to it?

Phil

Re: [PATCH 0/2] Add bcm2835aux interrupt controller

[Florian Fainelli]

On 06/08/2017 02:55 AM, Phil Elwell wrote:
> On 07/06/2017 21:58, Florian Fainelli wrote:
>> On 06/07/2017 04:11 AM, Phil Elwell wrote:
>>> Devices in the AUX block share a common interrupt line, with a register
>>> indicating which devices have active IRQs. Expose this as a nested
>>> interrupt controller to avoid IRQ sharing problems (easily observed if
>>> UART1 and SPI1/2 are enabled simultaneously).
>>>
>>> The interrupt functionality could arguably be forked off as a separate
>>> irqchip driver, but the clock driver has already claimed the AUX_IRQ
>>> register so some driver and DT surgery would still be required.
>>> Eric Anholt thought that including it here is reasonable, but I'm
>>> prepared to split it out if this is considered too hacky.
>>
>> You probably remember your fix to the irqchip drive being flamed because
>> the irqchip driver was re-purposed as an ARM SMP secondary core bringup
>> method, maybe we can avoid doing the same mistake and having this a
>> separate interrupt controller be under drivers/irqchip/*?
>>
>> Even if the clock driver already claims the AUX_IRQ register space, we
>> can still have an irqchip ioremap() the two register offsets that it
>> cares about (AUXIRQ, AUXENB) and just manage that 8 bytes worth of
>> register space. We just need to make sure that the clock driver really
>> does not touch those (why would it) and that there won't be any
>> conflicting request_mem_region() against the same register range.
>>
>> Thanks!
> 
> The "clock" driver (it's really just a block enabler) and IRQ functionality
> each use one of two adjacent word registers. Is it not simpler, clearer and
> more accurate to represent those as separate nodes in the device tree,
> rather than continuing to allow the clock node to claim the neighbouring
> register and expecting the IRQ driver to use other mechanisms to locate
> (phandle node reference? hard-coded base address?) and use the register that
> rightfully belongs to it?

There is a possible DT binary compatibility here, although in that
particular case, that would not be a problem, but I was thinking more in
the lines of:

- keeping the current clock controller node's "reg" property with the
extra 8 bytes
- add an interrupt controller node with a re "reg" property that covers
these 8 bytes for AUX{IRQ,ENB} purposes

You could have a subsequent patch that removes the 8 extra bytes from
the clock controller node's at some point, but if we are guaranteed that
a) the clock controller does not exclusively manage this region, and b)
does not touch it, we are neither breaking DT binary compatibility, nor
are we introducing problems.

I don't think phandles or cross-node references of any kind would be
necessary here.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html