On Wed, Aug 26, 2020 at 3:42 PM Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxx> wrote: > > Hi Rob, > > Thanks for the review! > > On Tue, Aug 25, 2020 at 04:09:13PM -0600, Rob Herring wrote: > > On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote: > > > Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700 > > > and S900 SoCs and provides support for handling up to 3 external > > > interrupt lines. > > > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxx> > > > --- > > > Changes in v5: > > > - Updated controller description statements both in the commit message > > > and the binding doc > > > > > > .../actions,owl-sirq.yaml | 68 +++++++++++++++++++ > > > 1 file changed, 68 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml > > > new file mode 100644 > > > index 000000000000..cf9b7a514e4e > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml > > > @@ -0,0 +1,68 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Actions Semi Owl SoCs SIRQ interrupt controller > > > + > > > +maintainers: > > > + - Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > > + - Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxx> > > > + > > > +description: | > > > + This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700 > > > + and S900) and provides support for handling up to 3 external interrupt lines. > > > + > > > +properties: > > > + compatible: > > > + oneOf: > > > + - items: > > > + - enum: > > > + - actions,s500-sirq > > > + - actions,s700-sirq > > > + - actions,s900-sirq > > > + - const: actions,owl-sirq > > > + - const: actions,owl-sirq > > > > This should be dropped. You should always have the SoC specific > > compatible. > > Sure, I will get rid of the 'owl-sirq' compatible. > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + interrupt-controller: true > > > + > > > + '#interrupt-cells': > > > + const: 2 > > > + description: > > > + The first cell is the input IRQ number, between 0 and 2, while the second > > > + cell is the trigger type as defined in interrupt.txt in this directory. > > > + > > > + 'actions,ext-interrupts': > > > + description: | > > > + Contains the GIC SPI IRQ numbers mapped to the external interrupt > > > + lines. They shall be specified sequentially from output 0 to 2. > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + minItems: 3 > > > + maxItems: 3 > > > > Can't you use 'interrupts' here? > > This was actually my initial idea, but it might confuse the users since > this is not following the parent controller IRQ specs, i.e. the trigger > type is set internally by the SIRQ driver, it's not taken from DT. Then what's the 2nd cell for? > Please see the DTS sample bellow where both devices are on the same > level and have GIC as interrupt parent. The 'interrupts' property > in the sirq node looks incomplete now. That is why I decided to use > a custom name for it, although I'm not sure it's the most relevant one, > I am open to any other suggestion. > > i2c0: i2c@b0170000 { > [...] > interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>; > [...] > }; > > sirq: interrupt-controller@b01b0200 { > [...] > interrupt-controller; > #interrupt-cells = <2>; > interrupts = <13>, /* SIRQ0 */ > <14>, /* SIRQ1 */ > <15>; /* SIRQ2 */ This isn't valid if the GIC is the parent as you have to have 3 cells for each interrupt. Ultimately the GIC trigger type has to be something. Is it fixed or passed thru? If the latter, just use 0 (IRQ_TYPE_NONE) if the GIC trigger mode is not fixed. Having some sort of translation of the trigger is pretty common. Rob
