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 <akdwived@xxxxxxxxxxxxxx>
---
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
