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@xxxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxx> >>>> + >>>> +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
