Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1629876516-16513-1-git-send-email-shengjiu.wang@nxp.com> <YTDq/kWFPLHUnHMN@robh.at.kernel.org>
In-Reply-To: <YTDq/kWFPLHUnHMN@robh.at.kernel.org>
From:   Shengjiu Wang <shengjiu.wang@gmail.com>
Date:   Fri, 3 Sep 2021 15:54:39 +0800
Message-ID: <CAA+D8AOPRQRPtafZ2yryP8pn7=Foaj6ctehpZ9S_c1YQ3kUL9Q@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: remoteproc: Add fsl,imx-dsp-rproc
 binding document
To:     Rob Herring <robh@kernel.org>
Cc:     Shengjiu Wang <shengjiu.wang@nxp.com>,
        Ohad Ben Cohen <ohad@wizery.com>, bjorn.andersson@linaro.org,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Sascha Hauer <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-remoteproc@vger.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Rob

On Thu, Sep 2, 2021 at 11:17 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Aug 25, 2021 at 03:28:35PM +0800, Shengjiu Wang wrote:
> > Define the compatible string and properties needed by imx_dsp_rproc
> > driver.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > changes in v2:
> > - add items for clock-names
> > - change syscon to fsl,dsp-ctrl
> >
> >  .../remoteproc/fsl,imx-dsp-rproc.yaml         | 150 ++++++++++++++++++
> >  1 file changed, 150 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/fsl,imx-dsp-rproc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-dsp-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-dsp-rproc.yaml
> > new file mode 100644
> > index 000000000000..edf6e4b8d7bb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-dsp-rproc.yaml
> > @@ -0,0 +1,150 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/fsl,imx-dsp-rproc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: i.MX DSP Remoteproc Devices
> > +
> > +maintainers:
> > +  - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +description:
> > +  This binding provides support for DSP processors found on i.mX family of SoCs
>
> i.MX

I will update it.

>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,imx8qxp-hifi4
> > +      - fsl,imx8qm-hifi4
> > +      - fsl,imx8mp-hifi4
> > +      - fsl,imx8ulp-hifi4
> > +
> > +  clocks:
> > +    description:
> > +      Main functional clock for the remote processor
> > +    minItems: 1
> > +    maxItems: 32
> > +
> > +  clock-names:
> > +    description: |
> > +      List of clock names for the remote processor.
> > +      dsp_clkx for clocks of dsp itself.
> > +      per_clkx for clocks of peripherals used by dsp.
>
> I still don't like your random collection of clocks. What if you have
> other resources besides clocks to manage? Pin mux, resets, power
> domains, etc. It would be better if you describe the peripherals in DT
> and then link to them here. But maybe the devices themselves aren't
> visible?

Ok,  I will remove these per_clks in next version.
Maybe it is better to handle them in firmware side.

>
> > +    minItems: 1
> > +    maxItems: 26
> > +    items:
> > +      - const: dsp_clk1
> > +      - const: dsp_clk2
> > +      - const: dsp_clk3
> > +      - const: dsp_clk4
> > +      - const: dsp_clk5
> > +      - const: dsp_clk6
> > +      - const: dsp_clk7
> > +      - const: dsp_clk8
>
> Surely you can name these by function? e.g. core, bus, axi, etc.

Ok, I will update them in next version.

Thanks for reviewing.

Best regards
Wang shengjiu

>
> > +      - const: per_clk1
> > +      - const: per_clk2
> > +      - const: per_clk3
> > +      - const: per_clk4
> > +      - const: per_clk5
> > +      - const: per_clk6
> > +      - const: per_clk7
> > +      - const: per_clk8
> > +      - const: per_clk9
> > +      - const: per_clk10
> > +      - const: per_clk11
> > +      - const: per_clk12
> > +      - const: per_clk13
> > +      - const: per_clk14
> > +      - const: per_clk15
> > +      - const: per_clk16
> > +      - const: per_clk17
> > +      - const: per_clk18
> > +
> > +  fsl,dsp-ctrl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to syscon block which provide access for processor enablement
> > +
> > +  mbox-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +      - const: rxdb
> > +
> > +  mboxes:
> > +    description:
> > +      This property is required only if the rpmsg/virtio functionality is used.
> > +      List of <&phandle type channel> - 1 channel for TX, 1 channel for RX, 1 channel for RXDB.
> > +      (see mailbox/fsl,mu.yaml)
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  firmware-name:
> > +    description: |
> > +      Default name of the firmware to load to the remote processor.
> > +
> > +  memory-region:
> > +    description:
> > +      If present, a phandle for a reserved memory area that used for vdev buffer,
> > +      resource table, vring region and others used by remote processor.
> > +    minItems: 1
> > +    maxItems: 32
> > +
> > +  reg:
> > +    description: |
> > +      Address space for any remoteproc memories present on the SoC.
> > +
> > +  power-domains:
> > +    minItems: 1
> > +    maxItems: 32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - mboxes
> > +  - mbox-names
> > +  - clocks
> > +  - clock-names
> > +  - firmware-name
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8mp-clock.h>
> > +    dsp_reserved: dsp@92400000 {
> > +      reg = <0x92400000 0x1000000>;
> > +      no-map;
> > +    };
> > +    dsp_vdev0vring0: vdev0vring0@942f0000 {
> > +      reg = <0x942f0000 0x8000>;
> > +      no-map;
> > +    };
> > +    dsp_vdev0vring1: vdev0vring1@942f8000 {
> > +      reg = <0x942f8000 0x8000>;
> > +      no-map;
> > +    };
> > +    dsp_vdev0buffer: vdev0buffer@94300000 {
> > +      compatible = "shared-dma-pool";
> > +      reg = <0x94300000 0x100000>;
> > +      no-map;
> > +    };
> > +
> > +    dsp: dsp@3b6e8000 {
> > +      compatible = "fsl,imx8mp-hifi4";
> > +      reg = <0x3B6E8000 0x88000>;
> > +      clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_OCRAMA_IPG>,
> > +               <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_DSP_ROOT>,
> > +               <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_DSPDBG_ROOT>;
> > +      clock-names = "dsp_clk1", "dsp_clk2", "dsp_clk3";
> > +      firmware-name = "imx/dsp/hifi4.bin";
> > +      power-domains = <&audiomix_pd>;
> > +      mbox-names = "tx", "rx", "rxdb";
> > +      mboxes = <&mu2 0 0>,
> > +               <&mu2 1 0>,
> > +               <&mu2 3 0>;
> > +      memory-region = <&dsp_vdev0buffer>, <&dsp_vdev0vring0>,
> > +                      <&dsp_vdev0vring1>, <&dsp_reserved>;
> > +      fsl,dsp-ctrl = <&audio_blk_ctrl>;
> > +    };
> > --
> > 2.17.1
> >
> >