Re: [PATCH v3 1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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