| Message ID | 20250318092648.2298280-2-sai.krishna.musham@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Add support for PCIe RP PERST# | expand |
On 18/03/2025 10:26, Sai Krishna Musham wrote: > Changes for v2: > - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity > - Update commit message > --- > .../bindings/pci/xilinx-versal-cpm.yaml | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > index d674a24c8ccc..904594138af2 100644 > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > @@ -24,15 +24,20 @@ properties: > items: > - description: CPM system level control and status registers. > - description: Configuration space region and bridge registers. > + - description: CPM clock and reset control registers. > - description: CPM5 control and status registers. You cannot add items to the middle, that's an ABI break. Adding required properties is also an ABI break. Why you cannot add it to the end of the list? Or at least explain ABI break impact in commit msg? > - minItems: 2 > + minItems: 3 > > reg-names: > items: > - const: cpm_slcr > - const: cfg > + - const: cpm_crx > - const: cpm_csr > - minItems: 2 > + minItems: 3 > + > + reset-gpios: > + description: GPIO used as PERST# signal Isn't this already in pci-bus-common.yaml? Best regards, Krzysztof
[AMD Official Use Only - AMD Internal Distribution Only] Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Tuesday, March 18, 2025 3:23 PM > To: Musham, Sai Krishna <sai.krishna.musham@amd.com>; > 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 > Cc: 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 v4 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 18/03/2025 10:26, Sai Krishna Musham wrote: > > Changes for v2: > > - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity > > - Update commit message > > --- > > .../bindings/pci/xilinx-versal-cpm.yaml | 21 ++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > index d674a24c8ccc..904594138af2 100644 > > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > > @@ -24,15 +24,20 @@ properties: > > items: > > - description: CPM system level control and status registers. > > - description: Configuration space region and bridge registers. > > + - description: CPM clock and reset control registers. > > - description: CPM5 control and status registers. > > You cannot add items to the middle, that's an ABI break. Adding required properties > is also an ABI break. Why you cannot add it to the end of the list? > > Or at least explain ABI break impact in commit msg? > When I add property at the end, I'm observing failure during dt_binding_check. $ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb: pcie@fca10000: reg-names:2: 'cpm_csr' was expected from schema $id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml# > > > - minItems: 2 > > + minItems: 3 > > > > reg-names: > > items: > > - const: cpm_slcr > > - const: cfg > > + - const: cpm_crx > > - const: cpm_csr > > - minItems: 2 > > + minItems: 3 > > + > > + reset-gpios: > > + description: GPIO used as PERST# signal > > Isn't this already in pci-bus-common.yaml? > > > Best regards, > Krzysztof
On 21/03/2025 10:42, Musham, Sai Krishna wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi Krzysztof, > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzk@kernel.org> >> Sent: Tuesday, March 18, 2025 3:23 PM >> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>; >> 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 >> Cc: 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 v4 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 18/03/2025 10:26, Sai Krishna Musham wrote: >>> Changes for v2: >>> - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity >>> - Update commit message >>> --- >>> .../bindings/pci/xilinx-versal-cpm.yaml | 21 ++++++++++++++----- >>> 1 file changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml >>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml >>> index d674a24c8ccc..904594138af2 100644 >>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml >>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml >>> @@ -24,15 +24,20 @@ properties: >>> items: >>> - description: CPM system level control and status registers. >>> - description: Configuration space region and bridge registers. >>> + - description: CPM clock and reset control registers. >>> - description: CPM5 control and status registers. >> >> You cannot add items to the middle, that's an ABI break. Adding required properties >> is also an ABI break. Why you cannot add it to the end of the list? >> >> Or at least explain ABI break impact in commit msg? >> > When I add property at the end, I'm observing failure during dt_binding_check. > $ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb: pcie@fca10000: reg-names:2: 'cpm_csr' was expected > from schema $id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml# Maybe for a good reason. Best regards, Krzysztof
[AMD Official Use Only - AMD Internal Distribution Only] Hi Krzysztof, Thanks, I have resolved the ABI break and added cpm_crx at the end. I will send in v6 series. > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Monday, March 24, 2025 12:50 PM > To: Musham, Sai Krishna <sai.krishna.musham@amd.com>; > 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 > Cc: 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 v4 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 21/03/2025 10:42, Musham, Sai Krishna wrote: > > [AMD Official Use Only - AMD Internal Distribution Only] > > > > Hi Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@kernel.org> > >> Sent: Tuesday, March 18, 2025 3:23 PM > >> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>; > >> 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 > >> Cc: 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 v4 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 18/03/2025 10:26, Sai Krishna Musham wrote: > >>> Changes for v2: > >>> - Add define from include/dt-bindings/gpio/gpio.h for PERST# > >>> polarity > >>> - Update commit message > >>> --- > >>> .../bindings/pci/xilinx-versal-cpm.yaml | 21 ++++++++++++++----- > >>> 1 file changed, 16 insertions(+), 5 deletions(-) > >>> > >>> diff --git > >>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > >>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > >>> index d674a24c8ccc..904594138af2 100644 > >>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > >>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml > >>> @@ -24,15 +24,20 @@ properties: > >>> items: > >>> - description: CPM system level control and status registers. > >>> - description: Configuration space region and bridge registers. > >>> + - description: CPM clock and reset control registers. > >>> - description: CPM5 control and status registers. > >> > >> You cannot add items to the middle, that's an ABI break. Adding > >> required properties is also an ABI break. Why you cannot add it to the end of the > list? > >> > >> Or at least explain ABI break impact in commit msg? > >> > > When I add property at the end, I'm observing failure during dt_binding_check. > > $ make DT_CHECKER_FLAGS=-m dt_binding_check > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cp > > m.yaml > > Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb: > pcie@fca10000: reg-names:2: 'cpm_csr' was expected > > from schema $id: > > http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml# > Maybe for a good reason. > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml index d674a24c8ccc..904594138af2 100644 --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml @@ -24,15 +24,20 @@ properties: items: - description: CPM system level control and status registers. - description: Configuration space region and bridge registers. + - description: CPM clock and reset control registers. - description: CPM5 control and status registers. - minItems: 2 + minItems: 3 reg-names: items: - const: cpm_slcr - const: cfg + - const: cpm_crx - const: cpm_csr - minItems: 2 + minItems: 3 + + reset-gpios: + description: GPIO used as PERST# signal interrupts: maxItems: 1 @@ -64,6 +69,7 @@ properties: required: - reg - reg-names + - reset-gpios - "#interrupt-cells" - interrupts - interrupt-map @@ -76,6 +82,7 @@ unevaluatedProperties: false examples: - | + #include <dt-bindings/gpio/gpio.h> versal { #address-cells = <2>; @@ -98,8 +105,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 +135,10 @@ examples: msi-map = <0x0 &its_gic 0x0 0x10000>; reg = <0x00 0xfcdd0000 0x00 0x1000>, <0x06 0x00000000 0x00 0x1000000>, + <0x00 0xfcdc0000 0x00 0x10000>, <0x00 0xfce20000 0x00 0x1000000>; - reg-names = "cpm_slcr", "cfg", "cpm_csr"; + reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm_csr"; + 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 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 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 | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)