Re: OMAP4 DSS clock setup

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html