[v2,2/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

Message ID 1440757911-9120-3-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones Aug. 28, 2015, 10:31 a.m.
Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt

Comments

Rob Herring Aug. 31, 2015, 3:28 p.m. | #1
On Fri, Aug 28, 2015 at 5:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> new file mode 100644
> index 0000000..fbd7d78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> @@ -0,0 +1,35 @@
> +STMicroelectronics Remote Processor

"remote processor" is what STM datasheets call them? This seems like
Linux naming is leaking in here.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Griffin Sept. 1, 2015, 8:58 a.m. | #2
Hi Lee,

On Fri, 28 Aug 2015, Lee Jones wrote:

> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt

The patch documening the DT bindings should be ordered before the patch which adds
the DT node to aid reviewing.
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> new file mode 100644
> index 0000000..fbd7d78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> @@ -0,0 +1,35 @@
> +STMicroelectronics Remote Processor
> +-----------------------------------
> +
> +This binding provides support for adjunct processors found on ST SoCs.
> +
> +The remote processors can be controlled from the bootloader or the primary OS.
> +If the bootloader starts a remote processor processor the primary OS must detect
> +its state and act accordingly.
> +
> +Required properties:
> +- compatible		Should be one of:
> +				"st,st231-rproc"
> +				"st,st40-rproc"

st40-proc isn't used anywhere. The stih407 doesn't have a ST40 copro, and
looking in the vendor tree remoteproc support isn't present for stih415/6 which
are the the only upstream SoC's to have a ST40 co-pro.

So I think st40-rproc support can be removed.

> +- reg			Size and length of reserved co-processor memory
> +- resets		Reset lines (See: ../reset/reset.txt)
> +- reset-names		Must be "sw_reset" and "pwr_reset"

pwr_reset isn't used by any of the st231 co-processors. It seems to
be related to ST40 support which I don't think is required upstream.
Removing it would make the driver a fair bit smaller.

> +- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
> +- clock-names		Must be "rproc_clk"

I can't see any co-pro which uses more than one clock, so clock-names looks
superflous.

> +- clock-frequency	Clock frequency to set co-processor at if the bootloader
> +			hasn't already done so
> +- st,syscfg-boot	The register that holds the boot vector for the co-processor

I would prefer to see this binding match how most other sti drivers reference syscfg
registers which is: -

st,syscfg       = <&syscfg_core 0xf4>;

Description: phandle of sysconfig bank plus integer array containing register offsets.

It also means it is easily extendable if more than one syscfg register is
required in the future to boot a co-pro.

regards,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Lee Jones Sept. 1, 2015, 9:14 a.m. | #3
On Tue, 01 Sep 2015, Peter Griffin wrote:

> Hi Lee,
> 
> On Fri, 28 Aug 2015, Lee Jones wrote:
> 
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> 
> The patch documening the DT bindings should be ordered before the patch which adds
> the DT node to aid reviewing.
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > new file mode 100644
> > index 0000000..fbd7d78
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > @@ -0,0 +1,35 @@
> > +STMicroelectronics Remote Processor
> > +-----------------------------------
> > +
> > +This binding provides support for adjunct processors found on ST SoCs.
> > +
> > +The remote processors can be controlled from the bootloader or the primary OS.
> > +If the bootloader starts a remote processor processor the primary OS must detect
> > +its state and act accordingly.
> > +
> > +Required properties:
> > +- compatible		Should be one of:
> > +				"st,st231-rproc"
> > +				"st,st40-rproc"
> 
> st40-proc isn't used anywhere. The stih407 doesn't have a ST40 copro, and
> looking in the vendor tree remoteproc support isn't present for stih415/6 which
> are the the only upstream SoC's to have a ST40 co-pro.
> 
> So I think st40-rproc support can be removed.
> 
> > +- reg			Size and length of reserved co-processor memory
> > +- resets		Reset lines (See: ../reset/reset.txt)
> > +- reset-names		Must be "sw_reset" and "pwr_reset"
> 
> pwr_reset isn't used by any of the st231 co-processors. It seems to
> be related to ST40 support which I don't think is required upstream.
> Removing it would make the driver a fair bit smaller.
> 
> > +- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
> > +- clock-names		Must be "rproc_clk"
> 
> I can't see any co-pro which uses more than one clock, so clock-names looks
> superflous.
> 
> > +- clock-frequency	Clock frequency to set co-processor at if the bootloader
> > +			hasn't already done so
> > +- st,syscfg-boot	The register that holds the boot vector for the co-processor
> 
> I would prefer to see this binding match how most other sti drivers reference syscfg
> registers which is: -
> 
> st,syscfg       = <&syscfg_core 0xf4>;
> 
> Description: phandle of sysconfig bank plus integer array containing register offsets.
> 
> It also means it is easily extendable if more than one syscfg register is
> required in the future to boot a co-pro.

Ack.  Good points, will fix.
Lee Jones Sept. 1, 2015, 10:41 a.m. | #4
On Mon, 31 Aug 2015, Rob Herring wrote:

> On Fri, Aug 28, 2015 at 5:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > new file mode 100644
> > index 0000000..fbd7d78
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > @@ -0,0 +1,35 @@
> > +STMicroelectronics Remote Processor
> 
> "remote processor" is what STM datasheets call them? This seems like
> Linux naming is leaking in here.

That's what this directory is called and the other binding within it.
Rob Herring Sept. 1, 2015, 11:49 a.m. | #5
On Tue, Sep 1, 2015 at 5:41 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 31 Aug 2015, Rob Herring wrote:
>
>> On Fri, Aug 28, 2015 at 5:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> > ---
>> >  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
>> >  1 file changed, 35 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
>> > new file mode 100644
>> > index 0000000..fbd7d78
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
>> > @@ -0,0 +1,35 @@
>> > +STMicroelectronics Remote Processor
>>
>> "remote processor" is what STM datasheets call them? This seems like
>> Linux naming is leaking in here.
>
> That's what this directory is called and the other binding within it.

That doesn't make it right or answer my question. Certainly our
directory structure too closely mirrors the kernel.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Sept. 1, 2015, 12:54 p.m. | #6
Keeping interested parties in the loop.

Peter and I had a discussion with ST.  Here is the result.

On Tue, 01 Sep 2015, Peter Griffin wrote:
> On Fri, 28 Aug 2015, Lee Jones wrote:
> 
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/remoteproc/st-rproc.txt    | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> 
> The patch documening the DT bindings should be ordered before the patch which adds
> the DT node to aid reviewing.

I've always thought this was a silly rule, but I will re-order
nonetheless.

> > diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > new file mode 100644
> > index 0000000..fbd7d78
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > @@ -0,0 +1,35 @@
> > +STMicroelectronics Remote Processor
> > +-----------------------------------
> > +
> > +This binding provides support for adjunct processors found on ST SoCs.
> > +
> > +The remote processors can be controlled from the bootloader or the primary OS.
> > +If the bootloader starts a remote processor processor the primary OS must detect
> > +its state and act accordingly.
> > +
> > +Required properties:
> > +- compatible		Should be one of:
> > +				"st,st231-rproc"
> > +				"st,st40-rproc"
> 
> st40-proc isn't used anywhere. The stih407 doesn't have a ST40 copro, and
> looking in the vendor tree remoteproc support isn't present for stih415/6 which
> are the the only upstream SoC's to have a ST40 co-pro.
> 
> So I think st40-rproc support can be removed.

Agreed that the current state of platforms present in Mainline do not
warrant support for more than one type of co-processor; however,
platforms exist which are either not going upstream, or aren't
upstreamable *yet*, which will require support for a) different types
of co-processors and/or b) the capability to configure which reset
lines, if any, are present.

I'm keen to keep this option for two reasons.  Firstly, it allows ST
to use the driver in Mainline for existing platforms and secondly,
this driver will not have to be heavily modified when new platforms
are added.

> > +- reg			Size and length of reserved co-processor memory
> > +- resets		Reset lines (See: ../reset/reset.txt)
> > +- reset-names		Must be "sw_reset" and "pwr_reset"
> 
> pwr_reset isn't used by any of the st231 co-processors. It seems to
> be related to ST40 support which I don't think is required upstream.
> Removing it would make the driver a fair bit smaller.
> 
> > +- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
> > +- clock-names		Must be "rproc_clk"
> 
> I can't see any co-pro which uses more than one clock, so clock-names looks
> superflous.

Sounds reasonable.  I will remove it.

> > +- clock-frequency	Clock frequency to set co-processor at if the bootloader
> > +			hasn't already done so
> > +- st,syscfg-boot	The register that holds the boot vector for the co-processor
> 
> I would prefer to see this binding match how most other sti drivers reference syscfg
> registers which is: -
> 
> st,syscfg       = <&syscfg_core 0xf4>;

Agreed.  Will change.

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
new file mode 100644
index 0000000..fbd7d78
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
@@ -0,0 +1,35 @@ 
+STMicroelectronics Remote Processor
+-----------------------------------
+
+This binding provides support for adjunct processors found on ST SoCs.
+
+The remote processors can be controlled from the bootloader or the primary OS.
+If the bootloader starts a remote processor processor the primary OS must detect
+its state and act accordingly.
+
+Required properties:
+- compatible		Should be one of:
+				"st,st231-rproc"
+				"st,st40-rproc"
+- reg			Size and length of reserved co-processor memory
+- resets		Reset lines (See: ../reset/reset.txt)
+- reset-names		Must be "sw_reset" and "pwr_reset"
+- clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
+- clock-names		Must be "rproc_clk"
+- clock-frequency	Clock frequency to set co-processor at if the bootloader
+			hasn't already done so
+- st,syscfg-boot	The register that holds the boot vector for the co-processor
+
+Example:
+
+	st231-audio@bae00000 {
+		compatible	= "st,st231-rproc";
+		reg		= <0xbae00000 0x01000000>;
+		reg-names	= "rproc_mem";
+		resets		= <&softreset STIH407_ST231_AUD_SOFTRESET>;
+		reset-names	= "sw_reset";
+		clocks		= <&clk_s_c0_flexgen CLK_ST231_AUD_0>;
+		clock-names	= "rproc_clk";
+		clock-frequency	= <600000000>;
+		st,syscfg-boot	= <&syscfg_core 0x228>;
+	};