diff mbox series

[v12,07/11] remoteproc: mediatek: Control SCP core 1 by rproc subdevice

Message ID 20230517043449.26352-8-tinghan.shen@mediatek.com (mailing list archive)
State Superseded
Headers show
Series Add support for MT8195 SCP 2nd core | expand

Commit Message

Tinghan Shen May 17, 2023, 4:34 a.m. UTC 
Register SCP core 1 as a subdevice of core 0 for the boot sequence
and watchdog timeout handling. The core 1 has to boot after core 0
because the SCP clock and SRAM power is controlled by SCP core 0.
As for watchdog timeout handling, the remoteproc framework helps to
stop/start subdevices automatically when SCP driver receives watchdog
timeout event.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/remoteproc/mtk_common.h |  9 ++++
 drivers/remoteproc/mtk_scp.c    | 88 +++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

Comments

Mathieu Poirier May 17, 2023, 5:58 p.m. UTC | #1 
On Wed, May 17, 2023 at 12:34:45PM +0800, Tinghan Shen wrote:
> Register SCP core 1 as a subdevice of core 0 for the boot sequence
> and watchdog timeout handling. The core 1 has to boot after core 0
> because the SCP clock and SRAM power is controlled by SCP core 0.
> As for watchdog timeout handling, the remoteproc framework helps to
> stop/start subdevices automatically when SCP driver receives watchdog
> timeout event.
>

Subdevices should be reserved for virtio devices and not used for remote
processors.  Mixing both will get us into trouble at some point in the future.
The way to address power and clock dependencies is to use a different rproc_ops
for SCP1.  That rproc_ops would implemented the prepare() and unprepare()
functions to power up SCP1's power domain and deal with its clock.  

I will stop here for this revision.

Thanks,
Mathieu


> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/remoteproc/mtk_common.h |  9 ++++
>  drivers/remoteproc/mtk_scp.c    | 88 +++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> index 56395e8664cb..85afed2e6928 100644
> --- a/drivers/remoteproc/mtk_common.h
> +++ b/drivers/remoteproc/mtk_common.h
> @@ -100,6 +100,13 @@ struct mtk_scp_of_data {
>  	size_t ipi_buf_offset;
>  };
>  
> +struct mtk_scp_core_subdev {
> +	struct rproc_subdev subdev;
> +	struct mtk_scp *scp;
> +};
> +
> +#define to_core_subdev(d) container_of(d, struct mtk_scp_core_subdev, subdev)
> +
>  struct mtk_scp {
>  	struct device *dev;
>  	struct rproc *rproc;
> @@ -130,6 +137,8 @@ struct mtk_scp {
>  	struct rproc_subdev *rpmsg_subdev;
>  
>  	struct list_head elem;
> +	struct list_head *cluster;
> +	struct mtk_scp_core_subdev *core_subdev;
>  };
>  
>  /**
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index d644e232dfec..18534b1179b5 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -863,6 +863,60 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
>  	}
>  }
>  
> +static int scp_core_subdev_start(struct rproc_subdev *subdev)
> +{
> +	struct mtk_scp_core_subdev *core_subdev = to_core_subdev(subdev);
> +	struct mtk_scp *scp = core_subdev->scp;
> +
> +	rproc_boot(scp->rproc);
> +
> +	return 0;
> +}
> +
> +static void scp_core_subdev_stop(struct rproc_subdev *subdev, bool crashed)
> +{
> +	struct mtk_scp_core_subdev *core_subdev = to_core_subdev(subdev);
> +	struct mtk_scp *scp = core_subdev->scp;
> +
> +	rproc_shutdown(scp->rproc);
> +}
> +
> +static int scp_core_subdev_register(struct mtk_scp *scp)
> +{
> +	struct device *dev = scp->dev;
> +	struct mtk_scp_core_subdev *core_subdev;
> +	struct mtk_scp *scp_c0;
> +
> +	scp_c0 = list_first_entry(scp->cluster, struct mtk_scp, elem);
> +	if (!scp_c0)
> +		return -ENODATA;
> +
> +	core_subdev = devm_kzalloc(dev, sizeof(*core_subdev), GFP_KERNEL);
> +	if (!core_subdev)
> +		return -ENOMEM;
> +
> +	core_subdev->scp = scp;
> +	core_subdev->subdev.start = scp_core_subdev_start;
> +	core_subdev->subdev.stop = scp_core_subdev_stop;
> +
> +	scp->core_subdev = core_subdev;
> +	rproc_add_subdev(scp_c0->rproc, &scp->core_subdev->subdev);
> +
> +	return 0;
> +}
> +
> +static void scp_core_subdev_unregister(struct mtk_scp *scp)
> +{
> +	struct mtk_scp *scp_c0;
> +
> +	if (scp->core_subdev) {
> +		scp_c0 = list_first_entry(scp->cluster, struct mtk_scp, elem);
> +		rproc_remove_subdev(scp_c0->rproc, &scp->core_subdev->subdev);
> +		devm_kfree(scp->dev, scp->core_subdev);
> +		scp->core_subdev = NULL;
> +	}
> +}
> +
>  static struct mtk_scp *scp_rproc_init(struct platform_device *pdev,
>  				      struct mtk_scp_of_cluster *scp_cluster,
>  				      const struct mtk_scp_of_data *of_data)
> @@ -957,6 +1011,7 @@ static void scp_free(struct mtk_scp *scp)
>  {
>  	int i;
>  
> +	scp_core_subdev_unregister(scp);
>  	scp_remove_rpmsg_subdev(scp);
>  	scp_ipi_unregister(scp, SCP_IPI_INIT);
>  	scp_unmap_memory_region(scp);
> @@ -989,6 +1044,15 @@ static int scp_add_single_core(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void scp_rproc_boot_callback(const struct firmware *fw, void *context)
> +{
> +	struct rproc *rproc = context;
> +
> +	rproc_boot(rproc);
> +
> +	release_firmware(fw);
> +}
> +
>  static int scp_add_multi_core(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1029,6 +1093,20 @@ static int scp_add_multi_core(struct platform_device *pdev)
>  			goto init_fail;
>  		}
>  
> +		scp->cluster = cluster;
> +		if (!list_empty(cluster)) {
> +			ret = scp_core_subdev_register(scp);
> +			if (ret) {
> +				dev_err(dev, "Failed to register core %d as subdev\n", core_id);
> +				of_node_put(child);
> +				scp_free(scp);
> +				goto init_fail;
> +			}
> +		}
> +
> +		/* boot after all cores finished rproc_add() */
> +		scp->rproc->auto_boot = false;
> +
>  		ret = rproc_add(scp->rproc);
>  		if (ret) {
>  			dev_err(dev, "Failed to add rproc of core %d\n", core_id);
> @@ -1041,6 +1119,16 @@ static int scp_add_multi_core(struct platform_device *pdev)
>  		core_id++;
>  	}
>  
> +	/* boot core 0, and other cores are booted following core 0 as subdevices */
> +	scp = list_first_entry(cluster, struct mtk_scp, elem);
> +	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +				      scp->rproc->firmware, &scp->rproc->dev, GFP_KERNEL,
> +				      scp->rproc, scp_rproc_boot_callback);
> +	if (ret < 0) {
> +		dev_err(dev, "request_firmware_nowait err: %d\n", ret);
> +		goto init_fail;
> +	}
> +
>  	return 0;
>  
>  init_fail:
> -- 
> 2.18.0
>
Dan Carpenter May 18, 2023, 11:50 a.m. UTC | #2 
Hi Tinghan,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tinghan-Shen/dt-bindings-remoteproc-mediatek-Improve-the-rpmsg-subnode-definition/20230517-123659
base:   git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20230517043449.26352-8-tinghan.shen%40mediatek.com
patch subject: [PATCH v12 07/11] remoteproc: mediatek: Control SCP core 1 by rproc subdevice
config: csky-randconfig-m041-20230517
compiler: csky-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202305181957.ZMXChgJV-lkp@intel.com/

smatch warnings:
drivers/remoteproc/mtk_scp.c:891 scp_core_subdev_register() warn: can 'scp_c0' even be NULL?

vim +/scp_c0 +891 drivers/remoteproc/mtk_scp.c

69e1791d92cfa0 Tinghan Shen 2023-05-17  884  static int scp_core_subdev_register(struct mtk_scp *scp)
69e1791d92cfa0 Tinghan Shen 2023-05-17  885  {
69e1791d92cfa0 Tinghan Shen 2023-05-17  886  	struct device *dev = scp->dev;
69e1791d92cfa0 Tinghan Shen 2023-05-17  887  	struct mtk_scp_core_subdev *core_subdev;
69e1791d92cfa0 Tinghan Shen 2023-05-17  888  	struct mtk_scp *scp_c0;
69e1791d92cfa0 Tinghan Shen 2023-05-17  889  
69e1791d92cfa0 Tinghan Shen 2023-05-17  890  	scp_c0 = list_first_entry(scp->cluster, struct mtk_scp, elem);
69e1791d92cfa0 Tinghan Shen 2023-05-17 @891  	if (!scp_c0)
69e1791d92cfa0 Tinghan Shen 2023-05-17  892  		return -ENODATA;

This NULL check isn't right.  Use list_first_entry_or_null() if the list
can be empty.

69e1791d92cfa0 Tinghan Shen 2023-05-17  893  
69e1791d92cfa0 Tinghan Shen 2023-05-17  894  	core_subdev = devm_kzalloc(dev, sizeof(*core_subdev), GFP_KERNEL);
69e1791d92cfa0 Tinghan Shen 2023-05-17  895  	if (!core_subdev)
69e1791d92cfa0 Tinghan Shen 2023-05-17  896  		return -ENOMEM;
69e1791d92cfa0 Tinghan Shen 2023-05-17  897  
69e1791d92cfa0 Tinghan Shen 2023-05-17  898  	core_subdev->scp = scp;
69e1791d92cfa0 Tinghan Shen 2023-05-17  899  	core_subdev->subdev.start = scp_core_subdev_start;
69e1791d92cfa0 Tinghan Shen 2023-05-17  900  	core_subdev->subdev.stop = scp_core_subdev_stop;
69e1791d92cfa0 Tinghan Shen 2023-05-17  901  
69e1791d92cfa0 Tinghan Shen 2023-05-17  902  	scp->core_subdev = core_subdev;
69e1791d92cfa0 Tinghan Shen 2023-05-17  903  	rproc_add_subdev(scp_c0->rproc, &scp->core_subdev->subdev);
69e1791d92cfa0 Tinghan Shen 2023-05-17  904  
69e1791d92cfa0 Tinghan Shen 2023-05-17  905  	return 0;
69e1791d92cfa0 Tinghan Shen 2023-05-17  906  }
diff mbox series

Patch

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 56395e8664cb..85afed2e6928 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -100,6 +100,13 @@  struct mtk_scp_of_data {
 	size_t ipi_buf_offset;
 };
 
+struct mtk_scp_core_subdev {
+	struct rproc_subdev subdev;
+	struct mtk_scp *scp;
+};
+
+#define to_core_subdev(d) container_of(d, struct mtk_scp_core_subdev, subdev)
+
 struct mtk_scp {
 	struct device *dev;
 	struct rproc *rproc;
@@ -130,6 +137,8 @@  struct mtk_scp {
 	struct rproc_subdev *rpmsg_subdev;
 
 	struct list_head elem;
+	struct list_head *cluster;
+	struct mtk_scp_core_subdev *core_subdev;
 };
 
 /**
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index d644e232dfec..18534b1179b5 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -863,6 +863,60 @@  static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
 	}
 }
 
+static int scp_core_subdev_start(struct rproc_subdev *subdev)
+{
+	struct mtk_scp_core_subdev *core_subdev = to_core_subdev(subdev);
+	struct mtk_scp *scp = core_subdev->scp;
+
+	rproc_boot(scp->rproc);
+
+	return 0;
+}
+
+static void scp_core_subdev_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct mtk_scp_core_subdev *core_subdev = to_core_subdev(subdev);
+	struct mtk_scp *scp = core_subdev->scp;
+
+	rproc_shutdown(scp->rproc);
+}
+
+static int scp_core_subdev_register(struct mtk_scp *scp)
+{
+	struct device *dev = scp->dev;
+	struct mtk_scp_core_subdev *core_subdev;
+	struct mtk_scp *scp_c0;
+
+	scp_c0 = list_first_entry(scp->cluster, struct mtk_scp, elem);
+	if (!scp_c0)
+		return -ENODATA;
+
+	core_subdev = devm_kzalloc(dev, sizeof(*core_subdev), GFP_KERNEL);
+	if (!core_subdev)
+		return -ENOMEM;
+
+	core_subdev->scp = scp;
+	core_subdev->subdev.start = scp_core_subdev_start;
+	core_subdev->subdev.stop = scp_core_subdev_stop;
+
+	scp->core_subdev = core_subdev;
+	rproc_add_subdev(scp_c0->rproc, &scp->core_subdev->subdev);
+
+	return 0;
+}
+
+static void scp_core_subdev_unregister(struct mtk_scp *scp)
+{
+	struct mtk_scp *scp_c0;
+
+	if (scp->core_subdev) {
+		scp_c0 = list_first_entry(scp->cluster, struct mtk_scp, elem);
+		rproc_remove_subdev(scp_c0->rproc, &scp->core_subdev->subdev);
+		devm_kfree(scp->dev, scp->core_subdev);
+		scp->core_subdev = NULL;
+	}
+}
+
 static struct mtk_scp *scp_rproc_init(struct platform_device *pdev,
 				      struct mtk_scp_of_cluster *scp_cluster,
 				      const struct mtk_scp_of_data *of_data)
@@ -957,6 +1011,7 @@  static void scp_free(struct mtk_scp *scp)
 {
 	int i;
 
+	scp_core_subdev_unregister(scp);
 	scp_remove_rpmsg_subdev(scp);
 	scp_ipi_unregister(scp, SCP_IPI_INIT);
 	scp_unmap_memory_region(scp);
@@ -989,6 +1044,15 @@  static int scp_add_single_core(struct platform_device *pdev)
 	return 0;
 }
 
+static void scp_rproc_boot_callback(const struct firmware *fw, void *context)
+{
+	struct rproc *rproc = context;
+
+	rproc_boot(rproc);
+
+	release_firmware(fw);
+}
+
 static int scp_add_multi_core(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1029,6 +1093,20 @@  static int scp_add_multi_core(struct platform_device *pdev)
 			goto init_fail;
 		}
 
+		scp->cluster = cluster;
+		if (!list_empty(cluster)) {
+			ret = scp_core_subdev_register(scp);
+			if (ret) {
+				dev_err(dev, "Failed to register core %d as subdev\n", core_id);
+				of_node_put(child);
+				scp_free(scp);
+				goto init_fail;
+			}
+		}
+
+		/* boot after all cores finished rproc_add() */
+		scp->rproc->auto_boot = false;
+
 		ret = rproc_add(scp->rproc);
 		if (ret) {
 			dev_err(dev, "Failed to add rproc of core %d\n", core_id);
@@ -1041,6 +1119,16 @@  static int scp_add_multi_core(struct platform_device *pdev)
 		core_id++;
 	}
 
+	/* boot core 0, and other cores are booted following core 0 as subdevices */
+	scp = list_first_entry(cluster, struct mtk_scp, elem);
+	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
+				      scp->rproc->firmware, &scp->rproc->dev, GFP_KERNEL,
+				      scp->rproc, scp_rproc_boot_callback);
+	if (ret < 0) {
+		dev_err(dev, "request_firmware_nowait err: %d\n", ret);
+		goto init_fail;
+	}
+
 	return 0;
 
 init_fail: