[PATCH 0/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[PATCH 0/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Johan Hovold]

Christophe noticed that a recent patch adding a missing clock disable to
one of the Qualcomm rpm driver error paths was broken. This would lead
to the provider not being registered on msm8996 which is the only
platform using this code path.

Turns out, however, that the power domain attach during probe is bogus
and would always succeed as any power domain would already have been
attached by the platform bus code.

Georgi, the offending commit is currently in your icc-next (and local
icc-fixes) branch. Perhaps you can fold in the fixup unless you prefer
applying it on top.

The bogus PM domain lookup is redundant and confusing but should
otherwise be benign so the removal could be applied to either branch.

Johan


Johan Hovold (2):
  interconnect: qcom: rpm: fix msm8996 interconnect registration
  interconnect: qcom: rpm: drop bogus pm domain attach

 drivers/interconnect/qcom/icc-rpm.c | 5 -----
 drivers/interconnect/qcom/icc-rpm.h | 1 -
 drivers/interconnect/qcom/msm8996.c | 1 -
 3 files changed, 7 deletions(-)

-- 
2.39.2

[PATCH 1/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Johan Hovold]

A recent commit broke interconnect provider registration for the msm8996
platform by accidentally removing a conditional when adding the missing
clock disable in the power-domain lookup error path.

Fixes: b6edcc7570b2 ("interconnect: qcom: rpm: fix probe PM domain error handling")
Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Link: https://lore.kernel.org/r/641d04a3-9236-fe76-a20f-11466a01460e@wanadoo.fr
Cc: stable@vger.kernel.org      # 5.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 4d0997b210f7..3b055cd893ea 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -498,7 +498,8 @@ int qnoc_probe(struct platform_device *pdev)
 
 	if (desc->has_bus_pd) {
 		ret = dev_pm_domain_attach(dev, true);
-		goto err_disable_clks;
+		if (ret)
+			goto err_disable_clks;
 	}
 
 	provider = &qp->provider;
-- 
2.39.2

[PATCH 2/2] interconnect: qcom: rpm: drop bogus pm domain attach

[Johan Hovold]

Any power domain would already have been attached by the platform bus
code so drop the bogus power domain attach which always succeeds from
probe.

This effectively reverts commit 7de109c0abe9 ("interconnect: icc-rpm:
Add support for bus power domain").

Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
Cc: Yassine Oudjana <y.oudjana@protonmail.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 6 ------
 drivers/interconnect/qcom/icc-rpm.h | 1 -
 drivers/interconnect/qcom/msm8996.c | 1 -
 3 files changed, 8 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 3b055cd893ea..c303ce22a7cd 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -496,12 +496,6 @@ int qnoc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (desc->has_bus_pd) {
-		ret = dev_pm_domain_attach(dev, true);
-		if (ret)
-			goto err_disable_clks;
-	}
-
 	provider = &qp->provider;
 	provider->dev = dev;
 	provider->set = qcom_icc_set;
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index a49af844ab13..02257b0d3d5c 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -91,7 +91,6 @@ struct qcom_icc_desc {
 	size_t num_nodes;
 	const char * const *clocks;
 	size_t num_clocks;
-	bool has_bus_pd;
 	enum qcom_icc_type type;
 	const struct regmap_config *regmap_cfg;
 	unsigned int qos_offset;
diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 25a1a32bc611..14efd2761b7a 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -1823,7 +1823,6 @@ static const struct qcom_icc_desc msm8996_a0noc = {
 	.num_nodes = ARRAY_SIZE(a0noc_nodes),
 	.clocks = bus_a0noc_clocks,
 	.num_clocks = ARRAY_SIZE(bus_a0noc_clocks),
-	.has_bus_pd = true,
 	.regmap_cfg = &msm8996_a0noc_regmap_config
 };
 
-- 
2.39.2

Re: [PATCH 1/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Konrad Dybcio]

On 13.03.2023 09:49, Johan Hovold wrote:
> A recent commit broke interconnect provider registration for the msm8996
> platform by accidentally removing a conditional when adding the missing
> clock disable in the power-domain lookup error path.
> 
> Fixes: b6edcc7570b2 ("interconnect: qcom: rpm: fix probe PM domain error handling")
The hash seems to be different:

https://git.kernel.org/pub/scm/linux/kernel/git/djakov/icc.git/commit/?h=icc-next&id=9038710161f0f028e36ef383fca59080f48420ee

> Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Link: https://lore.kernel.org/r/641d04a3-9236-fe76-a20f-11466a01460e@wanadoo.fr
> Cc: stable@vger.kernel.org      # 5.17
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 4d0997b210f7..3b055cd893ea 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -498,7 +498,8 @@ int qnoc_probe(struct platform_device *pdev)
>  
>  	if (desc->has_bus_pd) {
>  		ret = dev_pm_domain_attach(dev, true);
> -		goto err_disable_clks;
> +		if (ret)
> +			goto err_disable_clks;
*wipes glasses*.. right..

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  	}
>  
>  	provider = &qp->provider;

Re: [PATCH 2/2] interconnect: qcom: rpm: drop bogus pm domain attach

[Konrad Dybcio]

On 13.03.2023 09:49, Johan Hovold wrote:
> Any power domain would already have been attached by the platform bus
> code so drop the bogus power domain attach which always succeeds from
> probe.
> 
> This effectively reverts commit 7de109c0abe9 ("interconnect: icc-rpm:
> Add support for bus power domain").
> 
> Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> Cc: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Running Friday's -next (so, without your cleanups):

cat /sys/kernel/debug/pm_genpd/aggre0_noc/current_state
> on

Removing the pd attach:

cat /sys/kernel/debug/pm_genpd/aggre0_noc/current_state
> on

Also removing the other consumer of this genpd, the simple-pm-bus
hosting PCIe:

cat /sys/kernel/debug/pm_genpd/aggre0_noc/current_state
> on

So it looks like that's the case!

Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # MSM8996 Sony Kagura
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/interconnect/qcom/icc-rpm.c | 6 ------
>  drivers/interconnect/qcom/icc-rpm.h | 1 -
>  drivers/interconnect/qcom/msm8996.c | 1 -
>  3 files changed, 8 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 3b055cd893ea..c303ce22a7cd 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -496,12 +496,6 @@ int qnoc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (desc->has_bus_pd) {
> -		ret = dev_pm_domain_attach(dev, true);
> -		if (ret)
> -			goto err_disable_clks;
> -	}
> -
>  	provider = &qp->provider;
>  	provider->dev = dev;
>  	provider->set = qcom_icc_set;
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index a49af844ab13..02257b0d3d5c 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -91,7 +91,6 @@ struct qcom_icc_desc {
>  	size_t num_nodes;
>  	const char * const *clocks;
>  	size_t num_clocks;
> -	bool has_bus_pd;
>  	enum qcom_icc_type type;
>  	const struct regmap_config *regmap_cfg;
>  	unsigned int qos_offset;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 25a1a32bc611..14efd2761b7a 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -1823,7 +1823,6 @@ static const struct qcom_icc_desc msm8996_a0noc = {
>  	.num_nodes = ARRAY_SIZE(a0noc_nodes),
>  	.clocks = bus_a0noc_clocks,
>  	.num_clocks = ARRAY_SIZE(bus_a0noc_clocks),
> -	.has_bus_pd = true,
>  	.regmap_cfg = &msm8996_a0noc_regmap_config
>  };
>  

Re: [PATCH 1/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Johan Hovold]

On Mon, Mar 13, 2023 at 09:57:46AM +0100, Konrad Dybcio wrote:
> On 13.03.2023 09:49, Johan Hovold wrote:
> > A recent commit broke interconnect provider registration for the msm8996
> > platform by accidentally removing a conditional when adding the missing
> > clock disable in the power-domain lookup error path.
> > 
> > Fixes: b6edcc7570b2 ("interconnect: qcom: rpm: fix probe PM domain error handling")
> The hash seems to be different:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djakov/icc.git/commit/?h=icc-next&id=9038710161f0f028e36ef383fca59080f48420ee

Bah, thanks for catching that.

Georgi, can you change this to:

Fixes: a8f1b7ca53c2 ("interconnect: qcom: rpm: fix probe PM domain error handling")

unless you decide to fold this fixup in?

> > Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Link: https://lore.kernel.org/r/641d04a3-9236-fe76-a20f-11466a01460e@wanadoo.fr
> > Cc: stable@vger.kernel.org      # 5.17
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH 0/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Georgi Djakov]

Hi Johan,

Thank you for all the fixes!

On 13.03.23 10:49, Johan Hovold wrote:
> Christophe noticed that a recent patch adding a missing clock disable to
> one of the Qualcomm rpm driver error paths was broken. This would lead
> to the provider not being registered on msm8996 which is the only
> platform using this code path.

Thanks Christophe for catching this!

> Turns out, however, that the power domain attach during probe is bogus
> and would always succeed as any power domain would already have been
> attached by the platform bus code.

Thanks Konrad for testing and confirming!

> Georgi, the offending commit is currently in your icc-next (and local
> icc-fixes) branch. Perhaps you can fold in the fixup unless you prefer
> applying it on top.
> 
> The bogus PM domain lookup is redundant and confusing but should
> otherwise be benign so the removal could be applied to either branch.

Maybe then I'll just drop this:
	interconnect: qcom: rpm: fix probe PM domain error handling
and apply this one instead:
	interconnect: qcom: rpm: drop bogus pm domain attach

BR,
Georgi

> 
> Johan
> 
> 
> Johan Hovold (2):
>    interconnect: qcom: rpm: fix msm8996 interconnect registration
>    interconnect: qcom: rpm: drop bogus pm domain attach
> 
>   drivers/interconnect/qcom/icc-rpm.c | 5 -----
>   drivers/interconnect/qcom/icc-rpm.h | 1 -
>   drivers/interconnect/qcom/msm8996.c | 1 -
>   3 files changed, 7 deletions(-)
> 

Re: [PATCH 2/2] interconnect: qcom: rpm: drop bogus pm domain attach

[Georgi Djakov]

Hi Johan,

On 13.03.23 10:49, Johan Hovold wrote:
> Any power domain would already have been attached by the platform bus
> code so drop the bogus power domain attach which always succeeds from
> probe.
> 
> This effectively reverts commit 7de109c0abe9 ("interconnect: icc-rpm:
> Add support for bus power domain").
> 
> Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> Cc: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 6 ------
>   drivers/interconnect/qcom/icc-rpm.h | 1 -
>   drivers/interconnect/qcom/msm8996.c | 1 -
>   3 files changed, 8 deletions(-)
>  > diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 3b055cd893ea..c303ce22a7cd 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -496,12 +496,6 @@ int qnoc_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	if (desc->has_bus_pd) {
> -		ret = dev_pm_domain_attach(dev, true);
> -		if (ret)
> -			goto err_disable_clks;
> -	}
> -
>   	provider = &qp->provider;
>   	provider->dev = dev;
>   	provider->set = qcom_icc_set;
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index a49af844ab13..02257b0d3d5c 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -91,7 +91,6 @@ struct qcom_icc_desc {
>   	size_t num_nodes;
>   	const char * const *clocks;
>   	size_t num_clocks;
> -	bool has_bus_pd;
>   	enum qcom_icc_type type;
>   	const struct regmap_config *regmap_cfg;
>   	unsigned int qos_offset;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 25a1a32bc611..14efd2761b7a 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -1823,7 +1823,6 @@ static const struct qcom_icc_desc msm8996_a0noc = {
>   	.num_nodes = ARRAY_SIZE(a0noc_nodes),
>   	.clocks = bus_a0noc_clocks,
>   	.num_clocks = ARRAY_SIZE(bus_a0noc_clocks),
> -	.has_bus_pd = true,
>   	.regmap_cfg = &msm8996_a0noc_regmap_config
>   };
> 
To make it a complete revert, I'll fold this in:

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 0badd2c75161..c80819557923 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -11,7 +11,6 @@
  #include <linux/of_device.h>
  #include <linux/of_platform.h>
  #include <linux/platform_device.h>
-#include <linux/pm_domain.h>
  #include <linux/regmap.h>
  #include <linux/slab.h>

Thanks,
Georgi

Re: [PATCH 0/2] interconnect: qcom: rpm: fix msm8996 interconnect registration

[Johan Hovold]

On Mon, Mar 13, 2023 at 09:57:37PM +0200, Georgi Djakov wrote:

> > Georgi, the offending commit is currently in your icc-next (and local
> > icc-fixes) branch. Perhaps you can fold in the fixup unless you prefer
> > applying it on top.
> > 
> > The bogus PM domain lookup is redundant and confusing but should
> > otherwise be benign so the removal could be applied to either branch.
> 
> Maybe then I'll just drop this:
> 	interconnect: qcom: rpm: fix probe PM domain error handling
> and apply this one instead:
> 	interconnect: qcom: rpm: drop bogus pm domain attach

Sure, that works too. Wasn't sure whether you preferred incremental
fixes on top or not.

I've checked the end result in linux-next against my local branch and
everything looks good.

Thanks for getting that sorted. 

Johan

Re: [PATCH 2/2] interconnect: qcom: rpm: drop bogus pm domain attach

[Johan Hovold]

On Mon, Mar 13, 2023 at 09:58:24PM +0200, Georgi Djakov wrote:
> Hi Johan,
> 
> On 13.03.23 10:49, Johan Hovold wrote:
> > Any power domain would already have been attached by the platform bus
> > code so drop the bogus power domain attach which always succeeds from
> > probe.
> > 
> > This effectively reverts commit 7de109c0abe9 ("interconnect: icc-rpm:
> > Add support for bus power domain").
> > 
> > Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> > Cc: Yassine Oudjana <y.oudjana@protonmail.com>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

> To make it a complete revert, I'll fold this in:
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 0badd2c75161..c80819557923 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -11,7 +11,6 @@
>   #include <linux/of_device.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
> -#include <linux/pm_domain.h>
>   #include <linux/regmap.h>
>   #include <linux/slab.h>

Thanks for catching that!

Johan