| Message ID | 20250414-apr_14_for_sending-v2-4-70c5af2af96c@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] PM: device: Introduce platform_resources_managed flag | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote: > Update the Imagination PVR driver to skip clock management during > initialization if the platform PM has indicated that it manages platform > resources. > > This is necessary for platforms like the T-HEAD TH1520, where the GPU's > clocks and resets are managed via a PM domain, and should not be > manipulated directly by the GPU driver. > > Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> > --- > drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c > index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644 > --- a/drivers/gpu/drm/imagination/pvr_device.c > +++ b/drivers/gpu/drm/imagination/pvr_device.c > @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev) > if (err) > return err; > > - /* Enable and initialize clocks required for the device to operate. */ > - err = pvr_device_clk_init(pvr_dev); > - if (err) > - return err; > + /* > + * Only initialize clocks if they are not managed by the platform's > + * PM domain. > + */ > + if (!device_platform_resources_pm_managed(dev)) { > + /* Enable and initialize clocks required for the device to operate. */ > + err = pvr_device_clk_init(pvr_dev); > + if (err) > + return err; > + } So, how does that work for devfreq? I can understand the rationale for resets and the sys clock, but the core clock at least should really be handled by the driver. Maxime
On 15/04/2025 09:55, Maxime Ripard wrote: > On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote: >> Update the Imagination PVR driver to skip clock management during >> initialization if the platform PM has indicated that it manages platform >> resources. >> >> This is necessary for platforms like the T-HEAD TH1520, where the GPU's >> clocks and resets are managed via a PM domain, and should not be >> manipulated directly by the GPU driver. >> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> >> --- >> drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c >> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644 >> --- a/drivers/gpu/drm/imagination/pvr_device.c >> +++ b/drivers/gpu/drm/imagination/pvr_device.c >> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev) >> if (err) >> return err; >> >> - /* Enable and initialize clocks required for the device to operate. */ >> - err = pvr_device_clk_init(pvr_dev); >> - if (err) >> - return err; >> + /* >> + * Only initialize clocks if they are not managed by the platform's >> + * PM domain. >> + */ >> + if (!device_platform_resources_pm_managed(dev)) { >> + /* Enable and initialize clocks required for the device to operate. */ >> + err = pvr_device_clk_init(pvr_dev); >> + if (err) >> + return err; >> + } > > So, how does that work for devfreq? I can understand the rationale for > resets and the sys clock, but the core clock at least should really be > handled by the driver. I agree, this feels a bit "all or nothing" to me. There's only one clock on this platform that has issues, we can still control the other two just fine. I thought fixed clocks were the standard mechanism for exposing non-controllable clocks to device drivers? Cheers, Matt > > Maxime
On 4/15/25 11:15, Matt Coster wrote: > On 15/04/2025 09:55, Maxime Ripard wrote: >> On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote: >>> Update the Imagination PVR driver to skip clock management during >>> initialization if the platform PM has indicated that it manages platform >>> resources. >>> >>> This is necessary for platforms like the T-HEAD TH1520, where the GPU's >>> clocks and resets are managed via a PM domain, and should not be >>> manipulated directly by the GPU driver. >>> >>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> >>> --- >>> drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c >>> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644 >>> --- a/drivers/gpu/drm/imagination/pvr_device.c >>> +++ b/drivers/gpu/drm/imagination/pvr_device.c >>> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev) >>> if (err) >>> return err; >>> >>> - /* Enable and initialize clocks required for the device to operate. */ >>> - err = pvr_device_clk_init(pvr_dev); >>> - if (err) >>> - return err; >>> + /* >>> + * Only initialize clocks if they are not managed by the platform's >>> + * PM domain. >>> + */ >>> + if (!device_platform_resources_pm_managed(dev)) { >>> + /* Enable and initialize clocks required for the device to operate. */ >>> + err = pvr_device_clk_init(pvr_dev); >>> + if (err) >>> + return err; >>> + } >> >> So, how does that work for devfreq? I can understand the rationale for >> resets and the sys clock, but the core clock at least should really be >> handled by the driver. Hi Maxime, Matt, Thanks for the feedback. This commit is trying to prevent the pvr RUNTIME_PM_OPS from controlling the clocks or resets, as there is a custom start/stop sequence needed for the TH1520 SoC coded in patch 3 of this series. static const struct dev_pm_ops pvr_pm_ops = { RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, pvr_power_device_idle) }; So, if the core clock needs to be used for other purposes like devfreq, we could move the device_platform_resources_pm_managed() check to the pvr_power_* functions instead. This would prevent the clocks and resets from being managed in runtime PM in the consumer driver, while still allowing the GPU driver to access and control clocks like the core clock as needed for other purposes. That way, clocks could be safely shared between the PM domain driver and the device driver, with generic PM driver controlling the start/stop sequence for reset and clocks. We would probably need to find a better name for the flag then, to more clearly reflect that it's about delegating clock/reset PM runtime control, rather than full resource ownership. > > I agree, this feels a bit "all or nothing" to me. There's only one clock > on this platform that has issues, we can still control the other two > just fine. > > I thought fixed clocks were the standard mechanism for exposing > non-controllable clocks to device drivers? That's correct — and it's not really about the MEM clock at this point. The main goal is to ensure the custom power-up sequence for the TH1520 SoC is followed. That sequence is implemented in th1520_gpu_domain_start() in patch 3 of this series. Regards, Michał > > Cheers, > Matt > >> >> Maxime > >
diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644 --- a/drivers/gpu/drm/imagination/pvr_device.c +++ b/drivers/gpu/drm/imagination/pvr_device.c @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev) if (err) return err; - /* Enable and initialize clocks required for the device to operate. */ - err = pvr_device_clk_init(pvr_dev); - if (err) - return err; + /* + * Only initialize clocks if they are not managed by the platform's + * PM domain. + */ + if (!device_platform_resources_pm_managed(dev)) { + /* Enable and initialize clocks required for the device to operate. */ + err = pvr_device_clk_init(pvr_dev); + if (err) + return err; + } /* Explicitly power the GPU so we can access control registers before the FW is booted. */ err = pm_runtime_resume_and_get(dev);
Update the Imagination PVR driver to skip clock management during initialization if the platform PM has indicated that it manages platform resources. This is necessary for platforms like the T-HEAD TH1520, where the GPU's clocks and resets are managed via a PM domain, and should not be manipulated directly by the GPU driver. Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> --- drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)