[v2,04/14] dt-bindings: misc: Add device specific bindings for RaspberryPi RP1

The RP1 is a MFD that exposes its peripherals through PCI BARs. This
schema is intended as minimal support for the clock generator and
gpio controller peripherals which are accessible through BAR1.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 .../devicetree/bindings/misc/pci1de4,1.yaml   | 110 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml

On Mon, Oct 07, 2024 at 02:39:47PM +0200, Andrea della Porta wrote:
> The RP1 is a MFD that exposes its peripherals through PCI BARs. This
> schema is intended as minimal support for the clock generator and
> gpio controller peripherals which are accessible through BAR1.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>  .../devicetree/bindings/misc/pci1de4,1.yaml   | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/pci1de4,1.yaml b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> new file mode 100644
> index 000000000000..3f099b16e672
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/pci1de4,1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RaspberryPi RP1 MFD PCI device
> +
> +maintainers:
> +  - Andrea della Porta <andrea.porta@suse.com>
> +
> +description:
> +  The RaspberryPi RP1 is a PCI multi function device containing
> +  peripherals ranging from Ethernet to USB controller, I2C, SPI
> +  and others.
> +  The peripherals are accessed by addressing the PCI BAR1 region.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-ep-bus.yaml
> +
> +properties:
> +  compatible:
> +    additionalItems: true

Why is this true? This is final schema, not a "common" part.

> +    maxItems: 3
> +    items:
> +      - const: pci1de4,1
> +
> +patternProperties:
> +  "^pci-ep-bus@[0-2]$":
> +    $ref: '#/$defs/bar-bus'
> +    description:
> +      The bus on which the peripherals are attached, which is addressable
> +      through the BAR.
> +
> +unevaluatedProperties: false
> +
> +$defs:
> +  bar-bus:
> +    $ref: /schemas/pci/pci-ep-bus.yaml#/$defs/pci-ep-bus
> +    unevaluatedProperties: false
> +
> +    properties:
> +      "#interrupt-cells":
> +        const: 2
> +        description:
> +          Specifies respectively the interrupt number and flags as defined
> +          in include/dt-bindings/interrupt-controller/irq.h.
> +
> +      interrupt-controller: true
> +
> +      interrupt-parent:
> +        description:
> +          Must be the phandle of this 'pci-ep-bus' node. It will trigger
> +          PCI interrupts on behalf of peripheral generated interrupts.
> +
> +    patternProperties:
> +      "^clocks(@[0-9a-f]+)?$":

Why @ is optional? Your device is fixed, not flexible.

Best regards,
Krzysztof

On Mon, Oct 07, 2024 at 02:39:47PM +0200, Andrea della Porta wrote:
> The RP1 is a MFD that exposes its peripherals through PCI BARs. This
> schema is intended as minimal support for the clock generator and
> gpio controller peripherals which are accessible through BAR1.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>  .../devicetree/bindings/misc/pci1de4,1.yaml   | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/pci1de4,1.yaml b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> new file mode 100644
> index 000000000000..3f099b16e672
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/pci1de4,1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RaspberryPi RP1 MFD PCI device
> +
> +maintainers:
> +  - Andrea della Porta <andrea.porta@suse.com>
> +
> +description:
> +  The RaspberryPi RP1 is a PCI multi function device containing
> +  peripherals ranging from Ethernet to USB controller, I2C, SPI
> +  and others.
> +  The peripherals are accessed by addressing the PCI BAR1 region.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-ep-bus.yaml
> +
> +properties:
> +  compatible:
> +    additionalItems: true
> +    maxItems: 3
> +    items:
> +      - const: pci1de4,1
> +
> +patternProperties:
> +  "^pci-ep-bus@[0-2]$":
> +    $ref: '#/$defs/bar-bus'
> +    description:
> +      The bus on which the peripherals are attached, which is addressable
> +      through the BAR.

No need for this because pci-ep-bus.yaml already has a schema for the 
child nodes.

> +
> +unevaluatedProperties: false
> +
> +$defs:
> +  bar-bus:
> +    $ref: /schemas/pci/pci-ep-bus.yaml#/$defs/pci-ep-bus
> +    unevaluatedProperties: false
> +
> +    properties:
> +      "#interrupt-cells":
> +        const: 2
> +        description:
> +          Specifies respectively the interrupt number and flags as defined
> +          in include/dt-bindings/interrupt-controller/irq.h.
> +
> +      interrupt-controller: true
> +
> +      interrupt-parent:
> +        description:
> +          Must be the phandle of this 'pci-ep-bus' node. It will trigger
> +          PCI interrupts on behalf of peripheral generated interrupts.


Do you have an interrupt controller per bus? These should be in the 
parent node I think.


> +
> +    patternProperties:
> +      "^clocks(@[0-9a-f]+)?$":
> +        type: object
> +        $ref: /schemas/clock/raspberrypi,rp1-clocks.yaml
> +
> +      "^ethernet(@[0-9a-f]+)?$":
> +        type: object
> +        $ref: /schemas/net/cdns,macb.yaml
> +
> +      "^pinctrl(@[0-9a-f]+)?$":
> +        type: object
> +        $ref: /schemas/pinctrl/raspberrypi,rp1-gpio.yaml

IMO, these child nodes can be omitted. We generally don't define all the 
child nodes in an SoC.

If you do want to define them, then just do:

additionalProperties: true
properties:
  compatible:
    contains: the-child-compatible

> +
> +    required:
> +      - interrupt-parent
> +      - interrupt-controller
> +
> +examples:
> +  - |
> +    pci {
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +
> +        rp1@0,0 {
> +            compatible = "pci1de4,1";
> +            ranges = <0x01 0x00 0x00000000
> +                      0x82010000 0x00 0x00
> +                      0x00 0x400000>;
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +
> +            pci_ep_bus: pci-ep-bus@1 {
> +                compatible = "simple-bus";
> +                ranges = <0xc0 0x40000000
> +                          0x01 0x00 0x00000000
> +                          0x00 0x00400000>;
> +                dma-ranges = <0x10 0x00000000
> +                              0x43000000 0x10 0x00000000
> +                              0x10 0x00000000>;
> +                #address-cells = <2>;
> +                #size-cells = <2>;
> +                interrupt-controller;
> +                interrupt-parent = <&pci_ep_bus>;
> +                #interrupt-cells = <2>;
> +
> +                rp1_clocks: clocks@c040018000 {
> +                    compatible = "raspberrypi,rp1-clocks";
> +                    reg = <0xc0 0x40018000 0x0 0x10038>;
> +                    #clock-cells = <1>;
> +                    clocks = <&clk_rp1_xosc>;
> +                    clock-names =  "rp1-xosc";
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ccf123b805c8..2aea5a6166bd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19384,6 +19384,7 @@ RASPBERRY PI RP1 PCI DRIVER
>  M:	Andrea della Porta <andrea.porta@suse.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> +F:	Documentation/devicetree/bindings/misc/pci1de4,1.yaml
>  F:	Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
>  F:	include/dt-bindings/clock/rp1.h
> -- 
> 2.35.3
>

Hi Rob,

On 21:52 Wed 09 Oct     , Rob Herring wrote:
> On Mon, Oct 07, 2024 at 02:39:47PM +0200, Andrea della Porta wrote:
> > The RP1 is a MFD that exposes its peripherals through PCI BARs. This
> > schema is intended as minimal support for the clock generator and
> > gpio controller peripherals which are accessible through BAR1.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >  .../devicetree/bindings/misc/pci1de4,1.yaml   | 110 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 111 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/pci1de4,1.yaml b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > new file mode 100644
> > index 000000000000..3f099b16e672
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/pci1de4,1.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RaspberryPi RP1 MFD PCI device
> > +
> > +maintainers:
> > +  - Andrea della Porta <andrea.porta@suse.com>
> > +
> > +description:
> > +  The RaspberryPi RP1 is a PCI multi function device containing
> > +  peripherals ranging from Ethernet to USB controller, I2C, SPI
> > +  and others.
> > +  The peripherals are accessed by addressing the PCI BAR1 region.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-ep-bus.yaml
> > +
> > +properties:
> > +  compatible:
> > +    additionalItems: true
> > +    maxItems: 3
> > +    items:
> > +      - const: pci1de4,1
> > +
> > +patternProperties:
> > +  "^pci-ep-bus@[0-2]$":
> > +    $ref: '#/$defs/bar-bus'
> > +    description:
> > +      The bus on which the peripherals are attached, which is addressable
> > +      through the BAR.
> 
> No need for this because pci-ep-bus.yaml already has a schema for the 
> child nodes.

Hmmm... my intention here was to constrain the BARs from 0 to 2, since there are
only 3 BARs on RP1 (of which only 1 is currently interesting for peripherals).
Also, that bus should have the peripherals on it, hence I've added the clock,
ethernet and pinctrl nodes. Do you think it's not reasonable to define
all the peripherals on it, or if it's reasonable, is there any other way to
accomplish this in a more elegant way than what I proposed in this patch? See also
below.

> 
> > +
> > +unevaluatedProperties: false
> > +
> > +$defs:
> > +  bar-bus:
> > +    $ref: /schemas/pci/pci-ep-bus.yaml#/$defs/pci-ep-bus
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      "#interrupt-cells":
> > +        const: 2
> > +        description:
> > +          Specifies respectively the interrupt number and flags as defined
> > +          in include/dt-bindings/interrupt-controller/irq.h.
> > +
> > +      interrupt-controller: true
> > +
> > +      interrupt-parent:
> > +        description:
> > +          Must be the phandle of this 'pci-ep-bus' node. It will trigger
> > +          PCI interrupts on behalf of peripheral generated interrupts.
> 
> 
> Do you have an interrupt controller per bus? These should be in the 
> parent node I think.

Ack.

> 
> 
> > +
> > +    patternProperties:
> > +      "^clocks(@[0-9a-f]+)?$":
> > +        type: object
> > +        $ref: /schemas/clock/raspberrypi,rp1-clocks.yaml
> > +
> > +      "^ethernet(@[0-9a-f]+)?$":
> > +        type: object
> > +        $ref: /schemas/net/cdns,macb.yaml
> > +
> > +      "^pinctrl(@[0-9a-f]+)?$":
> > +        type: object
> > +        $ref: /schemas/pinctrl/raspberrypi,rp1-gpio.yaml
> 
> IMO, these child nodes can be omitted. We generally don't define all the 
> child nodes in an SoC.
> 
> If you do want to define them, then just do:
> 
> additionalProperties: true
> properties:
>   compatible:
>     contains: the-child-compatible
>

Right, but since you proposed above to get rid of the pci-ep-bus redeclaration
(being it alredy defined in pci-ep-bus.yaml) I'm not sure where to place this.
Should I just get rid af it all as you suggest?


Many thanks,
Andrea
 
> > +
> > +    required:
> > +      - interrupt-parent
> > +      - interrupt-controller
> > +
> > +examples:
> > +  - |
> > +    pci {
> > +        #address-cells = <3>;
> > +        #size-cells = <2>;
> > +
> > +        rp1@0,0 {
> > +            compatible = "pci1de4,1";
> > +            ranges = <0x01 0x00 0x00000000
> > +                      0x82010000 0x00 0x00
> > +                      0x00 0x400000>;
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +
> > +            pci_ep_bus: pci-ep-bus@1 {
> > +                compatible = "simple-bus";
> > +                ranges = <0xc0 0x40000000
> > +                          0x01 0x00 0x00000000
> > +                          0x00 0x00400000>;
> > +                dma-ranges = <0x10 0x00000000
> > +                              0x43000000 0x10 0x00000000
> > +                              0x10 0x00000000>;
> > +                #address-cells = <2>;
> > +                #size-cells = <2>;
> > +                interrupt-controller;
> > +                interrupt-parent = <&pci_ep_bus>;
> > +                #interrupt-cells = <2>;
> > +
> > +                rp1_clocks: clocks@c040018000 {
> > +                    compatible = "raspberrypi,rp1-clocks";
> > +                    reg = <0xc0 0x40018000 0x0 0x10038>;
> > +                    #clock-cells = <1>;
> > +                    clocks = <&clk_rp1_xosc>;
> > +                    clock-names =  "rp1-xosc";
> > +                };
> > +            };
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ccf123b805c8..2aea5a6166bd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19384,6 +19384,7 @@ RASPBERRY PI RP1 PCI DRIVER
> >  M:	Andrea della Porta <andrea.porta@suse.com>
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml
> > +F:	Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> >  F:	Documentation/devicetree/bindings/pci/pci-ep-bus.yaml
> >  F:	Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml
> >  F:	include/dt-bindings/clock/rp1.h
> > -- 
> > 2.35.3
> >

Hi Krzysztof,

On 08:26 Tue 08 Oct     , Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 02:39:47PM +0200, Andrea della Porta wrote:
> > The RP1 is a MFD that exposes its peripherals through PCI BARs. This
> > schema is intended as minimal support for the clock generator and
> > gpio controller peripherals which are accessible through BAR1.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >  .../devicetree/bindings/misc/pci1de4,1.yaml   | 110 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 111 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/misc/pci1de4,1.yaml b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > new file mode 100644
> > index 000000000000..3f099b16e672
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/pci1de4,1.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/misc/pci1de4,1.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RaspberryPi RP1 MFD PCI device
> > +
> > +maintainers:
> > +  - Andrea della Porta <andrea.porta@suse.com>
> > +
> > +description:
> > +  The RaspberryPi RP1 is a PCI multi function device containing
> > +  peripherals ranging from Ethernet to USB controller, I2C, SPI
> > +  and others.
> > +  The peripherals are accessed by addressing the PCI BAR1 region.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-ep-bus.yaml
> > +
> > +properties:
> > +  compatible:
> > +    additionalItems: true
> 
> Why is this true? This is final schema, not a "common" part.

The 'compatible' property I've specified in rp1.dtso is not strictly
necessary since it will be added automatically during the dynamic device
node creation by the OF subsystem, and will be something like this:

"pci1de4,1", "pciclass,0200000", "pciclass,0200"

I've redefined simply as "pci1de4,1" in the dtso so it can be validated
against the relative binding schema, and I opted for a shorter name since
the RP1 is not really a simple ethernet controller as advertised by the 
config space (pciclass=2). The schema definition allows for the "relaxed"
extended version, shoudl someone want to use it for resemblance with the
dynamically create compatibel string.

> 
> > +    maxItems: 3
> > +    items:
> > +      - const: pci1de4,1
> > +
> > +patternProperties:
> > +  "^pci-ep-bus@[0-2]$":
> > +    $ref: '#/$defs/bar-bus'
> > +    description:
> > +      The bus on which the peripherals are attached, which is addressable
> > +      through the BAR.
> > +
> > +unevaluatedProperties: false
> > +
> > +$defs:
> > +  bar-bus:
> > +    $ref: /schemas/pci/pci-ep-bus.yaml#/$defs/pci-ep-bus
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      "#interrupt-cells":
> > +        const: 2
> > +        description:
> > +          Specifies respectively the interrupt number and flags as defined
> > +          in include/dt-bindings/interrupt-controller/irq.h.
> > +
> > +      interrupt-controller: true
> > +
> > +      interrupt-parent:
> > +        description:
> > +          Must be the phandle of this 'pci-ep-bus' node. It will trigger
> > +          PCI interrupts on behalf of peripheral generated interrupts.
> > +
> > +    patternProperties:
> > +      "^clocks(@[0-9a-f]+)?$":
> 
> Why @ is optional? Your device is fixed, not flexible.

Right.

Many thanks,
Andrea

> 
> Best regards,
> Krzysztof
>