Re: [PATCH 1/4] dt-bindings: arm: add cznic,turris-omnia-mcu binding

["Marek Behún" <kabel@kernel.org>]

On Thu, 24 Aug 2023 10:23:27 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> You replied only to me. Instead please reply-to-all to keep discussions
> public.

Sorry about that, I've redirected the messages to the other people.

> >>> +++ b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml    
> >>
> >> ARM directory is only for top-level bindings, so this should go to soc.  
> > 
> > Hmm. The board uses a marvell SoC, but the board is from CZ.NIC (who
> > does not create its own SoCs). So should this go into soc/marvell or
> > soc/cznic?  
> 
> Indeed and it is a dedicated MCU (some Cortex-M or similar, right?), so
> maybe arm is suitable at the end.

There are three Cortex-M options (M0, M0+ or M3, from STM, NXP or
GigaDevice), depending on the board revision.

The bindings/arm already contain for example amazon,al.yaml, which is
platform binding (not SoC).

So I guess I will stick with bindings/arm, unless someone suggests
something else.

> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +
> >>> +    ic: interrupt-controller {
> >>> +        interrupt-controller;
> >>> +        #interrupt-cells = <2>;
> >>> +    };    
> >>
> >> Drop this node, not relevant.  
> > 
> > Will the binding example compile without the ic node if the
> > system-controller below uses it?  
> 
> Yes.

OK

> >>> +            interrupt-parent = <&ic>;
> >>> +            interrupts = <11 IRQ_TYPE_NONE>;    
> >>
> >> Are you sure interrupt is type none?  
> > 
> > The interrupt type is either LEVEL_LOW or EDGE_FALLING, depending on
> > the version of the MCU firmware, so this has to be selected by the
> > driver.
> > 
> > I tried setting LEVEL_LOW, since that is the one that is used by the
> > newest MCU firmware. But then if the firmware is old and I want to
> > select EDGE_FALLING in the driver when requesting the IRQ, it fails
> > with message
> >   type mismatch, failed to map hwirq-%lu for %s!
> > from
> >   kernel/irq/irqdomain.c function irq_create_fwspec_mapping
> > 
> > I guess I can use irqd_set_trigger_type() before requesting the IRQ to
> > avoid this error.
> > 
> > Should I use use LEVEL_LOW in the binding example and device-tree?  
> 
> I guess in such case this NONE is okay

Thx.