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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SEYPR06MB5134F8732785F280CB4339309D5DA@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Mon, 21 Jul 2025 03:44:42 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kwilczynski@...nel.org" <kwilczynski@...nel.org>,
	"mani@...nel.org" <mani@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "joel@....id.au" <joel@....id.au>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, BMC-SW <BMC-SW@...eedtech.com>
Subject:
 回覆: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

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@...).
That's why patternProperties was added here — to explicitly allow such
subnodes and validate them properly.

Thanks,
Jacky