[PATCH v2 0/4] remoteproc: Add driver for STMicroelectronics platforms

[PATCH v2 0/4] remoteproc: Add driver for STMicroelectronics platforms

[Lee Jones]

ST's platforms often have multiple co-processors (usually ST40s or ST231s)
on-board.  This provides the Linux-side infrastructure to flash and boot
them successfully.
 
This set has been tested on an STiH410-B2120.

v1 = v2:
 - Remove Linux implementation specific comment from binding document
 - Force debugfs '0' to shutdown co-processor - rather than !1
 - Supply more detailed commit message
 - Propagate errors back from .stop()
 - Review GPL wording
 - Supply original author's SoBs

Lee Jones (4):
  ARM: STiH407: Add nodes for RemoteProc
  remoteproc: dt: Provide bindings for ST's Remote Processor Controller
    driver
  remoteproc: Supply controller driver for ST's Remote Processors
  remoteproc: debugfs: Add ability to boot remote processor using
    debugfs

 .../devicetree/bindings/remoteproc/st-rproc.txt    |  35 +++
 arch/arm/boot/dts/stih407-family.dtsi              |  48 ++++
 drivers/remoteproc/Kconfig                         |   9 +
 drivers/remoteproc/Makefile                        |   1 +
 drivers/remoteproc/remoteproc_debugfs.c            |  29 ++
 drivers/remoteproc/st_remoteproc.c                 | 301 +++++++++++++++++++++
 6 files changed, 423 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
 create mode 100644 drivers/remoteproc/st_remoteproc.c

-- 
1.9.1

[PATCH v2 1/4] ARM: STiH407: Add nodes for RemoteProc

[Lee Jones]

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 33daa49..89a363c 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -608,5 +608,53 @@
 			status		= "okay";
 			tx-only;
 		};
+
+		st231_gp0: st231-gp0@0 {
+			compatible = "st,st231-rproc";
+			reg = <0x40000000 0x01000000>;
+			reg-names = "carveout";
+			resets = <&softreset STIH407_ST231_GP0_SOFTRESET>;
+			reset-names = "sw_reset";
+			clocks = <&clk_s_c0_flexgen CLK_ST231_GP_0>;
+			clock-names = "rproc_clk";
+			clock-frequency = <600000000>;
+			st,syscfg-boot = <&syscfg_core 0x22C>;
+		};
+
+		st231_gp1: st231-gp1@0 {
+			compatible = "st,st231-rproc";
+			reg = <0x41000000 0x01000000>;
+			reg-names = "carveout";
+			resets = <&softreset STIH407_ST231_GP1_SOFTRESET>;
+			reset-names = "sw_reset";
+			clocks = <&clk_s_c0_flexgen CLK_ST231_GP_1>;
+			clock-names = "rproc_clk";
+			clock-frequency = <600000000>;
+			st,syscfg-boot = <&syscfg_core 0x220>;
+		};
+
+		st231_audio: st231-audio@0 {
+			compatible = "st,st231-rproc";
+			reg = <0x42000000 0x01000000>;
+			reg-names = "carveout";
+			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>;
+		};
+
+		st231_dmu: st231-dmu@0 {
+			compatible = "st,st231-rproc";
+			reg = <0x43000000 0x01000000>;
+			reg-names = "carveout";
+			resets = <&softreset STIH407_ST231_DMU_SOFTRESET>;
+			reset-names = "sw_reset";
+			clocks = <&clk_s_c0_flexgen CLK_ST231_DMU>;
+			clock-names = "rproc_clk";
+			clock-frequency = <600000000>;
+			st,syscfg-boot = <&syscfg_core 0x224>;
+		};
 	};
 };
-- 
1.9.1

Re: [STLinux Kernel] [PATCH v2 1/4] ARM: STiH407: Add nodes for RemoteProc

[Peter Griffin]

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>
> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 48 +++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index 33daa49..89a363c 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -608,5 +608,53 @@
>  			status		= "okay";
>  			tx-only;
>  		};
> +
> +		st231_gp0: st231-gp0@0 {
> +			compatible = "st,st231-rproc";
> +			reg = <0x40000000 0x01000000>;
> +			reg-names = "carveout";
> +			resets = <&softreset STIH407_ST231_GP0_SOFTRESET>;

Support for these co-processor reset lines does not exist upstream. So
this series as it stands won't compile.

regards,

Peter.

Re: [STLinux Kernel] [PATCH v2 1/4] ARM: STiH407: Add nodes for RemoteProc

[Lee Jones]

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>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 48 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index 33daa49..89a363c 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -608,5 +608,53 @@
> >  			status		= "okay";
> >  			tx-only;
> >  		};
> > +
> > +		st231_gp0: st231-gp0@0 {
> > +			compatible = "st,st231-rproc";
> > +			reg = <0x40000000 0x01000000>;
> > +			reg-names = "carveout";
> > +			resets = <&softreset STIH407_ST231_GP0_SOFTRESET>;
> 
> Support for these co-processor reset lines does not exist upstream. So
> this series as it stands won't compile.

That's true.  I have the support as part of some DEBUG patches I have
in my branch.  Are you aware of a plan for these, or should I upstream
them myself as part of this set?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH v2 1/4] ARM: STiH407: Add nodes for RemoteProc

[Peter Griffin]

Hi Lee,

On Tue, 01 Sep 2015, Lee Jones wrote:
> 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>
> > > ---
> > >  arch/arm/boot/dts/stih407-family.dtsi | 48 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > > index 33daa49..89a363c 100644
> > > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > > @@ -608,5 +608,53 @@
> > >  			status		= "okay";
> > >  			tx-only;
> > >  		};
> > > +
> > > +		st231_gp0: st231-gp0@0 {
> > > +			compatible = "st,st231-rproc";
> > > +			reg = <0x40000000 0x01000000>;
> > > +			reg-names = "carveout";
> > > +			resets = <&softreset STIH407_ST231_GP0_SOFTRESET>;
> > 
> > Support for these co-processor reset lines does not exist upstream. So
> > this series as it stands won't compile.
> 
> That's true.  I have the support as part of some DEBUG patches I have
> in my branch.  Are you aware of a plan for these, or should I upstream
> them myself as part of this set?

I'm not aware of any plan, so I think upstreaming as part of this set
would be good.

regards,

Peter.

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

[Lee Jones]

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
+-----------------------------------
+
+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>;
+	};
-- 
1.9.1

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

[Rob Herring]

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

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

[Lee Jones]

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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

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

Re: [STLinux Kernel] [PATCH v2 2/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

[Peter Griffin]

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.

Re: [STLinux Kernel] [PATCH v2 2/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

[Lee Jones]

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
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH v2 2/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

[Lee Jones]

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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Lee Jones]

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/Kconfig         |   9 ++
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/st_remoteproc.c | 301 +++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/remoteproc/st_remoteproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 28c711f..72e97d7 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
 	  It's safe to say n here if you're not interested in multimedia
 	  offloading.
 
+config ST_REMOTEPROC
+	tristate "ST remoteproc support"
+	depends on ARCH_STI
+	select REMOTEPROC
+	help
+	  Say y here to support ST's adjunct processors via the remote
+	  processor framework.
+	  This can be either built-in or a loadable module.
+
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 81b04d1..279cb2e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_STE_MODEM_RPROC)	 	+= ste_modem_rproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
+obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
diff --git a/drivers/remoteproc/st_remoteproc.c b/drivers/remoteproc/st_remoteproc.c
new file mode 100644
index 0000000..96b784d
--- /dev/null
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -0,0 +1,301 @@
+/*
+ * ST's Remote Processor Control Driver
+ *
+ * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
+ *
+ * Author: Ludovic Barre <ludovic.barre@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/remoteproc.h>
+#include <linux/reset.h>
+
+struct st_rproc_config {
+	bool			sw_reset;
+	bool			pwr_reset;
+	u32			bootaddr_mask;
+};
+
+struct st_rproc {
+	struct st_rproc_config	*config;
+	struct reset_control	*sw_reset;
+	struct reset_control	*pwr_reset;
+	struct clk		*clk;
+	u32			clk_rate;
+	struct	regmap		*boot_base;
+	u32			boot_offset;
+};
+
+static int st_rproc_stop(struct rproc *rproc)
+{
+	struct st_rproc *st_rproc = rproc->priv;
+	int ret, err = 0;
+
+	if (st_rproc->config->sw_reset) {
+		ret = reset_control_assert(st_rproc->sw_reset);
+		if (ret)
+			dev_err(&rproc->dev, "Failed to assert S/W Reset\n");
+	}
+
+	if (st_rproc->config->pwr_reset) {
+		err = reset_control_assert(st_rproc->pwr_reset);
+		if (err)
+			dev_err(&rproc->dev, "Failed to assert Power Reset\n");
+	}
+
+	clk_disable(st_rproc->clk);
+
+	return ret ?: err;
+}
+
+static int st_rproc_start(struct rproc *rproc)
+{
+	struct st_rproc *st_rproc = rproc->priv;
+	int err;
+
+	regmap_update_bits(st_rproc->boot_base, st_rproc->boot_offset,
+			   st_rproc->config->bootaddr_mask, rproc->bootaddr);
+
+	err = clk_enable(st_rproc->clk);
+	if (err) {
+		dev_err(&rproc->dev, "Failed to enable clock\n");
+		return err;
+	}
+
+	if (st_rproc->config->sw_reset) {
+		err = reset_control_deassert(st_rproc->sw_reset);
+		if (err) {
+			dev_err(&rproc->dev, "Failed to deassert S/W Reset\n");
+			return err;
+		}
+	}
+
+	if (st_rproc->config->pwr_reset) {
+		err = reset_control_deassert(st_rproc->pwr_reset);
+		if (err) {
+			dev_err(&rproc->dev, "Failed to deassert Power Reset\n");
+			return err;
+		}
+	}
+
+	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
+
+	return 0;
+}
+
+static struct rproc_ops st_rproc_ops = {
+	.start		= st_rproc_start,
+	.stop		= st_rproc_stop,
+};
+
+/*
+ * Fetch state of the processor: 0 is off, 1 is on.
+ */
+static int st_rproc_state(struct st_rproc *st_rproc)
+{
+	int reset_sw, reset_pwr;
+
+	reset_sw = st_rproc->config->sw_reset ?
+		reset_control_status(st_rproc->sw_reset) : 0;
+
+	reset_pwr = st_rproc->config->pwr_reset ?
+		reset_control_status(st_rproc->pwr_reset) : 0;
+
+	if (reset_sw < 0 || reset_pwr < 0)
+		return -EINVAL;
+
+	return !reset_sw && !reset_pwr;
+}
+
+static const struct st_rproc_config st40_rproc_cfg = {
+	.sw_reset = true,
+	.pwr_reset = true,
+	.bootaddr_mask = GENMASK(28, 1),
+};
+
+static const struct st_rproc_config st231_rproc_cfg = {
+	.sw_reset = true,
+	.pwr_reset = false,
+	.bootaddr_mask = GENMASK(31, 6),
+};
+
+static struct of_device_id st_rproc_match[] = {
+	{ .compatible = "st,st40-rproc", .data = &st40_rproc_cfg },
+	{ .compatible = "st,st231-rproc", .data = &st231_rproc_cfg },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_rproc_match);
+
+static int st_rproc_parse_dt(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct st_rproc *st_rproc = rproc->priv;
+	struct device_node *np = dev->of_node;
+	struct resource	*mem;
+	int err;
+
+	if (st_rproc->config->sw_reset) {
+		st_rproc->sw_reset = devm_reset_control_get(dev, "sw_reset");
+		if (IS_ERR(st_rproc->sw_reset)) {
+			dev_err(dev, "Failed to get S/W Reset\n");
+			return PTR_ERR(st_rproc->sw_reset);
+		}
+	}
+
+	if (st_rproc->config->pwr_reset) {
+		st_rproc->pwr_reset = devm_reset_control_get(dev, "pwr_reset");
+		if (IS_ERR(st_rproc->pwr_reset)) {
+			dev_err(dev, "Failed to get Power Reset\n");
+			return PTR_ERR(st_rproc->pwr_reset);
+		}
+	}
+
+	st_rproc->clk = devm_clk_get(dev, "rproc_clk");
+	if (IS_ERR(st_rproc->clk)) {
+		dev_err(dev, "Failed to get clock\n");
+		return PTR_ERR(st_rproc->clk);
+	}
+
+	err = of_property_read_u32(np, "clock-frequency", &st_rproc->clk_rate);
+	if (err) {
+		dev_err(dev, "failed to get clock frequency\n");
+		return err;
+	}
+
+
+	st_rproc->boot_base =
+		syscon_regmap_lookup_by_phandle(np, "st,syscfg-boot");
+	if (!st_rproc->boot_base) {
+		dev_err(dev, "Boot base not found\n");
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32_index(np, "st,syscfg-boot", 1,
+					 &st_rproc->boot_offset);
+	if (err) {
+		dev_err(dev, "Boot offset not found\n");
+		return -EINVAL;
+	}
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "carveout");
+	if (!mem) {
+		dev_err(dev, "no rproc memory definition\n");
+		return -ENXIO;
+	}
+
+	if (!devm_request_mem_region(dev, mem->start,
+				     resource_size(mem), pdev->name)) {
+		dev_err(dev, "failed to get memory region resource\n");
+		return -EBUSY;
+	}
+	err = dmam_declare_coherent_memory(dev, mem->start, mem->start,
+					  resource_size(mem),
+					  DMA_MEMORY_MAP |
+					  DMA_MEMORY_EXCLUSIVE);
+	if (err < 0) {
+		dev_err(dev, "cannot declare coherent memory: %d\n", err);
+		return -ENXIO;
+	}
+
+	err = clk_prepare(st_rproc->clk);
+	if (err)
+		dev_err(dev, "failed to get clock\n");
+
+	return err;
+}
+
+static int st_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
+	struct st_rproc *st_rproc;
+	struct device_node *np = dev->of_node;
+	struct rproc *rproc;
+	int enabled;
+	int ret;
+
+	match = of_match_device((st_rproc_match), dev);
+	if (!match || !match->data) {
+		dev_err(dev, "No device match found\n");
+		return -ENODEV;
+	}
+
+	rproc = rproc_alloc(dev, np->name, &st_rproc_ops,
+			    NULL, sizeof(*st_rproc));
+	if (!rproc)
+		return -ENOMEM;
+
+	st_rproc = rproc->priv;
+	st_rproc->config = (struct st_rproc_config *)match->data;
+	platform_set_drvdata(pdev, rproc);
+
+	ret = st_rproc_parse_dt(pdev);
+	if (ret)
+		goto free_rproc;
+
+	ret = st_rproc_state(st_rproc);
+	if (ret < 0)
+		goto free_rproc;
+	enabled = ret;
+
+	if (enabled) {
+		atomic_inc(&rproc->power);
+		rproc->state = RPROC_RUNNING;
+	} else {
+		clk_set_rate(st_rproc->clk, st_rproc->clk_rate);
+	}
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto free_rproc;
+
+	return 0;
+
+free_rproc:
+	rproc_put(rproc);
+	return ret;
+}
+
+static int st_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct st_rproc *st_rproc = rproc->priv;
+
+	rproc_del(rproc);
+
+	clk_disable_unprepare(st_rproc->clk);
+
+	rproc_put(rproc);
+
+	return 0;
+}
+
+static struct platform_driver st_rproc_driver = {
+	.probe = st_rproc_probe,
+	.remove = st_rproc_remove,
+	.driver = {
+		.name = "st-rproc",
+		.of_match_table = of_match_ptr(st_rproc_match),
+	},
+};
+module_platform_driver(st_rproc_driver);
+
+MODULE_DESCRIPTION("ST Remote Processor Control Driver");
+MODULE_AUTHOR("Ludovic.barre <Ludovic.barre@st.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 28c711f..72e97d7 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
>  	  It's safe to say n here if you're not interested in multimedia
>  	  offloading.
>  
> +config ST_REMOTEPROC
> +	tristate "ST remoteproc support"
> +	depends on ARCH_STI
> +	select REMOTEPROC
> +	help
> +	  Say y here to support ST's adjunct processors via the remote
> +	  processor framework.
> +	  This can be either built-in or a loadable module.
> +

The code uses reset_control_* APIs, so this should depend on
RESET_CONTROLLER, no?

> +/*
> + * ST's Remote Processor Control Driver
> + *
> + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> + *
> + * Author: Ludovic Barre <ludovic.barre@st.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + */

OK, but:

> +MODULE_LICENSE("GPL v2");

These are not in agreement.  You want "GPL" for MODULE_LICENSE if you
intend v2 or later.


> +static int st_rproc_stop(struct rproc *rproc)
> +{
> +	struct st_rproc *st_rproc = rproc->priv;
> +	int ret, err = 0;
> +
> +	if (st_rproc->config->sw_reset) {
> +		ret = reset_control_assert(st_rproc->sw_reset);
> +		if (ret)
> +			dev_err(&rproc->dev, "Failed to assert S/W Reset\n");
> +	}
> +
> +	if (st_rproc->config->pwr_reset) {
> +		err = reset_control_assert(st_rproc->pwr_reset);
> +		if (err)
> +			dev_err(&rproc->dev, "Failed to assert Power Reset\n");
> +	}
> +
> +	clk_disable(st_rproc->clk);
> +
> +	return ret ?: err;
> +}

Sorry, but I think this is a stylistically inadequate response to my
earlier comments.  At least name the status variables sw_ret and pwr_ret
or something.  And it looks like ret could be used uninitialized.

Also, do you want to unconditionally call clk_disable even if you've
encountered errors?


> +static int st_rproc_start(struct rproc *rproc)
> +{
> +	struct st_rproc *st_rproc = rproc->priv;
> +	int err;
> +
> +	regmap_update_bits(st_rproc->boot_base, st_rproc->boot_offset,
> +			   st_rproc->config->bootaddr_mask, rproc->bootaddr);
> +
> +	err = clk_enable(st_rproc->clk);
> +	if (err) {
> +		dev_err(&rproc->dev, "Failed to enable clock\n");
> +		return err;
> +	}
> +
> +	if (st_rproc->config->sw_reset) {
> +		err = reset_control_deassert(st_rproc->sw_reset);
> +		if (err) {
> +			dev_err(&rproc->dev, "Failed to deassert S/W Reset\n");
> +			return err;
> +		}
> +	}
> +
> +	if (st_rproc->config->pwr_reset) {
> +		err = reset_control_deassert(st_rproc->pwr_reset);
> +		if (err) {
> +			dev_err(&rproc->dev, "Failed to deassert Power Reset\n");
> +			return err;
> +		}
> +	}
> +
> +	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
> +
> +	return 0;
> +}

Does this want to unwind any of its operations if it encounters a failure?

Re: [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Lee Jones]

On Fri, 28 Aug 2015, Nathan Lynch wrote:

> On 08/28/2015 05:31 AM, Lee Jones wrote:
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index 28c711f..72e97d7 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
> >  	  It's safe to say n here if you're not interested in multimedia
> >  	  offloading.
> >  
> > +config ST_REMOTEPROC
> > +	tristate "ST remoteproc support"
> > +	depends on ARCH_STI
> > +	select REMOTEPROC
> > +	help
> > +	  Say y here to support ST's adjunct processors via the remote
> > +	  processor framework.
> > +	  This can be either built-in or a loadable module.
> > +
> 
> The code uses reset_control_* APIs, so this should depend on
> RESET_CONTROLLER, no?

There's no need to explicitly depend on RESET_CONTROLLER.

With !RESET_CONTROLLER the user is WARN()ed about using the reset_*
API.

> > +/*
> > + * ST's Remote Processor Control Driver
> > + *
> > + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> > + *
> > + * Author: Ludovic Barre <ludovic.barre@st.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2, or (at your option)
> > + * any later version.
> > + */
> 
> OK, but:
> 
> > +MODULE_LICENSE("GPL v2");
> 
> These are not in agreement.  You want "GPL" for MODULE_LICENSE if you
> intend v2 or later.

Right, good spot.

I will clarify this with ST and make the necessary changes.

> > +static int st_rproc_stop(struct rproc *rproc)
> > +{
> > +	struct st_rproc *st_rproc = rproc->priv;
> > +	int ret, err = 0;
> > +
> > +	if (st_rproc->config->sw_reset) {
> > +		ret = reset_control_assert(st_rproc->sw_reset);
> > +		if (ret)
> > +			dev_err(&rproc->dev, "Failed to assert S/W Reset\n");
> > +	}
> > +
> > +	if (st_rproc->config->pwr_reset) {
> > +		err = reset_control_assert(st_rproc->pwr_reset);
> > +		if (err)
> > +			dev_err(&rproc->dev, "Failed to assert Power Reset\n");
> > +	}
> > +
> > +	clk_disable(st_rproc->clk);
> > +
> > +	return ret ?: err;
> > +}
> 
> Sorry, but I think this is a stylistically inadequate response to my
> earlier comments.  At least name the status variables sw_ret and pwr_ret
> or something.  And it looks like ret could be used uninitialized.
> 
> Also, do you want to unconditionally call clk_disable even if you've
> encountered errors?

Again this is something I need to clarify.  However, it doesn't strike
me as incorrect to gate the IP's clock just because the reset lines
haven't been successfully asserted.

Will check with the guys who know this IP and make suggested changes.

> > +static int st_rproc_start(struct rproc *rproc)
> > +{
> > +	struct st_rproc *st_rproc = rproc->priv;
> > +	int err;
> > +
> > +	regmap_update_bits(st_rproc->boot_base, st_rproc->boot_offset,
> > +			   st_rproc->config->bootaddr_mask, rproc->bootaddr);
> > +
> > +	err = clk_enable(st_rproc->clk);
> > +	if (err) {
> > +		dev_err(&rproc->dev, "Failed to enable clock\n");
> > +		return err;
> > +	}
> > +
> > +	if (st_rproc->config->sw_reset) {
> > +		err = reset_control_deassert(st_rproc->sw_reset);
> > +		if (err) {
> > +			dev_err(&rproc->dev, "Failed to deassert S/W Reset\n");
> > +			return err;
> > +		}
> > +	}
> > +
> > +	if (st_rproc->config->pwr_reset) {
> > +		err = reset_control_deassert(st_rproc->pwr_reset);
> > +		if (err) {
> > +			dev_err(&rproc->dev, "Failed to deassert Power Reset\n");
> > +			return err;
> > +		}
> > +	}
> > +
> > +	dev_info(&rproc->dev, "Started from 0x%x\n", rproc->bootaddr);
> > +
> > +	return 0;
> > +}
> 
> Does this want to unwind any of its operations if it encounters a failure?

Sounds sensible.  Will adapt.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Peter Griffin]

Hi,

On Tue, 01 Sep 2015, Lee Jones wrote:

> On Fri, 28 Aug 2015, Nathan Lynch wrote:
> 
> > On 08/28/2015 05:31 AM, Lee Jones wrote:
> > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > > index 28c711f..72e97d7 100644
> > > --- a/drivers/remoteproc/Kconfig
> > > +++ b/drivers/remoteproc/Kconfig
> > > @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
> > >  	  It's safe to say n here if you're not interested in multimedia
> > >  	  offloading.
> > >  
> > > +config ST_REMOTEPROC
> > > +	tristate "ST remoteproc support"
> > > +	depends on ARCH_STI
> > > +	select REMOTEPROC
> > > +	help
> > > +	  Say y here to support ST's adjunct processors via the remote
> > > +	  processor framework.
> > > +	  This can be either built-in or a loadable module.
> > > +
> > 
> > The code uses reset_control_* APIs, so this should depend on
> > RESET_CONTROLLER, no?
> 
> There's no need to explicitly depend on RESET_CONTROLLER.
> 
> With !RESET_CONTROLLER the user is WARN()ed about using the reset_*
> API.

ARCH_STI selects RESET_CONTROLLER, so it will always be enabled.

regards,

Peter.

Re: [STLinux Kernel] [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Lee Jones]

On Tue, 01 Sep 2015, Peter Griffin wrote:

> Hi,
> 
> On Tue, 01 Sep 2015, Lee Jones wrote:
> 
> > On Fri, 28 Aug 2015, Nathan Lynch wrote:
> > 
> > > On 08/28/2015 05:31 AM, Lee Jones wrote:
> > > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > > > index 28c711f..72e97d7 100644
> > > > --- a/drivers/remoteproc/Kconfig
> > > > +++ b/drivers/remoteproc/Kconfig
> > > > @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
> > > >  	  It's safe to say n here if you're not interested in multimedia
> > > >  	  offloading.
> > > >  
> > > > +config ST_REMOTEPROC
> > > > +	tristate "ST remoteproc support"
> > > > +	depends on ARCH_STI
> > > > +	select REMOTEPROC
> > > > +	help
> > > > +	  Say y here to support ST's adjunct processors via the remote
> > > > +	  processor framework.
> > > > +	  This can be either built-in or a loadable module.
> > > > +
> > > 
> > > The code uses reset_control_* APIs, so this should depend on
> > > RESET_CONTROLLER, no?
> > 
> > There's no need to explicitly depend on RESET_CONTROLLER.
> > 
> > With !RESET_CONTROLLER the user is WARN()ed about using the reset_*
> > API.
> 
> ARCH_STI selects RESET_CONTROLLER, so it will always be enabled.

That too.  Thanks for pointing that out.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Lee Jones]

This functionality is especially useful during the testing phase.  When
used in conjunction with Mailbox's Test Framework we can trivially conduct
end-to-end testing i.e. boot co-processor, send and receive messages to
the co-processor, then shut it down again (repeat as required).

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 9d30809..464470d 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -88,8 +88,37 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
 	return simple_read_from_buffer(userbuf, count, ppos, buf, i);
 }
 
+static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
+				 size_t count, loff_t *ppos)
+{
+	struct rproc *rproc = filp->private_data;
+	char buf[2];
+	int ret;
+
+	ret = copy_from_user(buf, userbuf, 1);
+	if (ret)
+		return -EFAULT;
+
+	switch (buf[0]) {
+	case '0':
+		rproc_shutdown(rproc);
+		break;
+	case '1':
+		ret = rproc_boot(rproc);
+		if (ret)
+			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
+		break;
+	default:
+		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
+		return -EINVAL;
+	}
+
+	return count;
+}
+
 static const struct file_operations rproc_state_ops = {
 	.read = rproc_state_read,
+	.write = rproc_state_write,
 	.open = simple_open,
 	.llseek	= generic_file_llseek,
 };
-- 
1.9.1

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
> +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> +				 size_t count, loff_t *ppos)
> +{
> +	struct rproc *rproc = filp->private_data;
> +	char buf[2];
> +	int ret;
> +
> +	ret = copy_from_user(buf, userbuf, 1);
> +	if (ret)
> +		return -EFAULT;
> +
> +	switch (buf[0]) {
> +	case '0':
> +		rproc_shutdown(rproc);
> +		break;
> +	case '1':
> +		ret = rproc_boot(rproc);
> +		if (ret)
> +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> +		break;
> +	default:
> +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> +		return -EINVAL;

This prints uninitialized kernel stack contents instead of what was
copied from user space.

Is the dev_err statement really necessary anyway?


> +	}
> +
> +	return count;
> +}

If rproc_boot fails, that should be reflected in the syscall result.

This interface is essentially exposing the remoteproc->power refcount to
user space; is that okay?  Seems like it makes it easy to underflow
remoteproc->power through successive shutdown calls.

The other debugfs interface in remoteproc that has a write method
(recovery) accepts more expressive string commands as opposed to 0/1.
It would be more consistent for this interface to take commands such as
"boot" and "shutdown" IMO.

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Lee Jones]

On Fri, 28 Aug 2015, Nathan Lynch wrote:

> On 08/28/2015 05:31 AM, Lee Jones wrote:
> > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > +				 size_t count, loff_t *ppos)
> > +{
> > +	struct rproc *rproc = filp->private_data;
> > +	char buf[2];
> > +	int ret;
> > +
> > +	ret = copy_from_user(buf, userbuf, 1);
> > +	if (ret)
> > +		return -EFAULT;
> > +
> > +	switch (buf[0]) {
> > +	case '0':
> > +		rproc_shutdown(rproc);
> > +		break;
> > +	case '1':
> > +		ret = rproc_boot(rproc);
> > +		if (ret)
> > +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> > +		break;
> > +	default:
> > +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> > +		return -EINVAL;
> 
> This prints uninitialized kernel stack contents instead of what was
> copied from user space.

Yes, you're right.  I will conduct a better test of the failure path
here. 

> Is the dev_err statement really necessary anyway?

Yes.  As I described in my other mail, this interface is for Kernel
Engineers, who read the kernel log.

> > +	}
> > +
> > +	return count;
> > +}
> 
> If rproc_boot fails, that should be reflected in the syscall result.
> 
> This interface is essentially exposing the remoteproc->power refcount to
> user space; is that okay?  Seems like it makes it easy to underflow
> remoteproc->power through successive shutdown calls.

If the subsystem is suseptable to underruns someone should think about
adding protection against imbalances in the 'core'.

> The other debugfs interface in remoteproc that has a write method
> (recovery) accepts more expressive string commands as opposed to 0/1.
> It would be more consistent for this interface to take commands such as
> "boot" and "shutdown" IMO.

Agreed.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Florian Fainelli]

On 28/08/15 03:31, Lee Jones wrote:
> This functionality is especially useful during the testing phase.  When
> used in conjunction with Mailbox's Test Framework we can trivially conduct
> end-to-end testing i.e. boot co-processor, send and receive messages to
> the co-processor, then shut it down again (repeat as required).

This does not strike me as a particularly well defined nor suitable
interface for controlling a remote processor's state. I know you are
just extending the existing debugfs interface here, but someone ought to
remove that piece of code and make it a proper character device or
netlink or whatever that allows someone to get proper signaling of
what's going on with the remote processor state by polling or listening
to a socket.

What's the intended use case behind debugfs for this after all? Is your
application expected to keep reading the state file in a loop until it
is happy and reads "running", how is that not racy by nature?

> 
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 9d30809..464470d 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -88,8 +88,37 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
>  	return simple_read_from_buffer(userbuf, count, ppos, buf, i);
>  }
>  
> +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> +				 size_t count, loff_t *ppos)
> +{
> +	struct rproc *rproc = filp->private_data;
> +	char buf[2];
> +	int ret;
> +
> +	ret = copy_from_user(buf, userbuf, 1);
> +	if (ret)
> +		return -EFAULT;
> +
> +	switch (buf[0]) {
> +	case '0':
> +		rproc_shutdown(rproc);
> +		break;
> +	case '1':
> +		ret = rproc_boot(rproc);
> +		if (ret)
> +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> +		break;
> +	default:
> +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
>  static const struct file_operations rproc_state_ops = {
>  	.read = rproc_state_read,
> +	.write = rproc_state_write,
>  	.open = simple_open,
>  	.llseek	= generic_file_llseek,
>  };
> 


-- 
Florian

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Lee Jones]

On Fri, 28 Aug 2015, Florian Fainelli wrote:
> On 28/08/15 03:31, Lee Jones wrote:
> > This functionality is especially useful during the testing phase.  When
> > used in conjunction with Mailbox's Test Framework we can trivially conduct
> > end-to-end testing i.e. boot co-processor, send and receive messages to
> > the co-processor, then shut it down again (repeat as required).
> 
> This does not strike me as a particularly well defined nor suitable
> interface for controlling a remote processor's state. I know you are
> just extending the existing debugfs interface here, but someone ought to
> remove that piece of code and make it a proper character device or
> netlink or whatever that allows someone to get proper signaling of
> what's going on with the remote processor state by polling or listening
> to a socket.

Please don't confuse this debugging interface as a solid means by
which to control co-processors.  It's in DebugFS for a reason.  The
idea of this feature is for driver writers to control *easily*
control co-processors for testing purposes only.

DebugFS will be disabled in production images.

> What's the intended use case behind debugfs for this after all? Is your
> application expected to keep reading the state file in a loop until it
> is happy and reads "running", how is that not racy by nature?

There is no 'application'.  One of the key wins for DebugFS is that
driver writers don't have to write applications during the testing
phase.  Using a character device instead would be a mistake IMO.

> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index 9d30809..464470d 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -88,8 +88,37 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
> >  	return simple_read_from_buffer(userbuf, count, ppos, buf, i);
> >  }
> >  
> > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > +				 size_t count, loff_t *ppos)
> > +{
> > +	struct rproc *rproc = filp->private_data;
> > +	char buf[2];
> > +	int ret;
> > +
> > +	ret = copy_from_user(buf, userbuf, 1);
> > +	if (ret)
> > +		return -EFAULT;
> > +
> > +	switch (buf[0]) {
> > +	case '0':
> > +		rproc_shutdown(rproc);
> > +		break;
> > +	case '1':
> > +		ret = rproc_boot(rproc);
> > +		if (ret)
> > +			dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> > +		break;
> > +	default:
> > +		dev_err(&rproc->dev, "Unrecognised option: %x\n", buf[1]);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> >  static const struct file_operations rproc_state_ops = {
> >  	.read = rproc_state_read,
> > +	.write = rproc_state_write,
> >  	.open = simple_open,
> >  	.llseek	= generic_file_llseek,
> >  };
> > 
> 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog