diff mbox series

interconnect: qcom: sdm660: Correct NOC_QOS_PRIORITY shift and mask

Message ID 20210823075405.382-1-shawn.guo@linaro.org
State Superseded
Headers show
Series interconnect: qcom: sdm660: Correct NOC_QOS_PRIORITY shift and mask | expand

Commit Message

Shawn Guo Aug. 23, 2021, 7:54 a.m. UTC
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 <shawn.guo@linaro.org>

---
 drivers/interconnect/qcom/sdm660.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Dmitry Baryshkov Sept. 1, 2021, 2:31 p.m. UTC | #1
On 23/08/2021 10:54, Shawn Guo wrote:
> 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 <shawn.guo@linaro.org>


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> ---

>   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

>   

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

>   	return regmap_update_bits(rmap, NOC_QOS_PRIORITYn_ADDR(qos->qos_port),

> -				  NOC_QOS_PRIORITY_MASK, val);

> +				  NOC_QOS_PRIORITY_P0_MASK, val);

>   }

>   

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

> 



-- 
With best wishes
Dmitry
AngeloGioacchino Del Regno Sept. 1, 2021, 2:51 p.m. UTC | #2
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 <shawn.guo@linaro.org>


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)

>
diff mbox series

Patch

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
 
 #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;
 	return regmap_update_bits(rmap, NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
-				  NOC_QOS_PRIORITY_MASK, val);
+				  NOC_QOS_PRIORITY_P0_MASK, val);
 }
 
 static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)