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

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

[Marek Behún]

Hello Krzysztof,

thanks for the review.

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

> On 23/08/2023 18:10, Marek Behún wrote:
> > Add binding for cznic,turris-omnia-mcu, the device-tree node
> > representing the system-controller features provided by the MCU on the
> > Turris Omnia router.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>  
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.

I shall do that.

> > ---
> >  .../bindings/arm/cznic,turris-omnia-mcu.yaml  | 72 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 73 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
> > new file mode 100644
> > index 000000000000..055485847e71
> > --- /dev/null
> > +++ 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?

> > @@ -0,0 +1,72 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/cznic,turris-omnia-mcu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: CZ.NIC's Turris Omnia MCU
> > +
> > +maintainers:
> > +  - Marek Behún <kabel@kernel.org>
> > +
> > +description:
> > +  The MCU on Turris Omnia acts as a system controller providing additional
> > +  GPIOs, interrupts, watchdog, system power off and wakeup configuration.
> > +
> > +properties:
> > +  compatible:
> > +    const: cznic,turris-omnia-mcu
> > +
> > +  reg:
> > +    description: MCU I2C slave address
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - gpio-controller
> > +
> > +additionalProperties: false
> > +
> > +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?

> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        system-controller@2a {
> > +            compatible = "cznic,turris-omnia-mcu";
> > +            reg = <0x2a>;
> > +
> > +            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?

Thank you for the review.

Marek

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

[Krzysztof Kozlowski]

On 24/08/2023 10:03, Marek Behún wrote:
> Hello Krzysztof,
> 
> thanks for the review.
> 
> On Thu, 24 Aug 2023 09:37:23 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On 23/08/2023 18:10, Marek Behún wrote:
>>> Add binding for cznic,turris-omnia-mcu, the device-tree node
>>> representing the system-controller features provided by the MCU on the
>>> Turris Omnia router.
>>>
>>> Signed-off-by: Marek Behún <kabel@kernel.org>  
>>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC (and consider --no-git-fallback argument). It might
>> happen, that command when run on an older kernel, gives you outdated
>> entries. Therefore please be sure you base your patches on recent Linux
>> kernel.
> 
> I shall do that.

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

> 
>>> ---
>>>  .../bindings/arm/cznic,turris-omnia-mcu.yaml  | 72 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 73 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml b/Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
>>> new file mode 100644
>>> index 000000000000..055485847e71
>>> --- /dev/null
>>> +++ 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.

> 
>>> @@ -0,0 +1,72 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/cznic,turris-omnia-mcu.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: CZ.NIC's Turris Omnia MCU
>>> +
>>> +maintainers:
>>> +  - Marek Behún <kabel@kernel.org>
>>> +
>>> +description:
>>> +  The MCU on Turris Omnia acts as a system controller providing additional
>>> +  GPIOs, interrupts, watchdog, system power off and wakeup configuration.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: cznic,turris-omnia-mcu
>>> +
>>> +  reg:
>>> +    description: MCU I2C slave address
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  interrupt-controller: true
>>> +
>>> +  '#interrupt-cells':
>>> +    const: 2
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  '#gpio-cells':
>>> +    const: 2
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - interrupt-controller
>>> +  - gpio-controller
>>> +
>>> +additionalProperties: false
>>> +
>>> +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.

> 
>>> +
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        system-controller@2a {
>>> +            compatible = "cznic,turris-omnia-mcu";
>>> +            reg = <0x2a>;
>>> +
>>> +            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

Best regards,
Krzysztof