[PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Zhenhua Huang]

TBU driver has no runtime pm support now, adding pm_runtime_enable()
seems to be useless. Remove it.

Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 36c6b36ad4ff..aff2fe1fda13 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
 
 static int qcom_smmu_tbu_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	int ret;
 
 	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
@@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	if (dev->pm_domain) {
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
-
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Zhenhua Huang]

Gentle reminder for review :)

On 2024/7/30 18:30, Zhenhua Huang wrote:
> TBU driver has no runtime pm support now, adding pm_runtime_enable()
> seems to be useless. Remove it.
> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 36c6b36ad4ff..aff2fe1fda13 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
>   
>   static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>   {
> -	struct device *dev = &pdev->dev;
>   	int ret;
>   
>   	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>   			return ret;
>   	}
>   
> -	if (dev->pm_domain) {
> -		pm_runtime_set_active(dev);
> -		pm_runtime_enable(dev);
> -	}
> -
>   	return 0;
>   }
>   

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Pranjal Shrivastava]

On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
> TBU driver has no runtime pm support now, adding pm_runtime_enable()
> seems to be useless. Remove it.
> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 36c6b36ad4ff..aff2fe1fda13 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
>  
>  static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &pdev->dev;
>  	int ret;
>  
>  	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> -	if (dev->pm_domain) {
> -		pm_runtime_set_active(dev);
> -		pm_runtime_enable(dev);

I assumed that this was required to avoid the TBU from being powered
down? If so, then I think we shall move it under the
previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG?

If not, we can remove it give that the TBU would be powered ON as needed

> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 
> 

Thanks,
Pranjal

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Zhenhua Huang]

On 2024/8/12 21:25, Pranjal Shrivastava wrote:
> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
>> TBU driver has no runtime pm support now, adding pm_runtime_enable()
>> seems to be useless. Remove it.
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> index 36c6b36ad4ff..aff2fe1fda13 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
>>   
>>   static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>>   {
>> -	struct device *dev = &pdev->dev;
>>   	int ret;
>>   
>>   	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
>> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>>   			return ret;
>>   	}
>>   
>> -	if (dev->pm_domain) {
>> -		pm_runtime_set_active(dev);
>> -		pm_runtime_enable(dev);
> 
> I assumed that this was required to avoid the TBU from being powered
> down? If so, then I think we shall move it under the

Hi Pranjal,

In my sense, this was giving the TBU ability to power down when 
necessary(through pm callbacks)? While I haven't seen any RPM impl for 
TBU device.. hence having the doubt..

Thanks,
Zhenhua

> previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG?
> 
> If not, we can remove it give that the TBU would be powered ON as needed
> 
>> -	}
>> -
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.7.4
>>
>>
> 
> Thanks,
> Pranjal

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Pranjal Shrivastava]

On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote:
> 
> 
> On 2024/8/12 21:25, Pranjal Shrivastava wrote:
> > On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
> > > TBU driver has no runtime pm support now, adding pm_runtime_enable()
> > > seems to be useless. Remove it.
> > > 
> > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
> > >   1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > index 36c6b36ad4ff..aff2fe1fda13 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
> > >   static int qcom_smmu_tbu_probe(struct platform_device *pdev)
> > >   {
> > > -	struct device *dev = &pdev->dev;
> > >   	int ret;
> > >   	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
> > > @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
> > >   			return ret;
> > >   	}
> > > -	if (dev->pm_domain) {
> > > -		pm_runtime_set_active(dev);
> > > -		pm_runtime_enable(dev);
> > 
> > I assumed that this was required to avoid the TBU from being powered
> > down? If so, then I think we shall move it under the
> 
> Hi Pranjal,
> 
> In my sense, this was giving the TBU ability to power down when
> necessary(through pm callbacks)? While I haven't seen any RPM impl for TBU
> device.. hence having the doubt..
> 
> Thanks,
> Zhenhua

Apologies for being unclear. I just meant to ask if there was a reason
to add pm_runtime_set_active & enable in the tbu probe previously? And I
*assumed* that it was to set the device state as RPM_ACTIVE to avoid it
being RPM_SUSPENDED after enabling pm_runtime. 

I agree that there are no pm_runtime_suspend/resume calls within the TBU
driver. I'm just trying to understand why was pm_runtime enabled here
earlier (since it's not implemented) in order to ensure that removing it
doesn't cause further troubles?

I see Georgi added it as a part of
https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/

But I'm unsure why was it required to fix that bug?

> 
> > previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG?
> > 
> > If not, we can remove it give that the TBU would be powered ON as needed
> > 
> > > -	}
> > > -
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.7.4
> > > 
> > > 
> > 
> > Thanks,
> > Pranjal

Thanks,
Pranjal

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Zhenhua Huang]

On 2024/8/13 15:20, Pranjal Shrivastava wrote:
> On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote:
>>
>>
>> On 2024/8/12 21:25, Pranjal Shrivastava wrote:
>>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
>>>> TBU driver has no runtime pm support now, adding pm_runtime_enable()
>>>> seems to be useless. Remove it.
>>>>
>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 6 ------
>>>>    1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 36c6b36ad4ff..aff2fe1fda13 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -566,7 +566,6 @@ static struct acpi_platform_list qcom_acpi_platlist[] = {
>>>>    static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>>>>    {
>>>> -	struct device *dev = &pdev->dev;
>>>>    	int ret;
>>>>    	if (IS_ENABLED(CONFIG_ARM_SMMU_QCOM_DEBUG)) {
>>>> @@ -575,11 +574,6 @@ static int qcom_smmu_tbu_probe(struct platform_device *pdev)
>>>>    			return ret;
>>>>    	}
>>>> -	if (dev->pm_domain) {
>>>> -		pm_runtime_set_active(dev);
>>>> -		pm_runtime_enable(dev);
>>>
>>> I assumed that this was required to avoid the TBU from being powered
>>> down? If so, then I think we shall move it under the
>>
>> Hi Pranjal,
>>
>> In my sense, this was giving the TBU ability to power down when
>> necessary(through pm callbacks)? While I haven't seen any RPM impl for TBU
>> device.. hence having the doubt..
>>
>> Thanks,
>> Zhenhua
> 
> Apologies for being unclear. I just meant to ask if there was a reason
> to add pm_runtime_set_active & enable in the tbu probe previously? And I

It's also my doubt and hope Georgi can help clarify :)
Actually I assume this part of codes was copied from arm-smmu driver
https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/iommu/arm/arm-smmu/arm-smmu.c#L2264 
.. but for TBU, seems no user ?

> *assumed* that it was to set the device state as RPM_ACTIVE to avoid it
> being RPM_SUSPENDED after enabling pm_runtime

Yeah, it's normal sequence from doc: 
https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/power/runtime_pm.rst#L574

> 
> I agree that there are no pm_runtime_suspend/resume calls within the TBU
> driver. I'm just trying to understand why was pm_runtime enabled here
> earlier (since it's not implemented) in order to ensure that removing it
> doesn't cause further troubles?

See above my assumption, need Georgi to comment but.

> 
> I see Georgi added it as a part of
> https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/
> 
> But I'm unsure why was it required to fix that bug?

I'm just thinking it is dead code and want to see if my understanding is 
correct.

Thanks,
Zhenhua

> 
>>
>>> previous if condition, i.e. CONFIG_ARM_SMMU_QCOM_DEBUG?
>>>
>>> If not, we can remove it give that the TBU would be powered ON as needed
>>>
>>>> -	}
>>>> -
>>>>    	return 0;
>>>>    }
>>>> -- 
>>>> 2.7.4
>>>>
>>>>
>>>
>>> Thanks,
>>> Pranjal
> 
> Thanks,
> Pranjal

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Georgi Djakov]

Hi Zhenhua,

On 8/13/2024 10:56 AM, Zhenhua Huang wrote:
> 
> On 2024/8/13 15:20, Pranjal Shrivastava wrote:
>> On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote:
>>>
>>> On 2024/8/12 21:25, Pranjal Shrivastava wrote:
>>>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
>>>>> TBU driver has no runtime pm support now, adding pm_runtime_enable()
>>>>> seems to be useless. Remove it.
>>>>>
>>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
[..]
>> I agree that there are no pm_runtime_suspend/resume calls within the TBU
>> driver. I'm just trying to understand why was pm_runtime enabled here
>> earlier (since it's not implemented) in order to ensure that removing it
>> doesn't cause further troubles?
> 
> See above my assumption, need Georgi to comment but.

Thank you for looking at the code! Your assumptions are mostly correct,
but if you try this patch on a real sdm845 device you will notice some
issues. So it's actually needed to re-configure the power-domains, three
of which (MMNOC GDSCs) are requiring this because of a HW bug. I should
have put a comment in the code to avoid confusion, but it took me some
time to confirm it.

I have sent a patch to handle this more cleanly:
https://lore.kernel.org/lkml/20240813120015.3242787-1-quic_c_gdjako@quicinc.com

So we should not remove the runtime pm calls until some version of the
above patch gets merged.

Thanks,
Georgi

>> I see Georgi added it as a part of
>> https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/
>>
>> But I'm unsure why was it required to fix that bug?
> 
> I'm just thinking it is dead code and want to see if my understanding is correct.

Re: [PATCH 1/1] iommu/arm-smmu-qcom: remove runtime pm enabling for TBU driver

[Zhenhua Huang]

Hi Georgi,

On 2024/8/13 20:06, Georgi Djakov wrote:
> Hi Zhenhua,
> 
> On 8/13/2024 10:56 AM, Zhenhua Huang wrote:
>>
>> On 2024/8/13 15:20, Pranjal Shrivastava wrote:
>>> On Tue, Aug 13, 2024 at 10:37:33AM +0800, Zhenhua Huang wrote:
>>>>
>>>> On 2024/8/12 21:25, Pranjal Shrivastava wrote:
>>>>> On Tue, Jul 30, 2024 at 06:30:43PM +0800, Zhenhua Huang wrote:
>>>>>> TBU driver has no runtime pm support now, adding pm_runtime_enable()
>>>>>> seems to be useless. Remove it.
>>>>>>
>>>>>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> [..]
>>> I agree that there are no pm_runtime_suspend/resume calls within the TBU
>>> driver. I'm just trying to understand why was pm_runtime enabled here
>>> earlier (since it's not implemented) in order to ensure that removing it
>>> doesn't cause further troubles?
>>
>> See above my assumption, need Georgi to comment but.
> 
> Thank you for looking at the code! Your assumptions are mostly correct,
> but if you try this patch on a real sdm845 device you will notice some
> issues. So it's actually needed to re-configure the power-domains, three

Thanks Georgi for your comments!
Hmm...  so you found some bugs on sdm845 ? sorry that I don't have 
sdm845 on hand...

> of which (MMNOC GDSCs) are requiring this because of a HW bug. I should
> have put a comment in the code to avoid confusion, but it took me some
> time to confirm it.
> 
> I have sent a patch to handle this more cleanly:
> https://lore.kernel.org/lkml/20240813120015.3242787-1-quic_c_gdjako@quicinc.com
> 
> So we should not remove the runtime pm calls until some version of the
> above patch gets merged.

In my sense, above patch should not result in turning off gdsc? It's 
just open the support for RPM.. I tried to do same change for arm-smmu 
driver, w/ test I see cx_gdsc which is the power-domain for gfx_smmu, is on:
..
/sys/kernel/debug/pm_genpd/cx_gdsc # cat current_state
on

Are you worrying that not setting active will turn off related PD? or 
Could you please explain a bit more about how the change impacted power 
domain status? Thanks in advance :)

> 
> Thanks,
> Georgi
> 
>>> I see Georgi added it as a part of
>>> https://lore.kernel.org/all/20240704010759.507798-1-quic_c_gdjako@quicinc.com/
>>>
>>> But I'm unsure why was it required to fix that bug?
>>
>> I'm just thinking it is dead code and want to see if my understanding is correct.
> 
>