| Message ID | 1301900022.2715.12.camel@deskari (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Paul Walmsley |
| Headers | show |
Paul, Benoit, On Mon, 2011-04-04 at 09:53 +0300, Tomi Valkeinen wrote: > Hi Paul, Benoit, > > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: > > > Based on the E-mail thread so far, I'd say that changing the clock aliases > > is the way to go for right now. The clock aliases are not hardware data; > > they just control how the clock hardware is mapped to the drivers. > > I'd very much prefer this option. Below is a patch for this. > > If Benoit doesn't complain too much about this, I'd like to get this > merged as soon as possible, as OMAP4 DSS is currently crashing in the > mainline kernel. > > I can either handle it myself if I get your acks, or you can send a pull > request for this if you have some other patches going in also. Ping. Can I get an ack/nack from you for the patch below? Tomi > > Of course, at some point soon, those clock aliases are going to go away. > > But hopefully you all will have converted the driver over to PM runtime at > > that point. > > > > Once that happens, there's another problem: omap_hwmod_enable() is defined > > that the hardware registers should be accessible by the MPU after it > > completes. So by that definition, the hwmod code should be > > enabling/disabling that dss_clk clock too when it enables/idles/shuts down > > the hwmod. Probably we'd need to mark that struct omap_hwmod_opt_clk with > > some flag. Then we'd need some kind of way for the DSS to tell the hwmod > > code whether it is or isn't reliant on the PRCM-provided functional clock > > for its internal functional clock. Maybe something like > > omap_hwmod_{release,require}_system_fclk()? > > Hmm, right. I guess no other HW module has clock setups like this? > > Currently DSS can use clk_enable/disable() for the system fclk when its > using DSI PLL for the fclk. So omap_hwmod_{release,require}_system_fclk > sounds like a simple solution to this. > > Not directly related, but something I've been wondering about is how to > abstract the DSI/HDMI PLLs in DSS. What do you think, would it be > possible/worth it to create struct clks for the clocks coming out of > those PLLs? These would, of course, be DSS internal clks. I'm not very > familiar with the clock framework, so I don't really have any idea what > this would require and what would be the pros and cons. > > --- > > From f9999ceb48b2e22217dccc85b33362b6a17e5a00 Mon Sep 17 00:00:00 2001 > From: Tomi Valkeinen <tomi.valkeinen@ti.com> > Date: Mon, 4 Apr 2011 09:26:19 +0300 > Subject: [PATCH] OMAP4: clock data: Change DSS clock aliases > > DSS driver has used fck and ick clocks on OMAP2/3 to get DSS HW up and > running, and also to get the pixel clock's source clock rate from the > fck. > > On OMAP4 the clock data is set up in a different way, as there's no ick, > dss_fck points to a fake clock which just affects DSS's MODULEMODE, and > dss_dss_clk if the DSS_FCK. > > From DSS driver's point of view the dss_fck sounds like an ick, and > dss_dss_clk is the fck. While this is not entirely correct from HW point > of view, especially for the ick, configuring the clock aliases that way > makes DSS "just work" with OMAP4's clock setup. > > In the (hopefully near) future DSS driver will be reworked to use > pm_runtime support which should clean up the clock code. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > arch/arm/mach-omap2/clock44xx_data.c | 9 ++------- > 1 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c > index 276992d..8c96567 100644 > --- a/arch/arm/mach-omap2/clock44xx_data.c > +++ b/arch/arm/mach-omap2/clock44xx_data.c > @@ -3116,14 +3116,9 @@ static struct omap_clk omap44xx_clks[] = { > CLK(NULL, "dsp_fck", &dsp_fck, CK_443X), > CLK("omapdss_dss", "sys_clk", &dss_sys_clk, CK_443X), > CLK("omapdss_dss", "tv_clk", &dss_tv_clk, CK_443X), > - CLK("omapdss_dss", "dss_clk", &dss_dss_clk, CK_443X), > CLK("omapdss_dss", "video_clk", &dss_48mhz_clk, CK_443X), > - CLK("omapdss_dss", "fck", &dss_fck, CK_443X), > - /* > - * On OMAP4, DSS ick is a dummy clock; this is needed for compatibility > - * with OMAP2/3. > - */ > - CLK("omapdss_dss", "ick", &dummy_ck, CK_443X), > + CLK("omapdss_dss", "fck", &dss_dss_clk, CK_443X), > + CLK("omapdss_dss", "ick", &dss_fck, CK_443X), > CLK(NULL, "efuse_ctrl_cust_fck", &efuse_ctrl_cust_fck, CK_443X), > CLK(NULL, "emif1_fck", &emif1_fck, CK_443X), > CLK(NULL, "emif2_fck", &emif2_fck, CK_443X), -- 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
Hello Tomi, Benoît, On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: > > > Based on the E-mail thread so far, I'd say that changing the clock aliases > > is the way to go for right now. The clock aliases are not hardware data; > > they just control how the clock hardware is mapped to the drivers. > > I'd very much prefer this option. Below is a patch for this. > > If Benoit doesn't complain too much about this, I'd like to get this > merged as soon as possible, as OMAP4 DSS is currently crashing in the > mainline kernel. I will queue your clock aliases patch and attempt to merge it upstream for the -rc series. I am not happy to be disagreeing with Benoît here, but this is the rationale that I am using. (Benoît, Tomi, please feel free to correct if there is something blatantly false below.) 1. The integration of the DSS module is an unusual case. The MODULEMODE bits for the DSS bits do not control the DSS "main" functional clock (the clock that is needed to access device registers); instead, they only control the DSS interface clock. (This is because the DSS can use a functional clock that is not provided by the OMAP core.) So calling the DSS MODULEMODE struct clk "dss_fck" is not really correct, since it is really controlling the interface clock. 2. This patch does not create a precedent for hacking around general driver clocking problems in the clock or clock data. This is only needed because the MODULEMODE bits don't control the module functional clock in this case. As I understand it, the only other device that this problem could occur with is McBSP, due to mcbsp_clks. 3. The existing DSS2 driver clock design apparently works fine when this change is implemented[1], so no driver changes will be needed. 4. The proposed DSS driver patch to work around this uses a nasty hack that really should be NACK'ed[2]. All this said, we may not be able to merge this change upstream, due to the recent unhappiness about the clock data changes. If Linus rejects it, you'll need a "Plan B." ... 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. Comments welcome. - Paul 1. Valkeinen, Tomi. _Re: OMAP4 DSS clock setup_. E-mail to linux-omap@vger.kernel.org mailing list dated Wed, 30 Mar 2011 05:59:06 -0700. <http://www.mail-archive.com/linux-omap@vger.kernel.org/msg47665.html> 2. ibid.
Hi Paul, On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote: > Hello Tomi, Benoît, > > On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > > > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: > > > > > Based on the E-mail thread so far, I'd say that changing the clock aliases > > > is the way to go for right now. The clock aliases are not hardware data; > > > they just control how the clock hardware is mapped to the drivers. > > > > I'd very much prefer this option. Below is a patch for this. > > > > If Benoit doesn't complain too much about this, I'd like to get this > > merged as soon as possible, as OMAP4 DSS is currently crashing in the > > mainline kernel. > > I will queue your clock aliases patch and attempt to merge it upstream for > the -rc series. I am not happy to be disagreeing with Benoît here, but > this is the rationale that I am using. (Benoît, Tomi, please feel free to > correct if there is something blatantly false below.) > > 1. The integration of the DSS module is an unusual case. The > MODULEMODE bits for the DSS bits do not control the DSS "main" > functional clock (the clock that is needed to access device > registers); instead, they only control the DSS interface clock. > (This is because the DSS can use a functional clock that is not > provided by the OMAP core.) So calling the DSS MODULEMODE struct > clk "dss_fck" is not really correct, since it is really controlling > the interface clock. If we don't apply the patch, I think the clock aliases are still broken. Let's forget the ick/fck thing for a second, and just think the clock from PRCM which is used as the source clock for pixel clock. That is currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This cannot be right, can it? From DSS's point of view that is the same clock, it should clearly have the same alias on all platforms. I don't care what the name is as long as it's consistent on all platforms. And I have to say I still don't quite get it what is the problem with "fck" name. Or is that meant to be a clock which enables the registers on the module, and not the clock used for the pixel clock? If so, I'm fine with having "fck" to be what it is currently, but then we need a new name for the clock used for pixel clock, which is consistent on all platforms. > 2. This patch does not create a precedent for hacking around general > driver clocking problems in the clock or clock data. This is only > needed because the MODULEMODE bits don't control the module > functional clock in this case. As I understand it, the only other > device that this problem could occur with is McBSP, due to > mcbsp_clks. > > 3. The existing DSS2 driver clock design apparently works fine when > this change is implemented[1], so no driver changes will be needed. > > 4. The proposed DSS driver patch to work around this uses a nasty hack > that really should be NACK'ed[2]. > > All this said, we may not be able to merge this change upstream, due to > the recent unhappiness about the clock data changes. If Linus rejects it, > you'll need a "Plan B." > > ... > > 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. 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. - Should every DSS module have their own PM runtime handling? (actually related to the question above) - 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? - 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. - 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()? Can/should omap_pm_get_dev_context_loss_count() still be used to optimize the restoring? 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
Hi Paul, On 4/7/2011 9:27 PM, Paul Walmsley wrote: > Hello Tomi, Benoît, > > On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > >> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: >> >>> Based on the E-mail thread so far, I'd say that changing the clock aliases >>> is the way to go for right now. The clock aliases are not hardware data; >>> they just control how the clock hardware is mapped to the drivers. >> >> I'd very much prefer this option. Below is a patch for this. >> >> If Benoit doesn't complain too much about this, I'd like to get this >> merged as soon as possible, as OMAP4 DSS is currently crashing in the >> mainline kernel. > > I will queue your clock aliases patch and attempt to merge it upstream for > the -rc series. I am not happy to be disagreeing with Benoît here, but > this is the rationale that I am using. (Benoît, Tomi, please feel free to > correct if there is something blatantly false below.) That's quite good that you were disagreeing with me, because I think that you are indeed right:-) > 1. The integration of the DSS module is an unusual case. The > MODULEMODE bits for the DSS bits do not control the DSS "main" > functional clock (the clock that is needed to access device > registers); instead, they only control the DSS interface clock. > (This is because the DSS can use a functional clock that is not > provided by the OMAP core.) So calling the DSS MODULEMODE struct > clk "dss_fck" is not really correct, since it is really controlling > the interface clock. I've just got the confirmation from a PRCM designer that this is indeed what is happening. In the case of the DSS the optional functional clocks are provided by the PRCM and thus managed by the PRCM. The only difference is that since two different functional clocks are available, the PRCM cannot chose automatically the proper one. Hence the term optional fck, meaning that the SW has to explicitly enable them. The PRCM will not do it when modulemode will be enabled. So in that case enabling the modulemode will effectively enable the module for the PRCM point of view and just request the iclk if not already available. The only point that we need to take care of is the sequence. The PRCM spec clearly specify that one of the optional clock must be active before the modulemode is enabled. That does not seems to be the case in the current DSS code. > 2. This patch does not create a precedent for hacking around general > driver clocking problems in the clock or clock data. This is only > needed because the MODULEMODE bits don't control the module > functional clock in this case. As I understand it, the only other > device that this problem could occur with is McBSP, due to > mcbsp_clks. In that case this is slightly different because even the PRCM does not control these external clocks. Whereas in the case of the dss_fck, if the DPLL is not locked when you request it, it will be enabled automatically. Assuming that auto mode are enabled. > 3. The existing DSS2 driver clock design apparently works fine when > this change is implemented[1], so no driver changes will be needed. Yeah, but my point was that driver changes will be needed anyway, hence my preference to hack the driver instead of hacking the clock data. > 4. The proposed DSS driver patch to work around this uses a nasty hack > that really should be NACK'ed[2]. > > All this said, we may not be able to merge this change upstream, due to > the recent unhappiness about the clock data changes. If Linus rejects it, > you'll need a "Plan B." That's was my #2 concern as well. See you soon at ELC. Regards, Benoit -- 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
Hi On Fri, 8 Apr 2011, Tomi Valkeinen wrote: > On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote: > > On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > > > > > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: > > > > > > > Based on the E-mail thread so far, I'd say that changing the clock aliases > > > > is the way to go for right now. The clock aliases are not hardware data; > > > > they just control how the clock hardware is mapped to the drivers. > > > > > > I'd very much prefer this option. Below is a patch for this. > > > > > > If Benoit doesn't complain too much about this, I'd like to get this > > > merged as soon as possible, as OMAP4 DSS is currently crashing in the > > > mainline kernel. > > > > I will queue your clock aliases patch and attempt to merge it upstream for > > the -rc series. I am not happy to be disagreeing with Benoît here, but > > this is the rationale that I am using. (Benoît, Tomi, please feel free to > > correct if there is something blatantly false below.) > > > > 1. The integration of the DSS module is an unusual case. The > > MODULEMODE bits for the DSS bits do not control the DSS "main" > > functional clock (the clock that is needed to access device > > registers); instead, they only control the DSS interface clock. > > (This is because the DSS can use a functional clock that is not > > provided by the OMAP core.) So calling the DSS MODULEMODE struct > > clk "dss_fck" is not really correct, since it is really controlling > > the interface clock. > > If we don't apply the patch, I think the clock aliases are still broken. > Let's forget the ick/fck thing for a second, and just think the clock > from PRCM which is used as the source clock for pixel clock. That is > currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This > cannot be right, can it? From DSS's point of view that is the same > clock, it should clearly have the same alias on all platforms. I don't > care what the name is as long as it's consistent on all platforms. Yes, I agree. That is another good point. > And I have to say I still don't quite get it what is the problem with > "fck" name. After the hwmod/PM runtime conversion, there shouldn't be any clock aliases left that are simply called "fck", because it is almost a meaningless name. > Or is that meant to be a clock which enables the registers > on the module, After the hwmod/PM runtime conversion, that should have an alias name of "main" that is automatically added by the omap_device code. (Note that it does not appear to do so now; that is a bug that needs be fixed.) > and not the clock used for the pixel clock? If so, I'm fine with having > "fck" to be what it is currently, but then we need a new name for the > clock used for pixel clock, which is consistent on all platforms. If there is a separate PRCM-provided clock used only for the pixel clock, then that clock should have an alias name of "system_pixel_ck" or something similar that is meaningful to the DSS driver. I think the problem in this case is that "dss_dss_clk" is (optionally) used for two purposes: optionally as a "main PRCM-provided functional clock" and optionally as a system-provided pixel clock. I'll reply to the rest of your mail in a separate E-mail... - Paul
Hi Tomi, On 4/8/2011 7:51 AM, Valkeinen, Tomi wrote: > Hi Paul, > > On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote: >> Hello Tomi, Benoît, >> >> On Mon, 4 Apr 2011, Tomi Valkeinen wrote: >> >>> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: >>> >>>> Based on the E-mail thread so far, I'd say that changing the clock aliases >>>> is the way to go for right now. The clock aliases are not hardware data; >>>> they just control how the clock hardware is mapped to the drivers. >>> >>> I'd very much prefer this option. Below is a patch for this. >>> >>> If Benoit doesn't complain too much about this, I'd like to get this >>> merged as soon as possible, as OMAP4 DSS is currently crashing in the >>> mainline kernel. >> >> I will queue your clock aliases patch and attempt to merge it upstream for >> the -rc series. I am not happy to be disagreeing with Benoît here, but >> this is the rationale that I am using. (Benoît, Tomi, please feel free to >> correct if there is something blatantly false below.) >> >> 1. The integration of the DSS module is an unusual case. The >> MODULEMODE bits for the DSS bits do not control the DSS "main" >> functional clock (the clock that is needed to access device >> registers); instead, they only control the DSS interface clock. >> (This is because the DSS can use a functional clock that is not >> provided by the OMAP core.) So calling the DSS MODULEMODE struct >> clk "dss_fck" is not really correct, since it is really controlling >> the interface clock. > > If we don't apply the patch, I think the clock aliases are still broken. > Let's forget the ick/fck thing for a second, and just think the clock > from PRCM which is used as the source clock for pixel clock. That is > currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This > cannot be right, can it? From DSS's point of view that is the same > clock, it should clearly have the same alias on all platforms. I don't > care what the name is as long as it's consistent on all platforms. > > And I have to say I still don't quite get it what is the problem with > "fck" name. Or is that meant to be a clock which enables the registers > on the module, and not the clock used for the pixel clock? If so, I'm > fine with having "fck" to be what it is currently, but then we need a > new name for the clock used for pixel clock, which is consistent on all > platforms. Both can be used for the system clock (sys_clk -> DSI DPLL or dss_dss_clk). In fact after multiple discussions with DSS and PRCM folks, I realized that OMAP3 and 4 are using exactly the same clock scheme :-( The confusion in my mind came from change in naming convention in both the PRCM and the DSS at the same time between OMAP3 and 4. OMAP3 -> OMAP4 dss1_alwon_fck -> dss_dss_clk (from dpll per output in both cases) dss2_alwon_fck -> dss_sys_clk dss_ick -> dss_fck (-> modulemode) that name really needs to be changed to avoid further confusion. So in fact the OMAP4 aliased should be: CLK("omapdss_dss", "fck", &dss_dss_clk, CK_443X), CLK("omapdss_dss", "sys_clk", &dss_sys_clk, CK_443X), CLK("omapdss_dss", "ick", &dss_fck, CK_443X), That will map perfectly what we have on OMAP3: CLK("omapdss_dss", "fck", &dss1_alwon_fck_3430es2, ...), CLK("omapdss_dss", "sys_clk", &dss2_alwon_fck, ...), CLK("omapdss_dss", "ick", &dss_ick_3430es2, ...), >> 2. This patch does not create a precedent for hacking around general >> driver clocking problems in the clock or clock data. This is only >> needed because the MODULEMODE bits don't control the module >> functional clock in this case. As I understand it, the only other >> device that this problem could occur with is McBSP, due to >> mcbsp_clks. >> >> 3. The existing DSS2 driver clock design apparently works fine when >> this change is implemented[1], so no driver changes will be needed. >> >> 4. The proposed DSS driver patch to work around this uses a nasty hack >> that really should be NACK'ed[2]. >> >> All this said, we may not be able to merge this change upstream, due to >> the recent unhappiness about the clock data changes. If Linus rejects it, >> you'll need a "Plan B." >> >> ... >> >> 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. No indeed. > 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. For dedicated clock like tv_clk, it probably makes sense, because the other DSS drivers do not have to care about that clock. > - Should every DSS module have their own PM runtime handling? (actually > related to the question above) > > - 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? Are you sure about that? The dss_core does not have any reset in the sysonfig (only a status), but the dispc does have one and the dsi as well. That being said, the dss modules have some issue with the softreset at boot time, so I'm not sure what these bits are really doing. Regards, Benoit -- 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
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
On Fri, 2011-04-08 at 17:36 +0200, Cousson, Benoit wrote: > Hi Tomi, > > On 4/8/2011 7:51 AM, Valkeinen, Tomi wrote: > > - 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? > > Are you sure about that? > The dss_core does not have any reset in the sysonfig (only a status), but the dispc does have one and the dsi as well. Ah, I might be speaking only of OMAP2/3. I remember somebody saying that the DSS reset bit on OMAP4 is marked as reserved. But for OMAP3 (and I think for OMAP2 also) the DSS_SYSCONFIG reset bit will reset also the rest of the DSS. 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
Hi Tomi, On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote: > > > Of course, at some point soon, those clock aliases are going to go away. > > But hopefully you all will have converted the driver over to PM runtime at > > that point. > > > > Once that happens, there's another problem: omap_hwmod_enable() is defined > > that the hardware registers should be accessible by the MPU after it > > completes. So by that definition, the hwmod code should be > > enabling/disabling that dss_clk clock too when it enables/idles/shuts down > > the hwmod. Probably we'd need to mark that struct omap_hwmod_opt_clk with > > some flag. Then we'd need some kind of way for the DSS to tell the hwmod > > code whether it is or isn't reliant on the PRCM-provided functional clock > > for its internal functional clock. Maybe something like > > omap_hwmod_{release,require}_system_fclk()? > > Hmm, right. I guess no other HW module has clock setups like this? I think McBSP can have an optional main functional clock. So something like that should be usable for that case too. (In the McBSP case, the optional clock isn't controlled by the PRCM, it's controlled by the SCM, but that doesn't really matter to the hwmod code.) > Currently DSS can use clk_enable/disable() for the system fclk when its > using DSI PLL for the fclk. So omap_hwmod_{release,require}_system_fclk > sounds like a simple solution to this. OK. The only problem with those functions (actually omap_device_{release,require}_system_fclk()) is that they will need to be called through platform_data function pointers for now. Maybe it is possible to handle something like this simply with the clock framework also. > Not directly related, but something I've been wondering about is how to > abstract the DSI/HDMI PLLs in DSS. What do you think, would it be > possible/worth it to create struct clks for the clocks coming out of > those PLLs? These would, of course, be DSS internal clks. I'm not very > familiar with the clock framework, so I don't really have any idea what > this would require and what would be the pros and cons. Yes, I think it would be good to try to implement the entire DSS clock tree in the clock framework. One change to the clock code that we know we'll need is to put a hwmod pointer in the struct clk which tells the clock code that the hwmod needs to be enabled in order to access the clock's registers. Right now, the clock code assumes that all of the clock registers are accessible, all of the time. - Paul -- 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
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
On Fri, 2011-04-08 at 08:55 -0600, Paul Walmsley wrote: > Hi > > On Fri, 8 Apr 2011, Tomi Valkeinen wrote: > > and not the clock used for the pixel clock? If so, I'm fine with having > > "fck" to be what it is currently, but then we need a new name for the > > clock used for pixel clock, which is consistent on all platforms. > > If there is a separate PRCM-provided clock used only for the pixel clock, > then that clock should have an alias name of "system_pixel_ck" or > something similar that is meaningful to the DSS driver. I think the > problem in this case is that "dss_dss_clk" is (optionally) used for two > purposes: optionally as a "main PRCM-provided functional clock" and > optionally as a system-provided pixel clock. Not only for pixel clock, but in the end it'll come out as pixel clock. I'm not sure what exactly is a "functional clock" here. I mean, one could think it as a basic functionality of DSS to read the pixels, manipulate them, and output them (with the rate of the pixel clock). However, I think there is one difference between the clock used just to enable the DSS registers, and the one used to output pixels: we need to be able to adjust the rate of the clock. Thus we need to have a common (omap2/3/4) clock name for it to be able to clk_get() it. Should that clock name be just the "main" clock provided automatically, or something else? 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
On Fri, 2011-04-08 at 10:50 -0600, Paul Walmsley wrote: > Hi Tomi, > > On Mon, 4 Apr 2011, Tomi Valkeinen wrote: > > Not directly related, but something I've been wondering about is how to > > abstract the DSI/HDMI PLLs in DSS. What do you think, would it be > > possible/worth it to create struct clks for the clocks coming out of > > those PLLs? These would, of course, be DSS internal clks. I'm not very > > familiar with the clock framework, so I don't really have any idea what > > this would require and what would be the pros and cons. > > Yes, I think it would be good to try to implement the entire DSS clock > tree in the clock framework. One change to the clock code that we know > we'll need is to put a hwmod pointer in the struct clk which tells the > clock code that the hwmod needs to be enabled in order to access the > clock's registers. Right now, the clock code assumes that all of the > clock registers are accessible, all of the time. It's not quite that simple, as DSI PLL also needs the vdds_dsi regulator to be enabled... And to use DSI PLL, not only do you need to access DSI PLL registers, you also need to use DSI registers. Is it possible to have the driver create its own clock structs when it's loaded, and have the code for that clock inside the driver, or are clocks something that has to be handled in the core platform code? 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
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. 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.". 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. regards, - Paul
Hi On Mon, 11 Apr 2011, Tomi Valkeinen wrote: > On Fri, 2011-04-08 at 08:55 -0600, Paul Walmsley wrote: > > On Fri, 8 Apr 2011, Tomi Valkeinen wrote: > > > > and not the clock used for the pixel clock? If so, I'm fine with having > > > "fck" to be what it is currently, but then we need a new name for the > > > clock used for pixel clock, which is consistent on all platforms. > > > > If there is a separate PRCM-provided clock used only for the pixel clock, > > then that clock should have an alias name of "system_pixel_ck" or > > something similar that is meaningful to the DSS driver. I think the > > problem in this case is that "dss_dss_clk" is (optionally) used for two > > purposes: optionally as a "main PRCM-provided functional clock" and > > optionally as a system-provided pixel clock. > > Not only for pixel clock, but in the end it'll come out as pixel clock. > I'm not sure what exactly is a "functional clock" here. I mean, one > could think it as a basic functionality of DSS to read the pixels, > manipulate them, and output them (with the rate of the pixel clock). Broadly speaking, a functional clock is defined negatively - 'anything that's not an interface clock.' That's why the term 'functional clock' is not so useful by itself. But when we talk about a module's 'main functional clock,' that means the functional clock that is needed for register accesses to work. > However, I think there is one difference between the clock used just to > enable the DSS registers, and the one used to output pixels: we need to > be able to adjust the rate of the clock. Thus we need to have a common > (omap2/3/4) clock name for it to be able to clk_get() it. > > Should that clock name be just the "main" clock provided automatically, > or something else? Are you referring here to the system DPLL and its output dividers, or are you referring to the DSS module's internal dividers? - Paul -- 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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. That will be doable only when main_clk will not be mapped to modulemode. You cannot change the clock rate of the modulemode since it is a fake clock :-( >> 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. And then even if we switch to dss2 as the functional clock, dss1 cannot be gated. >> 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. If we do have an hwmod for dss_venc, we might consider the tv_clk as the main clock, which is the case anyway. The only tricky part is the dependency with the dss modulemode / fclk that have to be enabled first. > 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. Do we really have to add that in hwmod core code? Cannot we let the driver stack handle that dependency? Assuming we have a device for the low level DSS code and assuming it is doing more than just enabling / disabling the module. Benoit -- 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
On Mon, 2011-04-11 at 12:20 -0600, Paul Walmsley wrote: > Hi > > On Mon, 11 Apr 2011, Tomi Valkeinen wrote: > > However, I think there is one difference between the clock used just to > > enable the DSS registers, and the one used to output pixels: we need to > > be able to adjust the rate of the clock. Thus we need to have a common > > (omap2/3/4) clock name for it to be able to clk_get() it. > > > > Should that clock name be just the "main" clock provided automatically, > > or something else? > > Are you referring here to the system DPLL and its output dividers, or are > you referring to the DSS module's internal dividers? The system DPLL and its divider. If we are using the dss_dss_clk as fclk, we need to adjust it depending on the required pixel clock and use cases (e.g. some scaling factors may need higher fclk). 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
On Mon, 2011-04-11 at 10:05 -0600, 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. Ok. > > 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 there's also room for improvement, as the dss1_alwon_fclk is an optional clock on OMAP3. > > 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. On OMAP3 DSS_96M_FCLK goes to VENC's video DACs. I don't quite understand how it goes in OMAP4, the clock tree figure shows something going into VDAC, but it's unclear what. Well, anyway, the point was not to dig out the clocks now, but just to point out that some extra clocks are needed =). > 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. Yes, I think sys_clk is the input clock for the HDMI PLL. The output from the PLL can then be used for DISPC fckl. I believe HDMI_PHY_48M_FCLK ("dss_48mhz_clk") is also needed. 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
diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c index 276992d..8c96567 100644 --- a/arch/arm/mach-omap2/clock44xx_data.c +++ b/arch/arm/mach-omap2/clock44xx_data.c @@ -3116,14 +3116,9 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "dsp_fck", &dsp_fck, CK_443X), CLK("omapdss_dss", "sys_clk", &dss_sys_clk, CK_443X), CLK("omapdss_dss", "tv_clk", &dss_tv_clk, CK_443X), - CLK("omapdss_dss", "dss_clk", &dss_dss_clk, CK_443X), CLK("omapdss_dss", "video_clk", &dss_48mhz_clk, CK_443X), - CLK("omapdss_dss", "fck", &dss_fck, CK_443X), - /* - * On OMAP4, DSS ick is a dummy clock; this is needed for compatibility - * with OMAP2/3. - */ - CLK("omapdss_dss", "ick", &dummy_ck, CK_443X), + CLK("omapdss_dss", "fck", &dss_dss_clk, CK_443X), + CLK("omapdss_dss", "ick", &dss_fck, CK_443X), CLK(NULL, "efuse_ctrl_cust_fck", &efuse_ctrl_cust_fck, CK_443X), CLK(NULL, "emif1_fck", &emif1_fck, CK_443X), CLK(NULL, "emif2_fck", &emif2_fck, CK_443X),