Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Brian, Thanks for the review! On 2018-10-18 06:24, Brian Norris wrote: Hi Sibi, On Sun, Sep 30, 2018 at 09:26:46PM +0530, Sibi Sankar wrote: From: Bjorn Andersson rmtfs_mem provides access to physical storage and is crucial for the operation of the Qualcomm modem subsystem. The rmtfs_mem implementation must be available before the modem subsystem is booted and a solution where the modem remoteproc will verify that the rmtfs_mem is available has been discussed in the past. But this would not handle the case where the rmtfs_mem provider is restarted, which would cause fatal loss of access to the storage device for the modem. The suggestion is therefore to link the rmtfs_mem to its associated remote processor instance and control it based on the availability of the rmtfs_mem implementation. Signed-off-by: Bjorn Andersson [sibis: Added qmi lookup for Remote file system service] Signed-off-by: Sibi Sankar --- The currently implemented workaround in the Linaro QCOMLT releases is to blacklist the qcom_q6v5_pil kernel module and load this explicitly after rmtfs has been started. With this patch the modem module can be loaded automatically by the platform_bus and will only be booted as the rmtfs becomes available. Performing actions such as upgrading (and restarting) the rmtfs service will cause the modem to automatically restart and hence continue to function after the upgrade. v2: Remove rproc_boot/shutdown from rmtfs_mem open/release and add qmi lookup for Remote file system service to address Brian's race concerns. .../reserved-memory/qcom,rmtfs-mem.txt| 7 ++ drivers/remoteproc/qcom_q6v5_pil.c| 1 + drivers/soc/qcom/Kconfig | 2 + drivers/soc/qcom/rmtfs_mem.c | 65 ++- 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt index 8562ba1dce69..95b209e7f5d1 100644 --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt @@ -32,6 +32,13 @@ access block device data using the Remote Filesystem protocol. Value type: Definition: vmid of the remote processor, to set up memory protection. +- rproc: + Usage: optional + Value type: + Definition: reference to a remoteproc node, that should be powered up + while the remote file system memory instance is ready to + handle requests from the remote subsystem. + I'll repeat my comment here: this is straying far into the territory of putting software configuration in the device tree. Per your own comments, the modem firmware can be configured to run with or without a remote FS, and now you're assuming that the device tree will include this property or not, based on how you configured said firmware. That's not how device tree is supposed to work. Yes makes sense, will remove all dt dependencies in the next re-spin = EXAMPLE The following example shows the remote filesystem memory setup for APQ8016, with the rmtfs region for the Hexagon DSP (id #1) located at 0x8670. diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index d7a4b9eca5d2..1445a38e8b34 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -1142,6 +1142,7 @@ static int q6v5_probe(struct platform_device *pdev) qproc = (struct q6v5 *)rproc->priv; qproc->dev = >dev; qproc->rproc = rproc; + rproc->auto_boot = false; So how is it supposed to work when you have an internal filesystem for the modem? User space just knows about this, and manually starts the remoteproc? I somehow missed this Since the default firmware configuration for 8916/8996/845 has rmtfs dependency I plan on adding the qmi lookup by default till we get a platform that needs rmtfs disabled by default for which I could easily add a flag for rmtfs dependency in rproc_hexagon_res in qcom_q6v5_mss driver and do qmi lookup only if rmtfs is supported. platform_set_drvdata(pdev, qproc); ret = q6v5_init_mem(qproc, pdev); diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 8a7b8dea6990..4e3345944325 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -86,7 +86,9 @@ config QCOM_QMI_HELPERS config QCOM_RMTFS_MEM tristate "Qualcomm Remote Filesystem memory driver" depends on ARCH_QCOM + depends on REMOTEPROC select QCOM_SCM + select QCOM_QMI_HELPERS help The Qualcomm remote filesystem memory driver is used for allocating and exposing regions of shared memory with remote processors for the diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c index 97bb5989aa21..757e30083f67 100644 ---
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Brian, Thanks for the review! On 2018-10-18 06:24, Brian Norris wrote: Hi Sibi, On Sun, Sep 30, 2018 at 09:26:46PM +0530, Sibi Sankar wrote: From: Bjorn Andersson rmtfs_mem provides access to physical storage and is crucial for the operation of the Qualcomm modem subsystem. The rmtfs_mem implementation must be available before the modem subsystem is booted and a solution where the modem remoteproc will verify that the rmtfs_mem is available has been discussed in the past. But this would not handle the case where the rmtfs_mem provider is restarted, which would cause fatal loss of access to the storage device for the modem. The suggestion is therefore to link the rmtfs_mem to its associated remote processor instance and control it based on the availability of the rmtfs_mem implementation. Signed-off-by: Bjorn Andersson [sibis: Added qmi lookup for Remote file system service] Signed-off-by: Sibi Sankar --- The currently implemented workaround in the Linaro QCOMLT releases is to blacklist the qcom_q6v5_pil kernel module and load this explicitly after rmtfs has been started. With this patch the modem module can be loaded automatically by the platform_bus and will only be booted as the rmtfs becomes available. Performing actions such as upgrading (and restarting) the rmtfs service will cause the modem to automatically restart and hence continue to function after the upgrade. v2: Remove rproc_boot/shutdown from rmtfs_mem open/release and add qmi lookup for Remote file system service to address Brian's race concerns. .../reserved-memory/qcom,rmtfs-mem.txt| 7 ++ drivers/remoteproc/qcom_q6v5_pil.c| 1 + drivers/soc/qcom/Kconfig | 2 + drivers/soc/qcom/rmtfs_mem.c | 65 ++- 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt index 8562ba1dce69..95b209e7f5d1 100644 --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt @@ -32,6 +32,13 @@ access block device data using the Remote Filesystem protocol. Value type: Definition: vmid of the remote processor, to set up memory protection. +- rproc: + Usage: optional + Value type: + Definition: reference to a remoteproc node, that should be powered up + while the remote file system memory instance is ready to + handle requests from the remote subsystem. + I'll repeat my comment here: this is straying far into the territory of putting software configuration in the device tree. Per your own comments, the modem firmware can be configured to run with or without a remote FS, and now you're assuming that the device tree will include this property or not, based on how you configured said firmware. That's not how device tree is supposed to work. Yes makes sense, will remove all dt dependencies in the next re-spin = EXAMPLE The following example shows the remote filesystem memory setup for APQ8016, with the rmtfs region for the Hexagon DSP (id #1) located at 0x8670. diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index d7a4b9eca5d2..1445a38e8b34 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -1142,6 +1142,7 @@ static int q6v5_probe(struct platform_device *pdev) qproc = (struct q6v5 *)rproc->priv; qproc->dev = >dev; qproc->rproc = rproc; + rproc->auto_boot = false; So how is it supposed to work when you have an internal filesystem for the modem? User space just knows about this, and manually starts the remoteproc? I somehow missed this Since the default firmware configuration for 8916/8996/845 has rmtfs dependency I plan on adding the qmi lookup by default till we get a platform that needs rmtfs disabled by default for which I could easily add a flag for rmtfs dependency in rproc_hexagon_res in qcom_q6v5_mss driver and do qmi lookup only if rmtfs is supported. platform_set_drvdata(pdev, qproc); ret = q6v5_init_mem(qproc, pdev); diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 8a7b8dea6990..4e3345944325 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -86,7 +86,9 @@ config QCOM_QMI_HELPERS config QCOM_RMTFS_MEM tristate "Qualcomm Remote Filesystem memory driver" depends on ARCH_QCOM + depends on REMOTEPROC select QCOM_SCM + select QCOM_QMI_HELPERS help The Qualcomm remote filesystem memory driver is used for allocating and exposing regions of shared memory with remote processors for the diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c index 97bb5989aa21..757e30083f67 100644 ---
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Bjorn,
Thanks for the review!
On 2018-10-22 01:46, Bjorn Andersson wrote:
On Sun 30 Sep 08:56 PDT 2018, Sibi Sankar wrote:
From: Bjorn Andersson
rmtfs_mem provides access to physical storage and is crucial for the
operation of the Qualcomm modem subsystem.
The rmtfs_mem implementation must be available before the modem
subsystem is booted and a solution where the modem remoteproc will
verify that the rmtfs_mem is available has been discussed in the past.
But this would not handle the case where the rmtfs_mem provider is
restarted, which would cause fatal loss of access to the storage
device
for the modem.
The suggestion is therefore to link the rmtfs_mem to its associated
remote processor instance and control it based on the availability of
the rmtfs_mem implementation.
Signed-off-by: Bjorn Andersson
[sibis: Added qmi lookup for Remote file system service]
Signed-off-by: Sibi Sankar
Thanks Sibi,
This looks clean and straight forward, but I think the logic should be
moved into the qcom_q6v5_mss driver itself - as we now only care about
the QMI service being present, not the rmtfs_memory driver.
Will move it to qcom_q6v5_mss in the next re-spin.
The only drawback I found is that occasionally we receive the
the watchdog immediately after we kill the rmtfs application.
But eventually it gets handled as expected.
SDM845 Logs:
2360 root 0:00 rmtfs
/ # kill 2360
remoteproc: watchdog received: sys_m_smsm_mpss.c:285:APPS force stop
remoteproc0: crash detected in 408.remoteproc: type watchdog
qcom-q6v5-mss 408.remoteproc: timed out on wait
qcom-q6v5-mss 408.remoteproc: port failed halt
remoteproc remoteproc0: stopped remote processor 408.remoteproc
There's nothing left of my original patch, so please credit yourself as
author of v3.
[..]
diff --git a/drivers/soc/qcom/rmtfs_mem.c
b/drivers/soc/qcom/rmtfs_mem.c
[..]
@@ -181,6 +217,22 @@ static int qcom_rmtfs_mem_probe(struct
platform_device *pdev)
rmtfs_mem->client_id = client_id;
rmtfs_mem->size = rmem->size;
+ ret = of_property_read_u32(node, "rproc", _phandle);
+ if (!ret) {
+ rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle);
+ if (!rmtfs_mem->rproc)
+ return -EPROBE_DEFER;
+ }
+
+ ret = qmi_handle_init(_mem->rmtfs_hdl, 0,
+ _lookup_ops, NULL);
+ if (ret < 0)
+ goto put_rproc;
+
+ ret = qmi_add_lookup(_mem->rmtfs_hdl, 14, 0, 0);
The 14 here deserves a define and the whole thing would benefit from a
comment describing the remoteproc's dependency on the RMTFS service
being present.
Will add it in the respin
+ if (ret < 0)
+ goto err_release_qmi_handle;
+
device_initialize(_mem->dev);
rmtfs_mem->dev.parent = >dev;
rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
Looking forward to v3!
Done :)
Regards,
Bjorn
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Bjorn,
Thanks for the review!
On 2018-10-22 01:46, Bjorn Andersson wrote:
On Sun 30 Sep 08:56 PDT 2018, Sibi Sankar wrote:
From: Bjorn Andersson
rmtfs_mem provides access to physical storage and is crucial for the
operation of the Qualcomm modem subsystem.
The rmtfs_mem implementation must be available before the modem
subsystem is booted and a solution where the modem remoteproc will
verify that the rmtfs_mem is available has been discussed in the past.
But this would not handle the case where the rmtfs_mem provider is
restarted, which would cause fatal loss of access to the storage
device
for the modem.
The suggestion is therefore to link the rmtfs_mem to its associated
remote processor instance and control it based on the availability of
the rmtfs_mem implementation.
Signed-off-by: Bjorn Andersson
[sibis: Added qmi lookup for Remote file system service]
Signed-off-by: Sibi Sankar
Thanks Sibi,
This looks clean and straight forward, but I think the logic should be
moved into the qcom_q6v5_mss driver itself - as we now only care about
the QMI service being present, not the rmtfs_memory driver.
Will move it to qcom_q6v5_mss in the next re-spin.
The only drawback I found is that occasionally we receive the
the watchdog immediately after we kill the rmtfs application.
But eventually it gets handled as expected.
SDM845 Logs:
2360 root 0:00 rmtfs
/ # kill 2360
remoteproc: watchdog received: sys_m_smsm_mpss.c:285:APPS force stop
remoteproc0: crash detected in 408.remoteproc: type watchdog
qcom-q6v5-mss 408.remoteproc: timed out on wait
qcom-q6v5-mss 408.remoteproc: port failed halt
remoteproc remoteproc0: stopped remote processor 408.remoteproc
There's nothing left of my original patch, so please credit yourself as
author of v3.
[..]
diff --git a/drivers/soc/qcom/rmtfs_mem.c
b/drivers/soc/qcom/rmtfs_mem.c
[..]
@@ -181,6 +217,22 @@ static int qcom_rmtfs_mem_probe(struct
platform_device *pdev)
rmtfs_mem->client_id = client_id;
rmtfs_mem->size = rmem->size;
+ ret = of_property_read_u32(node, "rproc", _phandle);
+ if (!ret) {
+ rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle);
+ if (!rmtfs_mem->rproc)
+ return -EPROBE_DEFER;
+ }
+
+ ret = qmi_handle_init(_mem->rmtfs_hdl, 0,
+ _lookup_ops, NULL);
+ if (ret < 0)
+ goto put_rproc;
+
+ ret = qmi_add_lookup(_mem->rmtfs_hdl, 14, 0, 0);
The 14 here deserves a define and the whole thing would benefit from a
comment describing the remoteproc's dependency on the RMTFS service
being present.
Will add it in the respin
+ if (ret < 0)
+ goto err_release_qmi_handle;
+
device_initialize(_mem->dev);
rmtfs_mem->dev.parent = >dev;
rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
Looking forward to v3!
Done :)
Regards,
Bjorn
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
On Sun 30 Sep 08:56 PDT 2018, Sibi Sankar wrote:
> From: Bjorn Andersson
>
> rmtfs_mem provides access to physical storage and is crucial for the
> operation of the Qualcomm modem subsystem.
>
> The rmtfs_mem implementation must be available before the modem
> subsystem is booted and a solution where the modem remoteproc will
> verify that the rmtfs_mem is available has been discussed in the past.
> But this would not handle the case where the rmtfs_mem provider is
> restarted, which would cause fatal loss of access to the storage device
> for the modem.
>
> The suggestion is therefore to link the rmtfs_mem to its associated
> remote processor instance and control it based on the availability of
> the rmtfs_mem implementation.
>
> Signed-off-by: Bjorn Andersson
> [sibis: Added qmi lookup for Remote file system service]
> Signed-off-by: Sibi Sankar
Thanks Sibi,
This looks clean and straight forward, but I think the logic should be
moved into the qcom_q6v5_mss driver itself - as we now only care about
the QMI service being present, not the rmtfs_memory driver.
There's nothing left of my original patch, so please credit yourself as
author of v3.
[..]
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
[..]
> @@ -181,6 +217,22 @@ static int qcom_rmtfs_mem_probe(struct platform_device
> *pdev)
> rmtfs_mem->client_id = client_id;
> rmtfs_mem->size = rmem->size;
>
> + ret = of_property_read_u32(node, "rproc", _phandle);
> + if (!ret) {
> + rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle);
> + if (!rmtfs_mem->rproc)
> + return -EPROBE_DEFER;
> + }
> +
> + ret = qmi_handle_init(_mem->rmtfs_hdl, 0,
> + _lookup_ops, NULL);
> + if (ret < 0)
> + goto put_rproc;
> +
> + ret = qmi_add_lookup(_mem->rmtfs_hdl, 14, 0, 0);
The 14 here deserves a define and the whole thing would benefit from a
comment describing the remoteproc's dependency on the RMTFS service
being present.
> + if (ret < 0)
> + goto err_release_qmi_handle;
> +
> device_initialize(_mem->dev);
> rmtfs_mem->dev.parent = >dev;
> rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
Looking forward to v3!
Regards,
Bjorn
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
On Sun 30 Sep 08:56 PDT 2018, Sibi Sankar wrote:
> From: Bjorn Andersson
>
> rmtfs_mem provides access to physical storage and is crucial for the
> operation of the Qualcomm modem subsystem.
>
> The rmtfs_mem implementation must be available before the modem
> subsystem is booted and a solution where the modem remoteproc will
> verify that the rmtfs_mem is available has been discussed in the past.
> But this would not handle the case where the rmtfs_mem provider is
> restarted, which would cause fatal loss of access to the storage device
> for the modem.
>
> The suggestion is therefore to link the rmtfs_mem to its associated
> remote processor instance and control it based on the availability of
> the rmtfs_mem implementation.
>
> Signed-off-by: Bjorn Andersson
> [sibis: Added qmi lookup for Remote file system service]
> Signed-off-by: Sibi Sankar
Thanks Sibi,
This looks clean and straight forward, but I think the logic should be
moved into the qcom_q6v5_mss driver itself - as we now only care about
the QMI service being present, not the rmtfs_memory driver.
There's nothing left of my original patch, so please credit yourself as
author of v3.
[..]
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
[..]
> @@ -181,6 +217,22 @@ static int qcom_rmtfs_mem_probe(struct platform_device
> *pdev)
> rmtfs_mem->client_id = client_id;
> rmtfs_mem->size = rmem->size;
>
> + ret = of_property_read_u32(node, "rproc", _phandle);
> + if (!ret) {
> + rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle);
> + if (!rmtfs_mem->rproc)
> + return -EPROBE_DEFER;
> + }
> +
> + ret = qmi_handle_init(_mem->rmtfs_hdl, 0,
> + _lookup_ops, NULL);
> + if (ret < 0)
> + goto put_rproc;
> +
> + ret = qmi_add_lookup(_mem->rmtfs_hdl, 14, 0, 0);
The 14 here deserves a define and the whole thing would benefit from a
comment describing the remoteproc's dependency on the RMTFS service
being present.
> + if (ret < 0)
> + goto err_release_qmi_handle;
> +
> device_initialize(_mem->dev);
> rmtfs_mem->dev.parent = >dev;
> rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
Looking forward to v3!
Regards,
Bjorn
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Sibi,
On Sun, Sep 30, 2018 at 09:26:46PM +0530, Sibi Sankar wrote:
> From: Bjorn Andersson
>
> rmtfs_mem provides access to physical storage and is crucial for the
> operation of the Qualcomm modem subsystem.
>
> The rmtfs_mem implementation must be available before the modem
> subsystem is booted and a solution where the modem remoteproc will
> verify that the rmtfs_mem is available has been discussed in the past.
> But this would not handle the case where the rmtfs_mem provider is
> restarted, which would cause fatal loss of access to the storage device
> for the modem.
>
> The suggestion is therefore to link the rmtfs_mem to its associated
> remote processor instance and control it based on the availability of
> the rmtfs_mem implementation.
>
> Signed-off-by: Bjorn Andersson
> [sibis: Added qmi lookup for Remote file system service]
> Signed-off-by: Sibi Sankar
> ---
>
> The currently implemented workaround in the Linaro QCOMLT releases is to
> blacklist the qcom_q6v5_pil kernel module and load this explicitly after rmtfs
> has been started.
>
> With this patch the modem module can be loaded automatically by the
> platform_bus and will only be booted as the rmtfs becomes available.
> Performing
> actions such as upgrading (and restarting) the rmtfs service will cause the
> modem to automatically restart and hence continue to function after the
> upgrade.
>
> v2:
> Remove rproc_boot/shutdown from rmtfs_mem open/release and add
> qmi lookup for Remote file system service to address Brian's
> race concerns.
>
> .../reserved-memory/qcom,rmtfs-mem.txt| 7 ++
> drivers/remoteproc/qcom_q6v5_pil.c| 1 +
> drivers/soc/qcom/Kconfig | 2 +
> drivers/soc/qcom/rmtfs_mem.c | 65 ++-
> 4 files changed, 72 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> index 8562ba1dce69..95b209e7f5d1 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> @@ -32,6 +32,13 @@ access block device data using the Remote Filesystem
> protocol.
> Value type:
> Definition: vmid of the remote processor, to set up memory protection.
>
> +- rproc:
> + Usage: optional
> + Value type:
> + Definition: reference to a remoteproc node, that should be powered up
> + while the remote file system memory instance is ready to
> + handle requests from the remote subsystem.
> +
I'll repeat my comment here: this is straying far into the territory of
putting software configuration in the device tree. Per your own
comments, the modem firmware can be configured to run with or without a
remote FS, and now you're assuming that the device tree will include
this property or not, based on how you configured said firmware. That's
not how device tree is supposed to work.
> = EXAMPLE
> The following example shows the remote filesystem memory setup for APQ8016,
> with the rmtfs region for the Hexagon DSP (id #1) located at 0x8670.
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> b/drivers/remoteproc/qcom_q6v5_pil.c
> index d7a4b9eca5d2..1445a38e8b34 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1142,6 +1142,7 @@ static int q6v5_probe(struct platform_device *pdev)
> qproc = (struct q6v5 *)rproc->priv;
> qproc->dev = >dev;
> qproc->rproc = rproc;
> + rproc->auto_boot = false;
So how is it supposed to work when you have an internal filesystem for
the modem? User space just knows about this, and manually starts the
remoteproc?
> platform_set_drvdata(pdev, qproc);
>
> ret = q6v5_init_mem(qproc, pdev);
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 8a7b8dea6990..4e3345944325 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -86,7 +86,9 @@ config QCOM_QMI_HELPERS
> config QCOM_RMTFS_MEM
> tristate "Qualcomm Remote Filesystem memory driver"
> depends on ARCH_QCOM
> + depends on REMOTEPROC
> select QCOM_SCM
> + select QCOM_QMI_HELPERS
> help
> The Qualcomm remote filesystem memory driver is used for allocating
> and exposing regions of shared memory with remote processors for the
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 97bb5989aa21..757e30083f67 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -18,11 +18,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
>
> #define QCOM_RMTFS_MEM_DEV_MAX (MINORMASK + 1)
>
> @@ -31,6 +33,7 @@ static dev_t qcom_rmtfs_mem_major;
> struct qcom_rmtfs_mem {
>
Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem
Hi Sibi,
On Sun, Sep 30, 2018 at 09:26:46PM +0530, Sibi Sankar wrote:
> From: Bjorn Andersson
>
> rmtfs_mem provides access to physical storage and is crucial for the
> operation of the Qualcomm modem subsystem.
>
> The rmtfs_mem implementation must be available before the modem
> subsystem is booted and a solution where the modem remoteproc will
> verify that the rmtfs_mem is available has been discussed in the past.
> But this would not handle the case where the rmtfs_mem provider is
> restarted, which would cause fatal loss of access to the storage device
> for the modem.
>
> The suggestion is therefore to link the rmtfs_mem to its associated
> remote processor instance and control it based on the availability of
> the rmtfs_mem implementation.
>
> Signed-off-by: Bjorn Andersson
> [sibis: Added qmi lookup for Remote file system service]
> Signed-off-by: Sibi Sankar
> ---
>
> The currently implemented workaround in the Linaro QCOMLT releases is to
> blacklist the qcom_q6v5_pil kernel module and load this explicitly after rmtfs
> has been started.
>
> With this patch the modem module can be loaded automatically by the
> platform_bus and will only be booted as the rmtfs becomes available.
> Performing
> actions such as upgrading (and restarting) the rmtfs service will cause the
> modem to automatically restart and hence continue to function after the
> upgrade.
>
> v2:
> Remove rproc_boot/shutdown from rmtfs_mem open/release and add
> qmi lookup for Remote file system service to address Brian's
> race concerns.
>
> .../reserved-memory/qcom,rmtfs-mem.txt| 7 ++
> drivers/remoteproc/qcom_q6v5_pil.c| 1 +
> drivers/soc/qcom/Kconfig | 2 +
> drivers/soc/qcom/rmtfs_mem.c | 65 ++-
> 4 files changed, 72 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> index 8562ba1dce69..95b209e7f5d1 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/qcom,rmtfs-mem.txt
> @@ -32,6 +32,13 @@ access block device data using the Remote Filesystem
> protocol.
> Value type:
> Definition: vmid of the remote processor, to set up memory protection.
>
> +- rproc:
> + Usage: optional
> + Value type:
> + Definition: reference to a remoteproc node, that should be powered up
> + while the remote file system memory instance is ready to
> + handle requests from the remote subsystem.
> +
I'll repeat my comment here: this is straying far into the territory of
putting software configuration in the device tree. Per your own
comments, the modem firmware can be configured to run with or without a
remote FS, and now you're assuming that the device tree will include
this property or not, based on how you configured said firmware. That's
not how device tree is supposed to work.
> = EXAMPLE
> The following example shows the remote filesystem memory setup for APQ8016,
> with the rmtfs region for the Hexagon DSP (id #1) located at 0x8670.
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> b/drivers/remoteproc/qcom_q6v5_pil.c
> index d7a4b9eca5d2..1445a38e8b34 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1142,6 +1142,7 @@ static int q6v5_probe(struct platform_device *pdev)
> qproc = (struct q6v5 *)rproc->priv;
> qproc->dev = >dev;
> qproc->rproc = rproc;
> + rproc->auto_boot = false;
So how is it supposed to work when you have an internal filesystem for
the modem? User space just knows about this, and manually starts the
remoteproc?
> platform_set_drvdata(pdev, qproc);
>
> ret = q6v5_init_mem(qproc, pdev);
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 8a7b8dea6990..4e3345944325 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -86,7 +86,9 @@ config QCOM_QMI_HELPERS
> config QCOM_RMTFS_MEM
> tristate "Qualcomm Remote Filesystem memory driver"
> depends on ARCH_QCOM
> + depends on REMOTEPROC
> select QCOM_SCM
> + select QCOM_QMI_HELPERS
> help
> The Qualcomm remote filesystem memory driver is used for allocating
> and exposing regions of shared memory with remote processors for the
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 97bb5989aa21..757e30083f67 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -18,11 +18,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
>
> #define QCOM_RMTFS_MEM_DEV_MAX (MINORMASK + 1)
>
> @@ -31,6 +33,7 @@ static dev_t qcom_rmtfs_mem_major;
> struct qcom_rmtfs_mem {
>
