| Message ID | 1526294812-23390-1-git-send-email-sricharan@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add support for Hexagon q6v5-wcss integrated core | expand |
On 14-05-18, 16:16, Sricharan R wrote: > +static int q6v5_reset(struct q6v5 *qproc) > +{ > + u32 ret; > + int val, i; > + > + /* 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 |= 0x1; > + writel(val, qproc->reg_base + QDSP6SS_XO_CBCR); consider adding a updatel macro which does read, update and write for you... > + > + /* Read CLKOFF bit to go low indicating CLK is enabled */ > + ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR, > + val, !(val & BIT(31)), 1, > + HALT_CHECK_MAX_LOOPS); > + if (ret) { > + dev_err(qproc->dev, > + "xo cbcr enabling timed out (rc:%d)\n", ret); > + return ret; > + } > + /* 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); > + val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); why is this read required > + udelay(1); > + > + /* Put LDO in bypass mode */ > + val |= QDSP6v56_LDO_BYP; > + 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 */ > + val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL); > + for (i = 19; i >= 0; i--) { where is the magic number 19 coming from?
On 14-05-18, 16:16, Sricharan R wrote: > +static int q6v5_wcss_start(struct rproc *rproc) > +{ > + struct q6v5 *qproc = rproc->priv; > + int ret = 0; Superfluous initialization > + > + ret = q6v5_clk_enable(qproc->dev, qproc->active_clks, > + qproc->active_clk_count); > + if (ret) { > + dev_err(qproc->dev, "failed to enable clocks\n"); > + return ret; > + } > + > + /* Release Q6 and WCSS reset */ > + ret = reset_control_deassert(qproc->wcss_reset); > + if (ret) > + dev_err(qproc->dev, "wcss_reset failed\n"); > + > + ret = reset_control_deassert(qproc->wcss_q6_reset); > + if (ret) > + dev_err(qproc->dev, "wcss_q6_reset failed\n"); shouldn't we abort on these two errors? > + > + /* Lithium configuration - clock gating and bus arbitration */ > + ret = regmap_update_bits(qproc->halt_map, > + qproc->halt_nc + TCSR_GLOBAL_CFG0, > + 0x1F, 0x14); > + if (ret) > + return ret; > + > + ret = regmap_update_bits(qproc->halt_map, > + qproc->halt_nc + TCSR_GLOBAL_CFG1, > + 1, 0); > + if (ret) > + return ret; > + > + /* Write bootaddr to EVB so that Q6WCSS will jump there after reset */ > + writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB); > + > + ret = q6v5_reset(qproc); > + if (ret) > + return ret; all these returns, aren't we leaving device in some dangling state? > +static int q6v5_wcss_powerdown(struct q6v5 *qproc) > +{ > + unsigned int val = 0; superfluous initialization > + int ret; > + > + /* 1 - Assert WCSS/Q6 HALTREQ */ > + q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); > + > + /* 2 - Enable WCSSAON_CONFIG */ > + val = readl(qproc->rmb_base + SSCAON_CONFIG); > + val |= SSCAON_ENABLE; > + writel(val, qproc->rmb_base + SSCAON_CONFIG); > + > + /* 3 - Set SSCAON_CONFIG */ > + val |= BIT(15); > + val &= ~BIT(16); > + val &= ~BIT(17); > + val &= ~BIT(18); shouldn't bit 15 thru 18 be defined on what they mean? > +static int q6v5_q6_powerdown(struct q6v5 *qproc) > +{ > + int i = 0, ret; > + unsigned int val = 0; > + > + /* 1 - Halt Q6 bus interface */ > + q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); > + > + /* 2 - Disable Q6 Core clock */ > + val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG); > + val &= ~Q6SS_CLK_ENABLE; > + writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG); > + > + /* 3 - Clamp I/O */ > + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + val |= Q6SS_CLAMP_IO; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* 4 - Clamp WL */ > + val |= QDSS_BHS_ON; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* 5 - Clear Erase standby */ > + val &= ~Q6SS_L2DATA_STBY_N; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* 6 - Clear Sleep RTN */ > + val &= ~Q6SS_SLP_RET_N; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* 7 - turn off QDSP6 memory foot/head switch one bank at a time */ > + for (i = 0; i < 20; i++) { > + val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL); > + val &= ~BIT(i); > + writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL); > + mdelay(1); > + } > + /* 8 - Assert QMC memory RTN */ > + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + val |= QDSP6v56_CLAMP_QMC_MEM; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + > + /* 9 - Turn off BHS */ > + val &= ~QDSP6v56_BHS_ON; > + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > + udelay(1); > + /* 10 - Wait till BHS Reset is done */ would help in readability if you can be consistent and add empty lines after each step as done for rest of the routine > +static int q6v5_wcss_stop(struct rproc *rproc) > +{ > + struct q6v5 *qproc = rproc->priv; > + int ret = 0; this one too, I think if you run with sparse, it should warn you about these > + > + qproc->running = false; > + > + /* WCSS powerdown */ > + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), > + BIT(qproc->stop_bit)); > + > + ret = wait_for_completion_timeout(&qproc->stop_done, > + msecs_to_jiffies(5000)); > + if (ret == 0) { > + dev_err(qproc->dev, "timed out on wait\n"); > + return -ETIMEDOUT; > + } > + > + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0); > + > + ret = q6v5_wcss_powerdown(qproc); > + if (ret) > + return ret; > + > + /* Q6 Power down */ > + ret = q6v5_q6_powerdown(qproc); > + if (ret) > + return ret; > + > + return 0; this could be optimized to: return q6v5_q6_powerdown()
Hi Vinod, Thanks for the review. On 5/18/2018 5:52 PM, Vinod wrote: > On 14-05-18, 16:16, Sricharan R wrote: > >> +static int q6v5_reset(struct q6v5 *qproc) >> +{ >> + u32 ret; >> + int val, i; >> + >> + /* 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 |= 0x1; >> + writel(val, qproc->reg_base + QDSP6SS_XO_CBCR); > > consider adding a updatel macro which does read, update and write for you... > ok. >> + >> + /* Read CLKOFF bit to go low indicating CLK is enabled */ >> + ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR, >> + val, !(val & BIT(31)), 1, >> + HALT_CHECK_MAX_LOOPS); >> + if (ret) { >> + dev_err(qproc->dev, >> + "xo cbcr enabling timed out (rc:%d)\n", ret); >> + return ret; >> + } >> + /* 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); >> + val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > > why is this read required > It was a ditto of what was given in the programming sequence from HW folks. yeah, logically the readl does not look needed. Will remove and update. >> + udelay(1); >> + >> + /* Put LDO in bypass mode */ >> + val |= QDSP6v56_LDO_BYP; >> + 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 */ >> + val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL); >> + for (i = 19; i >= 0; i--) { > > where is the magic number 19 coming from? > Its the total number of Q6's memory head/foot switch banks. Infact the magic was there even before my patch. But will add a Macro to fix it. Regards, Sricharan
Hi, On 5/18/2018 5:59 PM, Vinod wrote: > On 14-05-18, 16:16, Sricharan R wrote: > >> +static int q6v5_wcss_start(struct rproc *rproc) >> +{ >> + struct q6v5 *qproc = rproc->priv; >> + int ret = 0; > > Superfluous initialization > ok. >> + >> + ret = q6v5_clk_enable(qproc->dev, qproc->active_clks, >> + qproc->active_clk_count); >> + if (ret) { >> + dev_err(qproc->dev, "failed to enable clocks\n"); >> + return ret; >> + } >> + >> + /* Release Q6 and WCSS reset */ >> + ret = reset_control_deassert(qproc->wcss_reset); >> + if (ret) >> + dev_err(qproc->dev, "wcss_reset failed\n"); >> + >> + ret = reset_control_deassert(qproc->wcss_q6_reset); >> + if (ret) >> + dev_err(qproc->dev, "wcss_q6_reset failed\n"); > > shouldn't we abort on these two errors? > ha right. will fix it. >> + >> + /* Lithium configuration - clock gating and bus arbitration */ >> + ret = regmap_update_bits(qproc->halt_map, >> + qproc->halt_nc + TCSR_GLOBAL_CFG0, >> + 0x1F, 0x14); >> + if (ret) >> + return ret; >> + >> + ret = regmap_update_bits(qproc->halt_map, >> + qproc->halt_nc + TCSR_GLOBAL_CFG1, >> + 1, 0); >> + if (ret) >> + return ret; >> + >> + /* Write bootaddr to EVB so that Q6WCSS will jump there after reset */ >> + writel(rproc->bootaddr >> 4, qproc->reg_base + QDSP6SS_RST_EVB); >> + >> + ret = q6v5_reset(qproc); >> + if (ret) >> + return ret; > > all these returns, aren't we leaving device in some dangling state? > hmm ok. clocks and resets have to be reverted. will add error handling here. >> +static int q6v5_wcss_powerdown(struct q6v5 *qproc) >> +{ >> + unsigned int val = 0; > > superfluous initialization > ok. >> + int ret; >> + >> + /* 1 - Assert WCSS/Q6 HALTREQ */ >> + q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); >> + >> + /* 2 - Enable WCSSAON_CONFIG */ >> + val = readl(qproc->rmb_base + SSCAON_CONFIG); >> + val |= SSCAON_ENABLE; >> + writel(val, qproc->rmb_base + SSCAON_CONFIG); >> + >> + /* 3 - Set SSCAON_CONFIG */ >> + val |= BIT(15); >> + val &= ~BIT(16); >> + val &= ~BIT(17); >> + val &= ~BIT(18); > > shouldn't bit 15 thru 18 be defined on what they mean? > hmm, ok. would define them. >> +static int q6v5_q6_powerdown(struct q6v5 *qproc) >> +{ >> + int i = 0, ret; >> + unsigned int val = 0; >> + >> + /* 1 - Halt Q6 bus interface */ >> + q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); >> + >> + /* 2 - Disable Q6 Core clock */ >> + val = readl(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG); >> + val &= ~Q6SS_CLK_ENABLE; >> + writel(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG); >> + >> + /* 3 - Clamp I/O */ >> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= Q6SS_CLAMP_IO; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* 4 - Clamp WL */ >> + val |= QDSS_BHS_ON; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* 5 - Clear Erase standby */ >> + val &= ~Q6SS_L2DATA_STBY_N; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* 6 - Clear Sleep RTN */ >> + val &= ~Q6SS_SLP_RET_N; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* 7 - turn off QDSP6 memory foot/head switch one bank at a time */ >> + for (i = 0; i < 20; i++) { >> + val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL); >> + val &= ~BIT(i); >> + writel(val, qproc->reg_base + QDSP6SS_MEM_PWR_CTL); >> + mdelay(1); >> + } >> + /* 8 - Assert QMC memory RTN */ >> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= QDSP6v56_CLAMP_QMC_MEM; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* 9 - Turn off BHS */ >> + val &= ~QDSP6v56_BHS_ON; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + udelay(1); >> + /* 10 - Wait till BHS Reset is done */ > > would help in readability if you can be consistent and add empty lines after each > step as done for rest of the routine > ok. >> +static int q6v5_wcss_stop(struct rproc *rproc) >> +{ >> + struct q6v5 *qproc = rproc->priv; >> + int ret = 0; > > this one too, I think if you run with sparse, it should warn you about these > ok, sure would check. >> + >> + qproc->running = false; >> + >> + /* WCSS powerdown */ >> + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), >> + BIT(qproc->stop_bit)); >> + >> + ret = wait_for_completion_timeout(&qproc->stop_done, >> + msecs_to_jiffies(5000)); >> + if (ret == 0) { >> + dev_err(qproc->dev, "timed out on wait\n"); >> + return -ETIMEDOUT; >> + } >> + >> + qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0); >> + >> + ret = q6v5_wcss_powerdown(qproc); >> + if (ret) >> + return ret; >> + >> + /* Q6 Power down */ >> + ret = q6v5_q6_powerdown(qproc); >> + if (ret) >> + return ret; >> + >> + return 0; > > this could be optimized to: > return q6v5_q6_powerdown() > ok. Regards, Sricharan
IPQ8074 has an integrated Hexagon dsp core Q6v5 and a wireless lan (Lithium) IP. This series adds the remoteproc driver to reset, load and boot Q6 firmware. The first patch is to make the mdt_loader authenticate the firmware only if required, so that the code can be reused for self-authenticating firmware like the Q6v5 core in IPQ8074. The second patch exports the elf header's get_boot_addr helper to reuse it. The next couple of patches arranges the code in the original q6v5-mpss rproc to add q6v5-wcss later. The last couple of patches add the relevant bits for the q6v5-wcss core. This is done on top of Avaneesh's msm8996 rproc support [1] [1] https://lkml.org/lkml/2017/10/24/771 V6: Rebased on top of, https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1674300.html Dropped remoteproc: Export rproc_elf_get_boot_addr, as it is already merged by a different patch from Bjorn. Minor change to the q6 power down sequence. v5: No change. Just updated tags in PATCH #5 V4: Fixed Bjorn's comment in PATCH#1 and added his acked-by Rebased on top of Avinash's latest rproc for msm8996 q6 support. V3: Rebased on top of latest remoteproc next V2: Last time introduced this a new rproc driver, but there is lot of code that can be shared if it is added to the q6v5-mpss pil driver. Sricharan R (5): remoteproc: qcom: mdt_loader: Make the firmware authentication optional remoteproc: qcom: Push reset ops, rproc ops in to of_match data remoteproc: qcom: Split the head and tail of the q5v5-pil rproc reset function remoteproc: qcom: Add support for q6v5-wcss pil remoteproc: qcom: Add q6v5-wcss rproc ops .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +- drivers/remoteproc/Kconfig | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 468 +++++++++++++++++---- drivers/soc/qcom/mdt_loader.c | 87 ++-- include/linux/soc/qcom/mdt_loader.h | 4 + 5 files changed, 450 insertions(+), 117 deletions(-)