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@xxxxxxxxxxx>
> [..]
> > > 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@xxxxxxxxxxx
>
> 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@xxxxxxxxxxx/
> > >
> > > 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.