| Message ID | 20250326022811.3090688-2-sai.krishna.musham@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Add support for PCIe RP PERST# | expand |
On Wed, Mar 26, 2025 at 07:58:10AM +0530, Sai Krishna Musham wrote: > Introduce `reset-gpios` property to enable GPIO-based control of > the PCIe RP PERST# signal, generating assert and deassert signals. I think it was removed, so this is not necessary. The property was there all the time. > > Traditionally, the reset was managed in hardware and enabled during > initialization. With this patch set, the reset will be handled by the > driver. Consequently, the `reset-gpios` property must be explicitly > provided to ensure proper functionality. > > Add CPM clock and reset control registers base (`cpm_crx`) to handle > PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors. So does it mean it was not working before at all? > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com> > --- > Changes for v6: > - Resolve ABI break. > - Update commit message. > > Changes for v5: > - Remove `reset-gpios` property from required as it is already present > in pci-bus-common.yaml > - Update commit message > > Changes for v4: > - Add CPM clock and reset control registers base to handle PCIe IP > reset. > - Update commit message. > > Changes for v3: > - None > > Changes for v2: > - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity > - Update commit message > --- > .../bindings/pci/xilinx-versal-cpm.yaml | 72 ++++++++++++++----- > 1 file changed, 55 insertions(+), 17 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > index d674a24c8ccc..26e9cea41889 100644 > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > @@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx Versal SoCs > maintainers: > - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com> > > -allOf: > - - $ref: /schemas/pci/pci-host-bridge.yaml# > - > properties: > compatible: > enum: > @@ -21,18 +18,12 @@ properties: > - xlnx,versal-cpm5nc-host > > reg: > - items: > - - description: CPM system level control and status registers. > - - description: Configuration space region and bridge registers. > - - description: CPM5 control and status registers. > - minItems: 2 > + minItems: 3 That's an ABI break. > + maxItems: 4 > > reg-names: > - items: > - - const: cpm_slcr > - - const: cfg > - - const: cpm_csr > - minItems: 2 > + minItems: 3 > + maxItems: 4 > > interrupts: > maxItems: 1 > @@ -72,10 +63,53 @@ required: > - msi-map > - interrupt-controller > > +allOf: > + - $ref: /schemas/pci/pci-host-bridge.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - xlnx,versal-cpm-host-1.00 > + then: > + properties: > + reg: > + items: > + - description: CPM system level control and status registers. > + - description: Configuration space region and bridge registers. > + - description: CPM clock and reset control registers. Before two items, now min 3, so another ABI break. Missing minItems. > + reg-names: > + items: > + - const: cpm_slcr > + - const: cfg > + - const: cpm_crx Same > + - if: > + properties: > + compatible: > + contains: > + enum: > + - xlnx,versal-cpm5-host > + - xlnx,versal-cpm5-host1 > + then: > + properties: > + reg: > + items: > + - description: CPM system level control and status registers. > + - description: Configuration space region and bridge registers. > + - description: CPM5 control and status registers. > + - description: CPM clock and reset control registers. This makes no sense, you still add the entry in the middle. This patch fixed nothing from issues previously pointed out. It's the third or fourth try and you keep repeating the same mistake, which means you do not understand the problem. The problem is: you cannot change the order. If you change it, it's an ABI break and nothing in commit msg explained that. Best regards, Krzysztof
[AMD Official Use Only - AMD Internal Distribution Only] Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Wednesday, March 26, 2025 1:16 PM > To: Musham, Sai Krishna <sai.krishna.musham@amd.com> > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; > manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org; > conor+dt@kernel.org; cassel@kernel.org; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Simek, Michal > <michal.simek@amd.com>; Gogada, Bharat Kumar > <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy > <thippeswamy.havalige@amd.com> > Subject: Re: [PATCH v6 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe > RP PERST# > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On Wed, Mar 26, 2025 at 07:58:10AM +0530, Sai Krishna Musham wrote: > > Introduce `reset-gpios` property to enable GPIO-based control of the > > PCIe RP PERST# signal, generating assert and deassert signals. > > I think it was removed, so this is not necessary. The property was there all the time. Thank you for reviewing, I will update this in next patch. > > > > > Traditionally, the reset was managed in hardware and enabled during > > initialization. With this patch set, the reset will be handled by the > > driver. Consequently, the `reset-gpios` property must be explicitly > > provided to ensure proper functionality. > > > > Add CPM clock and reset control registers base (`cpm_crx`) to handle > > PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors. > > So does it mean it was not working before at all? Thank you for reviewing, it was working earlier also. Currently boards are designed to handle PERST# and IP reset from software driver, and IP reset is being removed from design. I will update the commit message in next patch. > > > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com> > > --- > > Changes for v6: > > - Resolve ABI break. > > - Update commit message. > > > > Changes for v5: > > - Remove `reset-gpios` property from required as it is already present > > in pci-bus-common.yaml > > - Update commit message > > > > Changes for v4: > > - Add CPM clock and reset control registers base to handle PCIe IP > > reset. > > - Update commit message. > > > > Changes for v3: > > - None > > > > Changes for v2: > > - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity > > - Update commit message > > --- > > .../bindings/pci/xilinx-versal-cpm.yaml | 72 ++++++++++++++----- > > 1 file changed, 55 insertions(+), 17 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > index d674a24c8ccc..26e9cea41889 100644 > > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > @@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx > > Versal SoCs > > maintainers: > > - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com> > > > > -allOf: > > - - $ref: /schemas/pci/pci-host-bridge.yaml# > > - > > properties: > > compatible: > > enum: > > @@ -21,18 +18,12 @@ properties: > > - xlnx,versal-cpm5nc-host > > > > reg: > > - items: > > - - description: CPM system level control and status registers. > > - - description: Configuration space region and bridge registers. > > - - description: CPM5 control and status registers. > > - minItems: 2 > > + minItems: 3 > > That's an ABI break. Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break. > > > + maxItems: 4 > > > > reg-names: > > - items: > > - - const: cpm_slcr > > - - const: cfg > > - - const: cpm_csr > > - minItems: 2 > > + minItems: 3 > > + maxItems: 4 > > > > interrupts: > > maxItems: 1 > > @@ -72,10 +63,53 @@ required: > > - msi-map > > - interrupt-controller > > > > +allOf: > > + - $ref: /schemas/pci/pci-host-bridge.yaml# > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - xlnx,versal-cpm-host-1.00 > > + then: > > + properties: > > + reg: > > + items: > > + - description: CPM system level control and status registers. > > + - description: Configuration space region and bridge registers. > > + - description: CPM clock and reset control registers. > > Before two items, now min 3, so another ABI break. Missing minItems. Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break. > > > + reg-names: > > + items: > > + - const: cpm_slcr > > + - const: cfg > > + - const: cpm_crx > > Same Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break. > > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - xlnx,versal-cpm5-host > > + - xlnx,versal-cpm5-host1 > > + then: > > + properties: > > + reg: > > + items: > > + - description: CPM system level control and status registers. > > + - description: Configuration space region and bridge registers. > > + - description: CPM5 control and status registers. > > + - description: CPM clock and reset control registers. > > This makes no sense, you still add the entry in the middle. This patch fixed nothing > from issues previously pointed out. > > It's the third or fourth try and you keep repeating the same mistake, which means > you do not understand the problem. The problem is: you cannot change the order. If > you change it, it's an ABI break and nothing in commit msg explained that. Thanks for reviewing, I will update minItems to 3, so it will not be an ABI break. And currently in this patch I have a cpm_crx property at the end. This is the description for cpm_crx. - description: CPM clock and reset control registers. Below is the updated version of this patch. Please let me know if you have any further comments. - xlnx,versal-cpm5nc-host reg: - minItems: 3 + minItems: 2 maxItems: 4 reg-names: - minItems: 3 + minItems: 2 maxItems: 4 interrupts: @@ -78,11 +78,13 @@ allOf: - description: CPM system level control and status registers. - description: Configuration space region and bridge registers. - description: CPM clock and reset control registers. + minItems: 2 reg-names: items: - const: cpm_slcr - const: cfg - const: cpm_crx + minItems: 2 - if: properties: compatible: @@ -98,12 +100,14 @@ allOf: - description: Configuration space region and bridge registers. - description: CPM5 control and status registers. - description: CPM clock and reset control registers. + minItems: 3 reg-names: items: - const: cpm_slcr - const: cfg - const: cpm_csr - const: cpm_crx + minItems: 3 unevaluatedProperties: false > > Best regards, > Krzysztof Regards, Sai Krishna
diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml index d674a24c8ccc..26e9cea41889 100644 --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml @@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx Versal SoCs maintainers: - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com> -allOf: - - $ref: /schemas/pci/pci-host-bridge.yaml# - properties: compatible: enum: @@ -21,18 +18,12 @@ properties: - xlnx,versal-cpm5nc-host reg: - items: - - description: CPM system level control and status registers. - - description: Configuration space region and bridge registers. - - description: CPM5 control and status registers. - minItems: 2 + minItems: 3 + maxItems: 4 reg-names: - items: - - const: cpm_slcr - - const: cfg - - const: cpm_csr - minItems: 2 + minItems: 3 + maxItems: 4 interrupts: maxItems: 1 @@ -72,10 +63,53 @@ required: - msi-map - interrupt-controller +allOf: + - $ref: /schemas/pci/pci-host-bridge.yaml# + - if: + properties: + compatible: + contains: + enum: + - xlnx,versal-cpm-host-1.00 + then: + properties: + reg: + items: + - description: CPM system level control and status registers. + - description: Configuration space region and bridge registers. + - description: CPM clock and reset control registers. + reg-names: + items: + - const: cpm_slcr + - const: cfg + - const: cpm_crx + - if: + properties: + compatible: + contains: + enum: + - xlnx,versal-cpm5-host + - xlnx,versal-cpm5-host1 + then: + properties: + reg: + items: + - description: CPM system level control and status registers. + - description: Configuration space region and bridge registers. + - description: CPM5 control and status registers. + - description: CPM clock and reset control registers. + reg-names: + items: + - const: cpm_slcr + - const: cfg + - const: cpm_csr + - const: cpm_crx + unevaluatedProperties: false examples: - | + #include <dt-bindings/gpio/gpio.h> versal { #address-cells = <2>; @@ -98,8 +132,10 @@ examples: <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>; msi-map = <0x0 &its_gic 0x0 0x10000>; reg = <0x0 0xfca10000 0x0 0x1000>, - <0x6 0x00000000 0x0 0x10000000>; - reg-names = "cpm_slcr", "cfg"; + <0x6 0x00000000 0x0 0x10000000>, + <0x0 0xfca00000 0x0 10000>; + reg-names = "cpm_slcr", "cfg", "cpm_crx"; + reset-gpios = <&gpio1 38 GPIO_ACTIVE_LOW>; pcie_intc_0: interrupt-controller { #address-cells = <0>; #interrupt-cells = <1>; @@ -126,8 +162,10 @@ examples: msi-map = <0x0 &its_gic 0x0 0x10000>; reg = <0x00 0xfcdd0000 0x00 0x1000>, <0x06 0x00000000 0x00 0x1000000>, - <0x00 0xfce20000 0x00 0x1000000>; - reg-names = "cpm_slcr", "cfg", "cpm_csr"; + <0x00 0xfce20000 0x00 0x1000000>, + <0x00 0xfcdc0000 0x00 0x10000>; + reg-names = "cpm_slcr", "cfg", "cpm_csr", "cpm_crx"; + reset-gpios = <&gpio1 38 GPIO_ACTIVE_LOW>; pcie_intc_1: interrupt-controller { #address-cells = <0>;
Introduce `reset-gpios` property to enable GPIO-based control of the PCIe RP PERST# signal, generating assert and deassert signals. Traditionally, the reset was managed in hardware and enabled during initialization. With this patch set, the reset will be handled by the driver. Consequently, the `reset-gpios` property must be explicitly provided to ensure proper functionality. Add CPM clock and reset control registers base (`cpm_crx`) to handle PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors. Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com> --- Changes for v6: - Resolve ABI break. - Update commit message. Changes for v5: - Remove `reset-gpios` property from required as it is already present in pci-bus-common.yaml - Update commit message Changes for v4: - Add CPM clock and reset control registers base to handle PCIe IP reset. - Update commit message. Changes for v3: - None Changes for v2: - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity - Update commit message --- .../bindings/pci/xilinx-versal-cpm.yaml | 72 ++++++++++++++----- 1 file changed, 55 insertions(+), 17 deletions(-)