| 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:
<SN7PR12MB72017A95DFE6F4B2EF09123B8B352@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Mon, 2 Dec 2024 08:23:45 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...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>,
"jingoohan1@...il.com" <jingoohan1@...il.com>, "Simek, Michal"
<michal.simek@....com>, "Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>
Subject: RE: [PATCH 1/2] dt-bindings: PCI: amd-mdb: Add YAML schemas for AMD
Versal2 MDB PCIe Root Port Bridge
Hi Krzysztof Kozlowski,
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@...nel.org>
> Sent: Thursday, November 28, 2024 12:59 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@....com>
> Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com;
> manivannan.sadhasivam@...aro.org; robh@...nel.org; krzk+dt@...nel.org;
> conor+dt@...nel.org; linux-pci@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; jingoohan1@...il.com; Simek, Michal
> <michal.simek@....com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@....com>
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: amd-mdb: Add YAML schemas for AMD
> Versal2 MDB PCIe Root Port Bridge
>
> On Wed, Nov 27, 2024 at 05:28:03PM +0530, Thippeswamy Havalige wrote:
> > Add YAML dtschemas of AMD Versal2 MDB (Multimedia DMA Bridge) PCIe Root
> > Port Bridge dt binding.
>
> A nit, subject: drop second/last, redundant "YAML schemas for". The
> "dt-bindings" prefix is already stating that these are schemas, cannot
> be anything else.
> See also:
> https://elixir.bootlin.com/linux/v6.7-
> rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Thanks for review, ll update in next patch.
>
>
> >
> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@....com>
> > ---
> > .../devicetree/bindings/pci/amd,mdb-pcie.yaml | 132 ++++++++++++++++++
> > 1 file changed, 132 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pci/amd,mdb-pcie.yaml
>
> Nope, use compatible as filename.
Thanks for review, ll update in next patch.
>
> >
> > diff --git a/Documentation/devicetree/bindings/pci/amd,mdb-pcie.yaml
> b/Documentation/devicetree/bindings/pci/amd,mdb-pcie.yaml
> > new file mode 100644
> > index 000000000000..ad9e447e87f2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/amd,mdb-pcie.yaml
> > @@ -0,0 +1,132 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/amd,mdb-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMD versal2 MDB(Multimedia DMA Bridge) Host Controller device tree
>
> Drop "device tree". This is about hardware. Also, "versal2" or
> "Versal2"? Just keep *consistent* in all AMD patchsets.
Thanks for review, ll update in next patch.
>
> > +
> > +maintainers:
> > + - Thippeswamy Havalige <thippeswamy.havalige@....com>
> > +
> > +properties:
> > + compatible:
> > + const: amd,versal2-mdb-host
> > +
> > + reg:
> > + items:
> > + - description: MDB PCIe controller 0 SLCR
> > + - description: configuration region
> > + - description: data bus interface
> > + - description: address translation unit register
> > +
> > + reg-names:
> > + items:
> > + - const: mdb_pcie_slcr
> > + - const: config
> > + - const: dbi
> > + - const: atu
> > +
> > + ranges:
> > + maxItems: 2
> > +
> > + msi-map:
> > + maxItems: 1
> > +
> > + bus-range:
> > + maxItems: 1
> > +
> > + "#address-cells":
> > + const: 3
> > +
> > + "#size-cells":
> > + const: 2
> > +
> > + device_type:
> > + const: pci
>
> I think you miss referencing schema. Why standard PCI properties are
> here?
Thanks for review, ll update in next patch.
>
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + interrupt-map-mask:
> > + items:
> > + - const: 0
> > + - const: 0
> > + - const: 0
> > + - const: 7
> > +
> > + interrupt-map:
> > + maxItems: 4
> > +
> > + "#interrupt-cells":
> > + const: 1
> > +
> > + interrupt-controller:
> > + description: Interrupt controller node for handling legacy PCI interrupts.
>
> Why the legacy is needed? This is a new binding and new device.
Thanks for review, ll update in next patch.
>
> > + type: object
> > + properties:
> > + interrupt-controller: true
> > +
> > + "#address-cells":
> > + const: 0
> > +
> > + "#interrupt-cells":
> > + const: 1
> > +
> > + required:
> > + - interrupt-controller
> > + - "#address-cells"
> > + - "#interrupt-cells"
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - reg
> > + - reg-names
> > + - interrupts
> > + - interrupt-map
> > + - interrupt-map-mask
> > + - msi-map
> > + - ranges
> > + - "#interrupt-cells"
> > + - interrupt-controller
> > +
> > +unevaluatedProperties: false
>
> You do not have any $ref, so this would not be correct, but OTOH this
> points exactly to missing $ref.
Thanks for review, ll update in next patch.
>
> > +
> > +examples:
> > +
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + pci@...31000 {
> > + compatible = "amd,versal2-mdb-host";
> > + reg = <0x0 0xed931000 0x0 0x2000>,
> > + <0x1000 0x100000 0x0 0xff00000>,
> > + <0x1000 0x0 0x0 0x100000>,
> > + <0x0 0xed860000 0x0 0x2000>;
> > + reg-names = "mdb_pcie_slcr", "config", "dbi", "atu";
> > + ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00
> 0x10000000>,
> > + <0x43000000 0x1100 0x00 0x1100 0x00 0x00 0x1000000>;
> > + interrupts = <0 198 4>;
>
> You included headers so use them.
>
> > + interrupt-parent = <&gic>;
> > + interrupt-map-mask = <0 0 0 7>;
> > + interrupt-map = <0 0 0 1 &pcie_intc_0 0>,
> > + <0 0 0 2 &pcie_intc_0 1>,
> > + <0 0 0 3 &pcie_intc_0 2>,
> > + <0 0 0 4 &pcie_intc_0 3>;
> > + msi-map = <0x0 &gic_its 0x00 0x10000>;
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + #interrupt-cells = <1>;
> > + device_type = "pci";
> > + pcie_intc_0: interrupt-controller {
> > + #address-cells = <0>;
>
> Messed indentation.
Thanks for review, ll update in next patch.
>
> Best regards,
> Krzysztof
Regards,
Thippeswamy H
Powered by blists - more mailing lists