Messages in this thread Patch in this message | | | Date | Wed, 31 Oct 2018 19:34:57 +0530 | From | Sibi Sankar <> | Subject | 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 <bjorn.andersson@linaro.org> >> >> 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 <bjorn.andersson@linaro.org> >> [sibis: Added qmi lookup for Remote file system service] >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > > 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 4080000.remoteproc: type watchdog qcom-q6v5-mss 4080000.remoteproc: timed out on wait qcom-q6v5-mss 4080000.remoteproc: port failed halt remoteproc remoteproc0: stopped remote processor 4080000.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", &rproc_phandle); >> + if (!ret) { >> + rmtfs_mem->rproc = rproc_get_by_phandle(rproc_phandle); >> + if (!rmtfs_mem->rproc) >> + return -EPROBE_DEFER; >> + } >> + >> + ret = qmi_handle_init(&rmtfs_mem->rmtfs_hdl, 0, >> + &rmtfs_lookup_ops, NULL); >> + if (ret < 0) >> + goto put_rproc; >> + >> + ret = qmi_add_lookup(&rmtfs_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(&rmtfs_mem->dev); >> rmtfs_mem->dev.parent = &pdev->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.
| |