diff mbox series

interconnect: qcom: Fix DT backwards compatibility for QoS

Message ID 20240704125515.22194-1-quic_okukatla@quicinc.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series interconnect: qcom: Fix DT backwards compatibility for QoS | expand

Commit Message

Odelu Kukatla July 4, 2024, 12:55 p.m. UTC
Add qos_clks_required flag to skip QoS configuration if clocks property
is not populated in devicetree for providers which require clocks to be
enabled for accessing registers. This is to keep the QoS configuration
backwards compatible with devices that have older DTB.

Reported-by: Bjorn Andersson <andersson@kernel.org>
Closes: https://lore.kernel.org/all/ciji6nlxn752ina4tmh6kwvek52nxpnguomqek6plwvwgvoqef@yrtexkpmn5br/
Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 drivers/interconnect/qcom/icc-rpmh.c | 2 +-
 drivers/interconnect/qcom/icc-rpmh.h | 1 +
 drivers/interconnect/qcom/sc7280.c   | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson July 4, 2024, 5:44 p.m. UTC | #1
On Thu, Jul 04, 2024 at 06:25:15PM GMT, Odelu Kukatla wrote:
> Add qos_clks_required flag to skip QoS configuration if clocks property
> is not populated in devicetree for providers which require clocks to be
> enabled for accessing registers. This is to keep the QoS configuration
> backwards compatible with devices that have older DTB.
> 

Please read "Describe your changes" [1], and make your commit message
start with the problem description - establish to the reader why this
change is needed, then follow that with a technical description of the
solution (likely in a separate paragraph).

[1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> Reported-by: Bjorn Andersson <andersson@kernel.org>
> Closes: https://lore.kernel.org/all/ciji6nlxn752ina4tmh6kwvek52nxpnguomqek6plwvwgvoqef@yrtexkpmn5br/
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  drivers/interconnect/qcom/icc-rpmh.c | 2 +-
>  drivers/interconnect/qcom/icc-rpmh.h | 1 +
>  drivers/interconnect/qcom/sc7280.c   | 2 ++
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index 93047defd5e2..f49a8e0cb03c 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -311,7 +311,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  		}
>  
>  		qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks);
> -		if (qp->num_clks < 0) {
> +		if (qp->num_clks < 0 || (!qp->num_clks && desc->qos_clks_required)) {

For this new case, I think the dev_info() below makes total sense. I.e.
this looks good to me.


However, the num_clks < 0 case would represent finding a devicetree node
with clocks specified, but failing to get these clocks. I believe that
this would include EPROBE_DEFER.

I don't think it's correct to print a informational message and continue
without QoS. I think we should fail here.

Also, in this case devm_clk_bulk_get_all() would have pr_err() a message
about which clock it failed to acquire and why, so printing again here
doesn't seem useful.

But I think that is a separate follow up patch.

>  			dev_info(dev, "Skipping QoS, failed to get clk: %d\n", qp->num_clks);
>  			goto skip_qos_config;
>  		}
> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h
> index 9a5142c70486..14db89850fb3 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.h
> +++ b/drivers/interconnect/qcom/icc-rpmh.h
> @@ -153,6 +153,7 @@ struct qcom_icc_desc {
>  	size_t num_nodes;
>  	struct qcom_icc_bcm * const *bcms;
>  	size_t num_bcms;
> +	bool qos_clks_required;
>  };
>  
>  int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> diff --git a/drivers/interconnect/qcom/sc7280.c b/drivers/interconnect/qcom/sc7280.c
> index 759c609a20bf..167971f8e8be 100644
> --- a/drivers/interconnect/qcom/sc7280.c
> +++ b/drivers/interconnect/qcom/sc7280.c
> @@ -1691,6 +1691,7 @@ static const struct qcom_icc_desc sc7280_aggre1_noc = {
>  	.num_nodes = ARRAY_SIZE(aggre1_noc_nodes),
>  	.bcms = aggre1_noc_bcms,
>  	.num_bcms = ARRAY_SIZE(aggre1_noc_bcms),
> +	.qos_clks_required = true,
>  };
>  
>  static struct qcom_icc_bcm * const aggre2_noc_bcms[] = {
> @@ -1722,6 +1723,7 @@ static const struct qcom_icc_desc sc7280_aggre2_noc = {
>  	.num_nodes = ARRAY_SIZE(aggre2_noc_nodes),
>  	.bcms = aggre2_noc_bcms,
>  	.num_bcms = ARRAY_SIZE(aggre2_noc_bcms),
> +	.qos_clks_required = true,

When reading the commit message and the name of this property, my first
reaction was "aggre2_noc requires qos clocks".

As such, I'd prefer if this was renamed "qos_requires_clocks" to more
clearly document what the requirement entails and hint the future reader
that the interconnect might still be operational.


With that said, thanks for your quick reply to my regression report!

Tested-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

>  };
>  
>  static struct qcom_icc_bcm * const clk_virt_bcms[] = {
> -- 
> 2.17.1
>
Konrad Dybcio July 6, 2024, 12:31 p.m. UTC | #2
On 4.07.2024 7:44 PM, Bjorn Andersson wrote:
> On Thu, Jul 04, 2024 at 06:25:15PM GMT, Odelu Kukatla wrote:
>> Add qos_clks_required flag to skip QoS configuration if clocks property
>> is not populated in devicetree for providers which require clocks to be
>> enabled for accessing registers. This is to keep the QoS configuration
>> backwards compatible with devices that have older DTB.
>>
> 
> Please read "Describe your changes" [1], and make your commit message
> start with the problem description - establish to the reader why this
> change is needed, then follow that with a technical description of the
> solution (likely in a separate paragraph).
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> Reported-by: Bjorn Andersson <andersson@kernel.org>
>> Closes: https://lore.kernel.org/all/ciji6nlxn752ina4tmh6kwvek52nxpnguomqek6plwvwgvoqef@yrtexkpmn5br/
>> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
>> ---
>>  drivers/interconnect/qcom/icc-rpmh.c | 2 +-
>>  drivers/interconnect/qcom/icc-rpmh.h | 1 +
>>  drivers/interconnect/qcom/sc7280.c   | 2 ++
>>  3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
>> index 93047defd5e2..f49a8e0cb03c 100644
>> --- a/drivers/interconnect/qcom/icc-rpmh.c
>> +++ b/drivers/interconnect/qcom/icc-rpmh.c
>> @@ -311,7 +311,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>>  		}
>>  
>>  		qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks);
>> -		if (qp->num_clks < 0) {
>> +		if (qp->num_clks < 0 || (!qp->num_clks && desc->qos_clks_required)) {
> 
> For this new case, I think the dev_info() below makes total sense. I.e.
> this looks good to me.
> 
> 
> However, the num_clks < 0 case would represent finding a devicetree node
> with clocks specified, but failing to get these clocks. I believe that
> this would include EPROBE_DEFER.
> 
> I don't think it's correct to print a informational message and continue
> without QoS. I think we should fail here.

Since setting QoS settings is optional, I'd say we should simply skip trying
to do so. Unless setting them on some buses (i.e. ones without failing clocks)
and not on the rest would cause issues. But then, these settings should be
bus-local, so perhaps it would still be fine?

Konrad
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 93047defd5e2..f49a8e0cb03c 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -311,7 +311,7 @@  int qcom_icc_rpmh_probe(struct platform_device *pdev)
 		}
 
 		qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks);
-		if (qp->num_clks < 0) {
+		if (qp->num_clks < 0 || (!qp->num_clks && desc->qos_clks_required)) {
 			dev_info(dev, "Skipping QoS, failed to get clk: %d\n", qp->num_clks);
 			goto skip_qos_config;
 		}
diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h
index 9a5142c70486..14db89850fb3 100644
--- a/drivers/interconnect/qcom/icc-rpmh.h
+++ b/drivers/interconnect/qcom/icc-rpmh.h
@@ -153,6 +153,7 @@  struct qcom_icc_desc {
 	size_t num_nodes;
 	struct qcom_icc_bcm * const *bcms;
 	size_t num_bcms;
+	bool qos_clks_required;
 };
 
 int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
diff --git a/drivers/interconnect/qcom/sc7280.c b/drivers/interconnect/qcom/sc7280.c
index 759c609a20bf..167971f8e8be 100644
--- a/drivers/interconnect/qcom/sc7280.c
+++ b/drivers/interconnect/qcom/sc7280.c
@@ -1691,6 +1691,7 @@  static const struct qcom_icc_desc sc7280_aggre1_noc = {
 	.num_nodes = ARRAY_SIZE(aggre1_noc_nodes),
 	.bcms = aggre1_noc_bcms,
 	.num_bcms = ARRAY_SIZE(aggre1_noc_bcms),
+	.qos_clks_required = true,
 };
 
 static struct qcom_icc_bcm * const aggre2_noc_bcms[] = {
@@ -1722,6 +1723,7 @@  static const struct qcom_icc_desc sc7280_aggre2_noc = {
 	.num_nodes = ARRAY_SIZE(aggre2_noc_nodes),
 	.bcms = aggre2_noc_bcms,
 	.num_bcms = ARRAY_SIZE(aggre2_noc_bcms),
+	.qos_clks_required = true,
 };
 
 static struct qcom_icc_bcm * const clk_virt_bcms[] = {