# Re: \[PATCH\] interconnect: qcom: sdm660: Correct NOC\_QOS\_PRIORITY shift and mask

- From: AngeloGioacchino Del Regno \<hidden\>
- Date: 2021-09-01 14:52:00 
- List: linux-pm
- Page: https://ratatoskr.run/linux-pm/2021/09/8287470
- Thread: https://ratatoskr.run/linux-arm-msm/2021/08/8287384/t.md

```text
Il 23/08/21 09:54, Shawn Guo ha scritto:
> The NOC_QOS_PRIORITY shift and mask do not match what vendor kernel
> defines [1].  Correct them per vendor kernel.
>
> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/soc/qcom/msm_bus/msm_bus_noc_adhoc.c?h=LA.UM.8.2.r1-04800-sdm660.0#n37
>
> Signed-off-by: Shawn Guo <redacted>

Hello Shawn,
the patch looks great and I'm sorry for getting it wrong in the first place; luckily there was no regression due to this mistake.

However, a few considerations:

> ---
>   drivers/interconnect/qcom/sdm660.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index ac13046537e8..c89c991a80a0 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -44,9 +44,10 @@
>   #define NOC_PERM_MODE_BYPASS		(1 << NOC_QOS_MODE_BYPASS)
>   
>   #define NOC_QOS_PRIORITYn_ADDR(n)	(0x8 + (n * 0x1000))
> -#define NOC_QOS_PRIORITY_MASK		0xf
> +#define NOC_QOS_PRIORITY_P1_MASK	0xc
> +#define NOC_QOS_PRIORITY_P0_MASK	0x3
>   #define NOC_QOS_PRIORITY_P1_SHIFT	0x2
> -#define NOC_QOS_PRIORITY_P0_SHIFT	0x3
> +#define NOC_QOS_PRIORITY_P0_SHIFT	0x0

Defining NOC_QOS_PRIORITY_P0_SHIFT as 0x0 means that there's no shifting action,
which means that we can avoid defining this at all.

>   
>   #define NOC_QOS_MODEn_ADDR(n)		(0xc + (n * 0x1000))
>   #define NOC_QOS_MODEn_MASK		0x3
> @@ -624,13 +625,13 @@ static int qcom_icc_noc_set_qos_priority(struct regmap *rmap,
>   	/* Must be updated one at a time, P1 first, P0 last */
>   	val = qos->areq_prio << NOC_QOS_PRIORITY_P1_SHIFT;
>   	rc = regmap_update_bits(rmap, NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
> -				NOC_QOS_PRIORITY_MASK, val);
> +				NOC_QOS_PRIORITY_P1_MASK, val);
>   	if (rc)
>   		return rc;
>   
>   	val = qos->prio_level << NOC_QOS_PRIORITY_P0_SHIFT;

... so this would be `qos->prio_level << 0`, so if we simply not define any NOC_QOS_PRIORITY_P0_SHIFT, this can be simplified as:

val = qos->prio_level;

...but that would also eliminate the need to assign it to `val`, so...

>   	return regmap_update_bits(rmap, NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
> -				  NOC_QOS_PRIORITY_MASK, val);
> +				  NOC_QOS_PRIORITY_P0_MASK, val);

Here we can instead do:
	return regmap_update_bits(rmap, NOC_QOS_PRIORITYn_ADDR(qos->qos_port),

				  NOC_QOS_PRIORITY_MASK, qos->prio_level);

				  NOC_QOS_PRIORITY_P0_MASK, qos->prio_level);

Cheers,
- Angelo

>   }
>   
>   static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)
>

```