| Message ID | 20220514000108.3070363-4-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | remoteproc: qcom: Add support for CDSP and MSS on SDM630/660 | expand |
On Sat, May 14, 2022 at 03:01:07AM +0300, Dmitry Baryshkov wrote: > From: Konrad Dybcio <konradybcio@gmail.com> > > This adds support for sdm630/636/660 modem subsystem > remote processor. > > Signed-off-by: Konrad Dybcio <konradybcio@gmail.com> > [DB: fixed commit message, removed note about modem restarting regularly] > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> I was only looking at this by coincidence recently but since it doesn't seem to be applied yet(?) some comments below. > --- > drivers/remoteproc/qcom_q6v5_mss.c | 111 +++++++++++++++++++++++++++++ > 1 file changed, 111 insertions(+) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index af217de75e4d..7a4cca30db8a 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c [...] > @@ -754,6 +759,79 @@ static int q6v5proc_reset(struct q6v5 *qproc) > val |= readl(qproc->reg_base + mem_pwr_ctl); > udelay(1); > } > + /* Remove word line clamp */ > + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + val &= ~QDSP6v56_CLAMP_WL; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + } else if (qproc->version == MSS_SDM660) { > + int mem_pwr_ctl; > + > + /* Override the ACC value if required */ > + writel(QDSP6SS_ACC_OVERRIDE_VAL, > + qproc->reg_base + QDSP6SS_STRAP_ACC); > + > + /* Assert resets, stop core */ > + val = readl(qproc->reg_base + QDSP6SS_RESET_REG); > + val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE; > + writel(val, qproc->reg_base + QDSP6SS_RESET_REG); > + > + /* BHS require xo cbcr to be enabled */ > + val = readl(qproc->reg_base + QDSP6SS_XO_CBCR); > + val |= 1; val |= Q6SS_CBCR_CLKEN; (like in the existing 8996 code) > + writel(val, qproc->reg_base + QDSP6SS_XO_CBCR); I would also expect this to wait for !Q6SS_CBCR_CLKOFF like on all the other SoCs. Pretty sure downstream does that for all of them in q6v55_branch_clk_enable(). > + > + /* Enable power block headswitch and wait for it to stabilize */ > + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + val |= QDSP6v56_BHS_ON; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + mb(); > + udelay(1); > + > + for (i = BHS_CHECK_MAX_LOOPS; i > 0; i--) { > + if (readl_relaxed(qproc->reg_base + QDSP6V62SS_BHS_STATUS) > + & QDSP6v55_BHS_EN_REST_ACK) > + break; > + udelay(1); > + } This looks like readl_poll_timeout(). > + if (!i) { > + pr_err("%s: BHS_EN_REST_ACK not set!\n", __func__); dev_err() > + return -ETIMEDOUT; > + } > + > + /* Put LDO in bypass mode */ > + val |= QDSP6v56_LDO_BYP; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* Remove QMC_MEM clamp */ > + val &= ~QDSP6v56_CLAMP_QMC_MEM; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* Deassert QDSP6 compiler memory clamp */ > + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + val &= ~QDSP6v56_CLAMP_QMC_MEM; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + Why does this clear QDSP6v56_CLAMP_QMC_MEM twice? > + /* Deassert memory peripheral sleep and L2 memory standby */ > + val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* Turn on L1, L2, ETB and JU memories 1 at a time */ > + mem_pwr_ctl = QDSP6V6SS_MEM_PWR_CTL; > + i = 29; > + > + val = readl(qproc->reg_base + mem_pwr_ctl); > + for (; i >= 0; i--) { > + val |= BIT(i); > + writel(val, qproc->reg_base + mem_pwr_ctl); > + /* > + * Read back value to ensure the write is done then > + * wait for 1us for both memory peripheral and data > + * array to turn on. > + */ > + val |= readl(qproc->reg_base + mem_pwr_ctl); > + udelay(1); > + } > + > /* Remove word line clamp */ > val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > val &= ~QDSP6v56_CLAMP_WL; All in all this looks almost exactly like the existing code for MSS_MSM8996/8. Wouldn't it be cleaner to just add an if statement for the QDSP6V62SS_BHS_STATUS readl_poll_timeout() to the existing code? Thanks, Stephan
On 3 July 2022 13:13:51 GMT+03:00, Stephan Gerhold <stephan@gerhold.net> wrote: >On Sat, May 14, 2022 at 03:01:07AM +0300, Dmitry Baryshkov wrote: >> From: Konrad Dybcio <konradybcio@gmail.com> >> >> This adds support for sdm630/636/660 modem subsystem >> remote processor. >> >> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com> >> [DB: fixed commit message, removed note about modem restarting regularly] >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >I was only looking at this by coincidence recently but since it doesn't >seem to be applied yet(?) some comments below. Thanks for your comments, I will take a look next week. > >> --- >> drivers/remoteproc/qcom_q6v5_mss.c | 111 +++++++++++++++++++++++++++++ >> 1 file changed, 111 insertions(+) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c >> index af217de75e4d..7a4cca30db8a 100644 >> --- a/drivers/remoteproc/qcom_q6v5_mss.c >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c >[...] >> + } >> + >> /* Remove word line clamp */ >> val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> val &= ~QDSP6v56_CLAMP_WL; > >All in all this looks almost exactly like the existing code for >MSS_MSM8996/8. Wouldn't it be cleaner to just add an if statement for >the QDSP6V62SS_BHS_STATUS readl_poll_timeout() to the existing code? For quite some time I was unhappy with this part of the mss driver. Maybe it's time to review platform-specific code in attempt to generalise it.
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index af217de75e4d..7a4cca30db8a 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -68,6 +68,7 @@ #define QDSP6SS_MEM_PWR_CTL 0x0B0 #define QDSP6V6SS_MEM_PWR_CTL 0x034 #define QDSP6SS_STRAP_ACC 0x110 +#define QDSP6V62SS_BHS_STATUS 0x0C4 /* AXI Halt Register Offsets */ #define AXI_HALTREQ_REG 0x0 @@ -117,6 +118,9 @@ #define QDSP6v56_CLAMP_QMC_MEM BIT(22) #define QDSP6SS_XO_CBCR 0x0038 #define QDSP6SS_ACC_OVERRIDE_VAL 0x20 +#define QDSP6v55_BHS_EN_REST_ACK BIT(0) + +#define BHS_CHECK_MAX_LOOPS (200) /* QDSP6v65 parameters */ #define QDSP6SS_CORE_CBCR 0x20 @@ -239,6 +243,7 @@ enum { MSS_MSM8998, MSS_SC7180, MSS_SC7280, + MSS_SDM660, MSS_SDM845, }; @@ -754,6 +759,79 @@ static int q6v5proc_reset(struct q6v5 *qproc) val |= readl(qproc->reg_base + mem_pwr_ctl); udelay(1); } + /* Remove word line clamp */ + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); + val &= ~QDSP6v56_CLAMP_WL; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + } else if (qproc->version == MSS_SDM660) { + int mem_pwr_ctl; + + /* Override the ACC value if required */ + writel(QDSP6SS_ACC_OVERRIDE_VAL, + qproc->reg_base + QDSP6SS_STRAP_ACC); + + /* Assert resets, stop core */ + val = readl(qproc->reg_base + QDSP6SS_RESET_REG); + val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE; + writel(val, qproc->reg_base + QDSP6SS_RESET_REG); + + /* BHS require xo cbcr to be enabled */ + val = readl(qproc->reg_base + QDSP6SS_XO_CBCR); + val |= 1; + writel(val, qproc->reg_base + QDSP6SS_XO_CBCR); + + /* Enable power block headswitch and wait for it to stabilize */ + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); + val |= QDSP6v56_BHS_ON; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + mb(); + udelay(1); + + for (i = BHS_CHECK_MAX_LOOPS; i > 0; i--) { + if (readl_relaxed(qproc->reg_base + QDSP6V62SS_BHS_STATUS) + & QDSP6v55_BHS_EN_REST_ACK) + break; + udelay(1); + } + if (!i) { + pr_err("%s: BHS_EN_REST_ACK not set!\n", __func__); + return -ETIMEDOUT; + } + + /* Put LDO in bypass mode */ + val |= QDSP6v56_LDO_BYP; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + + /* Remove QMC_MEM clamp */ + val &= ~QDSP6v56_CLAMP_QMC_MEM; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + + /* Deassert QDSP6 compiler memory clamp */ + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); + val &= ~QDSP6v56_CLAMP_QMC_MEM; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + + /* Deassert memory peripheral sleep and L2 memory standby */ + val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N; + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); + + /* Turn on L1, L2, ETB and JU memories 1 at a time */ + mem_pwr_ctl = QDSP6V6SS_MEM_PWR_CTL; + i = 29; + + val = readl(qproc->reg_base + mem_pwr_ctl); + for (; i >= 0; i--) { + val |= BIT(i); + writel(val, qproc->reg_base + mem_pwr_ctl); + /* + * Read back value to ensure the write is done then + * wait for 1us for both memory peripheral and data + * array to turn on. + */ + val |= readl(qproc->reg_base + mem_pwr_ctl); + udelay(1); + } + /* Remove word line clamp */ val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); val &= ~QDSP6v56_CLAMP_WL; @@ -785,6 +863,7 @@ static int q6v5proc_reset(struct q6v5 *qproc) val |= Q6SS_L2DATA_SLP_NRET_N_0; writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); } + /* Remove IO clamp */ val &= ~Q6SS_CLAMP_IO; writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); @@ -2098,6 +2177,37 @@ static const struct rproc_hexagon_res sc7280_mss = { .version = MSS_SC7280, }; +static const struct rproc_hexagon_res sdm660_mss = { + .hexagon_mba_image = "mba.mbn", + .proxy_clk_names = (char*[]){ + "xo", + "qdss", + NULL + }, + .active_clk_names = (char*[]){ + "iface", + "bus", + "gpll0_mss", + "mnoc_axi", + "snoc_axi", + "mem", + NULL + }, + .proxy_pd_names = (char*[]){ + "cx", + "mx", + NULL + }, + .need_mem_protection = true, + .has_alt_reset = false, + .has_mba_logs = false, + .has_spare_reg = false, + .has_qaccept_regs = false, + .has_ext_cntl_regs = false, + .has_vq6 = false, + .version = MSS_SDM660, +}; + static const struct rproc_hexagon_res sdm845_mss = { .hexagon_mba_image = "mba.mbn", .proxy_clk_names = (char*[]){ @@ -2304,6 +2414,7 @@ static const struct of_device_id q6v5_of_match[] = { { .compatible = "qcom,msm8998-mss-pil", .data = &msm8998_mss}, { .compatible = "qcom,sc7180-mss-pil", .data = &sc7180_mss}, { .compatible = "qcom,sc7280-mss-pil", .data = &sc7280_mss}, + { .compatible = "qcom,sdm660-mss-pil", .data = &sdm660_mss}, { .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss}, { }, };