| Message ID | 20210823075405.382-1-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | interconnect: qcom: sdm660: Correct NOC_QOS_PRIORITY shift and mask | expand |
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) >
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 --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)
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(-)