| 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
| ||
|
Message-ID:
<DM4PR12MB6158638F3ED4CD8BF548E046CD7CA@DM4PR12MB6158.namprd12.prod.outlook.com>
Date: Fri, 20 Jun 2025 02:22:53 +0000
From: "Musham, Sai Krishna" <sai.krishna.musham@....com>
To: Rob Herring <robh@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>, Manivannan Sadhasivam
<mani@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>, "cassel@...nel.org"
<cassel@...nel.org>, "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Simek,
Michal" <michal.simek@....com>, "Gogada, Bharat Kumar"
<bharat.kumar.gogada@....com>, "Havalige, Thippeswamy"
<thippeswamy.havalige@....com>
Subject: RE: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
and `cpm5nc_fw_attr` properties
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Rob,
Thanks for the review.
> -----Original Message-----
> From: Rob Herring <robh@...nel.org>
> Sent: Tuesday, April 15, 2025 8:44 PM
> To: Musham, Sai Krishna <sai.krishna.musham@....com>
> Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com;
> manivannan.sadhasivam@...aro.org; krzk+dt@...nel.org; conor+dt@...nel.org;
> cassel@...nel.org; linux-pci@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; Simek, Michal <michal.simek@....com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@....com>; Havalige, Thippeswamy
> <thippeswamy.havalige@....com>
> Subject: Re: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
> and `cpm5nc_fw_attr` properties
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Sun, Apr 13, 2025 at 10:23 PM Sai Krishna Musham
> <sai.krishna.musham@....com> wrote:
> >
> > Add the `cpm_crx` property to manage the PCIe IP reset, and
> > `cpm5nc_fw_attr` property to clear firewall after link reset, while
> > maintaining backward compatibility with existing device trees.
>
> You aren't adding properties. You are adding entries to 'reg'.
>
You're right — we are not adding new properties but rather extending
the reg entries, I will correct this in commit message.
> But the real problem here is you are adding reset and firewall
> controls, but not using the respective bindings. It looks like you
> need to use the 'resets' and possibly the 'access-controllers'
> bindings. Unless these controls are really part of the PCIe bridge
> (and only for it).
>
Thanks for pointing that out, sorry for the delayed response.
For the PCIe IP/Core reset we are working on it to handle
PCIe IP reset through 'resets' DT binding.
Regarding CPM5NC firewall control, we are discussing the
possibility of handling firewall in Firmware.
> >
> > Also, incorporate `reset-gpios` in example for GPIO-based handling of
> > the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
> > control.
>
> "Also" is a red flag for that change should be a separate commit.
>
Sure, I will correct the commit message.
> >
> > The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> > CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
> > `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
> > proper functionality.
> >
> > Include an example DTS node and complete the binding documentation for
> > CPM5NC. Also, fix the bridge register address size in the example for
> > CPM5.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@....com>
> > ---
> > Changes for v7:
> > - Update CPM5NC device tree binding.
> > - Add CPM5NC device tree example node.
> > - Update commit message.
> >
> > 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 | 129 +++++++++++++++---
> > 1 file changed, 109 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > index d674a24c8ccc..ed07896f763e 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@....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
> > + maxItems: 4
> >
> > reg-names:
> > - items:
> > - - const: cpm_slcr
> > - - const: cfg
> > - - const: cpm_csr
> > minItems: 2
> > + maxItems: 4
> >
> > interrupts:
> > maxItems: 1
> > @@ -64,18 +55,94 @@ properties:
> > required:
> > - reg
> > - reg-names
> > - - "#interrupt-cells"
> > - - interrupts
> > - - interrupt-map
> > - - interrupt-map-mask
> > - bus-range
> > - 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.
> > + minItems: 2
> > + reg-names:
> > + items:
> > + - const: cpm_slcr
> > + - const: cfg
> > + - const: cpm_crx
>
> The xlnx,versal-cpm-host-1.00 no longer has cpm_csr registers? Where
> did they go? This is an ABI issue.
>
Yes, xlnx,versal-cpm-host-1.00 doesn't have cpm_csr registers.
> > + minItems: 2
> > + required:
> > + - "#interrupt-cells"
> > + - interrupts
> > + - interrupt-map
> > + - interrupt-map-mask
> > + - interrupt-controller
> > + - 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.
> > + minItems: 3
> > + reg-names:
> > + items:
> > + - const: cpm_slcr
> > + - const: cfg
> > + - const: cpm_csr
> > + - const: cpm_crx
> > + minItems: 3
> > + required:
> > + - "#interrupt-cells"
> > + - interrupts
> > + - interrupt-map
> > + - interrupt-map-mask
> > + - interrupt-controller
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - xlnx,versal-cpm5nc-host
> > + 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.
> > + - description: CPM5NC Firewall attribute register.
>
> Just 1 register?
>
No, this register base contains other peripheral registers also.
> > + minItems: 2
> > + reg-names:
> > + items:
> > + - const: cpm_slcr
> > + - const: cfg
> > + - const: cpm_crx
> > + - const: cpm5nc_fw_attr
>
> The block name in the entry is redundant. Drop 'cpm5nc_'.
>
> > + minItems: 2
> >
> > unevaluatedProperties: false
> >
> > examples:
> > - |
> > + #include <dt-bindings/gpio/gpio.h>
> >
> > versal {
> > #address-cells = <2>;
Powered by blists - more mailing lists