[PATCH v6 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[PATCH v6 1/5] dt-bindings: Document JZ47xx VPU auxiliary processor

[Paul Cercueil]

Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
Ingenic is a second Xburst MIPS CPU very similar to the main core.
This document describes the devicetree bindings for this auxiliary
processor.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Notes:
    v2: Update TCSM0 address in example
    v3: Change node name to 'video-decoder'
    v4: Convert to YAML. I didn't add Rob's Ack on v3 because of that (sorry Rob)
    v5: - Fix 'reg' not in <addr, len> pairs
    	- Add missing include to devicetree example
    v6: No change

 .../bindings/remoteproc/ingenic,vpu.yaml      | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
new file mode 100644
index 000000000000..c019f9fbe916
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/ingenic,vpu.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Ingenic Video Processing Unit bindings
+
+description:
+  Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
+  Ingenic is a second Xburst MIPS CPU very similar to the main core.
+  This document describes the devicetree bindings for this auxiliary
+  processor.
+
+maintainers:
+  - Paul Cercueil <paul@crapouillou.net>
+
+properties:
+  compatible:
+    const: ingenic,jz4770-vpu-rproc
+
+  reg:
+    items:
+      - description: aux registers
+      - description: tcsm0 registers
+      - description: tcsm1 registers
+      - description: sram registers
+
+  reg-names:
+    items:
+      - const: aux
+      - const: tcsm0
+      - const: tcsm1
+      - const: sram
+
+  clocks:
+    items:
+      - description: aux clock
+      - description: vpu clock
+
+  clock-names:
+    items:
+      - const: aux
+      - const: vpu
+
+  interrupts:
+    description: VPU hardware interrupt
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/jz4770-cgu.h>
+
+    vpu: video-decoder@132a0000 {
+      compatible = "ingenic,jz4770-vpu-rproc";
+
+      reg = <0x132a0000 0x20>, /* AUX */
+            <0x132b0000 0x4000>, /* TCSM0 */
+            <0x132c0000 0xc000>, /* TCSM1 */
+            <0x132f0000 0x7000>; /* SRAM */
+      reg-names = "aux", "tcsm0", "tcsm1", "sram";
+
+      clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;
+      clock-names = "aux", "vpu";
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <3>;
+    };
-- 
2.25.1

[PATCH v6 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Paul Cercueil]

Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
like rproc_alloc() and rproc_add() respectively, but register their
respective cleanup function to be called on driver detach.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v3: New patch
    v4: No change
    v5: - Fix return value documentation
    	- Fix typo in documentation
    v6: No change

 drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  5 +++
 2 files changed, 72 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e12a54e67588..a7f96bc98406 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1949,6 +1949,33 @@ int rproc_add(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_add);
 
+static void devm_rproc_remove(void *rproc)
+{
+	rproc_del(rproc);
+}
+
+/**
+ * devm_rproc_add() - resource managed rproc_add()
+ * @dev: the underlying device
+ * @rproc: the remote processor handle to register
+ *
+ * This function performs like rproc_add() but the registered rproc device will
+ * automatically be removed on driver detach.
+ *
+ * Returns: 0 on success, negative errno on failure
+ */
+int devm_rproc_add(struct device *dev, struct rproc *rproc)
+{
+	int err;
+
+	err = rproc_add(rproc);
+	if (err)
+		return err;
+
+	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
+}
+EXPORT_SYMBOL(devm_rproc_add);
+
 /**
  * rproc_type_release() - release a remote processor instance
  * @dev: the rproc's device
@@ -2171,6 +2198,46 @@ int rproc_del(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_del);
 
+static void devm_rproc_free(struct device *dev, void *res)
+{
+	rproc_free(*(struct rproc **)res);
+}
+
+/**
+ * devm_rproc_alloc() - resource managed rproc_alloc()
+ * @dev: the underlying device
+ * @name: name of this remote processor
+ * @ops: platform-specific handlers (mainly start/stop)
+ * @firmware: name of firmware file to load, can be NULL
+ * @len: length of private data needed by the rproc driver (in bytes)
+ *
+ * This function performs like rproc_alloc() but the acquired rproc device will
+ * automatically be released on driver detach.
+ *
+ * Returns: new rproc instance, or NULL on failure
+ */
+struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
+			       const struct rproc_ops *ops,
+			       const char *firmware, int len)
+{
+	struct rproc **ptr, *rproc;
+
+	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	rproc = rproc_alloc(dev, name, ops, firmware, len);
+	if (rproc) {
+		*ptr = rproc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return rproc;
+}
+EXPORT_SYMBOL(devm_rproc_alloc);
+
 /**
  * rproc_add_subdev() - add a subdevice to a remoteproc
  * @rproc: rproc handle to add the subdevice to
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 9c07d7958c53..8c9c0dda03c3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -599,6 +599,11 @@ int rproc_add(struct rproc *rproc);
 int rproc_del(struct rproc *rproc);
 void rproc_free(struct rproc *rproc);
 
+struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
+			       const struct rproc_ops *ops,
+			       const char *firmware, int len);
+int devm_rproc_add(struct device *dev, struct rproc *rproc);
+
 void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
 
 struct rproc_mem_entry *
-- 
2.25.1

Re: [PATCH v6 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> like rproc_alloc() and rproc_add() respectively, but register their
> respective cleanup function to be called on driver detach.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v3: New patch
>     v4: No change
>     v5: - Fix return value documentation
>     	- Fix typo in documentation
>     v6: No change
> 
>  drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e12a54e67588..a7f96bc98406 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1949,6 +1949,33 @@ int rproc_add(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_add);
>  
> +static void devm_rproc_remove(void *rproc)
> +{
> +	rproc_del(rproc);
> +}
> +
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns: 0 on success, negative errno on failure
> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
> +	int err;
> +
> +	err = rproc_add(rproc);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> +}
> +EXPORT_SYMBOL(devm_rproc_add);
> +
>  /**
>   * rproc_type_release() - release a remote processor instance
>   * @dev: the rproc's device
> @@ -2171,6 +2198,46 @@ int rproc_del(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_del);
>  
> +static void devm_rproc_free(struct device *dev, void *res)
> +{
> +	rproc_free(*(struct rproc **)res);
> +}
> +
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acquired rproc device will
> + * automatically be released on driver detach.
> + *
> + * Returns: new rproc instance, or NULL on failure
> + */
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len)
> +{
> +	struct rproc **ptr, *rproc;
> +
> +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> +	if (rproc) {
> +		*ptr = rproc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(devm_rproc_alloc);
> +
>  /**
>   * rproc_add_subdev() - add a subdevice to a remoteproc
>   * @rproc: rproc handle to add the subdevice to
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 9c07d7958c53..8c9c0dda03c3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -599,6 +599,11 @@ int rproc_add(struct rproc *rproc);
>  int rproc_del(struct rproc *rproc);
>  void rproc_free(struct rproc *rproc);
>  
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len);
> +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> +
>  void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
>  
>  struct rproc_mem_entry *
> -- 
> 2.25.1
> 

Re: [PATCH v6 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> like rproc_alloc() and rproc_add() respectively, but register their
> respective cleanup function to be called on driver detach.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v3: New patch
>     v4: No change
>     v5: - Fix return value documentation
>     	- Fix typo in documentation
>     v6: No change
> 
>  drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e12a54e67588..a7f96bc98406 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1949,6 +1949,33 @@ int rproc_add(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_add);
>  
> +static void devm_rproc_remove(void *rproc)
> +{
> +	rproc_del(rproc);
> +}
> +
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns: 0 on success, negative errno on failure
> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
> +	int err;
> +
> +	err = rproc_add(rproc);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> +}
> +EXPORT_SYMBOL(devm_rproc_add);
> +
>  /**
>   * rproc_type_release() - release a remote processor instance
>   * @dev: the rproc's device
> @@ -2171,6 +2198,46 @@ int rproc_del(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_del);
>  
> +static void devm_rproc_free(struct device *dev, void *res)
> +{
> +	rproc_free(*(struct rproc **)res);
> +}
> +
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acquired rproc device will
> + * automatically be released on driver detach.
> + *
> + * Returns: new rproc instance, or NULL on failure
> + */
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len)
> +{
> +	struct rproc **ptr, *rproc;
> +
> +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> +	if (rproc) {
> +		*ptr = rproc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(devm_rproc_alloc);
> +
>  /**
>   * rproc_add_subdev() - add a subdevice to a remoteproc
>   * @rproc: rproc handle to add the subdevice to
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 9c07d7958c53..8c9c0dda03c3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -599,6 +599,11 @@ int rproc_add(struct rproc *rproc);
>  int rproc_del(struct rproc *rproc);
>  void rproc_free(struct rproc *rproc);
>  
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len);
> +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> +
>  void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
>  
>  struct rproc_mem_entry *
> -- 
> 2.25.1
> 

Re: [PATCH v6 2/5] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Add API functions devm_rproc_alloc() and devm_rproc_add(), which behave
> like rproc_alloc() and rproc_add() respectively, but register their
> respective cleanup function to be called on driver detach.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v3: New patch
>     v4: No change
>     v5: - Fix return value documentation
>     	- Fix typo in documentation
>     v6: No change
> 
>  drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e12a54e67588..a7f96bc98406 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1949,6 +1949,33 @@ int rproc_add(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_add);
>  
> +static void devm_rproc_remove(void *rproc)
> +{
> +	rproc_del(rproc);
> +}
> +
> +/**
> + * devm_rproc_add() - resource managed rproc_add()
> + * @dev: the underlying device
> + * @rproc: the remote processor handle to register
> + *
> + * This function performs like rproc_add() but the registered rproc device will
> + * automatically be removed on driver detach.
> + *
> + * Returns: 0 on success, negative errno on failure
> + */
> +int devm_rproc_add(struct device *dev, struct rproc *rproc)
> +{
> +	int err;
> +
> +	err = rproc_add(rproc);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_rproc_remove, rproc);
> +}
> +EXPORT_SYMBOL(devm_rproc_add);
> +
>  /**
>   * rproc_type_release() - release a remote processor instance
>   * @dev: the rproc's device
> @@ -2171,6 +2198,46 @@ int rproc_del(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_del);
>  
> +static void devm_rproc_free(struct device *dev, void *res)
> +{
> +	rproc_free(*(struct rproc **)res);
> +}
> +
> +/**
> + * devm_rproc_alloc() - resource managed rproc_alloc()
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * This function performs like rproc_alloc() but the acquired rproc device will
> + * automatically be released on driver detach.
> + *
> + * Returns: new rproc instance, or NULL on failure
> + */
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len)
> +{
> +	struct rproc **ptr, *rproc;
> +
> +	ptr = devres_alloc(devm_rproc_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rproc = rproc_alloc(dev, name, ops, firmware, len);
> +	if (rproc) {
> +		*ptr = rproc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(devm_rproc_alloc);
> +
>  /**
>   * rproc_add_subdev() - add a subdevice to a remoteproc
>   * @rproc: rproc handle to add the subdevice to
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 9c07d7958c53..8c9c0dda03c3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -599,6 +599,11 @@ int rproc_add(struct rproc *rproc);
>  int rproc_del(struct rproc *rproc);
>  void rproc_free(struct rproc *rproc);
>  
> +struct rproc *devm_rproc_alloc(struct device *dev, const char *name,
> +			       const struct rproc_ops *ops,
> +			       const char *firmware, int len);
> +int devm_rproc_add(struct device *dev, struct rproc *rproc);
> +
>  void rproc_add_carveout(struct rproc *rproc, struct rproc_mem_entry *mem);
>  
>  struct rproc_mem_entry *
> -- 
> 2.25.1
> 

[PATCH v6 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil]

Call pm_runtime_get_sync() before the firmware is loaded, and
pm_runtime_put() after the remote processor has been stopped.

Even though the remoteproc device has no PM callbacks, this allows the
parent device's PM callbacks to be properly called.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2-v4: No change
    v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
    v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks

 drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a7f96bc98406..d391b054efd8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -29,6 +29,7 @@
 #include <linux/devcoredump.h>
 #include <linux/rculist.h>
 #include <linux/remoteproc.h>
+#include <linux/pm_runtime.h>
 #include <linux/iommu.h>
 #include <linux/idr.h>
 #include <linux/elf.h>
@@ -1384,6 +1385,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
+	pm_runtime_get_sync(dev);
+
 	/*
 	 * if enabling an IOMMU isn't relevant for this rproc, this is
 	 * just a nop
@@ -1391,7 +1394,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	ret = rproc_enable_iommu(rproc);
 	if (ret) {
 		dev_err(dev, "can't enable iommu: %d\n", ret);
-		return ret;
+		goto put_pm_runtime;
 	}
 
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
@@ -1435,6 +1438,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->table_ptr = NULL;
 disable_iommu:
 	rproc_disable_iommu(rproc);
+put_pm_runtime:
+	pm_runtime_put(dev);
 	return ret;
 }
 
@@ -1840,6 +1845,8 @@ void rproc_shutdown(struct rproc *rproc)
 
 	rproc_disable_iommu(rproc);
 
+	pm_runtime_put(dev);
+
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
@@ -2118,6 +2125,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	rproc->state = RPROC_OFFLINE;
 
+	pm_runtime_no_callbacks(&rproc->dev);
+	pm_runtime_enable(&rproc->dev);
+
 	return rproc;
 }
 EXPORT_SYMBOL(rproc_alloc);
@@ -2133,6 +2143,7 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
+	pm_runtime_disable(&rproc->dev);
 	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_free);
-- 
2.25.1

Re: [PATCH v6 3/5] remoteproc: Add support for runtime PM

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Call pm_runtime_get_sync() before the firmware is loaded, and
> pm_runtime_put() after the remote processor has been stopped.
> 
> Even though the remoteproc device has no PM callbacks, this allows the
> parent device's PM callbacks to be properly called.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Notes:
>     v2-v4: No change
>     v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a7f96bc98406..d391b054efd8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
>  #include <linux/devcoredump.h>
>  #include <linux/rculist.h>
>  #include <linux/remoteproc.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/iommu.h>
>  #include <linux/idr.h>
>  #include <linux/elf.h>
> @@ -1384,6 +1385,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  
>  	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>  
> +	pm_runtime_get_sync(dev);

This can return an error, should we ignore this?

Apart from that this looks good.

Regards,
Bjorn

> +
>  	/*
>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
>  	 * just a nop
> @@ -1391,7 +1394,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	ret = rproc_enable_iommu(rproc);
>  	if (ret) {
>  		dev_err(dev, "can't enable iommu: %d\n", ret);
> -		return ret;
> +		goto put_pm_runtime;
>  	}
>  
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> @@ -1435,6 +1438,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	rproc->table_ptr = NULL;
>  disable_iommu:
>  	rproc_disable_iommu(rproc);
> +put_pm_runtime:
> +	pm_runtime_put(dev);
>  	return ret;
>  }
>  
> @@ -1840,6 +1845,8 @@ void rproc_shutdown(struct rproc *rproc)
>  
>  	rproc_disable_iommu(rproc);
>  
> +	pm_runtime_put(dev);
> +
>  	/* Free the copy of the resource table */
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
> @@ -2118,6 +2125,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> +	pm_runtime_no_callbacks(&rproc->dev);
> +	pm_runtime_enable(&rproc->dev);
> +
>  	return rproc;
>  }
>  EXPORT_SYMBOL(rproc_alloc);
> @@ -2133,6 +2143,7 @@ EXPORT_SYMBOL(rproc_alloc);
>   */
>  void rproc_free(struct rproc *rproc)
>  {
> +	pm_runtime_disable(&rproc->dev);
>  	put_device(&rproc->dev);
>  }
>  EXPORT_SYMBOL(rproc_free);
> -- 
> 2.25.1
> 

Re: [PATCH v6 3/5] remoteproc: Add support for runtime PM

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Call pm_runtime_get_sync() before the firmware is loaded, and
> pm_runtime_put() after the remote processor has been stopped.
> 
> Even though the remoteproc device has no PM callbacks, this allows the
> parent device's PM callbacks to be properly called.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Notes:
>     v2-v4: No change
>     v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a7f96bc98406..d391b054efd8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
>  #include <linux/devcoredump.h>
>  #include <linux/rculist.h>
>  #include <linux/remoteproc.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/iommu.h>
>  #include <linux/idr.h>
>  #include <linux/elf.h>
> @@ -1384,6 +1385,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  
>  	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>  
> +	pm_runtime_get_sync(dev);

This can return an error, should we ignore this?

Apart from that this looks good.

Regards,
Bjorn

> +
>  	/*
>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
>  	 * just a nop
> @@ -1391,7 +1394,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	ret = rproc_enable_iommu(rproc);
>  	if (ret) {
>  		dev_err(dev, "can't enable iommu: %d\n", ret);
> -		return ret;
> +		goto put_pm_runtime;
>  	}
>  
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> @@ -1435,6 +1438,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	rproc->table_ptr = NULL;
>  disable_iommu:
>  	rproc_disable_iommu(rproc);
> +put_pm_runtime:
> +	pm_runtime_put(dev);
>  	return ret;
>  }
>  
> @@ -1840,6 +1845,8 @@ void rproc_shutdown(struct rproc *rproc)
>  
>  	rproc_disable_iommu(rproc);
>  
> +	pm_runtime_put(dev);
> +
>  	/* Free the copy of the resource table */
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
> @@ -2118,6 +2125,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> +	pm_runtime_no_callbacks(&rproc->dev);
> +	pm_runtime_enable(&rproc->dev);
> +
>  	return rproc;
>  }
>  EXPORT_SYMBOL(rproc_alloc);
> @@ -2133,6 +2143,7 @@ EXPORT_SYMBOL(rproc_alloc);
>   */
>  void rproc_free(struct rproc *rproc)
>  {
> +	pm_runtime_disable(&rproc->dev);
>  	put_device(&rproc->dev);
>  }
>  EXPORT_SYMBOL(rproc_free);
> -- 
> 2.25.1
> 

Re: [PATCH v6 3/5] remoteproc: Add support for runtime PM

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> Call pm_runtime_get_sync() before the firmware is loaded, and
> pm_runtime_put() after the remote processor has been stopped.
> 
> Even though the remoteproc device has no PM callbacks, this allows the
> parent device's PM callbacks to be properly called.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Notes:
>     v2-v4: No change
>     v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a7f96bc98406..d391b054efd8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
>  #include <linux/devcoredump.h>
>  #include <linux/rculist.h>
>  #include <linux/remoteproc.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/iommu.h>
>  #include <linux/idr.h>
>  #include <linux/elf.h>
> @@ -1384,6 +1385,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  
>  	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>  
> +	pm_runtime_get_sync(dev);

This can return an error, should we ignore this?

Apart from that this looks good.

Regards,
Bjorn

> +
>  	/*
>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
>  	 * just a nop
> @@ -1391,7 +1394,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	ret = rproc_enable_iommu(rproc);
>  	if (ret) {
>  		dev_err(dev, "can't enable iommu: %d\n", ret);
> -		return ret;
> +		goto put_pm_runtime;
>  	}
>  
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> @@ -1435,6 +1438,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	rproc->table_ptr = NULL;
>  disable_iommu:
>  	rproc_disable_iommu(rproc);
> +put_pm_runtime:
> +	pm_runtime_put(dev);
>  	return ret;
>  }
>  
> @@ -1840,6 +1845,8 @@ void rproc_shutdown(struct rproc *rproc)
>  
>  	rproc_disable_iommu(rproc);
>  
> +	pm_runtime_put(dev);
> +
>  	/* Free the copy of the resource table */
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
> @@ -2118,6 +2125,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> +	pm_runtime_no_callbacks(&rproc->dev);
> +	pm_runtime_enable(&rproc->dev);
> +
>  	return rproc;
>  }
>  EXPORT_SYMBOL(rproc_alloc);
> @@ -2133,6 +2143,7 @@ EXPORT_SYMBOL(rproc_alloc);
>   */
>  void rproc_free(struct rproc *rproc)
>  {
> +	pm_runtime_disable(&rproc->dev);
>  	put_device(&rproc->dev);
>  }
>  EXPORT_SYMBOL(rproc_free);
> -- 
> 2.25.1
> 

[PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

This driver is used to boot, communicate with and load firmwares to the
MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---

Notes:
    v2: Remove exception for always-mapped memories
    v3: - Use clk_bulk API
    	- Move device-managed code to its own patch [3/4]
    	- Move devicetree table right above ingenic_rproc_driver
    	- Removed #ifdef CONFIG_OF around devicetree table
    	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
    	- Removed useless platform_set_drvdata()
    v4: - Add fix reported by Julia
    	- Change Kconfig symbol to INGENIC_VPU_RPROC
    	- Add documentation to struct vpu
    	- disable_irq_nosync() -> disable_irq()
    v5: No change
    v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks

 drivers/remoteproc/Kconfig         |   8 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/ingenic_rproc.c | 282 +++++++++++++++++++++++++++++
 3 files changed, 291 insertions(+)
 create mode 100644 drivers/remoteproc/ingenic_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index fbaed079b299..31da3e6c6281 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -240,6 +240,14 @@ config STM32_RPROC
 
 	  This can be either built-in or a loadable module.
 
+config INGENIC_VPU_RPROC
+	tristate "Ingenic JZ47xx VPU remoteproc support"
+	depends on MIPS || COMPILE_TEST
+	help
+	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
+	  This can be either built-in or a loadable module.
+	  If unsure say N.
+
 endif # REMOTEPROC
 
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 0effd3825035..e8b886e511f0 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -10,6 +10,7 @@ remoteproc-y				+= remoteproc_sysfs.o
 remoteproc-y				+= remoteproc_virtio.o
 remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
+obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
 obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
new file mode 100644
index 000000000000..bb14fe1c11f8
--- /dev/null
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ingenic JZ47xx remoteproc driver
+ * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define REG_AUX_CTRL		0x0
+#define REG_AUX_MSG_ACK		0x10
+#define REG_AUX_MSG		0x14
+#define REG_CORE_MSG_ACK	0x18
+#define REG_CORE_MSG		0x1C
+
+#define AUX_CTRL_SLEEP		BIT(31)
+#define AUX_CTRL_MSG_IRQ_EN	BIT(3)
+#define AUX_CTRL_NMI_RESETS	BIT(2)
+#define AUX_CTRL_NMI		BIT(1)
+#define AUX_CTRL_SW_RESET	BIT(0)
+
+struct vpu_mem_map {
+	const char *name;
+	unsigned int da;
+};
+
+struct vpu_mem_info {
+	const struct vpu_mem_map *map;
+	unsigned long len;
+	void __iomem *base;
+};
+
+static const struct vpu_mem_map vpu_mem_map[] = {
+	{ "tcsm0", 0x132b0000 },
+	{ "tcsm1", 0xf4000000 },
+	{ "sram",  0x132f0000 },
+};
+
+/**
+ * struct vpu - Ingenic VPU remoteproc private structure
+ * @irq: interrupt number
+ * @clks: pointers to the VPU and AUX clocks
+ * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
+ *            each of the external memories
+ * @dev: private pointer to the device
+ */
+struct vpu {
+	int irq;
+	struct clk_bulk_data clks[2];
+	void __iomem *aux_base;
+	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
+	struct device *dev;
+};
+
+static int ingenic_rproc_start(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+	u32 ctrl;
+
+	enable_irq(vpu->irq);
+
+	/* Reset the AUX and enable message IRQ */
+	ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
+	writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
+
+	return 0;
+}
+
+static int ingenic_rproc_stop(struct rproc *rproc)
+{
+	struct vpu *vpu = rproc->priv;
+
+	disable_irq(vpu->irq);
+
+	/* Keep AUX in reset mode */
+	writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
+
+	return 0;
+}
+
+static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct vpu *vpu = rproc->priv;
+
+	writel(vqid, vpu->aux_base + REG_CORE_MSG);
+}
+
+static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
+{
+	struct vpu *vpu = rproc->priv;
+	void __iomem *va = NULL;
+	unsigned int i;
+
+	if (len <= 0)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		const struct vpu_mem_info *info = &vpu->mem_info[i];
+		const struct vpu_mem_map *map = info->map;
+
+		if (da >= map->da && (da + len) < (map->da + info->len)) {
+			va = info->base + (da - map->da);
+			break;
+		}
+	}
+
+	return (__force void *)va;
+}
+
+static struct rproc_ops ingenic_rproc_ops = {
+	.start = ingenic_rproc_start,
+	.stop = ingenic_rproc_stop,
+	.kick = ingenic_rproc_kick,
+	.da_to_va = ingenic_rproc_da_to_va,
+};
+
+static irqreturn_t vpu_interrupt(int irq, void *data)
+{
+	struct rproc *rproc = data;
+	struct vpu *vpu = rproc->priv;
+	u32 vring;
+
+	vring = readl(vpu->aux_base + REG_AUX_MSG);
+
+	/* Ack the interrupt */
+	writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
+
+	return rproc_vq_interrupt(rproc, vring);
+}
+
+static void ingenic_rproc_disable_clks(void *data)
+{
+	struct vpu *vpu = data;
+
+	pm_runtime_resume(vpu->dev);
+	pm_runtime_disable(vpu->dev);
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
+}
+
+static int ingenic_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *mem;
+	struct rproc *rproc;
+	struct vpu *vpu;
+	unsigned int i;
+	int ret;
+
+	rproc = devm_rproc_alloc(dev, "ingenic-vpu",
+				 &ingenic_rproc_ops, NULL, sizeof(*vpu));
+	if (!rproc)
+		return -ENOMEM;
+
+	vpu = rproc->priv;
+	vpu->dev = &pdev->dev;
+	platform_set_drvdata(pdev, vpu);
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
+	vpu->aux_base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(vpu->aux_base)) {
+		dev_err(dev, "Failed to ioremap");
+		return PTR_ERR(vpu->aux_base);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   vpu_mem_map[i].name);
+
+		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
+		if (IS_ERR(vpu->mem_info[i].base)) {
+			ret = PTR_ERR(vpu->mem_info[i].base);
+			dev_err(dev, "Failed to ioremap");
+			return ret;
+		}
+
+		vpu->mem_info[i].len = resource_size(mem);
+		vpu->mem_info[i].map = &vpu_mem_map[i];
+	}
+
+	vpu->clks[0].id = "vpu";
+	vpu->clks[1].id = "aux";
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clocks");
+		return ret;
+	}
+
+	vpu->irq = platform_get_irq(pdev, 0);
+	if (vpu->irq < 0)
+		return vpu->irq;
+
+	ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request IRQ");
+		return ret;
+	}
+
+	disable_irq(vpu->irq);
+
+	/* The clocks must be enabled for the firmware to be loaded in TCSM */
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (ret) {
+		dev_err(dev, "Unable to start clocks");
+		return ret;
+	}
+
+	pm_runtime_irq_safe(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
+	if (ret) {
+		dev_err(dev, "Unable to register action");
+		goto out_pm_put;
+	}
+
+	ret = devm_rproc_add(dev, rproc);
+	if (ret) {
+		dev_err(dev, "Failed to register remote processor");
+		goto out_pm_put;
+	}
+
+out_pm_put:
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
+static const struct of_device_id ingenic_rproc_of_matches[] = {
+	{ .compatible = "ingenic,jz4770-vpu-rproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
+
+static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
+{
+	struct vpu *vpu = dev_get_drvdata(dev);
+
+	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_rproc_resume(struct device *dev)
+{
+	struct vpu *vpu = dev_get_drvdata(dev);
+
+	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
+	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
+};
+
+static struct platform_driver ingenic_rproc_driver = {
+	.probe = ingenic_rproc_probe,
+	.driver = {
+		.name = "ingenic-vpu",
+#ifdef CONFIG_PM
+		.pm = &ingenic_rproc_pm,
+#endif
+		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
+	},
+};
+module_platform_driver(ingenic_rproc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
-- 
2.25.1

Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> This driver is used to boot, communicate with and load firmwares to the
> MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
> Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

Please read Documentation/process/submitting-patches.rst about
"Developer's Certificate of Origin".

I suspect that you incorporated review feedback on previous revisions
from kbuild and Julia, this is generally omitted from the actual commit
message.

> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> 
> Notes:
>     v2: Remove exception for always-mapped memories
>     v3: - Use clk_bulk API
>     	- Move device-managed code to its own patch [3/4]
>     	- Move devicetree table right above ingenic_rproc_driver
>     	- Removed #ifdef CONFIG_OF around devicetree table
>     	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
>     	- Removed useless platform_set_drvdata()
>     v4: - Add fix reported by Julia
>     	- Change Kconfig symbol to INGENIC_VPU_RPROC
>     	- Add documentation to struct vpu
>     	- disable_irq_nosync() -> disable_irq()
>     v5: No change
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/Kconfig         |   8 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/ingenic_rproc.c | 282 +++++++++++++++++++++++++++++
>  3 files changed, 291 insertions(+)
>  create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index fbaed079b299..31da3e6c6281 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -240,6 +240,14 @@ config STM32_RPROC
>  
>  	  This can be either built-in or a loadable module.
>  
> +config INGENIC_VPU_RPROC

Please try to keep things alphabetically ordered.

> +	tristate "Ingenic JZ47xx VPU remoteproc support"
> +	depends on MIPS || COMPILE_TEST
> +	help
> +	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
> +	  This can be either built-in or a loadable module.
> +	  If unsure say N.
> +
>  endif # REMOTEPROC
>  
>  endmenu
[..]
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
[..]
> +/**
> + * struct vpu - Ingenic VPU remoteproc private structure
> + * @irq: interrupt number
> + * @clks: pointers to the VPU and AUX clocks

aux_base is missing

> + * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
> + *            each of the external memories
> + * @dev: private pointer to the device
> + */
> +struct vpu {
> +	int irq;
> +	struct clk_bulk_data clks[2];
> +	void __iomem *aux_base;
> +	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
> +	struct device *dev;
> +};
[..]
> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	if (len <= 0)

len can't be negative (also, does it add value to check for and fail len
== 0?)

> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +		const struct vpu_mem_info *info = &vpu->mem_info[i];
> +		const struct vpu_mem_map *map = info->map;
> +
> +		if (da >= map->da && (da + len) < (map->da + info->len)) {
> +			va = info->base + (da - map->da);
> +			break;
> +		}
> +	}
> +
> +	return (__force void *)va;
> +}
[..]
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +#ifdef CONFIG_PM

Please omit the #ifdef here.

> +		.pm = &ingenic_rproc_pm,
> +#endif
> +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),

Please omit the of_match_ptr()

Regards,
Bjorn

> +	},
> +};
> +module_platform_driver(ingenic_rproc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
> -- 
> 2.25.1
> 

Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> This driver is used to boot, communicate with and load firmwares to the
> MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
> Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

Please read Documentation/process/submitting-patches.rst about
"Developer's Certificate of Origin".

I suspect that you incorporated review feedback on previous revisions
from kbuild and Julia, this is generally omitted from the actual commit
message.

> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> 
> Notes:
>     v2: Remove exception for always-mapped memories
>     v3: - Use clk_bulk API
>     	- Move device-managed code to its own patch [3/4]
>     	- Move devicetree table right above ingenic_rproc_driver
>     	- Removed #ifdef CONFIG_OF around devicetree table
>     	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
>     	- Removed useless platform_set_drvdata()
>     v4: - Add fix reported by Julia
>     	- Change Kconfig symbol to INGENIC_VPU_RPROC
>     	- Add documentation to struct vpu
>     	- disable_irq_nosync() -> disable_irq()
>     v5: No change
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/Kconfig         |   8 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/ingenic_rproc.c | 282 +++++++++++++++++++++++++++++
>  3 files changed, 291 insertions(+)
>  create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index fbaed079b299..31da3e6c6281 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -240,6 +240,14 @@ config STM32_RPROC
>  
>  	  This can be either built-in or a loadable module.
>  
> +config INGENIC_VPU_RPROC

Please try to keep things alphabetically ordered.

> +	tristate "Ingenic JZ47xx VPU remoteproc support"
> +	depends on MIPS || COMPILE_TEST
> +	help
> +	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
> +	  This can be either built-in or a loadable module.
> +	  If unsure say N.
> +
>  endif # REMOTEPROC
>  
>  endmenu
[..]
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
[..]
> +/**
> + * struct vpu - Ingenic VPU remoteproc private structure
> + * @irq: interrupt number
> + * @clks: pointers to the VPU and AUX clocks

aux_base is missing

> + * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
> + *            each of the external memories
> + * @dev: private pointer to the device
> + */
> +struct vpu {
> +	int irq;
> +	struct clk_bulk_data clks[2];
> +	void __iomem *aux_base;
> +	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
> +	struct device *dev;
> +};
[..]
> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	if (len <= 0)

len can't be negative (also, does it add value to check for and fail len
== 0?)

> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +		const struct vpu_mem_info *info = &vpu->mem_info[i];
> +		const struct vpu_mem_map *map = info->map;
> +
> +		if (da >= map->da && (da + len) < (map->da + info->len)) {
> +			va = info->base + (da - map->da);
> +			break;
> +		}
> +	}
> +
> +	return (__force void *)va;
> +}
[..]
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +#ifdef CONFIG_PM

Please omit the #ifdef here.

> +		.pm = &ingenic_rproc_pm,
> +#endif
> +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),

Please omit the of_match_ptr()

Regards,
Bjorn

> +	},
> +};
> +module_platform_driver(ingenic_rproc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
> -- 
> 2.25.1
> 

Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:

> This driver is used to boot, communicate with and load firmwares to the
> MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
> Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

Please read Documentation/process/submitting-patches.rst about
"Developer's Certificate of Origin".

I suspect that you incorporated review feedback on previous revisions
from kbuild and Julia, this is generally omitted from the actual commit
message.

> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> 
> Notes:
>     v2: Remove exception for always-mapped memories
>     v3: - Use clk_bulk API
>     	- Move device-managed code to its own patch [3/4]
>     	- Move devicetree table right above ingenic_rproc_driver
>     	- Removed #ifdef CONFIG_OF around devicetree table
>     	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
>     	- Removed useless platform_set_drvdata()
>     v4: - Add fix reported by Julia
>     	- Change Kconfig symbol to INGENIC_VPU_RPROC
>     	- Add documentation to struct vpu
>     	- disable_irq_nosync() -> disable_irq()
>     v5: No change
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
> 
>  drivers/remoteproc/Kconfig         |   8 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/ingenic_rproc.c | 282 +++++++++++++++++++++++++++++
>  3 files changed, 291 insertions(+)
>  create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index fbaed079b299..31da3e6c6281 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -240,6 +240,14 @@ config STM32_RPROC
>  
>  	  This can be either built-in or a loadable module.
>  
> +config INGENIC_VPU_RPROC

Please try to keep things alphabetically ordered.

> +	tristate "Ingenic JZ47xx VPU remoteproc support"
> +	depends on MIPS || COMPILE_TEST
> +	help
> +	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
> +	  This can be either built-in or a loadable module.
> +	  If unsure say N.
> +
>  endif # REMOTEPROC
>  
>  endmenu
[..]
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
[..]
> +/**
> + * struct vpu - Ingenic VPU remoteproc private structure
> + * @irq: interrupt number
> + * @clks: pointers to the VPU and AUX clocks

aux_base is missing

> + * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
> + *            each of the external memories
> + * @dev: private pointer to the device
> + */
> +struct vpu {
> +	int irq;
> +	struct clk_bulk_data clks[2];
> +	void __iomem *aux_base;
> +	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
> +	struct device *dev;
> +};
[..]
> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	if (len <= 0)

len can't be negative (also, does it add value to check for and fail len
== 0?)

> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +		const struct vpu_mem_info *info = &vpu->mem_info[i];
> +		const struct vpu_mem_map *map = info->map;
> +
> +		if (da >= map->da && (da + len) < (map->da + info->len)) {
> +			va = info->base + (da - map->da);
> +			break;
> +		}
> +	}
> +
> +	return (__force void *)va;
> +}
[..]
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +#ifdef CONFIG_PM

Please omit the #ifdef here.

> +		.pm = &ingenic_rproc_pm,
> +#endif
> +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),

Please omit the of_match_ptr()

Regards,
Bjorn

> +	},
> +};
> +module_platform_driver(ingenic_rproc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");
> -- 
> 2.25.1
> 

Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

Hi Bjorn,

Le dim. 19 avril 2020 à 23:37, Bjorn Andersson 
<bjorn.andersson@linaro.org> a écrit :
> On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:
> 
>>  This driver is used to boot, communicate with and load firmwares to 
>> the
>>  MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
>>  Ingenic.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Signed-off-by: kbuild test robot <lkp@intel.com>
>>  Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> 
> Please read Documentation/process/submitting-patches.rst about
> "Developer's Certificate of Origin".
> 
> I suspect that you incorporated review feedback on previous revisions
> from kbuild and Julia, this is generally omitted from the actual 
> commit
> message.

Julia / kbuild sent a patch to fix an error in the driver, so my patch 
now has code from Julia / kbuild. That document clearly says that I 
should add their signed-off-by. Or what do you mean?

>>  Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>  ---
>> 
>>  Notes:
>>      v2: Remove exception for always-mapped memories
>>      v3: - Use clk_bulk API
>>      	- Move device-managed code to its own patch [3/4]
>>      	- Move devicetree table right above ingenic_rproc_driver
>>      	- Removed #ifdef CONFIG_OF around devicetree table
>>      	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
>>      	- Removed useless platform_set_drvdata()
>>      v4: - Add fix reported by Julia
>>      	- Change Kconfig symbol to INGENIC_VPU_RPROC
>>      	- Add documentation to struct vpu
>>      	- disable_irq_nosync() -> disable_irq()
>>      v5: No change
>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>> callbacks
>> 
>>   drivers/remoteproc/Kconfig         |   8 +
>>   drivers/remoteproc/Makefile        |   1 +
>>   drivers/remoteproc/ingenic_rproc.c | 282 
>> +++++++++++++++++++++++++++++
>>   3 files changed, 291 insertions(+)
>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>> 
>>  diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>  index fbaed079b299..31da3e6c6281 100644
>>  --- a/drivers/remoteproc/Kconfig
>>  +++ b/drivers/remoteproc/Kconfig
>>  @@ -240,6 +240,14 @@ config STM32_RPROC
>> 
>>   	  This can be either built-in or a loadable module.
>> 
>>  +config INGENIC_VPU_RPROC
> 
> Please try to keep things alphabetically ordered.
> 
>>  +	tristate "Ingenic JZ47xx VPU remoteproc support"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	help
>>  +	  Say y or m here to support the VPU in the JZ47xx SoCs from 
>> Ingenic.
>>  +	  This can be either built-in or a loadable module.
>>  +	  If unsure say N.
>>  +
>>   endif # REMOTEPROC
>> 
>>   endmenu
> [..]
>>  diff --git a/drivers/remoteproc/ingenic_rproc.c 
>> b/drivers/remoteproc/ingenic_rproc.c
> [..]
>>  +/**
>>  + * struct vpu - Ingenic VPU remoteproc private structure
>>  + * @irq: interrupt number
>>  + * @clks: pointers to the VPU and AUX clocks
> 
> aux_base is missing
> 
>>  + * @mem_info: array of struct vpu_mem_info, which contain the 
>> mapping info of
>>  + *            each of the external memories
>>  + * @dev: private pointer to the device
>>  + */
>>  +struct vpu {
>>  +	int irq;
>>  +	struct clk_bulk_data clks[2];
>>  +	void __iomem *aux_base;
>>  +	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
>>  +	struct device *dev;
>>  +};
> [..]
>>  +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, 
>> size_t len)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +	void __iomem *va = NULL;
>>  +	unsigned int i;
>>  +
>>  +	if (len <= 0)
> 
> len can't be negative (also, does it add value to check for and fail 
> len
> == 0?)
> 
>>  +		return NULL;
>>  +
>>  +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>>  +		const struct vpu_mem_info *info = &vpu->mem_info[i];
>>  +		const struct vpu_mem_map *map = info->map;
>>  +
>>  +		if (da >= map->da && (da + len) < (map->da + info->len)) {
>>  +			va = info->base + (da - map->da);
>>  +			break;
>>  +		}
>>  +	}
>>  +
>>  +	return (__force void *)va;
>>  +}
> [..]
>>  +static struct platform_driver ingenic_rproc_driver = {
>>  +	.probe = ingenic_rproc_probe,
>>  +	.driver = {
>>  +		.name = "ingenic-vpu",
>>  +#ifdef CONFIG_PM
> 
> Please omit the #ifdef here.

If I do, then the PM callbacks will be compiled in even if CONFIG_PM is 
disabled. That means dead code and I see no reason why you would want 
that.

If you don't mind, I'd like to keep the #ifdef CONFIG_PM for now, until 
this patchset is merged: https://lkml.org/lkml/2020/4/13/582

Then it would become a one-liner:
.pm = pm_ptr(&ingenic_rproc_pm),

Cheers,
-Paul

>>  +		.pm = &ingenic_rproc_pm,
>>  +#endif
>>  +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
> 
> Please omit the of_match_ptr()
> 
> Regards,
> Bjorn
> 
>>  +	},
>>  +};
>>  +module_platform_driver(ingenic_rproc_driver);
>>  +
>>  +MODULE_LICENSE("GPL");
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control 
>> driver");
>>  --
>>  2.25.1
>> 

Re: [PATCH v6 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Tue 21 Apr 08:43 PDT 2020, Paul Cercueil wrote:

> Hi Bjorn,
> 
> Le dim. 19 avril 2020 à 23:37, Bjorn Andersson <bjorn.andersson@linaro.org>
> a écrit :
> > On Fri 17 Apr 10:00 PDT 2020, Paul Cercueil wrote:
> > 
> > >  This driver is used to boot, communicate with and load firmwares to
> > > the
> > >  MIPS co-processor found in the VPU hardware of the JZ47xx SoCs from
> > >  Ingenic.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Signed-off-by: kbuild test robot <lkp@intel.com>
> > >  Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
> > 
> > Please read Documentation/process/submitting-patches.rst about
> > "Developer's Certificate of Origin".
> > 
> > I suspect that you incorporated review feedback on previous revisions
> > from kbuild and Julia, this is generally omitted from the actual commit
> > message.
> 
> Julia / kbuild sent a patch to fix an error in the driver, so my patch now
> has code from Julia / kbuild. That document clearly says that I should add
> their signed-off-by. Or what do you mean?
> 

We generally don't attribute people whom through code review affected
the outcome, unless perhaps it's significant.

But a bigger problem is that per "Developer's Certificate of Origin 1.1"
in submitting-patches.rst, what this says is:

1) You wrote the patch, in whole or in part and have the right to
   submit it to the public kernel. I.e. (a) and (d)

2) Then "kbuild test robot" claims that either it based it's
   contribution on your work, or that it forwards the unmodified work
   ((b) or (c)) and (d).

3) Then Julia again took the contribution from "kbuild test robot" and
   is claiming to follow either (b) or (c) - and (d).

Then somehow, after Julia stated that she dealt with the patch you
emailed it to me.

In order to claim that the three of you developed the patch together you
should add all three as "Co-developed-by:", in addition to the signed
off by.


But again, my recommendation is that you consider their input as "review
feedback" and just incorporate it in the patch without any additional
tags. You're still fulfilling the certificate of origin.

Regards,
Bjorn

> > >  Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > >  ---
> > > 
> > >  Notes:
> > >      v2: Remove exception for always-mapped memories
> > >      v3: - Use clk_bulk API
> > >      	- Move device-managed code to its own patch [3/4]
> > >      	- Move devicetree table right above ingenic_rproc_driver
> > >      	- Removed #ifdef CONFIG_OF around devicetree table
> > >      	- Removed .owner = THIS_MODULE in ingenic_rproc_driver
> > >      	- Removed useless platform_set_drvdata()
> > >      v4: - Add fix reported by Julia
> > >      	- Change Kconfig symbol to INGENIC_VPU_RPROC
> > >      	- Add documentation to struct vpu
> > >      	- disable_irq_nosync() -> disable_irq()
> > >      v5: No change
> > >      v6: Instead of prepare/unprepare callbacks, use PM runtime
> > > callbacks
> > > 
> > >   drivers/remoteproc/Kconfig         |   8 +
> > >   drivers/remoteproc/Makefile        |   1 +
> > >   drivers/remoteproc/ingenic_rproc.c | 282
> > > +++++++++++++++++++++++++++++
> > >   3 files changed, 291 insertions(+)
> > >   create mode 100644 drivers/remoteproc/ingenic_rproc.c
> > > 
> > >  diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > >  index fbaed079b299..31da3e6c6281 100644
> > >  --- a/drivers/remoteproc/Kconfig
> > >  +++ b/drivers/remoteproc/Kconfig
> > >  @@ -240,6 +240,14 @@ config STM32_RPROC
> > > 
> > >   	  This can be either built-in or a loadable module.
> > > 
> > >  +config INGENIC_VPU_RPROC
> > 
> > Please try to keep things alphabetically ordered.
> > 
> > >  +	tristate "Ingenic JZ47xx VPU remoteproc support"
> > >  +	depends on MIPS || COMPILE_TEST
> > >  +	help
> > >  +	  Say y or m here to support the VPU in the JZ47xx SoCs from
> > > Ingenic.
> > >  +	  This can be either built-in or a loadable module.
> > >  +	  If unsure say N.
> > >  +
> > >   endif # REMOTEPROC
> > > 
> > >   endmenu
> > [..]
> > >  diff --git a/drivers/remoteproc/ingenic_rproc.c
> > > b/drivers/remoteproc/ingenic_rproc.c
> > [..]
> > >  +/**
> > >  + * struct vpu - Ingenic VPU remoteproc private structure
> > >  + * @irq: interrupt number
> > >  + * @clks: pointers to the VPU and AUX clocks
> > 
> > aux_base is missing
> > 
> > >  + * @mem_info: array of struct vpu_mem_info, which contain the
> > > mapping info of
> > >  + *            each of the external memories
> > >  + * @dev: private pointer to the device
> > >  + */
> > >  +struct vpu {
> > >  +	int irq;
> > >  +	struct clk_bulk_data clks[2];
> > >  +	void __iomem *aux_base;
> > >  +	struct vpu_mem_info mem_info[ARRAY_SIZE(vpu_mem_map)];
> > >  +	struct device *dev;
> > >  +};
> > [..]
> > >  +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da,
> > > size_t len)
> > >  +{
> > >  +	struct vpu *vpu = rproc->priv;
> > >  +	void __iomem *va = NULL;
> > >  +	unsigned int i;
> > >  +
> > >  +	if (len <= 0)
> > 
> > len can't be negative (also, does it add value to check for and fail len
> > == 0?)
> > 
> > >  +		return NULL;
> > >  +
> > >  +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> > >  +		const struct vpu_mem_info *info = &vpu->mem_info[i];
> > >  +		const struct vpu_mem_map *map = info->map;
> > >  +
> > >  +		if (da >= map->da && (da + len) < (map->da + info->len)) {
> > >  +			va = info->base + (da - map->da);
> > >  +			break;
> > >  +		}
> > >  +	}
> > >  +
> > >  +	return (__force void *)va;
> > >  +}
> > [..]
> > >  +static struct platform_driver ingenic_rproc_driver = {
> > >  +	.probe = ingenic_rproc_probe,
> > >  +	.driver = {
> > >  +		.name = "ingenic-vpu",
> > >  +#ifdef CONFIG_PM
> > 
> > Please omit the #ifdef here.
> 
> If I do, then the PM callbacks will be compiled in even if CONFIG_PM is
> disabled. That means dead code and I see no reason why you would want that.
> 
> If you don't mind, I'd like to keep the #ifdef CONFIG_PM for now, until this
> patchset is merged: https://lkml.org/lkml/2020/4/13/582
> 
> Then it would become a one-liner:
> .pm = pm_ptr(&ingenic_rproc_pm),
> 
> Cheers,
> -Paul
> 
> > >  +		.pm = &ingenic_rproc_pm,
> > >  +#endif
> > >  +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
> > 
> > Please omit the of_match_ptr()
> > 
> > Regards,
> > Bjorn
> > 
> > >  +	},
> > >  +};
> > >  +module_platform_driver(ingenic_rproc_driver);
> > >  +
> > >  +MODULE_LICENSE("GPL");
> > >  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> > >  +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control
> > > driver");
> > >  --
> > >  2.25.1
> > > 
> 
> 

[PATCH v6 5/5] MAINTAINERS: Add myself as reviewer for Ingenic rproc driver

[Paul Cercueil]

Add myself as the reviewer for the Ingenic VPU remoteproc driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v4: New patch
    v5-v6: No change

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db31497..1677071197a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8418,6 +8418,7 @@ F:	drivers/mtd/nand/raw/ingenic/
 F:	drivers/pinctrl/pinctrl-ingenic.c
 F:	drivers/power/supply/ingenic-battery.c
 F:	drivers/pwm/pwm-jz4740.c
+F:	drivers/remoteproc/ingenic_rproc.c
 F:	drivers/rtc/rtc-jz4740.c
 F:	drivers/tty/serial/8250/8250_ingenic.c
 F:	drivers/usb/musb/jz4740.c
-- 
2.25.1