Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific changes.

["Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org>]

On 12/23/2016 6:34 AM, Bjorn Andersson wrote:
> On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:
>
>> +/* 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
> Indent these with tabs, like the others.
Ok.
>
>> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
>> +
>>   struct reg_info {
>>   	struct regulator *reg;
>>   	int uV;
>> @@ -111,6 +123,7 @@ struct rproc_hexagon_res {
>>   	struct qcom_mss_reg_res active_supply[2];
>>   	char **proxy_clk_string;
>>   	char **active_clk_string;
>> +	int hexagon_ver;
>>   };
>>   
>>   struct q6v5 {
>> @@ -152,8 +165,14 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	int hexagon_ver;
> Please name this "version" instead, there's little confusion to what it
> is.
OK.
>>   };
>>   
>> +enum {
>> +	MSS_MSM8916, /*hexagon on msm8916*/
>> +	MSS_MSM8974, /*hexagon on msm8974*/
>> +	MSS_MSM8996, /*hexagon on msm8996*/
> You can skip the comments now.
OK.
>
>> +};
>>   
>>   static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>>   				const struct qcom_mss_reg_res *reg_res)
>> @@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>>   
>>   static int q6v5proc_reset(struct q6v5 *qproc)
>>   {
>> -	u32 val;
>> +	u64 val;
>>   	int ret;
>> +	int 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);
>>   
>> -	/* 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);
>> +	if (qproc->hexagon_ver == 0x2) {
> == MSS_MSM8996
OK.
>> +		/* 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 |= 0x1;
>> +		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> Please add a comment here, describing what we're waiting for (rather
> than just "a magical bit 31")
In off condition bit 31 (CLKOFF) bit is set as 1, when we write 0x1 to 
this register and clock is turned on it become 0x0.
will add appropriate comment.
>
>> +		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;
>> +		}
>> +		if ((val & BIT(31))) {
> If bit 31 is set when the readl_poll_timeout() hits the timeout the
> condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
> don't think this can happen.
Sure this is not required. Will remove it
>> +			dev_err(qproc->dev,
>> +				"Failed to enable xo branch clock.\n");
>> +			return -EINVAL;
>> +		}
>> +		/*
>> +		 * Enable power block headswitch,
>> +		 * and wait for it to stabilize
> This comment fits within 80 characters, so no need to line wrap it.
OK, i had wrapped it because i was getting warning on running 
checkpatch.pl, if fit in one line without warning will turn it into one 
line comment.
>> +		 */
>> +		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);
>> +		udelay(1);
> Please insert a single empty line here, to create some separation
> between the "steps".
OK.
>
>> +		/* 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_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Please drop the _relaxed
Sorry for repeated comments on this. Will do it for sure.
>
>> +
>> +		/* 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--) {
>> +			val |= BIT(i);
>> +			writel(val, qproc->reg_base +
>> +						QDSP6SS_MEM_PWR_CTL);
>> +			/*
>> +			 * Wait for 1us for both memory peripheral and
>> +			 * data array to turn on.
>> +			 */
> /* Read back value to ensure the write is done */
>
> And move the 1us comment below the read.
Sure.
>
>> +			 val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> Please correct the indentation of this line.
OK.
>
>> +			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 {
>> +		/* 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);
>> +		/*
>> +		 * 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);
>> +		val |= readl(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);
>> +	}
>>   	/* Remove IO clamp */
>>   	val &= ~Q6SS_CLAMP_IO;
>>   	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> @@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
>>   {
>>   	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>> +	int val;
>>   
>>   	qproc->running = false;
>>   
>> @@ -681,6 +773,15 @@ static int q6v5_stop(struct rproc *rproc)
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>>   	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>>   
>> +	if (qproc->hexagon_ver == 0x2) {
> == MSS_MSM8996
OK
>> +		/*
>> +		 * To avoid high MX current during LPASS/MSS restart.
>> +		 */
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Please no _relaxed()
OK
>
>> +		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
>> +			QDSP6v56_CLAMP_QMC_MEM;
>> +		writel_relaxed(val,	qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Dito
OK
>
>> +	}
>>   	reset_control_assert(qproc->mss_restart);
>>   	q6v5_clk_disable(qproc->dev, qproc->active_clks,
>>   				qproc->active_clk_count);
> 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.