回覆: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support
From: Jacky Chou <jacky_chou@aspeedtech.com>
Date: 2025-07-21 03:44:47
Also in:
linux-aspeed, linux-devicetree, linux-gpio, linux-pci, lkml, openbmc
Hi Krzysztof, Thank you for your reply.
No, describe the hardware, not "this binding".quoted
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.
quoted
...quoted
+ + 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.
quoted
+ $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 PCIinterrupts.quoted
+ 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.
quoted
+ - $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@...). That's why patternProperties was added here — to explicitly allow such subnodes and validate them properly. Thanks, Jacky