From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sibi Sankar Subject: Re: [RFC PATCH v2] soc: qcom: rmtfs_mem: Control remoteproc from rmtfs_mem Date: Wed, 31 Oct 2018 19:34:57 +0530 Message-ID: <92359f8d35ef083f813461a1f4fd25ed@codeaurora.org> References: <20180930155646.20590-1-sibis@codeaurora.org> <20181021201624.GB1193@builder> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20181021201624.GB1193@builder> Sender: linux-kernel-owner@vger.kernel.org To: Bjorn Andersson Cc: briannorris@chromium.org, david.brown@linaro.org, robh+dt@kernel.org, mark.rutland@arm.com, andy.gross@linaro.org, akdwived@codeaurora.org, clew@codeaurora.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org 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 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.