| Message ID | 20220427145616.523557-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | interconnect: qcom: use icc_sync_state | expand |
On 27.04.22 17:56, Krzysztof Kozlowski wrote: > Use icc_sync_state for interconnect providers, so that the bandwidth > request doesn't need to stay on maximum value. Did you test this? In general, we should not enable this on boards that do not have full interconnect scaling support in consumer drivers yet. Some of the interconnects could be enabled by default by the bootloader and usually later during boot the consumer drivers request the bandwidth that they need. But if the requests are missing, the interconnects without bandwidth users will be disabled when we reach sync state. So this may (or not) cause issues... Thanks, Georgi > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/interconnect/qcom/msm8916.c | 1 + > drivers/interconnect/qcom/qcm2290.c | 1 + > drivers/interconnect/qcom/qcs404.c | 1 + > drivers/interconnect/qcom/sdm660.c | 1 + > drivers/interconnect/qcom/sm8150.c | 1 + > drivers/interconnect/qcom/sm8250.c | 1 + > drivers/interconnect/qcom/sm8350.c | 1 + > drivers/interconnect/qcom/sm8450.c | 1 + > 8 files changed, 8 insertions(+) > > diff --git a/drivers/interconnect/qcom/msm8916.c b/drivers/interconnect/qcom/msm8916.c > index 2f397a7c3322..811370fcd211 100644 > --- a/drivers/interconnect/qcom/msm8916.c > +++ b/drivers/interconnect/qcom/msm8916.c > @@ -1347,6 +1347,7 @@ static struct platform_driver msm8916_noc_driver = { > .driver = { > .name = "qnoc-msm8916", > .of_match_table = msm8916_noc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(msm8916_noc_driver); > diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c > index 74404e0b2080..6cf75da91428 100644 > --- a/drivers/interconnect/qcom/qcm2290.c > +++ b/drivers/interconnect/qcom/qcm2290.c > @@ -1355,6 +1355,7 @@ static struct platform_driver qcm2290_noc_driver = { > .driver = { > .name = "qnoc-qcm2290", > .of_match_table = qcm2290_noc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(qcm2290_noc_driver); > diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c > index 416c8bff8efa..d82f9add4933 100644 > --- a/drivers/interconnect/qcom/qcs404.c > +++ b/drivers/interconnect/qcom/qcs404.c > @@ -1086,6 +1086,7 @@ static struct platform_driver qcs404_noc_driver = { > .driver = { > .name = "qnoc-qcs404", > .of_match_table = qcs404_noc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(qcs404_noc_driver); > diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c > index 274a7139fe1a..706b49a4bb70 100644 > --- a/drivers/interconnect/qcom/sdm660.c > +++ b/drivers/interconnect/qcom/sdm660.c > @@ -1716,6 +1716,7 @@ static struct platform_driver sdm660_noc_driver = { > .driver = { > .name = "qnoc-sdm660", > .of_match_table = sdm660_noc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(sdm660_noc_driver); > diff --git a/drivers/interconnect/qcom/sm8150.c b/drivers/interconnect/qcom/sm8150.c > index 745e3c36a61a..2a85f53802b5 100644 > --- a/drivers/interconnect/qcom/sm8150.c > +++ b/drivers/interconnect/qcom/sm8150.c > @@ -535,6 +535,7 @@ static struct platform_driver qnoc_driver = { > .driver = { > .name = "qnoc-sm8150", > .of_match_table = qnoc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(qnoc_driver); > diff --git a/drivers/interconnect/qcom/sm8250.c b/drivers/interconnect/qcom/sm8250.c > index aa707582ea01..8dfb5dea562a 100644 > --- a/drivers/interconnect/qcom/sm8250.c > +++ b/drivers/interconnect/qcom/sm8250.c > @@ -551,6 +551,7 @@ static struct platform_driver qnoc_driver = { > .driver = { > .name = "qnoc-sm8250", > .of_match_table = qnoc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(qnoc_driver); > diff --git a/drivers/interconnect/qcom/sm8350.c b/drivers/interconnect/qcom/sm8350.c > index c79f93a1ac73..3e26a2175b28 100644 > --- a/drivers/interconnect/qcom/sm8350.c > +++ b/drivers/interconnect/qcom/sm8350.c > @@ -531,6 +531,7 @@ static struct platform_driver qnoc_driver = { > .driver = { > .name = "qnoc-sm8350", > .of_match_table = qnoc_of_match, > + .sync_state = icc_sync_state, > }, > }; > module_platform_driver(qnoc_driver); > diff --git a/drivers/interconnect/qcom/sm8450.c b/drivers/interconnect/qcom/sm8450.c > index 8d99ee6421df..d573018a6324 100644 > --- a/drivers/interconnect/qcom/sm8450.c > +++ b/drivers/interconnect/qcom/sm8450.c > @@ -1968,6 +1968,7 @@ static struct platform_driver qnoc_driver = { > .driver = { > .name = "qnoc-sm8450", > .of_match_table = qnoc_of_match, > + .sync_state = icc_sync_state, > }, > }; >
On 27/04/2022 17:34, Georgi Djakov wrote: > On 27.04.22 17:56, Krzysztof Kozlowski wrote: >> Use icc_sync_state for interconnect providers, so that the bandwidth >> request doesn't need to stay on maximum value. > > Did you test this? In general, we should not enable this on boards that > do not have full interconnect scaling support in consumer drivers yet. > Some of the interconnects could be enabled by default by the bootloader > and usually later during boot the consumer drivers request the bandwidth > that they need. But if the requests are missing, the interconnects > without bandwidth users will be disabled when we reach sync state. So > this may (or not) cause issues... I understand, thanks for bringing this up. It does not look like an issue of interconnect provider but instead consumers and DTS. It's not the job of provider driver to know all possible uses and DTS files. The driver should expose itself and if platform is not ready, should not use it by not enabling the interconnect. It's a job for DTS, not for the interconnect provider. Imagine some out of tree DTS which cannot use interconnects because we assume that all users of that provider are missing bandwidth requests. No, instead provider should allow anyone to use it. I understand my change might cause unexpected issues, but it is still technically correct, just maybe should be followed with disabling in DTS the providers without proper consumers? Best regards, Krzysztof
On 28.04.22 13:23, Krzysztof Kozlowski wrote: > On 27/04/2022 17:34, Georgi Djakov wrote: >> On 27.04.22 17:56, Krzysztof Kozlowski wrote: >>> Use icc_sync_state for interconnect providers, so that the bandwidth >>> request doesn't need to stay on maximum value. >> >> Did you test this? In general, we should not enable this on boards that >> do not have full interconnect scaling support in consumer drivers yet. >> Some of the interconnects could be enabled by default by the bootloader >> and usually later during boot the consumer drivers request the bandwidth >> that they need. But if the requests are missing, the interconnects >> without bandwidth users will be disabled when we reach sync state. So >> this may (or not) cause issues... > > I understand, thanks for bringing this up. It does not look like an > issue of interconnect provider but instead consumers and DTS. It's not > the job of provider driver to know all possible uses and DTS files. The > driver should expose itself and if platform is not ready, should not use > it by not enabling the interconnect. It's a job for DTS, not for the > interconnect provider. Agree, but we still need to make sure this is tested and does not introduce any regressions at least with the DT that is upstream. > Imagine some out of tree DTS which cannot use interconnects because we > assume that all users of that provider are missing bandwidth requests. > No, instead provider should allow anyone to use it. I have an idea to introduce a kernel parameter like clk_ignore_unused, but for interconnects. > I understand my change might cause unexpected issues, but it is still > technically correct, just maybe should be followed with disabling in DTS > the providers without proper consumers? Not sure that enabling/disabling stuff in DT is a good option. DT should be just a description of the hardware and it might not be updated as often as the kernel. Thanks, Georgi
diff --git a/drivers/interconnect/qcom/msm8916.c b/drivers/interconnect/qcom/msm8916.c index 2f397a7c3322..811370fcd211 100644 --- a/drivers/interconnect/qcom/msm8916.c +++ b/drivers/interconnect/qcom/msm8916.c @@ -1347,6 +1347,7 @@ static struct platform_driver msm8916_noc_driver = { .driver = { .name = "qnoc-msm8916", .of_match_table = msm8916_noc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(msm8916_noc_driver); diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c index 74404e0b2080..6cf75da91428 100644 --- a/drivers/interconnect/qcom/qcm2290.c +++ b/drivers/interconnect/qcom/qcm2290.c @@ -1355,6 +1355,7 @@ static struct platform_driver qcm2290_noc_driver = { .driver = { .name = "qnoc-qcm2290", .of_match_table = qcm2290_noc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(qcm2290_noc_driver); diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c index 416c8bff8efa..d82f9add4933 100644 --- a/drivers/interconnect/qcom/qcs404.c +++ b/drivers/interconnect/qcom/qcs404.c @@ -1086,6 +1086,7 @@ static struct platform_driver qcs404_noc_driver = { .driver = { .name = "qnoc-qcs404", .of_match_table = qcs404_noc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(qcs404_noc_driver); diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c index 274a7139fe1a..706b49a4bb70 100644 --- a/drivers/interconnect/qcom/sdm660.c +++ b/drivers/interconnect/qcom/sdm660.c @@ -1716,6 +1716,7 @@ static struct platform_driver sdm660_noc_driver = { .driver = { .name = "qnoc-sdm660", .of_match_table = sdm660_noc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(sdm660_noc_driver); diff --git a/drivers/interconnect/qcom/sm8150.c b/drivers/interconnect/qcom/sm8150.c index 745e3c36a61a..2a85f53802b5 100644 --- a/drivers/interconnect/qcom/sm8150.c +++ b/drivers/interconnect/qcom/sm8150.c @@ -535,6 +535,7 @@ static struct platform_driver qnoc_driver = { .driver = { .name = "qnoc-sm8150", .of_match_table = qnoc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(qnoc_driver); diff --git a/drivers/interconnect/qcom/sm8250.c b/drivers/interconnect/qcom/sm8250.c index aa707582ea01..8dfb5dea562a 100644 --- a/drivers/interconnect/qcom/sm8250.c +++ b/drivers/interconnect/qcom/sm8250.c @@ -551,6 +551,7 @@ static struct platform_driver qnoc_driver = { .driver = { .name = "qnoc-sm8250", .of_match_table = qnoc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(qnoc_driver); diff --git a/drivers/interconnect/qcom/sm8350.c b/drivers/interconnect/qcom/sm8350.c index c79f93a1ac73..3e26a2175b28 100644 --- a/drivers/interconnect/qcom/sm8350.c +++ b/drivers/interconnect/qcom/sm8350.c @@ -531,6 +531,7 @@ static struct platform_driver qnoc_driver = { .driver = { .name = "qnoc-sm8350", .of_match_table = qnoc_of_match, + .sync_state = icc_sync_state, }, }; module_platform_driver(qnoc_driver); diff --git a/drivers/interconnect/qcom/sm8450.c b/drivers/interconnect/qcom/sm8450.c index 8d99ee6421df..d573018a6324 100644 --- a/drivers/interconnect/qcom/sm8450.c +++ b/drivers/interconnect/qcom/sm8450.c @@ -1968,6 +1968,7 @@ static struct platform_driver qnoc_driver = { .driver = { .name = "qnoc-sm8450", .of_match_table = qnoc_of_match, + .sync_state = icc_sync_state, }, };
Use icc_sync_state for interconnect providers, so that the bandwidth request doesn't need to stay on maximum value. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/interconnect/qcom/msm8916.c | 1 + drivers/interconnect/qcom/qcm2290.c | 1 + drivers/interconnect/qcom/qcs404.c | 1 + drivers/interconnect/qcom/sdm660.c | 1 + drivers/interconnect/qcom/sm8150.c | 1 + drivers/interconnect/qcom/sm8250.c | 1 + drivers/interconnect/qcom/sm8350.c | 1 + drivers/interconnect/qcom/sm8450.c | 1 + 8 files changed, 8 insertions(+)