Messages in this thread | | | Date | Mon, 1 Apr 2024 13:25:13 -0700 (PDT) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH] dt-bindings: pci: altera: covert to yaml |
| |
On Fri, 29 Mar 2024, Krzysztof Kozlowski wrote:
> On 29/03/2024 18:00, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Covert the device tree bindings for the Altera Root >> Port controller from text to yaml. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- > > ... > >> diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml >> new file mode 100644 >> index 000000000000..8f1ad1362ad1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml >> @@ -0,0 +1,106 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright (C) 2024, Intel Corporation >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/altr,pcie-root-port.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Altera PCIe Root Port >> + >> +maintainers: >> + - Matthew Gerlach <matthew.gerlach@linux.intel.com> >> + >> +allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - altr,pcie-root-port-1.0 >> + - altr,pcie-root-port-2.0 >> + >> + reg: >> + minItems: 2 >> + maxItems: 3 >> + >> + reg-names: >> + description: >> + TX slave port region (Txs) >> + Control register access region (Cra) >> + Hard IP region if altr,pcie-root-port-2.0 (Hip) > > All these go to reg as description of items. > > Both - reg and reg-names - need constraints per variant in > allOf:if:then:. Move allOf: to bottom of file, just like example-schema > is showing.
I understand. I added a constraint and moved allOf: to bottom of file, just like the example-schema is showing.
> > >> + >> + items: >> + - const: Txs >> + - const: Cra >> + - const: Hip >> + minItems: 2 >> + >> + device_type: >> + const: pci > > I don't think you need it.
I removed it.
> >> + >> + "#address-cells": >> + const: 3 > > Drop
Dropped
> >> + >> + "#size-cells": >> + const: 2 > > Drop
Dropped
> >> + >> + interrupts: >> + minItems: 1 > > This should be maxItems.
I changed it to maxItems
> >> + >> + interrupt-map-mask: >> + items: >> + - const: 0 >> + - const: 0 >> + - const: 0 >> + - const: 7 > > I guess as well. > >> + >> + interrupt-map: >> + maxItems: 4 >> + >> + "#interrupt-cells": >> + const: 1 > > Drop
If I remove "#interrupt-cells", then I get the following error: /home/mgerlach/git/linux-next/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml: properties: '#interrupt-cells' is a dependency of 'interrupt-map' from schema $id: http://devicetree.org/meta-schemas/interrupts.yaml#
> >> + >> + msi-parent: >> + description: Link to the hardware entity that serves as the MSI controller. > > Just true. > > Please open existing, recent PCI bindings and look how it is done.
I see a couple of examples of the following:
msi-parent: true
> >> + >> + bus-range: >> + description: PCI bus numbers covered. > > Drop
Dropped.
> >> + >> +required: >> + - compatible >> + - reg >> + - reg-names >> + - device_type >> + - "#address-cells" >> + - "#size-cells" >> + - interrupts >> + - interrupt-map >> + - interrupt-map-mask >> + - "#interrupt-cells" > > This also needs cleaning.
I removed Dropped items.
> >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + pcie_0: pcie@c00000000 { >> + compatible = "altr,pcie-root-port-1.0"; >> + reg = <0xc0000000 0x20000000>, >> + <0xff220000 0x00004000>; > > Misaligned.
I fixed the alignments.
> >> + reg-names = "Txs", "Cra"; >> + interrupt-parent = <&hps_0_arm_gic_0>; >> + interrupts = <0 40 4>; > > Use defines for common constnats.
I added constants from arm_gic.h and irq.h.
> >> + #interrupt-cells = <1>; >> + bus-range = <0x0 0xFF>; > > Lowercase hex
I changed to lower case.
> >> + device_type = "pci"; >> + msi-parent = <&msi_to_gic_gen_0>; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + interrupt-map-mask = <0 0 0 7>; >> + interrupt-map = <0 0 0 1 &pcie_intc 1>, >> + <0 0 0 2 &pcie_intc 2>, >> + <0 0 0 3 &pcie_intc 3>, >> + <0 0 0 4 &pcie_intc 4>; >> + ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000 >> + 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>; > > Misaligned. > > > Best regards, > Krzysztof > >
Thank you for the timely and thorough review. Version 2 of the patch will be submitted soon.
Matthew Gerlach
| |