Re: [PATCH] mfd: qcom_rpm: fix offset error for msm8660

[Bjorn Andersson]

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@xxxxxxxxxxxxxxx
> Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")
> Cc: Björn Andersson <bjorn.andersson@xxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

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@xxxxxxxxxx>

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