linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660
@ 2016-06-14 23:02 Linus Walleij
  2016-06-15  8:38 ` Lee Jones
  2016-06-16  0:20 ` Stephen Boyd
  0 siblings, 2 replies; 4+ messages in thread
From: Linus Walleij @ 2016-06-14 23:02 UTC (permalink / raw)
  To: lee.jones, linux-kernel, Bjorn Andersson, Stephen Boyd
  Cc: Linus Walleij, stable

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 and a select size of 4, except the MSM8660/APQ8060
which was using offsets 3,11,19,27 and a select size of 7.

All other platforms apart from msm8660 were affected by reading
excess registers, since 7 was hardcoded as the number of select
words, this patch makes also this part dynamic so we only write/read
as many select words as the platform actually use.

Symptoms of this bug when using msm8660: the first RPM transaction
would work, but the next would stall or raise an error since the
previous transaction was not properly ACKed as the ACK words were
read at the wrong offset.

Cc: stable@vger.kernel.org
Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")
Cc: Stephen Boyd <sboyd@codeaurora.org>
Reviewed-by: Björn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Fix the odd indentation pointed out by Björn, add reviewed-by.
ChangeLog v1->v2:
- Also augment the device data to hold the number of select words
  as the was wrong on all other platforms: 4 vs 7.
---
 drivers/mfd/qcom_rpm.c | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
index 1be47ad6441b..9364f88264e5 100644
--- a/drivers/mfd/qcom_rpm.c
+++ b/drivers/mfd/qcom_rpm.c
@@ -34,7 +34,12 @@ 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;
+	unsigned int sel_size;
 };
 
 struct qcom_rpm {
@@ -61,11 +66,7 @@ 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_MAX_SEL_SIZE	7
 
 #define RPM_NOTIFICATION	BIT(30)
 #define RPM_REJECTED		BIT(31)
@@ -157,6 +158,11 @@ 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,
+	.sel_size = 4,
 };
 
 static const struct qcom_rpm_resource msm8660_rpm_resource_table[] = {
@@ -240,6 +246,11 @@ 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,
+	.sel_size = 7,
 };
 
 static const struct qcom_rpm_resource msm8960_rpm_resource_table[] = {
@@ -322,6 +333,11 @@ 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,
+	.sel_size = 4,
 };
 
 static const struct qcom_rpm_resource ipq806x_rpm_resource_table[] = {
@@ -362,6 +378,11 @@ 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,
+	.sel_size = 4,
 };
 
 static const struct of_device_id qcom_rpm_of_match[] = {
@@ -380,7 +401,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm,
 {
 	const struct qcom_rpm_resource *res;
 	const struct qcom_rpm_data *data = rpm->data;
-	u32 sel_mask[RPM_SELECT_SIZE] = { 0 };
+	u32 sel_mask[RPM_MAX_SEL_SIZE] = { 0 };
 	int left;
 	int ret = 0;
 	int i;
@@ -398,12 +419,12 @@ int qcom_rpm_write(struct qcom_rpm *rpm,
 		writel_relaxed(buf[i], RPM_REQ_REG(rpm, res->target_id + i));
 
 	bitmap_set((unsigned long *)sel_mask, res->select_id, 1);
-	for (i = 0; i < ARRAY_SIZE(sel_mask); i++) {
+	for (i = 0; i < rpm->data->sel_size; 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));
@@ -426,10 +447,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));
-	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));
+	ack = readl_relaxed(RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
+	for (i = 0; i < rpm->data->sel_size; i++)
+		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");
-- 
2.4.11

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660
  2016-06-14 23:02 [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660 Linus Walleij
@ 2016-06-15  8:38 ` Lee Jones
  2016-06-16  0:20 ` Stephen Boyd
  1 sibling, 0 replies; 4+ messages in thread
From: Lee Jones @ 2016-06-15  8:38 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-kernel, Bjorn Andersson, Stephen Boyd, stable

On Wed, 15 Jun 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 and a select size of 4, except the MSM8660/APQ8060
> which was using offsets 3,11,19,27 and a select size of 7.
> 
> All other platforms apart from msm8660 were affected by reading
> excess registers, since 7 was hardcoded as the number of select
> words, this patch makes also this part dynamic so we only write/read
> as many select words as the platform actually use.
> 
> Symptoms of this bug when using msm8660: the first RPM transaction
> would work, but the next would stall or raise an error since the
> previous transaction was not properly ACKed as the ACK words were
> read at the wrong offset.
> 
> Cc: stable@vger.kernel.org
> Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Reviewed-by: Björn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:
> - Fix the odd indentation pointed out by Björn, add reviewed-by.
> ChangeLog v1->v2:
> - Also augment the device data to hold the number of select words
>   as the was wrong on all other platforms: 4 vs 7.
> ---
>  drivers/mfd/qcom_rpm.c | 50 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 14 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
> index 1be47ad6441b..9364f88264e5 100644
> --- a/drivers/mfd/qcom_rpm.c
> +++ b/drivers/mfd/qcom_rpm.c
> @@ -34,7 +34,12 @@ 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;
> +	unsigned int sel_size;
>  };
>  
>  struct qcom_rpm {
> @@ -61,11 +66,7 @@ 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_MAX_SEL_SIZE	7
>  
>  #define RPM_NOTIFICATION	BIT(30)
>  #define RPM_REJECTED		BIT(31)
> @@ -157,6 +158,11 @@ 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,
> +	.sel_size = 4,
>  };
>  
>  static const struct qcom_rpm_resource msm8660_rpm_resource_table[] = {
> @@ -240,6 +246,11 @@ 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,
> +	.sel_size = 7,
>  };
>  
>  static const struct qcom_rpm_resource msm8960_rpm_resource_table[] = {
> @@ -322,6 +333,11 @@ 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,
> +	.sel_size = 4,
>  };
>  
>  static const struct qcom_rpm_resource ipq806x_rpm_resource_table[] = {
> @@ -362,6 +378,11 @@ 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,
> +	.sel_size = 4,
>  };
>  
>  static const struct of_device_id qcom_rpm_of_match[] = {
> @@ -380,7 +401,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm,
>  {
>  	const struct qcom_rpm_resource *res;
>  	const struct qcom_rpm_data *data = rpm->data;
> -	u32 sel_mask[RPM_SELECT_SIZE] = { 0 };
> +	u32 sel_mask[RPM_MAX_SEL_SIZE] = { 0 };
>  	int left;
>  	int ret = 0;
>  	int i;
> @@ -398,12 +419,12 @@ int qcom_rpm_write(struct qcom_rpm *rpm,
>  		writel_relaxed(buf[i], RPM_REQ_REG(rpm, res->target_id + i));
>  
>  	bitmap_set((unsigned long *)sel_mask, res->select_id, 1);
> -	for (i = 0; i < ARRAY_SIZE(sel_mask); i++) {
> +	for (i = 0; i < rpm->data->sel_size; 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));
> @@ -426,10 +447,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));
> -	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));
> +	ack = readl_relaxed(RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
> +	for (i = 0; i < rpm->data->sel_size; i++)
> +		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");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660
  2016-06-14 23:02 [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660 Linus Walleij
  2016-06-15  8:38 ` Lee Jones
@ 2016-06-16  0:20 ` Stephen Boyd
  2016-06-16  7:20   ` Linus Walleij
  1 sibling, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2016-06-16  0:20 UTC (permalink / raw)
  To: Linus Walleij; +Cc: lee.jones, linux-kernel, Bjorn Andersson, stable

On 06/15, Linus Walleij wrote:
> @@ -426,10 +447,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));
> -	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));
> +	ack = readl_relaxed(RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
> +	for (i = 0; i < rpm->data->sel_size; i++)
> +		writel_relaxed(0,
> +			RPM_CTRL_REG(rpm, rpm->data->ack_sel_off + i));
> +	writel(0, RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));

Does the ack size vary though? I thought that was always 7. It
seems that really only the request selector size varies?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660
  2016-06-16  0:20 ` Stephen Boyd
@ 2016-06-16  7:20   ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2016-06-16  7:20 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Lee Jones, linux-kernel, Bjorn Andersson, stable

On Thu, Jun 16, 2016 at 2:20 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/15, Linus Walleij wrote:
>> @@ -426,10 +447,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));
>> -     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));
>> +     ack = readl_relaxed(RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
>> +     for (i = 0; i < rpm->data->sel_size; i++)
>> +             writel_relaxed(0,
>> +                     RPM_CTRL_REG(rpm, rpm->data->ack_sel_off + i));
>> +     writel(0, RPM_CTRL_REG(rpm, rpm->data->ack_ctx_off));
>
> Does the ack size vary though? I thought that was always 7. It
> seems that really only the request selector size varies?

Ah you're right, I'll send a patch on top of this one fixing it up.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-06-16  7:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-14 23:02 [PATCH v3] mfd: qcom_rpm: fix offset error for msm8660 Linus Walleij
2016-06-15  8:38 ` Lee Jones
2016-06-16  0:20 ` Stephen Boyd
2016-06-16  7:20   ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).