Re: [RESEND PATCH v4 5/7] remoteproc: qcom: Modify reset sequence for hexagon to support v56 1.5.0

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  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.

> +#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.

> +
> +	/* 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.

> +		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.

> +	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);

> +		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.

> +		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.

>  
> +		/* Put LDO in bypass mode */
> +		val |= QDSP6v56_LDO_BYP;
> +		writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +

Remove empty line

> +	} 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.

> +		/*
> +		 * 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?

> +			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