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
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
