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

[Zhenhua Huang <quic_zhenhuah@quicinc.com>]

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.
> 
>