mfd: qcom_rpm: fix offset error for msm8660

Message ID 1465897725-16213-1-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij June 14, 2016, 9:48 a.m.
The RPM in MSM8660/APQ8060 has different offsets to the selector
ACK and request context ACK registers. Make all these register
offsets part of the per-SoC data and assign the right values.

The bug was found by verifying backwards to the vendor tree in
the out-of-tree files <mach/rpm-[8660|8064|8960]>: all were using
offsets 3,11,15,23 except the MSM8660/APQ8060 which was using
offsets 3,11,19,27.

Cc: stable@vger.kernel.org
Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")
Cc: Björn Andersson <bjorn.andersson@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/mfd/qcom_rpm.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

-- 
2.4.11

--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Walleij June 14, 2016, 7:38 p.m. | #1
On Tue, Jun 14, 2016 at 8:51 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/14, Linus Walleij wrote:


>> -#define RPM_REQUEST_CONTEXT  3

>> -#define RPM_REQ_SELECT               11

>> -#define RPM_ACK_CONTEXT              15

>> -#define RPM_ACK_SELECTOR     23

>>  #define RPM_SELECT_SIZE              7

>

> The RPM_SELECT_SIZE is 7 on 8660, but now you've pointed out that

> otherwise the size is 4. I think you've uncovered another bug.


OMG you're right. I'll send a v2 fixing that too.

>> -                            RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));

>> +                            RPM_CTRL_REG(rpm, rpm->data->req_sel_off + i));

>

> Here we write from 0 to ARRAY_SIZE(sel_mask) which is 7. That

> would mean we write into the ack context that starts at 15 (we

> start writing at req_sel_off which is always 11). Oops.


Argh well it seems it didn't hurt so far but let's have it fixed.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson June 14, 2016, 7:38 p.m. | #2
On Tue 14 Jun 02:48 PDT 2016, Linus Walleij wrote:

> The RPM in MSM8660/APQ8060 has different offsets to the selector

> ACK and request context ACK registers. Make all these register

> offsets part of the per-SoC data and assign the right values.

> 

> The bug was found by verifying backwards to the vendor tree in

> the out-of-tree files <mach/rpm-[8660|8064|8960]>: all were using

> offsets 3,11,15,23 except the MSM8660/APQ8060 which was using

> offsets 3,11,19,27.

> 

> Cc: stable@vger.kernel.org

> Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")

> Cc: Björn Andersson <bjorn.andersson@linaro.org>

> Cc: Stephen Boyd <sboyd@codeaurora.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Thanks for finding this, sorry for not getting it right in the first
place.

With the note about the odd indentation below, and a wish that you fix
the sel_mask length

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> ---

>  drivers/mfd/qcom_rpm.c | 37 +++++++++++++++++++++++++++----------

>  1 file changed, 27 insertions(+), 10 deletions(-)

> 

> diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c

[..]
> @@ -155,6 +155,10 @@ static const struct qcom_rpm_data apq8064_template = {

>  	.version = 3,

>  	.resource_table = apq8064_rpm_resource_table,

>  	.n_resources = ARRAY_SIZE(apq8064_rpm_resource_table),

> +	.req_ctx_off = 3,

> +	.req_sel_off =  11,


Odd indentation of 11, here and below.

> +	.ack_ctx_off = 15,

> +	.ack_sel_off = 23,

>  };

>  

>  static const struct qcom_rpm_resource msm8660_rpm_resource_table[] = {

> @@ -238,6 +242,10 @@ static const struct qcom_rpm_data msm8660_template = {

>  	.version = 2,

>  	.resource_table = msm8660_rpm_resource_table,

>  	.n_resources = ARRAY_SIZE(msm8660_rpm_resource_table),

> +	.req_ctx_off = 3,

> +	.req_sel_off =  11,

> +	.ack_ctx_off = 19,

> +	.ack_sel_off = 27,

>  };

>  

>  static const struct qcom_rpm_resource msm8960_rpm_resource_table[] = {

> @@ -320,6 +328,10 @@ static const struct qcom_rpm_data msm8960_template = {

>  	.version = 3,

>  	.resource_table = msm8960_rpm_resource_table,

>  	.n_resources = ARRAY_SIZE(msm8960_rpm_resource_table),

> +	.req_ctx_off = 3,

> +	.req_sel_off =  11,

> +	.ack_ctx_off = 15,

> +	.ack_sel_off = 23,

>  };

>  

>  static const struct qcom_rpm_resource ipq806x_rpm_resource_table[] = {

> @@ -360,6 +372,10 @@ static const struct qcom_rpm_data ipq806x_template = {

>  	.version = 3,

>  	.resource_table = ipq806x_rpm_resource_table,

>  	.n_resources = ARRAY_SIZE(ipq806x_rpm_resource_table),

> +	.req_ctx_off = 3,

> +	.req_sel_off =  11,

> +	.ack_ctx_off = 15,

> +	.ack_sel_off = 23,

>  };

>  

>  static const struct of_device_id qcom_rpm_of_match[] = {

> @@ -398,10 +414,10 @@ int qcom_rpm_write(struct qcom_rpm *rpm,

>  	bitmap_set((unsigned long *)sel_mask, res->select_id, 1);

>  	for (i = 0; i < ARRAY_SIZE(sel_mask); i++) {


As Stephen pointed out, this constant should be platform specific as
well. Would you mind fold that in, or maybe write a separate patch for
it?

>  		writel_relaxed(sel_mask[i],

> -			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));

> +			       RPM_CTRL_REG(rpm, rpm->data->req_sel_off + i));

>  	}


Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
index 4f589cf75549..82cc7986e1ca 100644
--- a/drivers/mfd/qcom_rpm.c
+++ b/drivers/mfd/qcom_rpm.c
@@ -34,7 +34,11 @@  struct qcom_rpm_resource {
 struct qcom_rpm_data {
 	u32 version;
 	const struct qcom_rpm_resource *resource_table;
-	unsigned n_resources;
+	unsigned int n_resources;
+	unsigned int req_ctx_off;
+	unsigned int req_sel_off;
+	unsigned int ack_ctx_off;
+	unsigned int ack_sel_off;
 };
 
 struct qcom_rpm {
@@ -61,10 +65,6 @@  struct qcom_rpm {
 
 #define RPM_REQUEST_TIMEOUT	(5 * HZ)
 
-#define RPM_REQUEST_CONTEXT	3
-#define RPM_REQ_SELECT		11
-#define RPM_ACK_CONTEXT		15
-#define RPM_ACK_SELECTOR	23
 #define RPM_SELECT_SIZE		7
 
 #define RPM_NOTIFICATION	BIT(30)
@@ -155,6 +155,10 @@  static const struct qcom_rpm_data apq8064_template = {
 	.version = 3,
 	.resource_table = apq8064_rpm_resource_table,
 	.n_resources = ARRAY_SIZE(apq8064_rpm_resource_table),
+	.req_ctx_off = 3,
+	.req_sel_off =  11,
+	.ack_ctx_off = 15,
+	.ack_sel_off = 23,
 };
 
 static const struct qcom_rpm_resource msm8660_rpm_resource_table[] = {
@@ -238,6 +242,10 @@  static const struct qcom_rpm_data msm8660_template = {
 	.version = 2,
 	.resource_table = msm8660_rpm_resource_table,
 	.n_resources = ARRAY_SIZE(msm8660_rpm_resource_table),
+	.req_ctx_off = 3,
+	.req_sel_off =  11,
+	.ack_ctx_off = 19,
+	.ack_sel_off = 27,
 };
 
 static const struct qcom_rpm_resource msm8960_rpm_resource_table[] = {
@@ -320,6 +328,10 @@  static const struct qcom_rpm_data msm8960_template = {
 	.version = 3,
 	.resource_table = msm8960_rpm_resource_table,
 	.n_resources = ARRAY_SIZE(msm8960_rpm_resource_table),
+	.req_ctx_off = 3,
+	.req_sel_off =  11,
+	.ack_ctx_off = 15,
+	.ack_sel_off = 23,
 };
 
 static const struct qcom_rpm_resource ipq806x_rpm_resource_table[] = {
@@ -360,6 +372,10 @@  static const struct qcom_rpm_data ipq806x_template = {
 	.version = 3,
 	.resource_table = ipq806x_rpm_resource_table,
 	.n_resources = ARRAY_SIZE(ipq806x_rpm_resource_table),
+	.req_ctx_off = 3,
+	.req_sel_off =  11,
+	.ack_ctx_off = 15,
+	.ack_sel_off = 23,
 };
 
 static const struct of_device_id qcom_rpm_of_match[] = {
@@ -398,10 +414,10 @@  int qcom_rpm_write(struct qcom_rpm *rpm,
 	bitmap_set((unsigned long *)sel_mask, res->select_id, 1);
 	for (i = 0; i < ARRAY_SIZE(sel_mask); i++) {
 		writel_relaxed(sel_mask[i],
-			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
+			       RPM_CTRL_REG(rpm, rpm->data->req_sel_off + i));
 	}
 
-	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
+	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, rpm->data->req_ctx_off));
 
 	reinit_completion(&rpm->ack);
 	regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit));
@@ -424,10 +440,11 @@  static irqreturn_t qcom_rpm_ack_interrupt(int irq, void *dev)
 	u32 ack;
 	int i;
 
-	ack = readl_relaxed(RPM_CTRL_REG(rpm, RPM_ACK_CONTEXT));
+	ack = readl_relaxed(RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
 	for (i = 0; i < RPM_SELECT_SIZE; i++)
-		writel_relaxed(0, RPM_CTRL_REG(rpm, RPM_ACK_SELECTOR + i));
-	writel(0, RPM_CTRL_REG(rpm, RPM_ACK_CONTEXT));
+		writel_relaxed(0,
+			RPM_CTRL_REG(rpm, rpm->data->ack_sel_off + i));
+	writel(0, RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
 
 	if (ack & RPM_NOTIFICATION) {
 		dev_warn(rpm->dev, "ignoring notification!\n");