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

[Lee Jones]

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