On 4/11/2011 6:05 PM, Paul Walmsley wrote:
Hi
On Mon, 11 Apr 2011, Tomi Valkeinen wrote:
On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
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?
If the DSS needs to change the parent or the clock rate of the main clock,
then it will need to clk_get(dev, "main") and use the clock API. My only
point here was that the "main" alias should be automatically added by the
omap_device code, not via the clock aliases in clock*_data.c.
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?
Probably so.
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?
If the DSS driver were using PM runtime, then yes, that would be true.
Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
believe.
In terms of the VENC, I'd agree with that -- based on OMAP4 Public TRM Rev
O Figure 10-177 "Video Encoder Overview," it looks like VENC uses
DSS_TV_FCLK.
In terms of the HDMI IP block, based on OMAP4 Public TRM Rev O Figure
10-159 "HDMI Overview," it looks like sys_clk should be one of the
optional clocks for HDMI? Hard to tell from that diagram.
- 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.
That makes sense to me.
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.
Yeah, that's one of the downsides of that approach.
- 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?
Right now, there's no explicit support in omap_device for parent/child
relationships. Probably the right place for that is in the omap_hwmod
layer, since omap_device code just maps the hwmod data into the Linux
device/driver model. There is an interconnect hierarchy that's encoded in
the omap_hwmod data, but currently there's no explicit handling for a case
where a parent hwmod needs to be enabled for a child device to be
accessed. So far we haven't encountered any use-cases that require this,
but I've suspected that this needs to be added to the hwmod code, and DSS
may be the case that justifies it.
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.
What version are you looking at? Both address spaces are already there
today.
static struct omap_hwmod_addr_space omap44xx_dss_dma_addrs[] = {
{
.pa_start = 0x58000000,
.pa_end = 0x5800007f,
.flags = ADDR_TYPE_RT
},
};
/* l3_main_2 -> dss */
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
.master = &omap44xx_l3_main_2_hwmod,
.slave = &omap44xx_dss_hwmod,
.clk = "l3_div_ck",
.addr = omap44xx_dss_dma_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_dma_addrs),
.user = OCP_USER_SDMA,
};
static struct omap_hwmod_addr_space omap44xx_dss_addrs[] = {
{
.pa_start = 0x48040000,
.pa_end = 0x4804007f,
.flags = ADDR_TYPE_RT
},
};
/* l4_per -> dss */
static struct omap_hwmod_ocp_if omap44xx_l4_per__dss = {
.master = &omap44xx_l4_per_hwmod,
.slave = &omap44xx_dss_hwmod,
.clk = "l4_div_ck",
.addr = omap44xx_dss_addrs,
.addr_cnt = ARRAY_SIZE(omap44xx_dss_addrs),
.user = OCP_USER_MPU,
};
/* dss slave ports */
static struct omap_hwmod_ocp_if *omap44xx_dss_slaves[] = {
&omap44xx_l3_main_2__dss,
&omap44xx_l4_per__dss,
};
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[*].
Today, hwmod will use only the L4 one just because the OCP_USER_MPU flag
is used for that one only. We can just add the SDMA flag to L3 and
remove the L4 mapping, if needed, but it will only change the one use by
hwmod code. The driver will not impacted by that at all.
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.".
Okay, good to know. We may leave them in there - the goal of the hwmod
data is to describe the hardware independently of the Linux drivers. But
either way, the other set of addresses shouldn't appear to the DSS driver,
so it's really a core code issue.
If we do that, we will have to name the address space to allow the
driver to choose the proper one.
The is doable since we added that support for McBSP in 2.6.39.
I have some patches ready to add name address space for all IPs with
dual mapping.
But in that case, removing the L4 completely will avoid any further
change to the DSS driver. For the moment we are lucky just because the
L3 entry is the first one in the list:-)
Regards,
Benoit
--
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
