On 20/06/2022 17:42, Krzysztof Kozlowski wrote:

On 20/06/2022 09:55, Wangseok Lee wrote:

quoted

On 17/06/2022 07:54, Krzysztof Kozlowski wrote:

quoted

On 13/06/2022 18:27, Wangseok Lee wrote:

quoted

 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 [off-list ref]
 ---
 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 [off-list ref]
 +
 +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.

quoted

samsung,syscon-phandle:
  description:
    Phandle to the PMU system controller node.
  $ref: /schemas/types.yaml#/definitions/phandle

This is ok.

quoted

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