| Message ID | 20220225033224.2238425-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | remoteproc: qcom: q6v5: Add interconnect path proxy vote | expand |
On 25.02.22 5:32, Bjorn Andersson wrote: > Many remoteproc instances requires that Linux casts a proxy vote for an > interconnect path during boot, until they can do it themselves. Add > support for voting for a single path. > > As this is a shared problem between both PAS and MSS drivers, the path > is acquired and votes casted from the common helper code. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Sibi posted recently a patch to add interconnect votes from the modem driver, > today I needed the same feature for one of the PAS remoteprocs. After > essentially duplicating Sibi's patch I realized that it doesn't look too bad to > put this in the common Q6V5 code. > > The main difference is that this would be messy if we need to support multiple > paths, so we probably would have to push it out to the individual drivers at > that point. > > Sibi's patch can be found here. > https://lore.kernel.org/all/1644813252-12897-3-git-send-email-quic_sibis@quicinc.com/ > > > This makes the implementation pick up one path, relevant DT bindings would > still need to be updated in order be allowed to this in the DeviceTree files. > > drivers/remoteproc/qcom_q6v5.c | 21 ++++++++++++++++++++- > drivers/remoteproc/qcom_q6v5.h | 3 +++ > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c > index 442a388f8102..5280ec9b5449 100644 > --- a/drivers/remoteproc/qcom_q6v5.c > +++ b/drivers/remoteproc/qcom_q6v5.c > @@ -8,6 +8,7 @@ > */ > #include <linux/kernel.h> > #include <linux/platform_device.h> > +#include <linux/interconnect.h> > #include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/soc/qcom/qcom_aoss.h> > @@ -51,9 +52,17 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5) > { > int ret; > > + ret = icc_set_bw(q6v5->path, 0, UINT_MAX); > + if (ret < 0) { > + dev_err(q6v5->dev, "failed to set bandwidth request\n"); > + return ret; > + } > + > ret = q6v5_load_state_toggle(q6v5, true); > - if (ret) > + if (ret) { > + icc_set_bw(q6v5->path, 0, 0); > return ret; > + } > > reinit_completion(&q6v5->start_done); > reinit_completion(&q6v5->stop_done); > @@ -78,6 +87,9 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) > disable_irq(q6v5->handover_irq); > q6v5_load_state_toggle(q6v5, false); > > + /* Disable interconnect vote, in case handover never happened */ > + icc_set_bw(q6v5->path, 0, 0); > + > return !q6v5->handover_issued; > } > EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); > @@ -160,6 +172,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data) > if (q6v5->handover) > q6v5->handover(q6v5); > > + icc_set_bw(q6v5->path, 0, 0); > + > q6v5->handover_issued = true; > > return IRQ_HANDLED; > @@ -332,6 +346,11 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > return load_state ? -ENOMEM : -EINVAL; > } > > + q6v5->path = devm_of_icc_get(&pdev->dev, NULL); > + if (IS_ERR(q6v5->path)) > + return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), > + "failed to acquire interconnect path\n"); > + > return 0; > } > EXPORT_SYMBOL_GPL(qcom_q6v5_init); Probably we should also call icc_put(q6v5->path) in qcom_q6v5_deinit(). Reviewed-by: Georgi Djakov <djakov@kernel.org> Thanks, Georgi
On Mon 07 Mar 10:35 PST 2022, Georgi Djakov wrote: > > On 25.02.22 5:32, Bjorn Andersson wrote: > > Many remoteproc instances requires that Linux casts a proxy vote for an > > interconnect path during boot, until they can do it themselves. Add > > support for voting for a single path. > > > > As this is a shared problem between both PAS and MSS drivers, the path > > is acquired and votes casted from the common helper code. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Sibi posted recently a patch to add interconnect votes from the modem driver, > > today I needed the same feature for one of the PAS remoteprocs. After > > essentially duplicating Sibi's patch I realized that it doesn't look too bad to > > put this in the common Q6V5 code. > > > > The main difference is that this would be messy if we need to support multiple > > paths, so we probably would have to push it out to the individual drivers at > > that point. > > > > Sibi's patch can be found here. > > https://lore.kernel.org/all/1644813252-12897-3-git-send-email-quic_sibis@quicinc.com/ > > > > > > This makes the implementation pick up one path, relevant DT bindings would > > still need to be updated in order be allowed to this in the DeviceTree files. > > > > drivers/remoteproc/qcom_q6v5.c | 21 ++++++++++++++++++++- > > drivers/remoteproc/qcom_q6v5.h | 3 +++ > > 2 files changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c > > index 442a388f8102..5280ec9b5449 100644 > > --- a/drivers/remoteproc/qcom_q6v5.c > > +++ b/drivers/remoteproc/qcom_q6v5.c > > @@ -8,6 +8,7 @@ > > */ > > #include <linux/kernel.h> > > #include <linux/platform_device.h> > > +#include <linux/interconnect.h> > > #include <linux/interrupt.h> > > #include <linux/module.h> > > #include <linux/soc/qcom/qcom_aoss.h> > > @@ -51,9 +52,17 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5) > > { > > int ret; > > + ret = icc_set_bw(q6v5->path, 0, UINT_MAX); > > + if (ret < 0) { > > + dev_err(q6v5->dev, "failed to set bandwidth request\n"); > > + return ret; > > + } > > + > > ret = q6v5_load_state_toggle(q6v5, true); > > - if (ret) > > + if (ret) { > > + icc_set_bw(q6v5->path, 0, 0); > > return ret; > > + } > > reinit_completion(&q6v5->start_done); > > reinit_completion(&q6v5->stop_done); > > @@ -78,6 +87,9 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) > > disable_irq(q6v5->handover_irq); > > q6v5_load_state_toggle(q6v5, false); > > + /* Disable interconnect vote, in case handover never happened */ > > + icc_set_bw(q6v5->path, 0, 0); > > + > > return !q6v5->handover_issued; > > } > > EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); > > @@ -160,6 +172,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data) > > if (q6v5->handover) > > q6v5->handover(q6v5); > > + icc_set_bw(q6v5->path, 0, 0); > > + > > q6v5->handover_issued = true; > > return IRQ_HANDLED; > > @@ -332,6 +346,11 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > > return load_state ? -ENOMEM : -EINVAL; > > } > > + q6v5->path = devm_of_icc_get(&pdev->dev, NULL); > > + if (IS_ERR(q6v5->path)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), > > + "failed to acquire interconnect path\n"); > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(qcom_q6v5_init); > > Probably we should also call icc_put(q6v5->path) in qcom_q6v5_deinit(). > The use of devm_of_icc_get() should take care of that for us. Or am I missing something? > Reviewed-by: Georgi Djakov <djakov@kernel.org> > Thanks, Bjorn
On 8.03.22 0:42, Bjorn Andersson wrote: > On Mon 07 Mar 10:35 PST 2022, Georgi Djakov wrote: > >> >> On 25.02.22 5:32, Bjorn Andersson wrote: >>> Many remoteproc instances requires that Linux casts a proxy vote for an >>> interconnect path during boot, until they can do it themselves. Add >>> support for voting for a single path. >>> >>> As this is a shared problem between both PAS and MSS drivers, the path >>> is acquired and votes casted from the common helper code. >>> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>> --- >>> >>> Sibi posted recently a patch to add interconnect votes from the modem driver, >>> today I needed the same feature for one of the PAS remoteprocs. After >>> essentially duplicating Sibi's patch I realized that it doesn't look too bad to >>> put this in the common Q6V5 code. >>> >>> The main difference is that this would be messy if we need to support multiple >>> paths, so we probably would have to push it out to the individual drivers at >>> that point. >>> >>> Sibi's patch can be found here. >>> https://lore.kernel.org/all/1644813252-12897-3-git-send-email-quic_sibis@quicinc.com/ >>> >>> >>> This makes the implementation pick up one path, relevant DT bindings would >>> still need to be updated in order be allowed to this in the DeviceTree files. >>> >>> drivers/remoteproc/qcom_q6v5.c | 21 ++++++++++++++++++++- >>> drivers/remoteproc/qcom_q6v5.h | 3 +++ >>> 2 files changed, 23 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c >>> index 442a388f8102..5280ec9b5449 100644 >>> --- a/drivers/remoteproc/qcom_q6v5.c >>> +++ b/drivers/remoteproc/qcom_q6v5.c >>> @@ -8,6 +8,7 @@ >>> */ >>> #include <linux/kernel.h> >>> #include <linux/platform_device.h> >>> +#include <linux/interconnect.h> >>> #include <linux/interrupt.h> >>> #include <linux/module.h> >>> #include <linux/soc/qcom/qcom_aoss.h> >>> @@ -51,9 +52,17 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5) >>> { >>> int ret; >>> + ret = icc_set_bw(q6v5->path, 0, UINT_MAX); >>> + if (ret < 0) { >>> + dev_err(q6v5->dev, "failed to set bandwidth request\n"); >>> + return ret; >>> + } >>> + >>> ret = q6v5_load_state_toggle(q6v5, true); >>> - if (ret) >>> + if (ret) { >>> + icc_set_bw(q6v5->path, 0, 0); >>> return ret; >>> + } >>> reinit_completion(&q6v5->start_done); >>> reinit_completion(&q6v5->stop_done); >>> @@ -78,6 +87,9 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) >>> disable_irq(q6v5->handover_irq); >>> q6v5_load_state_toggle(q6v5, false); >>> + /* Disable interconnect vote, in case handover never happened */ >>> + icc_set_bw(q6v5->path, 0, 0); >>> + >>> return !q6v5->handover_issued; >>> } >>> EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); >>> @@ -160,6 +172,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data) >>> if (q6v5->handover) >>> q6v5->handover(q6v5); >>> + icc_set_bw(q6v5->path, 0, 0); >>> + >>> q6v5->handover_issued = true; >>> return IRQ_HANDLED; >>> @@ -332,6 +346,11 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, >>> return load_state ? -ENOMEM : -EINVAL; >>> } >>> + q6v5->path = devm_of_icc_get(&pdev->dev, NULL); >>> + if (IS_ERR(q6v5->path)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), >>> + "failed to acquire interconnect path\n"); >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(qcom_q6v5_init); >> >> Probably we should also call icc_put(q6v5->path) in qcom_q6v5_deinit(). >> > > The use of devm_of_icc_get() should take care of that for us. Or am I > missing something? Right, it's fine then. Sorry for the noise. BR, Georgi >> Reviewed-by: Georgi Djakov <djakov@kernel.org> >> > > Thanks, > Bjorn
diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 442a388f8102..5280ec9b5449 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -8,6 +8,7 @@ */ #include <linux/kernel.h> #include <linux/platform_device.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/module.h> #include <linux/soc/qcom/qcom_aoss.h> @@ -51,9 +52,17 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5) { int ret; + ret = icc_set_bw(q6v5->path, 0, UINT_MAX); + if (ret < 0) { + dev_err(q6v5->dev, "failed to set bandwidth request\n"); + return ret; + } + ret = q6v5_load_state_toggle(q6v5, true); - if (ret) + if (ret) { + icc_set_bw(q6v5->path, 0, 0); return ret; + } reinit_completion(&q6v5->start_done); reinit_completion(&q6v5->stop_done); @@ -78,6 +87,9 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) disable_irq(q6v5->handover_irq); q6v5_load_state_toggle(q6v5, false); + /* Disable interconnect vote, in case handover never happened */ + icc_set_bw(q6v5->path, 0, 0); + return !q6v5->handover_issued; } EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); @@ -160,6 +172,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data) if (q6v5->handover) q6v5->handover(q6v5); + icc_set_bw(q6v5->path, 0, 0); + q6v5->handover_issued = true; return IRQ_HANDLED; @@ -332,6 +346,11 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, return load_state ? -ENOMEM : -EINVAL; } + q6v5->path = devm_of_icc_get(&pdev->dev, NULL); + if (IS_ERR(q6v5->path)) + return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), + "failed to acquire interconnect path\n"); + return 0; } EXPORT_SYMBOL_GPL(qcom_q6v5_init); diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h index f35e04471ed7..5a859c41896e 100644 --- a/drivers/remoteproc/qcom_q6v5.h +++ b/drivers/remoteproc/qcom_q6v5.h @@ -7,6 +7,7 @@ #include <linux/completion.h> #include <linux/soc/qcom/qcom_aoss.h> +struct icc_path; struct rproc; struct qcom_smem_state; struct qcom_sysmon; @@ -18,6 +19,8 @@ struct qcom_q6v5 { struct qcom_smem_state *state; struct qmp *qmp; + struct icc_path *path; + unsigned stop_bit; int wdog_irq;
Many remoteproc instances requires that Linux casts a proxy vote for an interconnect path during boot, until they can do it themselves. Add support for voting for a single path. As this is a shared problem between both PAS and MSS drivers, the path is acquired and votes casted from the common helper code. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Sibi posted recently a patch to add interconnect votes from the modem driver, today I needed the same feature for one of the PAS remoteprocs. After essentially duplicating Sibi's patch I realized that it doesn't look too bad to put this in the common Q6V5 code. The main difference is that this would be messy if we need to support multiple paths, so we probably would have to push it out to the individual drivers at that point. Sibi's patch can be found here. https://lore.kernel.org/all/1644813252-12897-3-git-send-email-quic_sibis@quicinc.com/ This makes the implementation pick up one path, relevant DT bindings would still need to be updated in order be allowed to this in the DeviceTree files. drivers/remoteproc/qcom_q6v5.c | 21 ++++++++++++++++++++- drivers/remoteproc/qcom_q6v5.h | 3 +++ 2 files changed, 23 insertions(+), 1 deletion(-)