Quoting Bjorn Andersson (2022-11-01 19:49:27) > On Tue, Nov 01, 2022 at 04:34:21PM -0700, Stephen Boyd wrote: > > We shouldn't be calling runtime PM APIs from within the genpd > > enable/disable path for a couple reasons. > [..][ > > Upon closer inspection, calling runtime PM APIs like this in the GDSC > > driver doesn't make sense. It was intended to make sure the GDSC for the > > clock controller providing other GDSCs was enabled, specifically the > > MMCX GDSC for the display clk controller on SM8250 (sm8250-dispcc), so > > that GDSC register accesses succeeded. That will already happen because > > we make the 'dev->pm_domain' a parent domain of each GDSC we register in > > gdsc_register() via pm_genpd_add_subdomain(). When any of these GDSCs > > are accessed, we'll enable the parent domain (in this specific case > > MMCX). > > > > It's correct that adding the GDSCs as subdomains for the device's > parent-domain will ensure that enabling a GDSC will propagate up and > turn on the (typically) rpmhpd resource. > > But the purpose for the explicit calls was to ensure that the clock > controller itself is accessible. It's been a while since I looked at > this, but iirc letting MMCX to turn off would cause the register access > during dispcc probing to fail - similar to how > clk_pm_runtime_get()/put() ensures the clock registers are accessible. The dispcc and videocc on sm8250 don't use pm_clk APIs. They do use pm_runtime APIs during probe (i.e. pm_runtime_resume_and_get()). That will enable the MMCX domain and keep it on. Then when the GDSCs are registered it will create genpds for each GDSC and make them subdomains of the 'dev->pm_domain' genpd for MMCX. If the GDSCs are enabled at probe time they will increment the count on MMCX to put the count into sync between MMCX and the GDSC provided. The clk framework also has runtime PM calls throughout the code to make sure the device is runtime resumed when it is accessed. Maybe the problem is if probe defers and enough runtime puts are called to runtime suspend the device thus disabling MMCX? Can MMCX really ever be disabled or does disabling it act as a one way disable where you can never enable it again? Or maybe this is the problem where not all constraints are determined yet but we're letting runtime PM put calls from the dispcc device shut down the entire multimedia subsystem while other devices that are within the same domain haven't probed and been able to sync their state but they're actively accessing the bus (i.e. continuous splash screen). I could see this problem being avoided by the pm_runtime_get() call in gdsc registration keeping MMCX on forever because there isn't a matching put anywhere. > > Perhaps I misunderstood something in the process, or lost track of the > actual issues? I dunno. It clearly is a problem to call runtime PM in the noirq phase of system suspend though.
