On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
> 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
When should "main" clock be used by the driver? Or never, and pm_runtime
handles that?
> 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.
How about OMAP3? It also has an optional functional clock, but that
doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
Should it be there?
It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
hwmods. Does that mean it can never be turned off if DSS is in use?
> [*]. 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.
Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
believe.
> > - 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.
But this still leaves my question open: If this "dss2" needs to use,
say, dispc.c and dsi.c, those low level drivers need to be enabled. So I
guess we would have something like dispc_get() or dispc_enable(), which
dss2 would call first. Those functions would then use pm_runtime to
enable the HW module. And the higher level driver would keep the low
level devices enabled while its doing something.
I have been thinking something like this also earlier. It would separate
things cleanly. One thing I don't like about it is the big amount of low
level DSS internal functions that need to be exported.
> > - 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
How are the child/parent relationships done for omap_devices? Does it
come somehow from the hwmod data?
> 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[*].
As far as I'm concerned, L4 interface can be removed totally. TRM says
"The access through the L4_PER interconnect is provided for back
software compatibility.".
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
