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: [email protected]
> Fixes: 58e214382bdd ("mfd: qcom-rpm: Driver for the Qualcomm RPM")
> Cc: Björn Andersson <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>

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 <[email protected]>

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

Reply via email to