lkml.org 
[lkml]   [2022]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [RFC PATCH v2 1/6] Documentation: DT: Add entry for CDX controller
    Date
    [AMD Official Use Only - General]



    > -----Original Message-----
    > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
    > Sent: Thursday, August 18, 2022 3:24 PM
    > To: Gupta, Nipun <Nipun.Gupta@amd.com>; robh+dt@kernel.org;
    > krzysztof.kozlowski+dt@linaro.org; gregkh@linuxfoundation.org;
    > rafael@kernel.org; eric.auger@redhat.com; alex.williamson@redhat.com;
    > cohuck@redhat.com; Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>;
    > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org;
    > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com;
    > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com;
    > jgg@ziepe.ca; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
    > kvm@vger.kernel.org
    > Cc: okaya@kernel.org; Anand, Harpreet <harpreet.anand@amd.com>; Agarwal,
    > Nikhil <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>;
    > git (AMD-Xilinx) <git@amd.com>
    > Subject: Re: [RFC PATCH v2 1/6] Documentation: DT: Add entry for CDX
    > controller
    >
    > [CAUTION: External Email]
    >
    > On 17/08/2022 18:05, Nipun Gupta wrote:
    > > This patch adds a devicetree binding documentation for CDX
    > > controller.
    > >
    > Does not look like you tested the bindings. Please run `make
    > dt_binding_check` (see
    > Documentation/devicetree/bindings/writing-schema.rst for instructions).
    >

    Thanks for the detailed review. Will fix the issues observed in v3.

    > > CDX bus controller dynamically detects CDX bus and the
    > > devices on these bus using CDX firmware.
    > >
    > > Signed-off-by: Nipun Gupta <mailto:nipun.gupta@amd.com>
    >
    > Use subject perfixes matching the subsystem (git log --oneline -- ...).

    Agree, will update.

    >
    > > ---
    > > .../devicetree/bindings/bus/xlnx,cdx.yaml | 108 ++++++++++++++++++
    > > MAINTAINERS | 6 +
    > > 2 files changed, 114 insertions(+)
    > > create mode 100644 Documentation/devicetree/bindings/bus/xlnx,cdx.yaml
    > >
    > > diff --git a/Documentation/devicetree/bindings/bus/xlnx,cdx.yaml
    > b/Documentation/devicetree/bindings/bus/xlnx,cdx.yaml
    > > new file mode 100644
    > > index 000000000000..4247a1cff3c1
    > > --- /dev/null
    > > +++ b/Documentation/devicetree/bindings/bus/xlnx,cdx.yaml
    > > @@ -0,0 +1,108 @@
    > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
    > > +%YAML 1.2
    > > +---
    > > +$id:
    > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmisc%2Fxlnx%2Ccdx.yaml%23&amp;data=05%7C01%7Cnipun.gupta%40amd.com%7C36ea349b1b464c0de27208da80ffa39e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637964132708706641%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=2cB6xGI3%2Brd%2BKXvvoZ7bDQvIAjIc7djKatDrJcuLJIg%3D&amp;reserved=0
    > > +$schema:
    > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cnipun.gupta%40amd.com%7C36ea349b1b464c0de27208da80ffa39e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637964132708706641%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=DfEspCt84z77me2ShufHrL%2FK1X87p65XnbmVVr2xDrM%3D&amp;reserved=0
    > > +
    > > +title: Xilinx CDX bus controller
    > > +
    > > +description: |
    > > + CDX bus controller for Xilinx devices is implemented to
    >
    > You need to describe what is this CDX bus. Google says nothing...

    We will be adding more Arch related details in the cover letter patch to
    describe the CDX bus.

    >
    > > + dynamically detect CDX bus and devices on these bus using the
    > > + firmware. The CDX bus manages multiple FPGA based hardware
    > > + devices, which can support network, crypto or any other specialized
    > > + type of device. These FPGA based devices can be added/modified
    > > + dynamically on run-time.
    > > +
    > > + All devices on the CDX bus will have a unique streamid (for IOMMU)
    > > + and a unique device ID (for MSI) corresponding to a requestor ID
    > > + (one to one associated with the device). The streamid and deviceid
    > > + are used to configure SMMU and GIC-ITS respectively.
    > > +
    > > + iommu-map property is used to define the set of stream ids
    > > + corresponding to each device and the associated IOMMU.
    > > +
    > > + For generic IOMMU bindings, see:
    > > + Documentation/devicetree/bindings/iommu/iommu.txt.
    >
    > Drop sentence.

    Agree

    >
    > > +
    > > + For arm-smmu binding, see:
    > > + Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
    >
    > Drop sentence.

    Agree

    >
    > > +
    > > + The MSI writes are accompanied by sideband data (Device ID).
    > > + The msi-map property is used to associate the devices with the
    > > + device ID as well as the associated ITS controller.
    > > +
    > > + For generic MSI bindings, see:
    > > + Documentation/devicetree/bindings/interrupt-controller/msi.txt.
    >
    > Drop sentence.

    Agree

    >
    > > +
    > > + For GICv3 and GIC ITS bindings, see:
    > > + Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
    >
    > Drop sentence.

    Agree

    >
    > > +
    > > +maintainers:
    > > + - Nipun Gupta <mailto:nipun.gupta@amd.com>
    > > + - Nikhil Agarwal <mailto:nikhil.agarwal@amd.com>
    > > +
    > > +properties:
    > > + compatible:
    > > + const: "xlnx,cdxbus-controller-1.0"
    >
    > No quotes.

    Agree. Will update in v3

    >
    > > +
    > > + reg:
    > > + description: |
    > > + specifies the CDX firmware region shared memory accessible by the
    > > + ARM cores.
    >
    > You need to describe the items instead (e.g. maxItems:1).

    Will be updating in v3

    >
    > > +
    > > + iommu-map:
    > > + description: |
    > > + Maps device Requestor ID to a stream ID and associated IOMMU. The
    > > + property is an arbitrary number of tuples of
    > > + (rid-base,iommu,streamid-base,length).
    > > +
    > > + Any Requestor ID i in the interval [rid-base, rid-base + length) is
    > > + associated with the listed IOMMU, with the iommu-specifier
    > > + (i - streamid-base + streamid-base).
    >
    > You need type and constraints.

    Agree.

    >
    > > +
    > > + msi-map:
    > > + description:
    > > + Maps an Requestor ID to a GIC ITS and associated msi-specifier
    > > + data (device ID). The property is an arbitrary number of tuples of
    > > + (rid-base,gic-its,deviceid-base,length).
    > > +
    > > + Any Requestor ID in the interval [rid-base, rid-base + length) is
    > > + associated with the listed GIC ITS, with the msi-specifier
    > > + (i - rid-base + deviceid-base).
    >
    > You need type and constraints.
    >
    >
    > > +
    > > +required:
    > > + - compatible
    > > + - reg
    > > + - iommu-map
    > > + - msi-map
    > > +
    > > +additionalProperties: false
    > > +
    > > +examples:
    > > + - |
    > > + smmu@ec000000 {
    > > + compatible = "arm,smmu-v3";
    > > + #iommu-cells = <1>;
    > > + ...
    >
    > ???

    Will be fixed in v3

    >
    > > +
    > > + gic@e2000000 {
    > > + compatible = "arm,gic-v3";
    > > + interrupt-controller;
    > > + ...
    > > + its: gic-its@e2040000 {
    > > + compatible = "arm,gic-v3-its";
    > > + msi-controller;
    > > + ...
    > > + }
    > > + };
    > > +
    > > + cdxbus: cdxbus@@4000000 {
    >
    > Node names should be generic, so "cdx"

    Would be using bus: cdxbus@4000000.
    Kindly correct me if this does not seem to be correct.

    Thanks,
    Nipun

    >
    > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-names-recommendation&amp;data=05%7C01%7Cnipun.gupta%40amd.com%7C36ea349b1b464c0de27208da80ffa39e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637964132708706641%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=QN8hk1CEOSmNSV5f0Z2uL4hatFrc1xYC5JBbptcCISA%3D&amp;reserved=0
    >
    > Drop the label.
    >
    >
    > > + compatible = "xlnx,cdxbus-controller-1.0";
    > > + reg = <0x00000000 0x04000000 0 0x1000>;
    > > + /* define map for RIDs 250-259 */
    > > + iommu-map = <250 &smmu 250 10>;
    > > + /* define msi map for RIDs 250-259 */
    > > + msi-map = <250 &its 250 10>;
    > > + };
    > Best regards,
    > Krzysztof

    \
     
     \ /
      Last update: 2022-09-05 16:06    [W:4.118 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site