Hi Tomi,
On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> > Also, I hope you and the other DSS hackers can finish the PM runtime
> > conversion of the DSS driver soon. Ideally before any new DSS
> > features are added. We really need to be able to separate the OMAP
> > integration details from the drivers, and right now, hwmod and PM
> > runtime are the best way we have to do that.
>
> I also started to look at the PM runtime, but it doesn't fix the issue
> with the inconsistent clock name I described above.
After the hwmod/PM runtime conversion, I don't think any of the clock
aliases currently in clock*_data.c should be used by the DSS driver (or by
anything else on the system, for that matter). That's because the
omap_device code should set up the "main" alias for the DSS main
functional clock[*], as well as the aliases in the optional clock data in
the OMAP hwmod data files:
static struct omap_hwmod_opt_clk dss_opt_clks[] = {
{ .role = "sys_clk", .clk = "dss_sys_clk" },
{ .role = "tv_clk", .clk = "dss_tv_clk" },
{ .role = "dss_clk", .clk = "dss_dss_clk" },
{ .role = "video_clk", .clk = "dss_48mhz_clk" },
};
It might be that some of these role names aren't quite accurate and need
to be changed. Those are intended to be meaningful to the driver, so
comments there are definitely welcome.
[*]. The "main" alias should be defined by the omap_device code
automatically, similarly to how _add_optional_clock_clkdev() does it. It
does not do so currently. This needs to be fixed in the omap_device.c
code.
> I also have some questions regarding PM runtime, perhaps I'll just put
> them here as they are slightly related:
>
> - Should every DSS module handle their clocks independently, i.e. should
> VENC get its own clocks and DSI should get its own? If so, we need a
> bunch of new clock aliases so the devices can get their clocks.
If all that driver code needs to do is to enable its main functional clock
when it is active and disable that clock when the driver is inactive,
then, no, the drivers shouldn't need their own clock aliases. Same if the
driver only needs to get the rate or set the rate on that main functional
clock, since that alias should be set up automatically.
But if the driver for that submodule needs to control PRCM-provided
optional clocks, then it will need to have struct omap_hwmod_opt_clk
entries defined in the hwmod data.
> - Should every DSS module have their own PM runtime handling? (actually
> related to the question above)
Yes, I think so. From the integration perspective, we are trying to get
to the point where each omap_device maps to only one hwmod.
> - If the modules are handled separately, how should the dependencies be
> handled? For example, dss_core's reset will reset all the other modules
> also, and most of the submodules need functions from dss_core and
> dss_dispc. So should, say, dss_dsi then call functions in core and dispc
> to "get" them, i.e. increase their pm runtime use count?
Probably not.
Here is how I would suggest structuring the code. This is only a naÃve
suggestion; you and your team almost certainly know better than I.
I'd suggest that you separate low-level register access into .c files
that are targeted specifically for the IP block. So in the DSS case,
you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c. Each one should
be a separate platform_device and would export symbols. Hopefully, there
should be no cross-dependencies between these low-level files.
Then you'd have "higher-level" code that might need to read/write from
multiple DSS submodules to complete some higher-level operation. That
would be at least one separate driver -- say, "dss2" or something -- with
dependencies on the low-level drivers. So, for example, when that
higher-level driver is modprobe'd, Linux would also load the drivers for
the underlying hardware blocks that it uses.
> - There seems to be some child/parent relationships in PM runtime.
> Should dss_core be the parent for the rest of the DSS modules? This
> would at least solve the reset issue easily, I guess.
Yes, I think that's more accurate, anyway. Something isn't right with the
DSS hwmod data. According to the OMAP4 Public TRM Rev O Table 10-3 "DSS
Integration," there's a Sonics LX bus lurking in there. All of the DSS
submodules should have slave sockets with that Sonics LX bus as their
master. And the hwmod associated with the SLX should have an address
range that covers all of the DSS submodules. I assume that the logical
hwmod to associate the SLX with is "dss_core", as you write.
Also, I notice the "CAUTION" boxes in Section 10.1.3 "DSS Register
Manual", 10.2.7 "Display Controller Register Manual", etc. etc., that say
that the DSS and submodules should be accessed through the L3 address
space. But all of the DSS hwmod register targets are listed as the L4_PER
variants. So the hwmod data also doesn't appear to be correct there. The
correct approach would be to have both address spaces listed in struct
omap_hwmod_addr_space arrays, but to mark only the struct
omap_hwmod_ocp_if entry associated with the L3 bus as OCP_USER_MPU |
OCP_USER_SDMA[*].
[*]. On the OMAP core code, looks like we'll also want to modify
omap_hwmod_build_resources() to mark resources with IORESOURCE_DISABLED if
they are not marked with any OCP_USER_* flags, and patch the functions in
drivers/base/platform.c to skip any resources with IORESOURCE_DISABLED
set.
> - How does saving/restoring the registers for OFF mode go with PM
> runtime? Should the registers be saved in runtime_suspend(), and
> restored in runtime_resume()?
If you don't use shadow registers, then I think so, yes, although Kevin
might know better than I.
> Can/should omap_pm_get_dev_context_loss_count() still be used to
> optimize the restoring?
Yes.
...
I regret that this process is relatively complicated :-( As you and/or
your team works on this, changes to the hwmod/omap_device/clock core will
almost certainly be needed. Please don't hesitate to let us know what's
not working for you, or to try out patches to the core infrastructure to
fix what you need. If I don't respond, please just keep pinging. I
wasn't able to fully analyze the DSS module when we originally wrote the
hwmod code, so surely we'll need to fix some bugs or make some changes for
things to make sense.
best regards,
- Paul
