linux-kernel - Re: [PATCH v1 1/2] remoteproc: dt: Provide bindings for iMX6SX/7D Remote Processor Controller driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170710221408.GD1618@tuxbook>
Date:   Mon, 10 Jul 2017 15:14:08 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     devicetree@...r.kernel.org, kernel@...gutronix.de,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Shawn Guo <shawnguo@...nel.org>,
        Fabio Estevam <fabio.estevam@....com>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        linux-remoteproc@...r.kernel.org, linux@...pel-privat.de
Subject: Re: [PATCH v1 1/2] remoteproc: dt: Provide bindings for iMX6SX/7D
 Remote Processor Controller driver

On Mon 10 Jul 07:42 PDT 2017, Oleksij Rempel wrote:

> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
>  .../devicetree/bindings/remoteproc/imx-rproc.txt   | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> new file mode 100644
> index 000000000000..e7c61993e1b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> @@ -0,0 +1,44 @@
> +NXP iMX6SX/iMX7D Co-Processor Bindings
> +----------------------------------------
> +
> +This binding provides support for ARM Cortex M4 Co-processor found on some
> +NXP iMX SoCs.
> +
> +Required properties:
> +- compatible		Should be one of:
> +				"fsl,imx7d-rproc"
> +				"fsl,imx6sx-rproc"
> +- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
> +- syscfg		Phandle to syscon block which provide access to

This is called "syscon" in the code and the example.

> +			System Reset Controller
> +
> +Optional properties:
> +- reg:			Should contain the address ranges for some of internal
> +			memory regions.

Something less hand-wavy, like: "Should list the memory regions for the
remoteproc"

> +- reg-names:		Contains the corresponding names for the memory
> +			regions. These should be named "ddr", "ocram", "ocram-s",
> +			"ocram-epdc" or "ocram-pxp".

Make this comment define which memory regions are required for each of
the systems.

> +Fallowing memory ranges are expected:
> +- For "fsl,imx7d-rproc"
> +	<0x00900000 0x00020000> - "ocram"
> +	<0x00920000 0x00020000> - "ocram-epdc"
> +	<0x00940000 0x00008000> - "ocram-pxp"
> +	<0x80000000 0x0FFF0000> - "ddr" (code area)
> +	<0x80000000 0x60000000> - "ddr" (data area)

There's no reason to list the actual regions in the binding
document. Just list the requires regions for each system.

> +- For "fsl,imx6sx-rproc"
> +	<0x008F8000 0x00004000> - "ocram-s"
> +	<0x80000000 0x0FFF8000> - "ddr" (code area)
> +	<0x80000000 0x60000000> - "ddr" (data area)
> +
> +Note: the "ddr" code and data ranges are overlapping. Code area is smaller
> +than data area.  So this range should be carefully chosen according to your
> +system and application requirements.
> +

This is a source of future issues as this indicates that I should have
reg-names list "ddr" twice.

Also, as you warn about the user needing to pick the values for "ddr",
does that mean that it's a carveout of System RAM?

> +Example:
> +	imx_rproc: imx7d-rp0@0 {

imx7d-rproc@...00000 {

And there's currently no reason to label this node.

> +		compatible	= "fsl,imx7d-rproc";
> +		reg		= <0x80000000 0x80000>, <0x00900000 0x2000>;
> +		reg-names	= "ddr", "ocram";
> +		syscon		= <&src>;
> +		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
> +	};

Regards,
Bjorn