回覆: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Jacky Chou]

Hi Krzysztof,

Thank you for your reply.

> No, describe the hardware, not "this binding".
> 
> > configuring the PCIe RC node, including support for syscon phandles,
> > MSI, clocks, resets, and interrupt mapping. The schema enforces strict
> > property validation and provides a comprehensive example for reference.
> 
> Don't say what schema does or does not. It's completely redundant.
> Describe the hardware.
> 
> Your entire commit is redundantn and not helpful at all.
> 

I will revise the commit message in the next version.
Thanks for your guidance.

> >
> 
> ...
>
> > +
> > +  aspeed,ahbc:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED AHB Controller (AHBC) syscon node.
> > +      This reference is used by the PCIe controller to access
> > +      system-level configuration registers related to the AHB bus.
> > +      To enable AHB access for the PCIe controller.
> > +
> > +  aspeed,pciecfg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED PCIe configuration syscon node.
> > +      This reference allows the PCIe controller to access
> > +      SoC-specific PCIe configuration registers. There are the others
> > +      functions such PCIe RC and PCIe EP will use this common register
> > +      to configure the SoC interfaces.
> > +
> > +  aspeed,pciephy:
> 
> No, phys are not syscons. I already told you that in v1.
> 

I will remove the aspeed,pciephy syscon reference and rework this part to use the standard phys binding properly.
Sorry for overlooking your previous feedback in v1.
Thanks again for your patience.

> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED PCIe PHY syscon node.
> > +      This property provides access to the PCIe PHY control
> > +      registers required for link initialization and management.
> > +      The other functions such PCIe RC and PCIe EP will use this
> > +      common register to configure the PHY interfaces and get some
> > +      information from the PHY.
> > +
> > +  interrupt-controller:
> > +    description: Interrupt controller node for handling legacy PCI
> interrupts.
> > +    type: object
> > +    properties:
> > +      '#address-cells':
> > +        const: 0
> > +      '#interrupt-cells':
> > +        const: 1
> > +      interrupt-controller: true
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#interrupt-cells'
> > +      - interrupt-controller
> > +
> > +    additionalProperties: false
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus-common.yaml#
> 
> No other binding references this. Don't write completely different code than all
> other SoCs. This entire binding is written such way.
> 

Agreed. I will remove it in next version.

> > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-pcie
> > +    then:
> > +      required:
> > +        - aspeed,ahbc
> > +    else:
> > +      properties:
> > +        aspeed,ahbc: false
> > +
> > +required:
> > +  - reg
> > +  - interrupts
> > +  - bus-range
> > +  - ranges
> > +  - resets
> > +  - reset-names
> > +  - msi-parent
> > +  - msi-controller
> > +  - aspeed,pciecfg
> > +  - interrupt-map-mask
> > +  - interrupt-map
> > +  - interrupt-controller
> > +
> > +unevaluatedProperties: false
> > +
> > +patternProperties:
> > +  "^pcie@[0-9a-f,]+$":
> 
> Why do you need it? Also, order things according to example schema.
> 

Thanks for your question.

In the v1 discussion, another reviewer suggested that we should support a
multi-port structure for the PCIe root complex, 
where each port is represented as a child node (e.g., pcie at ...).
That's why patternProperties was added here — to explicitly allow such
subnodes and validate them properly.

Thanks,
Jacky