linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add VPU support for new Ingenic SoCs.
@ 2021-07-24  9:11 周琰杰 (Zhou Yanjie)
  2021-07-24  9:11 ` [PATCH 1/2] dt-bindings: remoteproc: Add bindings " 周琰杰 (Zhou Yanjie)
  2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
  0 siblings, 2 replies; 9+ messages in thread
From: 周琰杰 (Zhou Yanjie) @ 2021-07-24  9:11 UTC (permalink / raw)
  To: ohad, bjorn.andersson, mathieu.poirier, robh+dt, paul
  Cc: devicetree, linux-remoteproc, linux-mips, linux-kernel,
	dongsheng.qiu, aric.pzqi, rick.tyliu, sihui.liu, jun.jiang,
	sernia.zhou

1.Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
  the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
2.Add support for probing the ingenic_rproc driver on the JZ4760 SoC,
  the JZ4760B SoC, the JZ4775 SoC, and the JZ4780 SoC from Ingenic.

It should be noted that the VPU of JZ4775 and JZ4780 has only one TCSM.

周琰杰 (Zhou Yanjie) (2):
  dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.
  remoteproc: Ingenic: Add support for new Ingenic SoCs.

 .../bindings/remoteproc/ingenic,vpu.yaml           |  74 +++++++++----
 drivers/remoteproc/ingenic_rproc.c                 | 115 ++++++++++++++++-----
 2 files changed, 147 insertions(+), 42 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.
  2021-07-24  9:11 [PATCH 0/2] Add VPU support for new Ingenic SoCs 周琰杰 (Zhou Yanjie)
@ 2021-07-24  9:11 ` 周琰杰 (Zhou Yanjie)
  2021-07-24 11:02   ` Paul Cercueil
  2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
  1 sibling, 1 reply; 9+ messages in thread
From: 周琰杰 (Zhou Yanjie) @ 2021-07-24  9:11 UTC (permalink / raw)
  To: ohad, bjorn.andersson, mathieu.poirier, robh+dt, paul
  Cc: devicetree, linux-remoteproc, linux-mips, linux-kernel,
	dongsheng.qiu, aric.pzqi, rick.tyliu, sihui.liu, jun.jiang,
	sernia.zhou

Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
the JZ4775 SoC, and the JZ4780 SoC from Ingenic.

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---
 .../bindings/remoteproc/ingenic,vpu.yaml           | 74 ++++++++++++++++------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
index d0aa91b..6154596 100644
--- a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
@@ -17,31 +17,52 @@ maintainers:
 
 properties:
   compatible:
-    const: ingenic,jz4770-vpu-rproc
+    enum:
+      - ingenic,jz4760-vpu-rproc
+      - ingenic,jz4760b-vpu-rproc
+      - ingenic,jz4770-vpu-rproc
+      - ingenic,jz4775-vpu-rproc
+      - ingenic,jz4780-vpu-rproc
 
   reg:
-    items:
-      - description: aux registers
-      - description: tcsm0 registers
-      - description: tcsm1 registers
-      - description: sram registers
+    oneOf:
+      - items:
+          - description: aux registers
+          - description: tcsm0 registers
+          - description: tcsm1 registers
+          - description: sram registers
+      - items:
+          - description: aux registers
+          - description: tcsm registers
+          - description: sram registers
 
   reg-names:
-    items:
-      - const: aux
-      - const: tcsm0
-      - const: tcsm1
-      - const: sram
+    oneOf:
+      - items:
+          - const: aux
+          - const: tcsm0
+          - const: tcsm1
+          - const: sram
+      - items:
+          - const: aux
+          - const: tcsm
+          - const: sram
 
   clocks:
-    items:
-      - description: aux clock
-      - description: vpu clock
+    oneOf:
+      - items:
+          - description: aux clock
+          - description: vpu clock
+      - items:
+          - description: vpu clock
 
   clock-names:
-    items:
-      - const: aux
-      - const: vpu
+    oneOf:
+      - items:
+          - const: aux
+          - const: vpu
+      - items:
+          - const: vpu
 
   interrupts:
     maxItems: 1
@@ -60,7 +81,7 @@ examples:
   - |
     #include <dt-bindings/clock/jz4770-cgu.h>
 
-    vpu: video-decoder@132a0000 {
+    video-decoder@132a0000 {
       compatible = "ingenic,jz4770-vpu-rproc";
 
       reg = <0x132a0000 0x20>, /* AUX */
@@ -75,3 +96,20 @@ examples:
       interrupt-parent = <&cpuintc>;
       interrupts = <3>;
     };
+  - |
+    #include <dt-bindings/clock/jz4780-cgu.h>
+
+    video-decoder@132a0000 {
+      compatible = "ingenic,jz4780-vpu-rproc";
+
+      reg = <0x132a0000 0x20>, /* AUX */
+            <0x132c0000 0x8000>, /* TCSM */
+            <0x132f0000 0x4000>; /* SRAM */
+      reg-names = "aux", "tcsm", "sram";
+
+      clocks = <&cgu JZ4780_CLK_VPU>;
+      clock-names = "vpu";
+
+      interrupt-parent = <&intc>;
+      interrupts = <62>;
+    };
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] remoteproc: Ingenic: Add support for new Ingenic SoCs.
  2021-07-24  9:11 [PATCH 0/2] Add VPU support for new Ingenic SoCs 周琰杰 (Zhou Yanjie)
  2021-07-24  9:11 ` [PATCH 1/2] dt-bindings: remoteproc: Add bindings " 周琰杰 (Zhou Yanjie)
@ 2021-07-24  9:11 ` 周琰杰 (Zhou Yanjie)
  2021-07-24 11:15   ` Paul Cercueil
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: 周琰杰 (Zhou Yanjie) @ 2021-07-24  9:11 UTC (permalink / raw)
  To: ohad, bjorn.andersson, mathieu.poirier, robh+dt, paul
  Cc: devicetree, linux-remoteproc, linux-mips, linux-kernel,
	dongsheng.qiu, aric.pzqi, rick.tyliu, sihui.liu, jun.jiang,
	sernia.zhou

Add support for probing the ingenic_rproc driver on the JZ4760 SoC,
the JZ4760B SoC, the JZ4775 SoC, and the JZ4780 SoC from Ingenic.

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---
 drivers/remoteproc/ingenic_rproc.c | 115 +++++++++++++++++++++++++++++--------
 1 file changed, 91 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
index a356738..6a2e864 100644
--- a/drivers/remoteproc/ingenic_rproc.c
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -2,6 +2,7 @@
 /*
  * Ingenic JZ47xx remoteproc driver
  * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
+ * Copyright 2021, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
  */
 
 #include <linux/bitops.h>
@@ -17,7 +18,7 @@
 
 #define REG_AUX_CTRL		0x0
 #define REG_AUX_MSG_ACK		0x10
-#define REG_AUX_MSG		0x14
+#define REG_AUX_MSG			0x14
 #define REG_CORE_MSG_ACK	0x18
 #define REG_CORE_MSG		0x1C
 
@@ -32,6 +33,20 @@ module_param(auto_boot, bool, 0400);
 MODULE_PARM_DESC(auto_boot,
 		 "Auto-boot the remote processor [default=false]");
 
+enum ingenic_vpu_version {
+	ID_JZ4760,
+	ID_JZ4770,
+	ID_JZ4775,
+};
+
+struct ingenic_soc_info {
+	enum ingenic_vpu_version version;
+	const struct vpu_mem_map *mem_map;
+
+	unsigned int num_clks;
+	unsigned int num_mems;
+};
+
 struct vpu_mem_map {
 	const char *name;
 	unsigned int da;
@@ -43,26 +58,21 @@ struct vpu_mem_info {
 	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
  * @aux_base: raw pointer to the AUX interface registers
- * @mem_info: array of struct vpu_mem_info, which contain the mapping info of
+ * @mem_info: pointers to the 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)];
+	const struct ingenic_soc_info *soc_info;
+	struct clk_bulk_data *clks;
+	struct vpu_mem_info *mem_info;
 	struct device *dev;
 };
 
@@ -72,7 +82,7 @@ static int ingenic_rproc_prepare(struct rproc *rproc)
 	int ret;
 
 	/* The clocks must be enabled for the firmware to be loaded in TCSM */
-	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+	ret = clk_bulk_prepare_enable(vpu->soc_info->num_clks, vpu->clks);
 	if (ret)
 		dev_err(vpu->dev, "Unable to start clocks: %d\n", ret);
 
@@ -83,7 +93,7 @@ static int ingenic_rproc_unprepare(struct rproc *rproc)
 {
 	struct vpu *vpu = rproc->priv;
 
-	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
+	clk_bulk_disable_unprepare(vpu->soc_info->num_clks, vpu->clks);
 
 	return 0;
 }
@@ -127,7 +137,7 @@ static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, boo
 	void __iomem *va = NULL;
 	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+	for (i = 0; i < vpu->soc_info->num_mems; i++) {
 		const struct vpu_mem_info *info = &vpu->mem_info[i];
 		const struct vpu_mem_map *map = info->map;
 
@@ -163,8 +173,60 @@ static irqreturn_t vpu_interrupt(int irq, void *data)
 	return rproc_vq_interrupt(rproc, vring);
 }
 
+static const struct vpu_mem_map jz4760_vpu_mem_map[] = {
+	{ "tcsm0", 0x132b0000 },
+	{ "tcsm1", 0xf4000000 },
+	{ "sram",  0x132d0000 },
+};
+
+static const struct vpu_mem_map jz4770_vpu_mem_map[] = {
+	{ "tcsm0", 0x132b0000 },
+	{ "tcsm1", 0xf4000000 },
+	{ "sram",  0x132f0000 },
+};
+
+static const struct vpu_mem_map jz4775_vpu_mem_map[] = {
+	{ "tcsm",  0xf4000000 },
+	{ "sram",  0x132f0000 },
+};
+
+static const struct ingenic_soc_info jz4760_soc_info = {
+	.version = ID_JZ4760,
+	.mem_map = jz4760_vpu_mem_map,
+
+	.num_clks = 2,
+	.num_mems = 3,
+};
+
+static const struct ingenic_soc_info jz4770_soc_info = {
+	.version = ID_JZ4770,
+	.mem_map = jz4770_vpu_mem_map,
+
+	.num_clks = 2,
+	.num_mems = 3,
+};
+
+static const struct ingenic_soc_info jz4775_soc_info = {
+	.version = ID_JZ4775,
+	.mem_map = jz4775_vpu_mem_map,
+
+	.num_clks = 1,
+	.num_mems = 2,
+};
+
+static const struct of_device_id ingenic_rproc_of_matches[] = {
+	{ .compatible = "ingenic,jz4760-vpu-rproc", .data = &jz4760_soc_info },
+	{ .compatible = "ingenic,jz4760b-vpu-rproc", .data = &jz4760_soc_info },
+	{ .compatible = "ingenic,jz4770-vpu-rproc", .data = &jz4770_soc_info },
+	{ .compatible = "ingenic,jz4775-vpu-rproc", .data = &jz4775_soc_info },
+	{ .compatible = "ingenic,jz4780-vpu-rproc", .data = &jz4775_soc_info },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
+
 static int ingenic_rproc_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *id = of_match_node(ingenic_rproc_of_matches, pdev->dev.of_node);
 	struct device *dev = &pdev->dev;
 	struct resource *mem;
 	struct rproc *rproc;
@@ -181,6 +243,7 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
 
 	vpu = rproc->priv;
 	vpu->dev = &pdev->dev;
+	vpu->soc_info = id->data;
 	platform_set_drvdata(pdev, vpu);
 
 	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
@@ -190,9 +253,13 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
 		return PTR_ERR(vpu->aux_base);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
+	vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
+	if (!vpu->mem_info)
+		return -ENOMEM;
+
+	for (i = 0; i < vpu->soc_info->num_mems; i++) {
 		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						   vpu_mem_map[i].name);
+						   vpu->soc_info->mem_map[i].name);
 
 		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
 		if (IS_ERR(vpu->mem_info[i].base)) {
@@ -202,13 +269,19 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
 		}
 
 		vpu->mem_info[i].len = resource_size(mem);
-		vpu->mem_info[i].map = &vpu_mem_map[i];
+		vpu->mem_info[i].map = &vpu->soc_info->mem_map[i];
 	}
 
+	vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * vpu->soc_info->num_clks, GFP_KERNEL);
+	if (!vpu->clks)
+		return -ENOMEM;
+
 	vpu->clks[0].id = "vpu";
-	vpu->clks[1].id = "aux";
 
-	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
+	if (vpu->soc_info->version == ID_JZ4770)
+		vpu->clks[1].id = "aux";
+
+	ret = devm_clk_bulk_get(dev, vpu->soc_info->num_clks, vpu->clks);
 	if (ret) {
 		dev_err(dev, "Failed to get clocks\n");
 		return ret;
@@ -235,12 +308,6 @@ static int ingenic_rproc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id ingenic_rproc_of_matches[] = {
-	{ .compatible = "ingenic,jz4770-vpu-rproc", },
-	{}
-};
-MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
-
 static struct platform_driver ingenic_rproc_driver = {
 	.probe = ingenic_rproc_probe,
 	.driver = {
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.
  2021-07-24  9:11 ` [PATCH 1/2] dt-bindings: remoteproc: Add bindings " 周琰杰 (Zhou Yanjie)
@ 2021-07-24 11:02   ` Paul Cercueil
  2021-07-24 13:06     ` Zhou Yanjie
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2021-07-24 11:02 UTC (permalink / raw)
  To: 周琰杰
  Cc: ohad, bjorn.andersson, mathieu.poirier, robh+dt, devicetree,
	linux-remoteproc, linux-mips, linux-kernel, dongsheng.qiu,
	aric.pzqi, rick.tyliu, sihui.liu, jun.jiang, sernia.zhou

Hi Zhou,

Le sam., juil. 24 2021 at 17:11:37 +0800, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
> the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  .../bindings/remoteproc/ingenic,vpu.yaml           | 74 
> ++++++++++++++++------
>  1 file changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml 
> b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> index d0aa91b..6154596 100644
> --- a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> @@ -17,31 +17,52 @@ maintainers:
> 
>  properties:
>    compatible:
> -    const: ingenic,jz4770-vpu-rproc
> +    enum:
> +      - ingenic,jz4760-vpu-rproc
> +      - ingenic,jz4760b-vpu-rproc
> +      - ingenic,jz4770-vpu-rproc
> +      - ingenic,jz4775-vpu-rproc
> +      - ingenic,jz4780-vpu-rproc
> 
>    reg:
> -    items:
> -      - description: aux registers
> -      - description: tcsm0 registers
> -      - description: tcsm1 registers
> -      - description: sram registers
> +    oneOf:
> +      - items:
> +          - description: aux registers
> +          - description: tcsm0 registers
> +          - description: tcsm1 registers
> +          - description: sram registers
> +      - items:
> +          - description: aux registers
> +          - description: tcsm registers
> +          - description: sram registers

Since we have "reg-names" already, we don't really need any 
description, so you could just have:

reg:
  minItems: 3
  maxItems: 4

> 
>    reg-names:
> -    items:
> -      - const: aux
> -      - const: tcsm0
> -      - const: tcsm1
> -      - const: sram
> +    oneOf:
> +      - items:
> +          - const: aux
> +          - const: tcsm0
> +          - const: tcsm1
> +          - const: sram
> +      - items:
> +          - const: aux
> +          - const: tcsm
> +          - const: sram

You could just add "tcsm" to the items list, and add:
minItems: 3
maxItems: 4

> 
>    clocks:
> -    items:
> -      - description: aux clock
> -      - description: vpu clock
> +    oneOf:
> +      - items:
> +          - description: aux clock
> +          - description: vpu clock
> +      - items:
> +          - description: vpu clock

Same as above, since we already have clock-names, the descriptions 
don't bring much.

You can replace with:

clocks:
  minItems: 1
  maxItems: 2

> 
>    clock-names:
> -    items:
> -      - const: aux
> -      - const: vpu
> +    oneOf:
> +      - items:
> +          - const: aux
> +          - const: vpu
> +      - items:
> +          - const: vpu

I think you could just add:
minItems: 1

Cheers,
-Paul

> 
>    interrupts:
>      maxItems: 1
> @@ -60,7 +81,7 @@ examples:
>    - |
>      #include <dt-bindings/clock/jz4770-cgu.h>
> 
> -    vpu: video-decoder@132a0000 {
> +    video-decoder@132a0000 {
>        compatible = "ingenic,jz4770-vpu-rproc";
> 
>        reg = <0x132a0000 0x20>, /* AUX */
> @@ -75,3 +96,20 @@ examples:
>        interrupt-parent = <&cpuintc>;
>        interrupts = <3>;
>      };
> +  - |
> +    #include <dt-bindings/clock/jz4780-cgu.h>
> +
> +    video-decoder@132a0000 {
> +      compatible = "ingenic,jz4780-vpu-rproc";
> +
> +      reg = <0x132a0000 0x20>, /* AUX */
> +            <0x132c0000 0x8000>, /* TCSM */
> +            <0x132f0000 0x4000>; /* SRAM */
> +      reg-names = "aux", "tcsm", "sram";
> +
> +      clocks = <&cgu JZ4780_CLK_VPU>;
> +      clock-names = "vpu";
> +
> +      interrupt-parent = <&intc>;
> +      interrupts = <62>;
> +    };
> --
> 2.7.4
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] remoteproc: Ingenic: Add support for new Ingenic SoCs.
  2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
@ 2021-07-24 11:15   ` Paul Cercueil
  2021-07-24 13:32     ` Zhou Yanjie
  2021-07-24 19:30   ` kernel test robot
  2021-07-25  1:26   ` kernel test robot
  2 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2021-07-24 11:15 UTC (permalink / raw)
  To: 周琰杰
  Cc: ohad, bjorn.andersson, mathieu.poirier, robh+dt, devicetree,
	linux-remoteproc, linux-mips, linux-kernel, dongsheng.qiu,
	aric.pzqi, rick.tyliu, sihui.liu, jun.jiang, sernia.zhou

Hi Zhou,

Le sam., juil. 24 2021 at 17:11:38 +0800, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> Add support for probing the ingenic_rproc driver on the JZ4760 SoC,
> the JZ4760B SoC, the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  drivers/remoteproc/ingenic_rproc.c | 115 
> +++++++++++++++++++++++++++++--------
>  1 file changed, 91 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/remoteproc/ingenic_rproc.c 
> b/drivers/remoteproc/ingenic_rproc.c
> index a356738..6a2e864 100644
> --- a/drivers/remoteproc/ingenic_rproc.c
> +++ b/drivers/remoteproc/ingenic_rproc.c
> @@ -2,6 +2,7 @@
>  /*
>   * Ingenic JZ47xx remoteproc driver
>   * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
> + * Copyright 2021, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com>
>   */
> 
>  #include <linux/bitops.h>
> @@ -17,7 +18,7 @@
> 
>  #define REG_AUX_CTRL		0x0
>  #define REG_AUX_MSG_ACK		0x10
> -#define REG_AUX_MSG		0x14
> +#define REG_AUX_MSG			0x14
>  #define REG_CORE_MSG_ACK	0x18
>  #define REG_CORE_MSG		0x1C
> 
> @@ -32,6 +33,20 @@ module_param(auto_boot, bool, 0400);
>  MODULE_PARM_DESC(auto_boot,
>  		 "Auto-boot the remote processor [default=false]");
> 
> +enum ingenic_vpu_version {
> +	ID_JZ4760,
> +	ID_JZ4770,
> +	ID_JZ4775,
> +};

The "version" field of ingenic_so_cinfo is not used anywhere, so you 
can drop this enum completely.

> +
> +struct ingenic_soc_info {
> +	enum ingenic_vpu_version version;
> +	const struct vpu_mem_map *mem_map;
> +
> +	unsigned int num_clks;
> +	unsigned int num_mems;
> +};
> +
>  struct vpu_mem_map {
>  	const char *name;
>  	unsigned int da;
> @@ -43,26 +58,21 @@ struct vpu_mem_info {
>  	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
>   * @aux_base: raw pointer to the AUX interface registers
> - * @mem_info: array of struct vpu_mem_info, which contain the 
> mapping info of
> + * @mem_info: pointers to the 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)];
> +	const struct ingenic_soc_info *soc_info;
> +	struct clk_bulk_data *clks;
> +	struct vpu_mem_info *mem_info;
>  	struct device *dev;
>  };
> 
> @@ -72,7 +82,7 @@ static int ingenic_rproc_prepare(struct rproc 
> *rproc)
>  	int ret;
> 
>  	/* The clocks must be enabled for the firmware to be loaded in TCSM 
> */
> -	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +	ret = clk_bulk_prepare_enable(vpu->soc_info->num_clks, vpu->clks);
>  	if (ret)
>  		dev_err(vpu->dev, "Unable to start clocks: %d\n", ret);
> 
> @@ -83,7 +93,7 @@ static int ingenic_rproc_unprepare(struct rproc 
> *rproc)
>  {
>  	struct vpu *vpu = rproc->priv;
> 
> -	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> +	clk_bulk_disable_unprepare(vpu->soc_info->num_clks, vpu->clks);
> 
>  	return 0;
>  }
> @@ -127,7 +137,7 @@ static void *ingenic_rproc_da_to_va(struct rproc 
> *rproc, u64 da, size_t len, boo
>  	void __iomem *va = NULL;
>  	unsigned int i;
> 
> -	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +	for (i = 0; i < vpu->soc_info->num_mems; i++) {
>  		const struct vpu_mem_info *info = &vpu->mem_info[i];
>  		const struct vpu_mem_map *map = info->map;
> 
> @@ -163,8 +173,60 @@ static irqreturn_t vpu_interrupt(int irq, void 
> *data)
>  	return rproc_vq_interrupt(rproc, vring);
>  }
> 
> +static const struct vpu_mem_map jz4760_vpu_mem_map[] = {
> +	{ "tcsm0", 0x132b0000 },
> +	{ "tcsm1", 0xf4000000 },
> +	{ "sram",  0x132d0000 },
> +};
> +
> +static const struct vpu_mem_map jz4770_vpu_mem_map[] = {
> +	{ "tcsm0", 0x132b0000 },
> +	{ "tcsm1", 0xf4000000 },
> +	{ "sram",  0x132f0000 },
> +};
> +
> +static const struct vpu_mem_map jz4775_vpu_mem_map[] = {
> +	{ "tcsm",  0xf4000000 },
> +	{ "sram",  0x132f0000 },
> +};
> +
> +static const struct ingenic_soc_info jz4760_soc_info = {
> +	.version = ID_JZ4760,
> +	.mem_map = jz4760_vpu_mem_map,
> +
> +	.num_clks = 2,
> +	.num_mems = 3,

.num_mems = ARRAY_SIZE(jz4760_vpu_mem_map),

And the same for the other ingenic_soc_info below.

> +};
> +
> +static const struct ingenic_soc_info jz4770_soc_info = {
> +	.version = ID_JZ4770,
> +	.mem_map = jz4770_vpu_mem_map,
> +
> +	.num_clks = 2,
> +	.num_mems = 3,
> +};
> +
> +static const struct ingenic_soc_info jz4775_soc_info = {
> +	.version = ID_JZ4775,
> +	.mem_map = jz4775_vpu_mem_map,
> +
> +	.num_clks = 1,
> +	.num_mems = 2,
> +};
> +
> +static const struct of_device_id ingenic_rproc_of_matches[] = {
> +	{ .compatible = "ingenic,jz4760-vpu-rproc", .data = 
> &jz4760_soc_info },
> +	{ .compatible = "ingenic,jz4760b-vpu-rproc", .data = 
> &jz4760_soc_info },
> +	{ .compatible = "ingenic,jz4770-vpu-rproc", .data = 
> &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4775-vpu-rproc", .data = 
> &jz4775_soc_info },
> +	{ .compatible = "ingenic,jz4780-vpu-rproc", .data = 
> &jz4775_soc_info },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
> +
>  static int ingenic_rproc_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *id = 
> of_match_node(ingenic_rproc_of_matches, pdev->dev.of_node);
>  	struct device *dev = &pdev->dev;
>  	struct resource *mem;
>  	struct rproc *rproc;
> @@ -181,6 +243,7 @@ static int ingenic_rproc_probe(struct 
> platform_device *pdev)
> 
>  	vpu = rproc->priv;
>  	vpu->dev = &pdev->dev;
> +	vpu->soc_info = id->data;

Use of_device_get_match_data(dev).

Then you can get rid of the "id" variable, and you won't have to move 
the "ingenic_rproc_of_matches" array.

>  	platform_set_drvdata(pdev, vpu);
> 
>  	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
> @@ -190,9 +253,13 @@ static int ingenic_rproc_probe(struct 
> platform_device *pdev)
>  		return PTR_ERR(vpu->aux_base);
>  	}
> 
> -	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +	vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * 
> vpu->soc_info->num_mems, GFP_KERNEL);

You are leaking memory here.

Also, why not just fix the current mem_info array size to 3? That 
sounds way simpler.

> +	if (!vpu->mem_info)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < vpu->soc_info->num_mems; i++) {
>  		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -						   vpu_mem_map[i].name);
> +						   vpu->soc_info->mem_map[i].name);
> 
>  		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
>  		if (IS_ERR(vpu->mem_info[i].base)) {
> @@ -202,13 +269,19 @@ static int ingenic_rproc_probe(struct 
> platform_device *pdev)
>  		}
> 
>  		vpu->mem_info[i].len = resource_size(mem);
> -		vpu->mem_info[i].map = &vpu_mem_map[i];
> +		vpu->mem_info[i].map = &vpu->soc_info->mem_map[i];
>  	}
> 
> +	vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * 
> vpu->soc_info->num_clks, GFP_KERNEL);

Same here, the "clks" array is already size 2, so it won't be a problem 
if you have only one clock. No need to alloc "clks" dynamically.

> +	if (!vpu->clks)
> +		return -ENOMEM;
> +
>  	vpu->clks[0].id = "vpu";
> -	vpu->clks[1].id = "aux";
> 
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
> +	if (vpu->soc_info->version == ID_JZ4770)
> +		vpu->clks[1].id = "aux";
> +
> +	ret = devm_clk_bulk_get(dev, vpu->soc_info->num_clks, vpu->clks);
>  	if (ret) {
>  		dev_err(dev, "Failed to get clocks\n");
>  		return ret;
> @@ -235,12 +308,6 @@ static int ingenic_rproc_probe(struct 
> platform_device *pdev)
>  	return 0;
>  }
> 
> -static const struct of_device_id ingenic_rproc_of_matches[] = {
> -	{ .compatible = "ingenic,jz4770-vpu-rproc", },
> -	{}
> -};
> -MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);

As I wrote above - you don't need to move this.

Cheers,
-Paul

> -
>  static struct platform_driver ingenic_rproc_driver = {
>  	.probe = ingenic_rproc_probe,
>  	.driver = {
> --
> 2.7.4
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.
  2021-07-24 11:02   ` Paul Cercueil
@ 2021-07-24 13:06     ` Zhou Yanjie
  0 siblings, 0 replies; 9+ messages in thread
From: Zhou Yanjie @ 2021-07-24 13:06 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: ohad, bjorn.andersson, mathieu.poirier, robh+dt, devicetree,
	linux-remoteproc, linux-mips, linux-kernel, dongsheng.qiu,
	aric.pzqi, rick.tyliu, sihui.liu, jun.jiang, sernia.zhou

Hi Paul,

On 2021/7/24 下午7:02, Paul Cercueil wrote:
> Hi Zhou,
>
> Le sam., juil. 24 2021 at 17:11:37 +0800, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
>> the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>  .../bindings/remoteproc/ingenic,vpu.yaml           | 74 
>> ++++++++++++++++------
>>  1 file changed, 56 insertions(+), 18 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml 
>> b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> index d0aa91b..6154596 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
>> @@ -17,31 +17,52 @@ maintainers:
>>
>>  properties:
>>    compatible:
>> -    const: ingenic,jz4770-vpu-rproc
>> +    enum:
>> +      - ingenic,jz4760-vpu-rproc
>> +      - ingenic,jz4760b-vpu-rproc
>> +      - ingenic,jz4770-vpu-rproc
>> +      - ingenic,jz4775-vpu-rproc
>> +      - ingenic,jz4780-vpu-rproc
>>
>>    reg:
>> -    items:
>> -      - description: aux registers
>> -      - description: tcsm0 registers
>> -      - description: tcsm1 registers
>> -      - description: sram registers
>> +    oneOf:
>> +      - items:
>> +          - description: aux registers
>> +          - description: tcsm0 registers
>> +          - description: tcsm1 registers
>> +          - description: sram registers
>> +      - items:
>> +          - description: aux registers
>> +          - description: tcsm registers
>> +          - description: sram registers
>
> Since we have "reg-names" already, we don't really need any 
> description, so you could just have:
>
> reg:
>  minItems: 3
>  maxItems: 4


Sure, I will do it in the next version.


>
>>
>>    reg-names:
>> -    items:
>> -      - const: aux
>> -      - const: tcsm0
>> -      - const: tcsm1
>> -      - const: sram
>> +    oneOf:
>> +      - items:
>> +          - const: aux
>> +          - const: tcsm0
>> +          - const: tcsm1
>> +          - const: sram
>> +      - items:
>> +          - const: aux
>> +          - const: tcsm
>> +          - const: sram
>
> You could just add "tcsm" to the items list, and add:
> minItems: 3
> maxItems: 4


Sure.


>
>>
>>    clocks:
>> -    items:
>> -      - description: aux clock
>> -      - description: vpu clock
>> +    oneOf:
>> +      - items:
>> +          - description: aux clock
>> +          - description: vpu clock
>> +      - items:
>> +          - description: vpu clock
>
> Same as above, since we already have clock-names, the descriptions 
> don't bring much.
>
> You can replace with:
>
> clocks:
>  minItems: 1
>  maxItems: 2


Sure.


>
>>
>>    clock-names:
>> -    items:
>> -      - const: aux
>> -      - const: vpu
>> +    oneOf:
>> +      - items:
>> +          - const: aux
>> +          - const: vpu
>> +      - items:
>> +          - const: vpu
>
> I think you could just add:
> minItems: 1


Sure.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>>
>>    interrupts:
>>      maxItems: 1
>> @@ -60,7 +81,7 @@ examples:
>>    - |
>>      #include <dt-bindings/clock/jz4770-cgu.h>
>>
>> -    vpu: video-decoder@132a0000 {
>> +    video-decoder@132a0000 {
>>        compatible = "ingenic,jz4770-vpu-rproc";
>>
>>        reg = <0x132a0000 0x20>, /* AUX */
>> @@ -75,3 +96,20 @@ examples:
>>        interrupt-parent = <&cpuintc>;
>>        interrupts = <3>;
>>      };
>> +  - |
>> +    #include <dt-bindings/clock/jz4780-cgu.h>
>> +
>> +    video-decoder@132a0000 {
>> +      compatible = "ingenic,jz4780-vpu-rproc";
>> +
>> +      reg = <0x132a0000 0x20>, /* AUX */
>> +            <0x132c0000 0x8000>, /* TCSM */
>> +            <0x132f0000 0x4000>; /* SRAM */
>> +      reg-names = "aux", "tcsm", "sram";
>> +
>> +      clocks = <&cgu JZ4780_CLK_VPU>;
>> +      clock-names = "vpu";
>> +
>> +      interrupt-parent = <&intc>;
>> +      interrupts = <62>;
>> +    };
>> -- 
>> 2.7.4
>>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] remoteproc: Ingenic: Add support for new Ingenic SoCs.
  2021-07-24 11:15   ` Paul Cercueil
@ 2021-07-24 13:32     ` Zhou Yanjie
  0 siblings, 0 replies; 9+ messages in thread
From: Zhou Yanjie @ 2021-07-24 13:32 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: ohad, bjorn.andersson, mathieu.poirier, robh+dt, devicetree,
	linux-remoteproc, linux-mips, linux-kernel, dongsheng.qiu,
	aric.pzqi, rick.tyliu, sihui.liu, jun.jiang, sernia.zhou

Hi Paul,

On 2021/7/24 下午7:15, Paul Cercueil wrote:
> Hi Zhou,
>
> Le sam., juil. 24 2021 at 17:11:38 +0800, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add support for probing the ingenic_rproc driver on the JZ4760 SoC,
>> the JZ4760B SoC, the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>  drivers/remoteproc/ingenic_rproc.c | 115 
>> +++++++++++++++++++++++++++++--------
>>  1 file changed, 91 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>> b/drivers/remoteproc/ingenic_rproc.c
>> index a356738..6a2e864 100644
>> --- a/drivers/remoteproc/ingenic_rproc.c
>> +++ b/drivers/remoteproc/ingenic_rproc.c
>> @@ -2,6 +2,7 @@
>>  /*
>>   * Ingenic JZ47xx remoteproc driver
>>   * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
>> + * Copyright 2021, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>   */
>>
>>  #include <linux/bitops.h>
>> @@ -17,7 +18,7 @@
>>
>>  #define REG_AUX_CTRL        0x0
>>  #define REG_AUX_MSG_ACK        0x10
>> -#define REG_AUX_MSG        0x14
>> +#define REG_AUX_MSG            0x14
>>  #define REG_CORE_MSG_ACK    0x18
>>  #define REG_CORE_MSG        0x1C
>>
>> @@ -32,6 +33,20 @@ module_param(auto_boot, bool, 0400);
>>  MODULE_PARM_DESC(auto_boot,
>>           "Auto-boot the remote processor [default=false]");
>>
>> +enum ingenic_vpu_version {
>> +    ID_JZ4760,
>> +    ID_JZ4770,
>> +    ID_JZ4775,
>> +};
>
> The "version" field of ingenic_so_cinfo is not used anywhere, so you 
> can drop this enum completely.


Sure, I will remove it.


>
>> +
>> +struct ingenic_soc_info {
>> +    enum ingenic_vpu_version version;
>> +    const struct vpu_mem_map *mem_map;
>> +
>> +    unsigned int num_clks;
>> +    unsigned int num_mems;
>> +};
>> +
>>  struct vpu_mem_map {
>>      const char *name;
>>      unsigned int da;
>> @@ -43,26 +58,21 @@ struct vpu_mem_info {
>>      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
>>   * @aux_base: raw pointer to the AUX interface registers
>> - * @mem_info: array of struct vpu_mem_info, which contain the 
>> mapping info of
>> + * @mem_info: pointers to the 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)];
>> +    const struct ingenic_soc_info *soc_info;
>> +    struct clk_bulk_data *clks;
>> +    struct vpu_mem_info *mem_info;
>>      struct device *dev;
>>  };
>>
>> @@ -72,7 +82,7 @@ static int ingenic_rproc_prepare(struct rproc *rproc)
>>      int ret;
>>
>>      /* The clocks must be enabled for the firmware to be loaded in 
>> TCSM */
>> -    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>> +    ret = clk_bulk_prepare_enable(vpu->soc_info->num_clks, vpu->clks);
>>      if (ret)
>>          dev_err(vpu->dev, "Unable to start clocks: %d\n", ret);
>>
>> @@ -83,7 +93,7 @@ static int ingenic_rproc_unprepare(struct rproc 
>> *rproc)
>>  {
>>      struct vpu *vpu = rproc->priv;
>>
>> -    clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>> +    clk_bulk_disable_unprepare(vpu->soc_info->num_clks, vpu->clks);
>>
>>      return 0;
>>  }
>> @@ -127,7 +137,7 @@ static void *ingenic_rproc_da_to_va(struct rproc 
>> *rproc, u64 da, size_t len, boo
>>      void __iomem *va = NULL;
>>      unsigned int i;
>>
>> -    for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>> +    for (i = 0; i < vpu->soc_info->num_mems; i++) {
>>          const struct vpu_mem_info *info = &vpu->mem_info[i];
>>          const struct vpu_mem_map *map = info->map;
>>
>> @@ -163,8 +173,60 @@ static irqreturn_t vpu_interrupt(int irq, void 
>> *data)
>>      return rproc_vq_interrupt(rproc, vring);
>>  }
>>
>> +static const struct vpu_mem_map jz4760_vpu_mem_map[] = {
>> +    { "tcsm0", 0x132b0000 },
>> +    { "tcsm1", 0xf4000000 },
>> +    { "sram",  0x132d0000 },
>> +};
>> +
>> +static const struct vpu_mem_map jz4770_vpu_mem_map[] = {
>> +    { "tcsm0", 0x132b0000 },
>> +    { "tcsm1", 0xf4000000 },
>> +    { "sram",  0x132f0000 },
>> +};
>> +
>> +static const struct vpu_mem_map jz4775_vpu_mem_map[] = {
>> +    { "tcsm",  0xf4000000 },
>> +    { "sram",  0x132f0000 },
>> +};
>> +
>> +static const struct ingenic_soc_info jz4760_soc_info = {
>> +    .version = ID_JZ4760,
>> +    .mem_map = jz4760_vpu_mem_map,
>> +
>> +    .num_clks = 2,
>> +    .num_mems = 3,
>
> .num_mems = ARRAY_SIZE(jz4760_vpu_mem_map),
>
> And the same for the other ingenic_soc_info below.


Sure.


>
>> +};
>> +
>> +static const struct ingenic_soc_info jz4770_soc_info = {
>> +    .version = ID_JZ4770,
>> +    .mem_map = jz4770_vpu_mem_map,
>> +
>> +    .num_clks = 2,
>> +    .num_mems = 3,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4775_soc_info = {
>> +    .version = ID_JZ4775,
>> +    .mem_map = jz4775_vpu_mem_map,
>> +
>> +    .num_clks = 1,
>> +    .num_mems = 2,
>> +};
>> +
>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>> +    { .compatible = "ingenic,jz4760-vpu-rproc", .data = 
>> &jz4760_soc_info },
>> +    { .compatible = "ingenic,jz4760b-vpu-rproc", .data = 
>> &jz4760_soc_info },
>> +    { .compatible = "ingenic,jz4770-vpu-rproc", .data = 
>> &jz4770_soc_info },
>> +    { .compatible = "ingenic,jz4775-vpu-rproc", .data = 
>> &jz4775_soc_info },
>> +    { .compatible = "ingenic,jz4780-vpu-rproc", .data = 
>> &jz4775_soc_info },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>> +
>>  static int ingenic_rproc_probe(struct platform_device *pdev)
>>  {
>> +    const struct of_device_id *id = 
>> of_match_node(ingenic_rproc_of_matches, pdev->dev.of_node);
>>      struct device *dev = &pdev->dev;
>>      struct resource *mem;
>>      struct rproc *rproc;
>> @@ -181,6 +243,7 @@ static int ingenic_rproc_probe(struct 
>> platform_device *pdev)
>>
>>      vpu = rproc->priv;
>>      vpu->dev = &pdev->dev;
>> +    vpu->soc_info = id->data;
>
> Use of_device_get_match_data(dev).
>
> Then you can get rid of the "id" variable, and you won't have to move 
> the "ingenic_rproc_of_matches" array.


Sure, I will try.


>
>>      platform_set_drvdata(pdev, vpu);
>>
>>      mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
>> @@ -190,9 +253,13 @@ static int ingenic_rproc_probe(struct 
>> platform_device *pdev)
>>          return PTR_ERR(vpu->aux_base);
>>      }
>>
>> -    for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>> +    vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * 
>> vpu->soc_info->num_mems, GFP_KERNEL);
>
> You are leaking memory here.
>
> Also, why not just fix the current mem_info array size to 3? That 
> sounds way simpler.


Sure.


>
>> +    if (!vpu->mem_info)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < vpu->soc_info->num_mems; i++) {
>>          mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> -                           vpu_mem_map[i].name);
>> + vpu->soc_info->mem_map[i].name);
>>
>>          vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
>>          if (IS_ERR(vpu->mem_info[i].base)) {
>> @@ -202,13 +269,19 @@ static int ingenic_rproc_probe(struct 
>> platform_device *pdev)
>>          }
>>
>>          vpu->mem_info[i].len = resource_size(mem);
>> -        vpu->mem_info[i].map = &vpu_mem_map[i];
>> +        vpu->mem_info[i].map = &vpu->soc_info->mem_map[i];
>>      }
>>
>> +    vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * 
>> vpu->soc_info->num_clks, GFP_KERNEL);
>
> Same here, the "clks" array is already size 2, so it won't be a 
> problem if you have only one clock. No need to alloc "clks" dynamically.


Sure.


>
>> +    if (!vpu->clks)
>> +        return -ENOMEM;
>> +
>>      vpu->clks[0].id = "vpu";
>> -    vpu->clks[1].id = "aux";
>>
>> -    ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
>> +    if (vpu->soc_info->version == ID_JZ4770)
>> +        vpu->clks[1].id = "aux";
>> +
>> +    ret = devm_clk_bulk_get(dev, vpu->soc_info->num_clks, vpu->clks);
>>      if (ret) {
>>          dev_err(dev, "Failed to get clocks\n");
>>          return ret;
>> @@ -235,12 +308,6 @@ static int ingenic_rproc_probe(struct 
>> platform_device *pdev)
>>      return 0;
>>  }
>>
>> -static const struct of_device_id ingenic_rproc_of_matches[] = {
>> -    { .compatible = "ingenic,jz4770-vpu-rproc", },
>> -    {}
>> -};
>> -MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>
> As I wrote above - you don't need to move this.


Sure.


Thanks and best regards!


>
> Cheers,
> -Paul
>
>> -
>>  static struct platform_driver ingenic_rproc_driver = {
>>      .probe = ingenic_rproc_probe,
>>      .driver = {
>> -- 
>> 2.7.4
>>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] remoteproc: Ingenic: Add support for new Ingenic SoCs.
  2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
  2021-07-24 11:15   ` Paul Cercueil
@ 2021-07-24 19:30   ` kernel test robot
  2021-07-25  1:26   ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-07-24 19:30 UTC (permalink / raw)
  To: 周琰杰 (Zhou Yanjie),
	ohad, bjorn.andersson, mathieu.poirier, robh+dt, paul
  Cc: kbuild-all, devicetree, linux-remoteproc, linux-mips,
	linux-kernel, dongsheng.qiu

[-- Attachment #1: Type: text/plain, Size: 5389 bytes --]

Hi "周琰杰,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v5.14-rc2 next-20210723]
[cannot apply to remoteproc/for-next rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zhou-Yanjie/Add-VPU-support-for-new-Ingenic-SoCs/20210724-171238
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/288ce023c75dca6dd232f6166be5a07d5532aad7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhou-Yanjie/Add-VPU-support-for-new-Ingenic-SoCs/20210724-171238
        git checkout 288ce023c75dca6dd232f6166be5a07d5532aad7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/remoteproc/ingenic_rproc.c: In function 'ingenic_rproc_probe':
   drivers/remoteproc/ingenic_rproc.c:256:18: error: implicit declaration of function 'kzalloc'; did you mean 'kvzalloc'? [-Werror=implicit-function-declaration]
     256 |  vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
         |                  ^~~~~~~
         |                  kvzalloc
>> drivers/remoteproc/ingenic_rproc.c:256:16: warning: assignment to 'struct vpu_mem_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     256 |  vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
         |                ^
>> drivers/remoteproc/ingenic_rproc.c:275:12: warning: assignment to 'struct clk_bulk_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     275 |  vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * vpu->soc_info->num_clks, GFP_KERNEL);
         |            ^
   cc1: some warnings being treated as errors


vim +256 drivers/remoteproc/ingenic_rproc.c

   226	
   227	static int ingenic_rproc_probe(struct platform_device *pdev)
   228	{
   229		const struct of_device_id *id = of_match_node(ingenic_rproc_of_matches, pdev->dev.of_node);
   230		struct device *dev = &pdev->dev;
   231		struct resource *mem;
   232		struct rproc *rproc;
   233		struct vpu *vpu;
   234		unsigned int i;
   235		int ret;
   236	
   237		rproc = devm_rproc_alloc(dev, "ingenic-vpu",
   238					 &ingenic_rproc_ops, NULL, sizeof(*vpu));
   239		if (!rproc)
   240			return -ENOMEM;
   241	
   242		rproc->auto_boot = auto_boot;
   243	
   244		vpu = rproc->priv;
   245		vpu->dev = &pdev->dev;
   246		vpu->soc_info = id->data;
   247		platform_set_drvdata(pdev, vpu);
   248	
   249		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
   250		vpu->aux_base = devm_ioremap_resource(dev, mem);
   251		if (IS_ERR(vpu->aux_base)) {
   252			dev_err(dev, "Failed to ioremap\n");
   253			return PTR_ERR(vpu->aux_base);
   254		}
   255	
 > 256		vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
   257		if (!vpu->mem_info)
   258			return -ENOMEM;
   259	
   260		for (i = 0; i < vpu->soc_info->num_mems; i++) {
   261			mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   262							   vpu->soc_info->mem_map[i].name);
   263	
   264			vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
   265			if (IS_ERR(vpu->mem_info[i].base)) {
   266				ret = PTR_ERR(vpu->mem_info[i].base);
   267				dev_err(dev, "Failed to ioremap\n");
   268				return ret;
   269			}
   270	
   271			vpu->mem_info[i].len = resource_size(mem);
   272			vpu->mem_info[i].map = &vpu->soc_info->mem_map[i];
   273		}
   274	
 > 275		vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * vpu->soc_info->num_clks, GFP_KERNEL);
   276		if (!vpu->clks)
   277			return -ENOMEM;
   278	
   279		vpu->clks[0].id = "vpu";
   280	
   281		if (vpu->soc_info->version == ID_JZ4770)
   282			vpu->clks[1].id = "aux";
   283	
   284		ret = devm_clk_bulk_get(dev, vpu->soc_info->num_clks, vpu->clks);
   285		if (ret) {
   286			dev_err(dev, "Failed to get clocks\n");
   287			return ret;
   288		}
   289	
   290		vpu->irq = platform_get_irq(pdev, 0);
   291		if (vpu->irq < 0)
   292			return vpu->irq;
   293	
   294		ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
   295		if (ret < 0) {
   296			dev_err(dev, "Failed to request IRQ\n");
   297			return ret;
   298		}
   299	
   300		disable_irq(vpu->irq);
   301	
   302		ret = devm_rproc_add(dev, rproc);
   303		if (ret) {
   304			dev_err(dev, "Failed to register remote processor\n");
   305			return ret;
   306		}
   307	
   308		return 0;
   309	}
   310	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69609 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] remoteproc: Ingenic: Add support for new Ingenic SoCs.
  2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
  2021-07-24 11:15   ` Paul Cercueil
  2021-07-24 19:30   ` kernel test robot
@ 2021-07-25  1:26   ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-07-25  1:26 UTC (permalink / raw)
  To: 周琰杰 (Zhou Yanjie),
	ohad, bjorn.andersson, mathieu.poirier, robh+dt, paul
  Cc: kbuild-all, devicetree, linux-remoteproc, linux-mips,
	linux-kernel, dongsheng.qiu

[-- Attachment #1: Type: text/plain, Size: 5379 bytes --]

Hi "周琰杰,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v5.14-rc2 next-20210723]
[cannot apply to remoteproc/for-next rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zhou-Yanjie/Add-VPU-support-for-new-Ingenic-SoCs/20210724-171238
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/288ce023c75dca6dd232f6166be5a07d5532aad7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhou-Yanjie/Add-VPU-support-for-new-Ingenic-SoCs/20210724-171238
        git checkout 288ce023c75dca6dd232f6166be5a07d5532aad7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/remoteproc/ingenic_rproc.c: In function 'ingenic_rproc_probe':
>> drivers/remoteproc/ingenic_rproc.c:256:18: error: implicit declaration of function 'kzalloc'; did you mean 'kvzalloc'? [-Werror=implicit-function-declaration]
     256 |  vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
         |                  ^~~~~~~
         |                  kvzalloc
   drivers/remoteproc/ingenic_rproc.c:256:16: warning: assignment to 'struct vpu_mem_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     256 |  vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
         |                ^
   drivers/remoteproc/ingenic_rproc.c:275:12: warning: assignment to 'struct clk_bulk_data *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     275 |  vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * vpu->soc_info->num_clks, GFP_KERNEL);
         |            ^
   cc1: some warnings being treated as errors


vim +256 drivers/remoteproc/ingenic_rproc.c

   226	
   227	static int ingenic_rproc_probe(struct platform_device *pdev)
   228	{
   229		const struct of_device_id *id = of_match_node(ingenic_rproc_of_matches, pdev->dev.of_node);
   230		struct device *dev = &pdev->dev;
   231		struct resource *mem;
   232		struct rproc *rproc;
   233		struct vpu *vpu;
   234		unsigned int i;
   235		int ret;
   236	
   237		rproc = devm_rproc_alloc(dev, "ingenic-vpu",
   238					 &ingenic_rproc_ops, NULL, sizeof(*vpu));
   239		if (!rproc)
   240			return -ENOMEM;
   241	
   242		rproc->auto_boot = auto_boot;
   243	
   244		vpu = rproc->priv;
   245		vpu->dev = &pdev->dev;
   246		vpu->soc_info = id->data;
   247		platform_set_drvdata(pdev, vpu);
   248	
   249		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
   250		vpu->aux_base = devm_ioremap_resource(dev, mem);
   251		if (IS_ERR(vpu->aux_base)) {
   252			dev_err(dev, "Failed to ioremap\n");
   253			return PTR_ERR(vpu->aux_base);
   254		}
   255	
 > 256		vpu->mem_info = kzalloc(sizeof(struct vpu_mem_info) * vpu->soc_info->num_mems, GFP_KERNEL);
   257		if (!vpu->mem_info)
   258			return -ENOMEM;
   259	
   260		for (i = 0; i < vpu->soc_info->num_mems; i++) {
   261			mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   262							   vpu->soc_info->mem_map[i].name);
   263	
   264			vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
   265			if (IS_ERR(vpu->mem_info[i].base)) {
   266				ret = PTR_ERR(vpu->mem_info[i].base);
   267				dev_err(dev, "Failed to ioremap\n");
   268				return ret;
   269			}
   270	
   271			vpu->mem_info[i].len = resource_size(mem);
   272			vpu->mem_info[i].map = &vpu->soc_info->mem_map[i];
   273		}
   274	
   275		vpu->clks = kzalloc(sizeof(struct clk_bulk_data) * vpu->soc_info->num_clks, GFP_KERNEL);
   276		if (!vpu->clks)
   277			return -ENOMEM;
   278	
   279		vpu->clks[0].id = "vpu";
   280	
   281		if (vpu->soc_info->version == ID_JZ4770)
   282			vpu->clks[1].id = "aux";
   283	
   284		ret = devm_clk_bulk_get(dev, vpu->soc_info->num_clks, vpu->clks);
   285		if (ret) {
   286			dev_err(dev, "Failed to get clocks\n");
   287			return ret;
   288		}
   289	
   290		vpu->irq = platform_get_irq(pdev, 0);
   291		if (vpu->irq < 0)
   292			return vpu->irq;
   293	
   294		ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
   295		if (ret < 0) {
   296			dev_err(dev, "Failed to request IRQ\n");
   297			return ret;
   298		}
   299	
   300		disable_irq(vpu->irq);
   301	
   302		ret = devm_rproc_add(dev, rproc);
   303		if (ret) {
   304			dev_err(dev, "Failed to register remote processor\n");
   305			return ret;
   306		}
   307	
   308		return 0;
   309	}
   310	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69609 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-25  1:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-24  9:11 [PATCH 0/2] Add VPU support for new Ingenic SoCs 周琰杰 (Zhou Yanjie)
2021-07-24  9:11 ` [PATCH 1/2] dt-bindings: remoteproc: Add bindings " 周琰杰 (Zhou Yanjie)
2021-07-24 11:02   ` Paul Cercueil
2021-07-24 13:06     ` Zhou Yanjie
2021-07-24  9:11 ` [PATCH 2/2] remoteproc: Ingenic: Add support " 周琰杰 (Zhou Yanjie)
2021-07-24 11:15   ` Paul Cercueil
2021-07-24 13:32     ` Zhou Yanjie
2021-07-24 19:30   ` kernel test robot
2021-07-25  1:26   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).