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.
Lee Jones (4):
remoteproc: dt: Provide bindings for ST's Remote Processor Controller
driver
remoteproc: Supply controller driver for ST's Remote Processors
ARM: STiH407: Add nodes for RemoteProc
remoteproc: debugfs: Add ability to boot remote processor using
debugfs
.../devicetree/bindings/remoteproc/st-rproc.txt | 38 +++
arch/arm/boot/dts/stih407-family.dtsi | 48 ++++
drivers/remoteproc/Kconfig | 9 +
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/remoteproc_debugfs.c | 25 ++
drivers/remoteproc/st_remoteproc.c | 300 +++++++++++++++++++++
6 files changed, 421 insertions(+)
create mode 100644 Documentation/devicetree/bindings/remoteproc/st-rproc.txt
create mode 100644 drivers/remoteproc/st_remoteproc.c
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/remoteproc/st-rproc.txt | 38 ++++++++++++++++++++++
1 file changed, 38 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..6a933c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
@@ -0,0 +1,38 @@
+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.
+
+The node name is significant, as it defines the name of the CPU and the name
+of the firmware to load: "rproc-<name>-fw".
+
+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
Signed-off-by: Lee Jones <[email protected]>
---
drivers/remoteproc/Kconfig | 9 ++
drivers/remoteproc/Makefile | 1 +
drivers/remoteproc/st_remoteproc.c | 300 +++++++++++++++++++++++++++++++++++++
3 files changed, 310 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..7afcfaf
--- /dev/null
+++ b/drivers/remoteproc/st_remoteproc.c
@@ -0,0 +1,300 @@
+/*
+ * ST's Remote Processor Control Driver
+ *
+ * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
+ *
+ * Author: Ludovic Barre <[email protected]>
+ *
+ * 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 of the License.
+ */
+
+#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 err = 0;
+
+ if (st_rproc->config->sw_reset) {
+ err = reset_control_assert(st_rproc->sw_reset);
+ if (err)
+ dev_warn(&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_warn(&rproc->dev, "Failed to assert Power Reset\n");
+ }
+
+ clk_disable(st_rproc->clk);
+
+ return 0;
+}
+
+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 <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
1.9.1
Signed-off-by: Lee Jones <[email protected]>
---
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
Signed-off-by: Lee Jones <[email protected]>
---
drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 9d30809..9620962 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -88,8 +88,33 @@ 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 '1':
+ ret = rproc_boot(rproc);
+ if (ret)
+ dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
+ break;
+ default:
+ rproc_shutdown(rproc);
+ }
+
+ 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
On 08/26/2015 08:08 AM, Lee Jones wrote:
> diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> new file mode 100644
> index 0000000..6a933c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> @@ -0,0 +1,38 @@
> +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.
> +
> +The node name is significant, as it defines the name of the CPU and the name
> +of the firmware to load: "rproc-<name>-fw".
This business about the firmware name describes a behavior of Linux's
core remoteproc code and seems out of place in a binding document.
On Wed, 26 Aug 2015, Nathan Lynch wrote:
> On 08/26/2015 08:08 AM, Lee Jones wrote:
> > diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > new file mode 100644
> > index 0000000..6a933c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> > @@ -0,0 +1,38 @@
> > +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.
> > +
> > +The node name is significant, as it defines the name of the CPU and the name
> > +of the firmware to load: "rproc-<name>-fw".
>
> This business about the firmware name describes a behavior of Linux's
> core remoteproc code and seems out of place in a binding document.
Agreed, will remove.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 08/26/2015 08:08 AM, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -0,0 +1,300 @@
> +/*
> + * ST's Remote Processor Control Driver
> + *
> + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> + *
> + * Author: Ludovic Barre <[email protected]>
When submitting code you didn't write, I'd say it's better practice to
clearly indicate its provenance in the commit message. E.g. something
like "Driver based on code authored by Ludovic Barre for ST". And
obtain signoffs etc if possible.
> + *
> + * 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 of the License.
Please review the wording here. It's unclear whether this is intended
to be v2-only or v2 or later.
> +static int st_rproc_stop(struct rproc *rproc)
> +{
> + struct st_rproc *st_rproc = rproc->priv;
> + int err = 0;
> +
> + if (st_rproc->config->sw_reset) {
> + err = reset_control_assert(st_rproc->sw_reset);
> + if (err)
> + dev_warn(&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_warn(&rproc->dev, "Failed to assert Power Reset\n");
> + }
> +
> + clk_disable(st_rproc->clk);
> +
> + return 0;
> +}
Seems like st_rproc_stop should propagate errors back to its caller
instead of always returning 0.
On 08/26/2015 08:08 AM, Lee Jones wrote:
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
The commit message should describe why this is needed...
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index 9d30809..9620962 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -88,8 +88,33 @@ 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 '1':
> + ret = rproc_boot(rproc);
> + if (ret)
> + dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> + break;
> + default:
> + rproc_shutdown(rproc);
> + }
> +
> + return count;
> +}
... and I suggest that the user interface be reconsidered. If '1' means
"boot" and literally anything else means "shut down" then you can't add
operations in the future without potentially breaking things.
On Wed, 26 Aug 2015, Nathan Lynch wrote:
> On 08/26/2015 08:08 AM, Lee Jones wrote:
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_debugfs.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
>
> The commit message should describe why this is needed...
>
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index 9d30809..9620962 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -88,8 +88,33 @@ 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 '1':
> > + ret = rproc_boot(rproc);
> > + if (ret)
> > + dev_warn(&rproc->dev, "Boot failed: %d\n", ret);
> > + break;
> > + default:
> > + rproc_shutdown(rproc);
> > + }
> > +
> > + return count;
> > +}
>
> ... and I suggest that the user interface be reconsidered. If '1' means
> "boot" and literally anything else means "shut down" then you can't add
> operations in the future without potentially breaking things.
Good points, will fix.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 26 Aug 2015, Nathan Lynch wrote:
> On 08/26/2015 08:08 AM, Lee Jones wrote:
> > --- /dev/null
> > +++ b/drivers/remoteproc/st_remoteproc.c
> > @@ -0,0 +1,300 @@
> > +/*
> > + * ST's Remote Processor Control Driver
> > + *
> > + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> > + *
> > + * Author: Ludovic Barre <[email protected]>
>
> When submitting code you didn't write, I'd say it's better practice to
> clearly indicate its provenance in the commit message. E.g. something
> like "Driver based on code authored by Ludovic Barre for ST". And
> obtain signoffs etc if possible.
>
>
> > + *
> > + * 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 of the License.
>
> Please review the wording here. It's unclear whether this is intended
> to be v2-only or v2 or later.
>
>
> > +static int st_rproc_stop(struct rproc *rproc)
> > +{
> > + struct st_rproc *st_rproc = rproc->priv;
> > + int err = 0;
> > +
> > + if (st_rproc->config->sw_reset) {
> > + err = reset_control_assert(st_rproc->sw_reset);
> > + if (err)
> > + dev_warn(&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_warn(&rproc->dev, "Failed to assert Power Reset\n");
> > + }
> > +
> > + clk_disable(st_rproc->clk);
> > +
> > + return 0;
> > +}
>
> Seems like st_rproc_stop should propagate errors back to its caller
> instead of always returning 0.
Good points, will fix.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog