From: Matthias Kaehlcke <mka@chromium.org>
To: Sibi Sankar <sibis@codeaurora.org>
Cc: bjorn.andersson@linaro.org, robh+dt@kernel.org, will@kernel.org,
saiprakash.ranjan@codeaurora.org, ohad@wizery.com,
agross@kernel.org, mathieu.poirier@linaro.org,
robin.murphy@arm.com, joro@8bytes.org, p.zabel@pengutronix.de,
linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, evgreen@chromium.org,
dianders@chromium.org, swboyd@chromium.org
Subject: Re: [PATCH 5/9] remoteproc: mss: q6v5-mss: Add modem support on SC7280
Date: Fri, 25 Jun 2021 09:39:55 -0700 [thread overview]
Message-ID: <YNYG200n8Zh9vDWL@google.com> (raw)
In-Reply-To: <73f9814fb4f3aa2abeee0ece3aa26312@codeaurora.org>
Hi Sibi,
On Fri, Jun 25, 2021 at 07:51:38PM +0530, Sibi Sankar wrote:
> Hey Matthias,
> Thanks for taking time to review the patch
> series.
>
> On 2021-06-25 06:05, Matthias Kaehlcke wrote:
> > Hi Sibi,
> >
> > On Fri, Jun 25, 2021 at 01:17:34AM +0530, Sibi Sankar wrote:
> > > Add out of reset sequence support for modem sub-system on SC7280 SoCs.
> > > It requires access to an additional set of qaccept registers, external
> > > power/clk control registers and halt vq6 register to put the modem
> > > back
> > > into reset.
> > >
> > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > > ---
> > > drivers/remoteproc/qcom_q6v5_mss.c | 245
> > > ++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 241 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> > > b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 5d21084004cb..4e32811e0025 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -77,6 +77,14 @@
> > >
> > > #define HALT_ACK_TIMEOUT_US 100000
> > >
> > > +/* QACCEPT Register Offsets */
> > > +#define QACCEPT_ACCEPT_REG 0x0
> > > +#define QACCEPT_ACTIVE_REG 0x4
> > > +#define QACCEPT_DENY_REG 0x8
> > > +#define QACCEPT_REQ_REG 0xC
> > > +
> > > +#define QACCEPT_TIMEOUT_US 50
> > > +
> > > /* QDSP6SS_RESET */
> > > #define Q6SS_STOP_CORE BIT(0)
> > > #define Q6SS_CORE_ARES BIT(1)
> > > @@ -143,6 +151,9 @@ struct rproc_hexagon_res {
> > > bool has_alt_reset;
> > > bool has_mba_logs;
> > > bool has_spare_reg;
> > > + bool has_qaccept_regs;
> > > + bool has_ext_cntl_regs;
> > > + bool has_vq6;
> > > };
> > >
> > > struct q6v5 {
> > > @@ -158,8 +169,18 @@ struct q6v5 {
> > > u32 halt_q6;
> > > u32 halt_modem;
> > > u32 halt_nc;
> > > + u32 halt_vq6;
> > > u32 conn_box;
> > >
> > > + u32 qaccept_mdm;
> > > + u32 qaccept_cx;
> > > + u32 qaccept_axi;
> > > +
> > > + u32 axim1_clk_off;
> > > + u32 crypto_clk_off;
> > > + u32 force_clk_on;
> > > + u32 rscc_disable;
> > > +
> > > struct reset_control *mss_restart;
> > > struct reset_control *pdc_reset;
> > >
> > > @@ -201,6 +222,9 @@ struct q6v5 {
> > > bool has_alt_reset;
> > > bool has_mba_logs;
> > > bool has_spare_reg;
> > > + bool has_qaccept_regs;
> > > + bool has_ext_cntl_regs;
> > > + bool has_vq6;
> > > int mpss_perm;
> > > int mba_perm;
> > > const char *hexagon_mdt_image;
> > > @@ -213,6 +237,7 @@ enum {
> > > MSS_MSM8996,
> > > MSS_MSM8998,
> > > MSS_SC7180,
> > > + MSS_SC7280,
> > > MSS_SDM845,
> > > };
> > >
> > > @@ -473,6 +498,12 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
> > > regmap_update_bits(qproc->conn_map, qproc->conn_box,
> > > AXI_GATING_VALID_OVERRIDE, 0);
> > > ret = reset_control_deassert(qproc->mss_restart);
> > > + } else if (qproc->has_ext_cntl_regs) {
> > > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0);
> > > + reset_control_assert(qproc->pdc_reset);
> > > + reset_control_assert(qproc->mss_restart);
> > > + reset_control_deassert(qproc->pdc_reset);
> > > + ret = reset_control_deassert(qproc->mss_restart);
> > > } else {
> > > ret = reset_control_assert(qproc->mss_restart);
> > > }
> > > @@ -490,7 +521,7 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
> > > ret = reset_control_reset(qproc->mss_restart);
> > > writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
> > > reset_control_deassert(qproc->pdc_reset);
> > > - } else if (qproc->has_spare_reg) {
> > > + } else if (qproc->has_spare_reg || qproc->has_ext_cntl_regs) {
> > > ret = reset_control_reset(qproc->mss_restart);
> > > } else {
> > > ret = reset_control_deassert(qproc->mss_restart);
> > > @@ -604,7 +635,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
> > > }
> > >
> > > goto pbl_wait;
> > > - } else if (qproc->version == MSS_SC7180) {
> > > + } else if (qproc->version == MSS_SC7180 || qproc->version ==
> > > MSS_SC7280) {
> > > val = readl(qproc->reg_base + QDSP6SS_SLEEP);
> > > val |= Q6SS_CBCR_CLKEN;
> > > writel(val, qproc->reg_base + QDSP6SS_SLEEP);
> > > @@ -787,6 +818,82 @@ static int q6v5proc_reset(struct q6v5 *qproc)
> > > return ret;
> > > }
> > >
> > > +static int q6v5proc_enable_qchannel(struct q6v5 *qproc, struct
> > > regmap *map, u32 offset)
> > > +{
> > > + unsigned int val;
> > > + int ret;
> > > +
> > > + if (!qproc->has_qaccept_regs)
> > > + return 0;
> > > +
> > > + if (qproc->has_ext_cntl_regs) {
> > > + regmap_write(qproc->conn_map, qproc->rscc_disable, 0);
> > > + regmap_write(qproc->conn_map, qproc->force_clk_on, 1);
> > > +
> > > + ret = regmap_read_poll_timeout(qproc->halt_map,
> > > qproc->axim1_clk_off, val,
> > > + !val, 1, Q6SS_CBCR_TIMEOUT_US);
> > > + if (ret) {
> > > + dev_err(qproc->dev, "failed to enable axim1 clock\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > + }
> > > +
> > > + regmap_write(map, offset + QACCEPT_REQ_REG, 1);
> > > +
> > > + /* Wait for accept */
> > > + ret = regmap_read_poll_timeout(map, offset + QACCEPT_ACCEPT_REG,
> > > val, val, 5,
> > > + QACCEPT_TIMEOUT_US);
> > > + if (ret) {
> > > + dev_err(qproc->dev, "qchannel enable failed\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void q6v5proc_disable_qchannel(struct q6v5 *qproc, struct
> > > regmap *map, u32 offset)
> > > +{
> > > + int ret;
> > > + unsigned int val, retry;
> > > + unsigned int nretry = 10;
> > > + bool takedown_complete = false;
> > > +
> > > + if (!qproc->has_qaccept_regs)
> > > + return;
> > > +
> > > + while (!takedown_complete && nretry) {
> > > + nretry--;
> > > +
> > > + regmap_read_poll_timeout(map, offset + QACCEPT_ACTIVE_REG, val,
> > > !val, 5,
> > > + QACCEPT_TIMEOUT_US);
> > > +
> > > + regmap_write(map, offset + QACCEPT_REQ_REG, 0);
>
> Sure I'll add more comments to this func.
> After lowering the request ^^ we wait
> for deny to go high or accept to go low.
> If it's the former then we do a request
> high and repeat the entire process again.
> If it's the latter then its considered
> that the takedown is success.
The above essentially is a transcript of the code into prose. For a reader
who isn't familiar with the hardware and might not have access to the
corresponding documentation the exact roles of the ACCEPT registers might
not be evident.
I was looking for something slightly higher level, a one liner here and
there might be enough. E.g. something like 'request to disable the channel
denied, re-enable it' in the loop below, if that is semantically correct.
Is there a typical reason why such a request would be denied, maybe because
the channel was busy? Also why is re-enabling actually required if the
request to disable was denied?
> Let me know if you feel any other parts of the patch requires more
comments as well.
For now it's mainly the code involving the ACCEPT registers and
_disable_channel() in particular.
>
> > > +
> > > + retry = 10;
> > > + while (retry) {
> > > + usleep_range(5, 10);
> > > + retry--;
> > > + ret = regmap_read(map, offset + QACCEPT_DENY_REG, &val);
> > > + if (!ret && val) {
> > > + regmap_write(map, offset + QACCEPT_REQ_REG, 1);
> > > + break;
> > > + }
> > > +
> > > + ret = regmap_read(map, offset + QACCEPT_ACCEPT_REG, &val);
> > > + if (!ret && !val) {
> > > + takedown_complete = true;
> > > + break;
> > > + }
> >
> > A bit of commentary in this branch would do no harm. From the code flow
> > I can guess that disabling the channel failed when QACCEPT_DENY_REG !=
> > 0,
> > and hence the channel is re-enabled (?) for the next try, and apparently
> > things are fine when QACCEPT_ACCEPT_REG is 0 after disabling the
> > channel.
> > Would be good to be a bit more explicit about what all that actually
> > means.
next prev parent reply other threads:[~2021-06-25 16:40 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 19:47 [PATCH 0/9] Add Modem support on SC7280 SoCs Sibi Sankar
2021-06-24 19:47 ` [PATCH 1/9] dt-bindings: remoteproc: qcom: pas: Add SC7280 MPSS support Sibi Sankar
2021-06-25 17:12 ` Matthias Kaehlcke
2021-06-25 17:28 ` Sibi Sankar
2021-06-25 23:20 ` Matthias Kaehlcke
2021-07-14 19:36 ` Rob Herring
2021-06-24 19:47 ` [PATCH 2/9] remoteproc: qcom: pas: Add SC7280 Modem support Sibi Sankar
2021-06-25 23:23 ` Matthias Kaehlcke
2021-06-24 19:47 ` [PATCH 3/9] dt-bindings: remoteproc: qcom: Add Q6V5 Modem PIL binding Sibi Sankar
2021-06-25 23:43 ` Matthias Kaehlcke
2021-06-30 19:57 ` Sibi Sankar
2021-07-14 19:37 ` Rob Herring
2021-06-24 19:47 ` [PATCH 4/9] iommu/arm-smmu-qcom: Request direct mapping for modem device Sibi Sankar
2021-06-25 4:27 ` Sai Prakash Ranjan
2021-06-24 19:47 ` [PATCH 5/9] remoteproc: mss: q6v5-mss: Add modem support on SC7280 Sibi Sankar
2021-06-25 0:35 ` Matthias Kaehlcke
2021-06-25 14:21 ` Sibi Sankar
2021-06-25 16:39 ` Matthias Kaehlcke [this message]
2021-06-24 19:47 ` [PATCH 6/9] arm64: dts: qcom: sc7280: Update reserved memory map Sibi Sankar
2021-06-28 18:11 ` Matthias Kaehlcke
2021-06-30 20:02 ` Sibi Sankar
2021-07-30 18:24 ` Bjorn Andersson
2021-06-24 19:47 ` [PATCH 7/9] arm64: dts: qcom: sc7280: Add nodes to boot modem Sibi Sankar
2021-07-30 18:00 ` Bjorn Andersson
2021-06-24 19:47 ` [PATCH 8/9] arm64: dts: qcom: sc7280: Add Q6V5 MSS node Sibi Sankar
2021-06-28 18:39 ` Matthias Kaehlcke
2021-06-30 20:03 ` Sibi Sankar
2021-07-30 18:01 ` Bjorn Andersson
2021-06-24 19:47 ` [PATCH 9/9] arm64: dts: qcom: sc7280: Update " Sibi Sankar
2021-06-28 19:05 ` Matthias Kaehlcke
2021-06-30 20:08 ` Sibi Sankar
2021-07-30 18:14 ` Bjorn Andersson
2021-07-01 20:02 ` [PATCH 0/9] Add Modem support on SC7280 SoCs Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YNYG200n8Zh9vDWL@google.com \
--to=mka@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=joro@8bytes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=ohad@wizery.com \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
--cc=robin.murphy@arm.com \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=sibis@codeaurora.org \
--cc=swboyd@chromium.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).