From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dwivedi, Avaneesh Kumar (avani)" Subject: Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0 Date: Mon, 12 Dec 2016 18:15:39 +0530 Message-ID: <0f3dbcbb-4ed5-dac8-a9c7-a22d597f4be5@codeaurora.org> References: <1479981638-32069-1-git-send-email-akdwived@codeaurora.org> <1479981638-32069-6-git-send-email-akdwived@codeaurora.org> <20161209043510.GS30492@tuxbot> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:46336 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750933AbcLLMpp (ORCPT ); Mon, 12 Dec 2016 07:45:45 -0500 In-Reply-To: <20161209043510.GS30492@tuxbot> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Bjorn Andersson Cc: sboyd@codeaurora.org, agross@codeaurora.org, linux-arm-msm@vger.kernel.org On 12/9/2016 10:05 AM, Bjorn Andersson wrote: > On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> This change introduces appropriate additional steps in reset sequence so >> that hexagon v56 1.5.0 is brough out of reset. >> > You should use the non-_relaxed version throughout this patch, unless > you have good reason not to. OK. > >> Signed-off-by: Avaneesh Kumar Dwivedi >> --- >> drivers/remoteproc/qcom_q6v5_pil.c | 125 ++++++++++++++++++++++++++++++------- >> 1 file changed, 104 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index c55dc9a..7ea2f53 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -65,6 +65,8 @@ >> #define QDSP6SS_RESET_REG 0x014 >> #define QDSP6SS_GFMUX_CTL_REG 0x020 >> #define QDSP6SS_PWR_CTL_REG 0x030 >> +#define QDSP6SS_MEM_PWR_CTL 0x0B0 >> +#define QDSP6SS_STRAP_ACC 0x110 >> >> /* AXI Halt Register Offsets */ >> #define AXI_HALTREQ_REG 0x0 >> @@ -93,6 +95,15 @@ >> #define QDSS_BHS_ON BIT(21) >> #define QDSS_LDO_BYP BIT(22) >> >> +/* QDSP6v56 parameters */ >> +#define QDSP6v56_LDO_BYP BIT(25) >> +#define QDSP6v56_BHS_ON BIT(24) >> +#define QDSP6v56_CLAMP_WL BIT(21) >> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22) >> +#define HALT_CHECK_MAX_LOOPS (200) >> +#define QDSP6SS_XO_CBCR (0x0038) > Please drop these parenthesis. OK. > >> +#define QDSP6SS_ACC_OVERRIDE_VAL 0x20 >> + >> struct rproc_hexagon_res { >> char *hexagon_mba_image; >> char **proxy_reg_string; >> @@ -147,6 +158,8 @@ struct q6v5 { >> phys_addr_t mpss_reloc; >> void *mpss_region; >> size_t mpss_size; >> + >> + int hexagon_ver; >> }; >> >> enum { >> @@ -388,35 +401,103 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms) >> >> static int q6v5proc_reset(struct q6v5 *qproc) >> { >> - u32 val; >> - int ret; >> + u64 val; >> + int ret, i, count; > One variable per line, sorted descending by length, please. OK. > >> + >> + /* Override the ACC value if required */ >> + if (qproc->hexagon_ver & 0x2) > This will break when we reach the 6th version, compare (==) with the > related enum. OK. > >> + writel_relaxed(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 */ > This comment goes inside the if-statement. OK. > >> + if (qproc->hexagon_ver & 0x2) { > == > >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR); >> + val |= 0x1; >> + writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR); > Replace the rest of this block with: > > ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR, > val, !(val & BIT(31)), 1, 200); OK. > >> + for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) { >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR); >> + if (!(val & BIT(31))) >> + break; >> + udelay(1); >> + } >> + >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR); >> + if ((val & BIT(31))) >> + dev_err(qproc->dev, "Failed to enable xo branch clock.\n"); >> + } >> + >> + if (qproc->hexagon_ver & 0x2) { > == > >> /* Enable power block headswitch, and wait for it to stabilize */ >> - val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - val |= QDSS_BHS_ON | QDSS_LDO_BYP; >> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - udelay(1); >> - >> - /* >> - * Turn on memories. L2 banks should be done individually >> - * to minimize inrush current. >> - */ >> - val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N | >> - Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N; >> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - val |= Q6SS_L2DATA_SLP_NRET_N_2; >> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - val |= Q6SS_L2DATA_SLP_NRET_N_1; >> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> - val |= Q6SS_L2DATA_SLP_NRET_N_0; >> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG); > Use non-relaxed version of readl and writel, please. OK. > >> + val |= QDSP6v56_BHS_ON; >> + writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + udelay(1); > Please add a short comment on why this delay is needed. I think there is a comment that /* Enable power block headswitch, and wait for it to stabilize */ so block head switch need stabilization after enabling so 1 us delay, before LDO being put in bypass mode. > >> >> + /* Put LDO in bypass mode */ >> + val |= QDSP6v56_LDO_BYP; >> + writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + > Remove empty line ok. > >> + } else { >> + /* >> + * Enable power block headswitch, >> + * and wait for it to stabilize >> + */ >> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= QDSS_BHS_ON | QDSS_LDO_BYP; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + udelay(1); >> + } >> + >> + if (qproc->hexagon_ver & 0x2) { > Why do you have: > > if (ver == 2) { > ... > } > > if (ver == 2) { > ... > } else { > ... > } > > if (ver == 2) { > ... > } else { > ... > } > > As far as I can see you should be able to merge those into one if/else. wanted to utilize little piece of common code so put different if block. OK will combine them. > >> + /* >> + * Deassert QDSP6 compiler memory clamp >> + */ >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val &= ~QDSP6v56_CLAMP_QMC_MEM; >> + writel_relaxed(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_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + >> + /* Turn on L1, L2, ETB and JU memories 1 at a time */ >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL); >> + for (i = 19; i >= 0; i--) { >> + val |= BIT(i); >> + writel_relaxed(val, qproc->reg_base + >> + QDSP6SS_MEM_PWR_CTL); >> + /* >> + * Wait for 1us for both memory peripheral and >> + * data array to turn on. >> + */ >> + mb(); > mb() ensures that your writes are ordered, it does not ensure that the > write is done before the sleep. What is the actual requirement here? As in comment, order of turning need to be serialized so this memory barrier. Do u think its not required? >> + udelay(1); >> + } >> + /* Remove word line clamp */ >> + val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val &= ~QDSP6v56_CLAMP_WL; >> + writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + } else { >> + /* >> + * Turn on memories. L2 banks should be done individually >> + * to minimize inrush current. >> + */ >> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N | >> + Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= Q6SS_L2DATA_SLP_NRET_N_2; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + val |= Q6SS_L2DATA_SLP_NRET_N_1; >> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); >> + 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); > Regards, > Bjorn -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.