Messages in this thread | | | Date | Tue, 30 Apr 2024 12:10:26 -0700 | Subject | Re: [PATCH v7 6/6] remoteproc: qcom: enable in-kernel PD mapper | From | Chris Lew <> |
| |
On 4/26/2024 6:36 PM, Dmitry Baryshkov wrote: > On Sat, 27 Apr 2024 at 04:03, Chris Lew <quic_clew@quicinc.com> wrote: >> >> >> >> On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote: >>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c >>> index 1d24c9b656a8..02d0c626b03b 100644 >>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c >>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/remoteproc.h> >>> #include <linux/reset.h> >>> #include <linux/soc/qcom/mdt_loader.h> >>> +#include <linux/soc/qcom/pd_mapper.h> >>> #include <linux/soc/qcom/smem.h> >>> #include <linux/soc/qcom/smem_state.h> >>> >>> @@ -375,10 +376,14 @@ static int adsp_start(struct rproc *rproc) >>> int ret; >>> unsigned int val; >>> >>> - ret = qcom_q6v5_prepare(&adsp->q6v5); >>> + ret = qcom_pdm_get(); >>> if (ret) >>> return ret; >> >> Would it make sense to try and model this as a rproc subdev? This >> section of the remoteproc code seems to be focused on making specific >> calls to setup and enable hardware resources, where as pd mapper is >> software. >> >> sysmon and ssr are also purely software and they are modeled as subdevs >> in qcom_common. I'm not an expert on remoteproc organization but this >> was just a thought. > > Well, the issue is that the pd-mapper is a global, not a per-remoteproc instance >
Both sysmon and ssr have some kind of global states that they manage too. Each subdev functionality tends to be a mix of per-remoteproc instance management and global state management.
If pd-mapper was completely global, pd-mapper would be able to instantiate by itself. Instead, instantiation is dependent on each remoteproc instance properly getting and putting references.
The pdm subdev could manage the references to pd-mapper for that remoteproc instance.
On the other hand, I think Bjorn recommended this could be moved to probe time in v4. The v4 version was doing the reinitialization-dance, but I think the recommendation could still apply to this version.
>> >> Thanks! >> Chris >> >>> >>> + ret = qcom_q6v5_prepare(&adsp->q6v5); >>> + if (ret) >>> + goto put_pdm; >>> + >>> ret = adsp_map_carveout(rproc); >>> if (ret) { >>> dev_err(adsp->dev, "ADSP smmu mapping failed\n"); >>> @@ -446,6 +451,8 @@ static int adsp_start(struct rproc *rproc) >>> adsp_unmap_carveout(rproc); >>> disable_irqs: >>> qcom_q6v5_unprepare(&adsp->q6v5); >>> +put_pdm: >>> + qcom_pdm_release(); >>> >>> return ret; >>> } >> > >
| |