| Message ID | 20220614012713epcms2p810386a5137fbcf6aefc41fe086badc0b@epcms2p8 |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller | expand |
On 13/06/2022 18:27, Wangseok Lee wrote: > Add description to support Axis, ARTPEC-8 SoC. > ARTPEC-8 is the SoC platform of Axis Communications > and PCIe controller is designed based on Design-Ware PCIe controller. > > Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com> > --- > v2->v3 : > - modify version history to fit the linux commit rule > - remove 'Device Tree Bindings' on title > - remove the interrupt-names, phy-names entries > - remove '_clk' suffix > - add the compatible entries on required > - change node name to soc from artpec8 on examples > > v1->v2 : > -'make dt_binding_check' result improvement > -Add the missing property list > -Align the indentation of continued lines/entries > --- > .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++ > .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++ > 2 files changed, 229 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml > create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml > new file mode 100644 > index 0000000..d802bba > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml > @@ -0,0 +1,109 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/axis,artpec8-pcie-ep.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ARTPEC-8 SoC PCIe Controller > + > +maintainers: > + - Jesper Nilsson <jesper.nilsson@axis.com> > + > +description: |+ > + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP > + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml. > + > +allOf: > + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml# > + > +properties: > + compatible: > + const: axis,artpec8-pcie-ep > + > + reg: > + items: > + - description: Data Bus Interface (DBI) registers. > + - description: Data Bus Interface (DBI2) registers. > + - description: PCIe address space region. > + > + reg-names: > + items: > + - const: dbi > + - const: dbi2 > + - const: addr_space > + > + interrupts: > + maxItems: 1 > + > + clocks: > + items: > + - description: PIPE clock, used by the controller to clock the PIPE > + - description: PCIe dbi clock, ungated version > + - description: PCIe master clock, ungated version > + - description: PCIe slave clock, ungated version > + > + clock-names: > + items: > + - const: pipe > + - const: dbi > + - const: mstr > + - const: slv > + > + phys: > + maxItems: 1 > + > + num-lanes: > + const: 2 > + > +required: > + - compatible > + - reg > + - reg-names > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + - samsung,fsys-sysreg > + - samsung,syscon-phandle > + - samsung,syscon-bus-s-fsys > + - samsung,syscon-bus-p-fsys We are making circles... This was before and I commented already it is wrong. You cannot have some unknown/random properties in "required:" without describing them in "properties:". Please list all your properties in "properties:", except the ones coming from snps bindings/schema. > + - phys > + - num-lanes > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + pcie_ep: pcie-ep@17200000 { > + compatible = "axis,artpec8-pcie-ep"; > + reg = <0x0 0x17200000 0x0 0x1000>, > + <0x0 0x17201000 0x0 0x1000>, > + <0x2 0x00000000 0x6 0x00000000>; > + reg-names = "dbi", "dbi2", "addr_space"; > + #interrupt-cells = <1>; > + interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "intr"; > + clocks = <&clock_cmu_fsys 39>, > + <&clock_cmu_fsys 38>, > + <&clock_cmu_fsys 37>, > + <&clock_cmu_fsys 36>; > + clock-names = "pipe", "dbi", "mstr", "slv"; > + samsung,fsys-sysreg = <&syscon_fsys>; > + samsung,syscon-phandle = <&pmu_system_controller>; > + samsung,syscon-bus-s-fsys = <&syscon_bus_s_fsys>; > + samsung,syscon-bus-p-fsys = <&syscon_bus_p_fsys>; > + phys = <&pcie_phy>; > + phy-names = "pcie_phy"; > + num-lanes = <2>; > + bus-range = <0x00 0xff>; > + num-ib-windows = <16>; > + num-ob-windows = <16>; > + }; > + }; > +... > diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml > new file mode 100644 > index 0000000..dbbe1fd > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml > @@ -0,0 +1,120 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/axis,artpec8-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Artpec-8 SoC PCIe Controller > + > +maintainers: > + - Jesper Nilsson <jesper.nilsson@axis.com> > + > +description: |+ > + This PCIe host controller is based on the Synopsys DesignWare PCIe IP > + and thus inherits all the common properties defined in snps,dw-pcie.yaml. > + > +allOf: > + - $ref: /schemas/pci/snps,dw-pcie.yaml# > + > +properties: > + compatible: > + const: axis,artpec8-pcie > + > + reg: > + items: > + - description: Data Bus Interface (DBI) registers. > + - description: External Local Bus interface (ELBI) registers. > + - description: PCIe configuration space region. > + > + reg-names: > + items: > + - const: dbi > + - const: elbi > + - const: config > + > + ranges: > + maxItems: 2 > + > + num-lanes: > + const: 2 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + items: > + - description: PIPE clock, used by the controller to clock the PIPE > + - description: PCIe dbi clock, ungated version > + - description: PCIe master clock, ungated version > + - description: PCIe slave clock, ungated version > + > + clock-names: > + items: > + - const: pipe > + - const: dbi > + - const: mstr > + - const: slv > + > + phys: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - reg-names > + - device_type > + - ranges > + - num-lanes > + - interrupts > + - interrupt-names > + - clocks > + - clock-names > + - samsung,fsys-sysreg > + - samsung,syscon-phandle > + - samsung,syscon-bus-s-fsys > + - samsung,syscon-bus-p-fsys Same problem. Best regards, Krzysztof
On 17/06/2022 07:54, Krzysztof Kozlowski wrote: > On 13/06/2022 18:27, Wangseok Lee wrote: >> Add description to support Axis, ARTPEC-8 SoC. >> ARTPEC-8 is the SoC platform of Axis Communications >> and PCIe controller is designed based on Design-Ware PCIe controller. >> >> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com> >> --- >> v2->v3 : >> - modify version history to fit the linux commit rule >> - remove 'Device Tree Bindings' on title >> - remove the interrupt-names, phy-names entries >> - remove '_clk' suffix >> - add the compatible entries on required >> - change node name to soc from artpec8 on examples >> >> v1->v2 : >> -'make dt_binding_check' result improvement >> -Add the missing property list >> -Align the indentation of continued lines/entries >> --- >> .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++ >> .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++ >> 2 files changed, 229 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >> new file mode 100644 >> index 0000000..d802bba >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >> @@ -0,0 +1,109 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: https://protect2.fireeye.com/v1/url?k=87636683-e61e8c00-8762edcc-74fe48600158-e7a1c3794076f0b9&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpci%2Faxis%2Cartpec8-pcie-ep.yaml%23 >> +$schema: https://protect2.fireeye.com/v1/url?k=36f56c4e-578886cd-36f4e701-74fe48600158-afd7270f84937054&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >> + >> +title: ARTPEC-8 SoC PCIe Controller >> + >> +maintainers: >> + - Jesper Nilsson <jesper.nilsson@axis.com> >> + >> +description: |+ >> + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP >> + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml. >> + >> +allOf: >> + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml# >> + >> +properties: >> + compatible: >> + const: axis,artpec8-pcie-ep >> + >> + reg: >> + items: >> + - description: Data Bus Interface (DBI) registers. >> + - description: Data Bus Interface (DBI2) registers. >> + - description: PCIe address space region. >> + >> + reg-names: >> + items: >> + - const: dbi >> + - const: dbi2 >> + - const: addr_space >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: PIPE clock, used by the controller to clock the PIPE >> + - description: PCIe dbi clock, ungated version >> + - description: PCIe master clock, ungated version >> + - description: PCIe slave clock, ungated version >> + >> + clock-names: >> + items: >> + - const: pipe >> + - const: dbi >> + - const: mstr >> + - const: slv >> + >> + phys: >> + maxItems: 1 >> + >> + num-lanes: >> + const: 2 >> + >> +required: >> + - compatible >> + - reg >> + - reg-names >> + - interrupts >> + - interrupt-names >> + - clocks >> + - clock-names >> + - samsung,fsys-sysreg >> + - samsung,syscon-phandle >> + - samsung,syscon-bus-s-fsys >> + - samsung,syscon-bus-p-fsys > > > We are making circles... This was before and I commented already it is > wrong. You cannot have some unknown/random properties in "required:" > without describing them in "properties:". Please list all your > properties in "properties:", except the ones coming from snps > bindings/schema. > I missed that when adding new items to "required", it should also be added to "properties". I will add the following items to the property. samsung,fsys-sysreg: description: Phandle to system register of fsys block. $ref: /schemas/types.yaml#/definitions/phandle samsung,syscon-phandle: description: Phandle to the PMU system controller node. $ref: /schemas/types.yaml#/definitions/phandle samsung,syscon-bus-s-fsys: description: Phandle to bus-s path of fsys block, this register are used for enabling bus-s. $ref: /schemas/types.yaml#/definitions/phandle samsung,syscon-bus-p-fsys: description: Phandle to bus-p path of fsys block, this register are used for enabling bus-p. $ref: /schemas/types.yaml#/definitions/phandle >> + - phys >> + - num-lanes >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + pcie_ep: pcie-ep@17200000 { >> + compatible = "axis,artpec8-pcie-ep"; >> + reg = <0x0 0x17200000 0x0 0x1000>, >> + <0x0 0x17201000 0x0 0x1000>, >> + <0x2 0x00000000 0x6 0x00000000>; >> + reg-names = "dbi", "dbi2", "addr_space"; >> + #interrupt-cells = <1>; >> + interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "intr"; >> + clocks = <&clock_cmu_fsys 39>, >> + <&clock_cmu_fsys 38>, >> + <&clock_cmu_fsys 37>, >> + <&clock_cmu_fsys 36>; >> + clock-names = "pipe", "dbi", "mstr", "slv"; >> + samsung,fsys-sysreg = <&syscon_fsys>; >> + samsung,syscon-phandle = <&pmu_system_controller>; >> + samsung,syscon-bus-s-fsys = <&syscon_bus_s_fsys>; >> + samsung,syscon-bus-p-fsys = <&syscon_bus_p_fsys>; >> + phys = <&pcie_phy>; >> + phy-names = "pcie_phy"; >> + num-lanes = <2>; >> + bus-range = <0x00 0xff>; >> + num-ib-windows = <16>; >> + num-ob-windows = <16>; >> + }; >> + }; >> +... >> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml >> new file mode 100644 >> index 0000000..dbbe1fd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml >> @@ -0,0 +1,120 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: https://protect2.fireeye.com/v1/url?k=8f17d1f3-ee6a3b70-8f165abc-74fe48600158-c1db309737e1575c&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpci%2Faxis%2Cartpec8-pcie.yaml%23 >> +$schema: https://protect2.fireeye.com/v1/url?k=50e0873f-319d6dbc-50e10c70-74fe48600158-c376ff145b3e096a&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >> + >> +title: Artpec-8 SoC PCIe Controller >> + >> +maintainers: >> + - Jesper Nilsson <jesper.nilsson@axis.com> >> + >> +description: |+ >> + This PCIe host controller is based on the Synopsys DesignWare PCIe IP >> + and thus inherits all the common properties defined in snps,dw-pcie.yaml. >> + >> +allOf: >> + - $ref: /schemas/pci/snps,dw-pcie.yaml# >> + >> +properties: >> + compatible: >> + const: axis,artpec8-pcie >> + >> + reg: >> + items: >> + - description: Data Bus Interface (DBI) registers. >> + - description: External Local Bus interface (ELBI) registers. >> + - description: PCIe configuration space region. >> + >> + reg-names: >> + items: >> + - const: dbi >> + - const: elbi >> + - const: config >> + >> + ranges: >> + maxItems: 2 >> + >> + num-lanes: >> + const: 2 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: PIPE clock, used by the controller to clock the PIPE >> + - description: PCIe dbi clock, ungated version >> + - description: PCIe master clock, ungated version >> + - description: PCIe slave clock, ungated version >> + >> + clock-names: >> + items: >> + - const: pipe >> + - const: dbi >> + - const: mstr >> + - const: slv >> + >> + phys: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - reg-names >> + - device_type >> + - ranges >> + - num-lanes >> + - interrupts >> + - interrupt-names >> + - clocks >> + - clock-names >> + - samsung,fsys-sysreg >> + - samsung,syscon-phandle >> + - samsung,syscon-bus-s-fsys >> + - samsung,syscon-bus-p-fsys > > Same problem. > > Best regards, > Krzysztof Thank you for kindness reivew. Best regards, Wangseok Lee
On 20/06/2022 09:55, Wangseok Lee wrote: > On 17/06/2022 07:54, Krzysztof Kozlowski wrote: >> On 13/06/2022 18:27, Wangseok Lee wrote: >>> Add description to support Axis, ARTPEC-8 SoC. >>> ARTPEC-8 is the SoC platform of Axis Communications >>> and PCIe controller is designed based on Design-Ware PCIe controller. >>> >>> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com> >>> --- >>> v2->v3 : >>> - modify version history to fit the linux commit rule >>> - remove 'Device Tree Bindings' on title >>> - remove the interrupt-names, phy-names entries >>> - remove '_clk' suffix >>> - add the compatible entries on required >>> - change node name to soc from artpec8 on examples >>> >>> v1->v2 : >>> -'make dt_binding_check' result improvement >>> -Add the missing property list >>> -Align the indentation of continued lines/entries >>> --- >>> .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++ >>> .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++ >>> 2 files changed, 229 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>> new file mode 100644 >>> index 0000000..d802bba >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>> @@ -0,0 +1,109 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: https://protect2.fireeye.com/v1/url?k=87636683-e61e8c00-8762edcc-74fe48600158-e7a1c3794076f0b9&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpci%2Faxis%2Cartpec8-pcie-ep.yaml%23 >>> +$schema: https://protect2.fireeye.com/v1/url?k=36f56c4e-578886cd-36f4e701-74fe48600158-afd7270f84937054&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >>> + >>> +title: ARTPEC-8 SoC PCIe Controller >>> + >>> +maintainers: >>> + - Jesper Nilsson <jesper.nilsson@axis.com> >>> + >>> +description: |+ >>> + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP >>> + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml. >>> + >>> +allOf: >>> + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml# >>> + >>> +properties: >>> + compatible: >>> + const: axis,artpec8-pcie-ep >>> + >>> + reg: >>> + items: >>> + - description: Data Bus Interface (DBI) registers. >>> + - description: Data Bus Interface (DBI2) registers. >>> + - description: PCIe address space region. >>> + >>> + reg-names: >>> + items: >>> + - const: dbi >>> + - const: dbi2 >>> + - const: addr_space >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + clocks: >>> + items: >>> + - description: PIPE clock, used by the controller to clock the PIPE >>> + - description: PCIe dbi clock, ungated version >>> + - description: PCIe master clock, ungated version >>> + - description: PCIe slave clock, ungated version >>> + >>> + clock-names: >>> + items: >>> + - const: pipe >>> + - const: dbi >>> + - const: mstr >>> + - const: slv >>> + >>> + phys: >>> + maxItems: 1 >>> + >>> + num-lanes: >>> + const: 2 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - reg-names >>> + - interrupts >>> + - interrupt-names >>> + - clocks >>> + - clock-names >>> + - samsung,fsys-sysreg >>> + - samsung,syscon-phandle >>> + - samsung,syscon-bus-s-fsys >>> + - samsung,syscon-bus-p-fsys >> >> >> We are making circles... This was before and I commented already it is >> wrong. You cannot have some unknown/random properties in "required:" >> without describing them in "properties:". Please list all your >> properties in "properties:", except the ones coming from snps >> bindings/schema. >> > > I missed that when adding new items to "required", > it should also be added to "properties". > I will add the following items to the property. > > samsung,fsys-sysreg: > description: > Phandle to system register of fsys block. > $ref: /schemas/types.yaml#/definitions/phandle This is ok. > > samsung,syscon-phandle: > description: > Phandle to the PMU system controller node. > $ref: /schemas/types.yaml#/definitions/phandle This is ok. > > samsung,syscon-bus-s-fsys: > description: > Phandle to bus-s path of fsys block, this register > are used for enabling bus-s. > $ref: /schemas/types.yaml#/definitions/phandle > > samsung,syscon-bus-p-fsys: > description: > Phandle to bus-p path of fsys block, this register > are used for enabling bus-p. > $ref: /schemas/types.yaml#/definitions/phandle This two look unspecific and hacky workaround for missing drivers. Looks like instead of implementing interconnect or clock driver, you decided to poke some other registers. Why this cannot be an interconnect driver? Best regards, Krzysztof
On 20/06/2022 17:42, Krzysztof Kozlowski wrote: > On 20/06/2022 09:55, Wangseok Lee wrote: >> On 17/06/2022 07:54, Krzysztof Kozlowski wrote: >>> On 13/06/2022 18:27, Wangseok Lee wrote: >>>> Add description to support Axis, ARTPEC-8 SoC. >>>> ARTPEC-8 is the SoC platform of Axis Communications >>>> and PCIe controller is designed based on Design-Ware PCIe controller. >>>> >>>> Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com> >>>> --- >>>> v2->v3 : >>>> - modify version history to fit the linux commit rule >>>> - remove 'Device Tree Bindings' on title >>>> - remove the interrupt-names, phy-names entries >>>> - remove '_clk' suffix >>>> - add the compatible entries on required >>>> - change node name to soc from artpec8 on examples >>>> >>>> v1->v2 : >>>> -'make dt_binding_check' result improvement >>>> -Add the missing property list >>>> -Align the indentation of continued lines/entries >>>> --- >>>> .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++ >>>> .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++ >>>> 2 files changed, 229 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>>> create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>>> new file mode 100644 >>>> index 0000000..d802bba >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml >>>> @@ -0,0 +1,109 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: https://protect2.fireeye.com/v1/url?k=87636683-e61e8c00-8762edcc-74fe48600158-e7a1c3794076f0b9&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fpci%2Faxis%2Cartpec8-pcie-ep.yaml%23 >>>> +$schema: https://protect2.fireeye.com/v1/url?k=36f56c4e-578886cd-36f4e701-74fe48600158-afd7270f84937054&q=1&e=35e09b7f-4fb1-4c8f-83ac-7ec33e124d44&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23 >>>> + >>>> +title: ARTPEC-8 SoC PCIe Controller >>>> + >>>> +maintainers: >>>> + - Jesper Nilsson <jesper.nilsson@axis.com> >>>> + >>>> +description: |+ >>>> + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP >>>> + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml. >>>> + >>>> +allOf: >>>> + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml# >>>> + >>>> +properties: >>>> + compatible: >>>> + const: axis,artpec8-pcie-ep >>>> + >>>> + reg: >>>> + items: >>>> + - description: Data Bus Interface (DBI) registers. >>>> + - description: Data Bus Interface (DBI2) registers. >>>> + - description: PCIe address space region. >>>> + >>>> + reg-names: >>>> + items: >>>> + - const: dbi >>>> + - const: dbi2 >>>> + - const: addr_space >>>> + >>>> + interrupts: >>>> + maxItems: 1 >>>> + >>>> + clocks: >>>> + items: >>>> + - description: PIPE clock, used by the controller to clock the PIPE >>>> + - description: PCIe dbi clock, ungated version >>>> + - description: PCIe master clock, ungated version >>>> + - description: PCIe slave clock, ungated version >>>> + >>>> + clock-names: >>>> + items: >>>> + - const: pipe >>>> + - const: dbi >>>> + - const: mstr >>>> + - const: slv >>>> + >>>> + phys: >>>> + maxItems: 1 >>>> + >>>> + num-lanes: >>>> + const: 2 >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - reg-names >>>> + - interrupts >>>> + - interrupt-names >>>> + - clocks >>>> + - clock-names >>>> + - samsung,fsys-sysreg >>>> + - samsung,syscon-phandle >>>> + - samsung,syscon-bus-s-fsys >>>> + - samsung,syscon-bus-p-fsys >>> >>> >>> We are making circles... This was before and I commented already it is >>> wrong. You cannot have some unknown/random properties in "required:" >>> without describing them in "properties:". Please list all your >>> properties in "properties:", except the ones coming from snps >>> bindings/schema. >>> >> >> I missed that when adding new items to "required", >> it should also be added to "properties". >> I will add the following items to the property. >> >> samsung,fsys-sysreg: >> description: >> Phandle to system register of fsys block. >> $ref: /schemas/types.yaml#/definitions/phandle > > This is ok. > >> >> samsung,syscon-phandle: >> description: >> Phandle to the PMU system controller node. >> $ref: /schemas/types.yaml#/definitions/phandle > > This is ok. > >> >> samsung,syscon-bus-s-fsys: >> description: >> Phandle to bus-s path of fsys block, this register >> are used for enabling bus-s. >> $ref: /schemas/types.yaml#/definitions/phandle >> >> samsung,syscon-bus-p-fsys: >> description: >> Phandle to bus-p path of fsys block, this register >> are used for enabling bus-p. >> $ref: /schemas/types.yaml#/definitions/phandle > > This two look unspecific and hacky workaround for missing drivers. Looks > like instead of implementing interconnect or clock driver, you decided > to poke some other registers. Why this cannot be an interconnect driver? > > bus-s, bus-p is a register that exists in the sysreg of the fsys block. It is the same block as "fsys-sysreg" but is separated separately in hardware. So, get resource is performed separately from "fsys-sysreg". They set pcie slave, dbi related control settings, naming "bus-x" seems to be interconnect. I will add this description to property. I don't think it need to use the interconnect driver, so please let me know your opinion. > Best regards, > Krzysztof Thank you for kindness reivew. Best regards, Wangseok Lee
On 21/06/2022 09:42, Wangseok Lee wrote: >>> >>> samsung,syscon-bus-s-fsys: >>> description: >>> Phandle to bus-s path of fsys block, this register >>> are used for enabling bus-s. >>> $ref: /schemas/types.yaml#/definitions/phandle >>> >>> samsung,syscon-bus-p-fsys: >>> description: >>> Phandle to bus-p path of fsys block, this register >>> are used for enabling bus-p. >>> $ref: /schemas/types.yaml#/definitions/phandle >> >> This two look unspecific and hacky workaround for missing drivers. Looks >> like instead of implementing interconnect or clock driver, you decided >> to poke some other registers. Why this cannot be an interconnect driver? >> >> > > bus-s, bus-p is a register that exists in the sysreg of the fsys block. > It is the same block as "fsys-sysreg" but is separated separately in > hardware. Two points here: 1. If it is in FSYS, why it cannot be accessed with samsung,fsys-sysreg? 2. If it is only register, shuld be described like this. You must describe item: https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml#L42 > So, get resource is performed separately from "fsys-sysreg". > They set pcie slave, dbi related control settings, > naming "bus-x" seems to be interconnect. > I will add this description to property. > I don't think it need to use the interconnect driver, > so please let me know your opinion. Please document both in the bindings and in the driver usage of this register. Writing there "0" or "1" is not enough. If the documentation is good, I am fine with it. If the explanation is obfuscated/not sufficient, it will look like avoiding to implement a driver, which I don't want to accept. Best regards, Krzysztof
On 21/06/2022 21:44, Krzysztof Kozlowski wrote: > On 21/06/2022 09:42, Wangseok Lee wrote: >>>> >>>> samsung,syscon-bus-s-fsys: >>>> description: >>>> Phandle to bus-s path of fsys block, this register >>>> are used for enabling bus-s. >>>> $ref: /schemas/types.yaml#/definitions/phandle >>>> >>>> samsung,syscon-bus-p-fsys: >>>> description: >>>> Phandle to bus-p path of fsys block, this register >>>> are used for enabling bus-p. >>>> $ref: /schemas/types.yaml#/definitions/phandle >>> >>> This two look unspecific and hacky workaround for missing drivers. Looks >>> like instead of implementing interconnect or clock driver, you decided >>> to poke some other registers. Why this cannot be an interconnect driver? >>> >>> >> >> bus-s, bus-p is a register that exists in the sysreg of the fsys block. >> It is the same block as "fsys-sysreg" but is separated separately in >> hardware. > > Two points here: > 1. If it is in FSYS, why it cannot be accessed with samsung,fsys-sysreg? > 2. If it is only register, shuld be described like this. You must > describe item: > https://protect2.fireeye.com/v1/url?k=0f529a57-50c9a332-0f531118-000babff32e3-50938d8198077d59&q=1&e=32284e69-bbed-4d09-b6d6-0a43428aebf5&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.18-rc1%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fsoc%2Fsamsung%2Fexynos-usi.yaml%23L42 > It would be better to access with fsys-sysreg, but their h/w address are far from each other. The fsys block consists of a system register and an additional control system register. "bus-s-fsys" and "bus-p-fsys" are additional control system register. sysreg and additional control sysreg addresses are far from each other and there are h/w registers that perform different functions between them. >> So, get resource is performed separately from "fsys-sysreg". >> They set pcie slave, dbi related control settings, >> naming "bus-x" seems to be interconnect. >> I will add this description to property. >> I don't think it need to use the interconnect driver, >> so please let me know your opinion. > > Please document both in the bindings and in the driver usage of this > register. Writing there "0" or "1" is not enough. If the documentation > is good, I am fine with it. If the explanation is obfuscated/not > sufficient, it will look like avoiding to implement a driver, which I > don't want to accept. > I think i should add enough description. Is it sufficient to modify the name and description of property like this? samsung,fsys-bus-s: description: Phandle to bus-s of fsys block, this register is additional control sysreg in fsys block and this is used for pcie slave control setting. $ref: /schemas/types.yaml#/definitions/phandle samsung,fsys-bus-p: description: Phandle to bus-p of fsys block, this register is additional control sysreg in fsys block and this is used for pcie dbi control setting. $ref: /schemas/types.yaml#/definitions/phandle > Best regards, > Krzysztof Thank you for kindness reivew. Best regards, Wangseok Lee
diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml new file mode 100644 index 0000000..d802bba --- /dev/null +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml @@ -0,0 +1,109 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/axis,artpec8-pcie-ep.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ARTPEC-8 SoC PCIe Controller + +maintainers: + - Jesper Nilsson <jesper.nilsson@axis.com> + +description: |+ + This PCIe end-point controller is based on the Synopsys DesignWare PCIe IP + and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml. + +allOf: + - $ref: /schemas/pci/snps,dw-pcie-ep.yaml# + +properties: + compatible: + const: axis,artpec8-pcie-ep + + reg: + items: + - description: Data Bus Interface (DBI) registers. + - description: Data Bus Interface (DBI2) registers. + - description: PCIe address space region. + + reg-names: + items: + - const: dbi + - const: dbi2 + - const: addr_space + + interrupts: + maxItems: 1 + + clocks: + items: + - description: PIPE clock, used by the controller to clock the PIPE + - description: PCIe dbi clock, ungated version + - description: PCIe master clock, ungated version + - description: PCIe slave clock, ungated version + + clock-names: + items: + - const: pipe + - const: dbi + - const: mstr + - const: slv + + phys: + maxItems: 1 + + num-lanes: + const: 2 + +required: + - compatible + - reg + - reg-names + - interrupts + - interrupt-names + - clocks + - clock-names + - samsung,fsys-sysreg + - samsung,syscon-phandle + - samsung,syscon-bus-s-fsys + - samsung,syscon-bus-p-fsys + - phys + - num-lanes + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + pcie_ep: pcie-ep@17200000 { + compatible = "axis,artpec8-pcie-ep"; + reg = <0x0 0x17200000 0x0 0x1000>, + <0x0 0x17201000 0x0 0x1000>, + <0x2 0x00000000 0x6 0x00000000>; + reg-names = "dbi", "dbi2", "addr_space"; + #interrupt-cells = <1>; + interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "intr"; + clocks = <&clock_cmu_fsys 39>, + <&clock_cmu_fsys 38>, + <&clock_cmu_fsys 37>, + <&clock_cmu_fsys 36>; + clock-names = "pipe", "dbi", "mstr", "slv"; + samsung,fsys-sysreg = <&syscon_fsys>; + samsung,syscon-phandle = <&pmu_system_controller>; + samsung,syscon-bus-s-fsys = <&syscon_bus_s_fsys>; + samsung,syscon-bus-p-fsys = <&syscon_bus_p_fsys>; + phys = <&pcie_phy>; + phy-names = "pcie_phy"; + num-lanes = <2>; + bus-range = <0x00 0xff>; + num-ib-windows = <16>; + num-ob-windows = <16>; + }; + }; +... diff --git a/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml new file mode 100644 index 0000000..dbbe1fd --- /dev/null +++ b/Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml @@ -0,0 +1,120 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/axis,artpec8-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Artpec-8 SoC PCIe Controller + +maintainers: + - Jesper Nilsson <jesper.nilsson@axis.com> + +description: |+ + This PCIe host controller is based on the Synopsys DesignWare PCIe IP + and thus inherits all the common properties defined in snps,dw-pcie.yaml. + +allOf: + - $ref: /schemas/pci/snps,dw-pcie.yaml# + +properties: + compatible: + const: axis,artpec8-pcie + + reg: + items: + - description: Data Bus Interface (DBI) registers. + - description: External Local Bus interface (ELBI) registers. + - description: PCIe configuration space region. + + reg-names: + items: + - const: dbi + - const: elbi + - const: config + + ranges: + maxItems: 2 + + num-lanes: + const: 2 + + interrupts: + maxItems: 1 + + clocks: + items: + - description: PIPE clock, used by the controller to clock the PIPE + - description: PCIe dbi clock, ungated version + - description: PCIe master clock, ungated version + - description: PCIe slave clock, ungated version + + clock-names: + items: + - const: pipe + - const: dbi + - const: mstr + - const: slv + + phys: + maxItems: 1 + +required: + - compatible + - reg + - reg-names + - device_type + - ranges + - num-lanes + - interrupts + - interrupt-names + - clocks + - clock-names + - samsung,fsys-sysreg + - samsung,syscon-phandle + - samsung,syscon-bus-s-fsys + - samsung,syscon-bus-p-fsys + - phys + - phy-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + pcie: pcie@17200000 { + compatible = "axis,artpec8-pcie"; + reg = <0x0 0x17200000 0x0 0x1000>, + <0x0 0x16ca0000 0x0 0x2000>, + <0x7 0x0001e000 0x0 0x2000>; + reg-names = "dbi", "elbi", "config"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = </* non-prefetchable memory */ + 0x83000000 0x0 0x0000000 0x2 0x00000000 0x5 0x00000000 + /* downstream I/O */ + 0x81000000 0x0 0x0000000 0x7 0x00000000 0x0 0x00010000>; + num-lanes = <2>; + bus-range = <0x00 0xff>; + interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "intr"; + #interrupt-cells = <1>; + clocks = <&cmu_fsys 39>, + <&cmu_fsys 38>, + <&cmu_fsys 37>, + <&cmu_fsys 36>; + clock-names = "pipe", "dbi", "mstr", "slv"; + samsung,fsys-sysreg = <&syscon_fsys>; + samsung,syscon-phandle = <&pmu_system_controller>; + samsung,syscon-bus-s-fsys = <&syscon_bus_s_fsys>; + samsung,syscon-bus-p-fsys = <&syscon_bus_p_fsys>; + phys = <&pcie_phy>; + phy-names = "pcie_phy"; + }; + }; +...
Add description to support Axis, ARTPEC-8 SoC. ARTPEC-8 is the SoC platform of Axis Communications and PCIe controller is designed based on Design-Ware PCIe controller. Signed-off-by: Wangseok Lee <wangseok.lee@samsung.com> --- v2->v3 : - modify version history to fit the linux commit rule - remove 'Device Tree Bindings' on title - remove the interrupt-names, phy-names entries - remove '_clk' suffix - add the compatible entries on required - change node name to soc from artpec8 on examples v1->v2 : -'make dt_binding_check' result improvement -Add the missing property list -Align the indentation of continued lines/entries --- .../bindings/pci/axis,artpec8-pcie-ep.yaml | 109 +++++++++++++++++++ .../devicetree/bindings/pci/axis,artpec8-pcie.yaml | 120 +++++++++++++++++++++ 2 files changed, 229 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie-ep.yaml create mode 100644 Documentation/devicetree/bindings/pci/axis,artpec8-pcie.yaml