| Message ID | alpine.DEB.2.00.1302122026530.29149@utopia.booyaka.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wednesday 13 February 2013 01:58 AM, Paul Walmsley wrote: > > Commit 17b7e7d33530e2bbd3bdc90f4db09b91cfdde2bb ("ARM: OMAP4: > clock/hwmod data: start to remove some IP block control "clocks"") > introduced a regression preventing the L3INIT clockdomain of OMAP4 > systems from entering idle. This in turn prevented these systems from > entering full chip clock-stop. > > The regression was caused by the incorrect removal of a so-called > "optional functional clock" from the OMAP4 clock data. This wasn't > caught for two reasons. First, I missed the retention entry failure > in the branch test logs: > > http://www.pwsan.com/omap/testlogs/cleanup_a_3.9/20130126014242/pm/4460pandaes/4460pandaes_log.txt > > Second, the integration data for the OCP2SCP PHY IP block, added by > commit 0c6688753f9912c6a7013549ec31c8844020bbc1 ("ARM: OMAP4: hwmod > data: add remaining USB-related IP blocks"), should have associated this > clock with the IP block, but did not. > > Fix by adding back the so-called "optional" functional clock to the > clock data, and by linking that clock to the OCP2SCP PHY IP block > integration hwmod data. This patch breaks MUSB in OMAP4 panda. The 48M clock is modeled as main clk [1] for ocp2scp so after doing get_sync, this optional clock gets enabled. But after this patch, the optional clock seems to be not enabled after get_sync. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/118285.html (this patch is not directly merged but I guess the discussion here is taken care of) Thanks Kishon -- 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, 8 Apr 2013, Kishon Vijay Abraham I wrote: > On Wednesday 13 February 2013 01:58 AM, Paul Walmsley wrote: > > > > Commit 17b7e7d33530e2bbd3bdc90f4db09b91cfdde2bb ("ARM: OMAP4: > > clock/hwmod data: start to remove some IP block control "clocks"") > > introduced a regression preventing the L3INIT clockdomain of OMAP4 > > systems from entering idle. This in turn prevented these systems from > > entering full chip clock-stop. > > > > The regression was caused by the incorrect removal of a so-called > > "optional functional clock" from the OMAP4 clock data. This wasn't > > caught for two reasons. First, I missed the retention entry failure > > in the branch test logs: > > > > http://www.pwsan.com/omap/testlogs/cleanup_a_3.9/20130126014242/pm/4460pandaes/4460pandaes_log.txt > > > > Second, the integration data for the OCP2SCP PHY IP block, added by > > commit 0c6688753f9912c6a7013549ec31c8844020bbc1 ("ARM: OMAP4: hwmod > > data: add remaining USB-related IP blocks"), should have associated this > > clock with the IP block, but did not. > > > > Fix by adding back the so-called "optional" functional clock to the > > clock data, and by linking that clock to the OCP2SCP PHY IP block > > integration hwmod data. > > This patch breaks MUSB in OMAP4 panda. The 48M clock is modeled as main clk > [1] for ocp2scp so after doing get_sync, this optional clock gets enabled. But > after this patch, the optional clock seems to be not enabled after get_sync. > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/118285.html > (this patch is not directly merged but I guess the discussion here is taken > care of) The OMAP integration for the MUSB driver needs to clk_enable() and clk_disable() its optional clock. It's not correct to work around a driver problem by changing the hardware description data - that data is intended to be driver-neutral. - 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 Mon, 8 Apr 2013, Paul Walmsley wrote: > On Mon, 8 Apr 2013, Kishon Vijay Abraham I wrote: > > > On Wednesday 13 February 2013 01:58 AM, Paul Walmsley wrote: > > > > > > Commit 17b7e7d33530e2bbd3bdc90f4db09b91cfdde2bb ("ARM: OMAP4: > > > clock/hwmod data: start to remove some IP block control "clocks"") > > > introduced a regression preventing the L3INIT clockdomain of OMAP4 > > > systems from entering idle. This in turn prevented these systems from > > > entering full chip clock-stop. > > > integration hwmod data. > > > > This patch breaks MUSB in OMAP4 panda. The 48M clock is modeled as main clk > > [1] for ocp2scp so after doing get_sync, this optional clock gets enabled. But > > after this patch, the optional clock seems to be not enabled after get_sync. > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/118285.html > > (this patch is not directly merged but I guess the discussion here is taken > > care of) > > The OMAP integration for the MUSB driver needs to clk_enable() and > clk_disable() its optional clock. It's not correct to work around a > driver problem by changing the hardware description data - that data is > intended to be driver-neutral. Ugh, took a closer look at this one. Now I recall what's going on here. This is a case where a bit that's clearly marked as an "optional clock" in the hardware is really the main functional clock for the IP block. So I'm open to solving this problem in the integration data as we did before, by making ocp2scp_usb_phy_phy_48m the main clock of the ocp2scp_usb_phy. But we need to have a big comment in the hwmod data to indicate what's going on here so it doesn't get missed again, since it's an inconsistency. - 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 Tue, 12 Feb 2013, Paul Walmsley wrote: > First, I missed the retention entry failure in the branch test logs: > > http://www.pwsan.com/omap/testlogs/cleanup_a_3.9/20130126014242/pm/4460pandaes/4460pandaes_log.txt Just a brief note. For the past few -rc releases, have switched here to using scripts that parse the PM result logs, for PM validation. The scripts are probably not perfect, but they're less likely to miss the cases they're set up to test. - 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
diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c index cebe2b3..54ebb89 100644 --- a/arch/arm/mach-omap2/cclock44xx_data.c +++ b/arch/arm/mach-omap2/cclock44xx_data.c @@ -1000,6 +1000,10 @@ DEFINE_CLK_OMAP_MUX(hsmmc2_fclk, "l3_init_clkdm", hsmmc1_fclk_sel, OMAP4430_CM_L3INIT_MMC2_CLKCTRL, OMAP4430_CLKSEL_MASK, hsmmc1_fclk_parents, func_dmic_abe_gfclk_ops); +DEFINE_CLK_GATE(ocp2scp_usb_phy_phy_48m, "func_48m_fclk", &func_48m_fclk, 0x0, + OMAP4430_CM_L3INIT_USBPHYOCP2SCP_CLKCTRL, + OMAP4430_OPTFCLKEN_PHY_48M_SHIFT, 0x0, NULL); + DEFINE_CLK_GATE(sha2md5_fck, "l3_div_ck", &l3_div_ck, 0x0, OMAP4430_CM_L4SEC_SHA2MD51_CLKCTRL, OMAP4430_MODULEMODE_SWCTRL_SHIFT, 0x0, NULL); @@ -1527,6 +1531,7 @@ static struct omap_clk omap44xx_clks[] = { CLK(NULL, "per_mcbsp4_gfclk", &per_mcbsp4_gfclk, CK_443X), CLK(NULL, "hsmmc1_fclk", &hsmmc1_fclk, CK_443X), CLK(NULL, "hsmmc2_fclk", &hsmmc2_fclk, CK_443X), + CLK(NULL, "ocp2scp_usb_phy_phy_48m", &ocp2scp_usb_phy_phy_48m, CK_443X), CLK(NULL, "sha2md5_fck", &sha2md5_fck, CK_443X), CLK(NULL, "slimbus1_fclk_1", &slimbus1_fclk_1, CK_443X), CLK(NULL, "slimbus1_fclk_0", &slimbus1_fclk_0, CK_443X), diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c index a1849a8..d55c769 100644 --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c @@ -2720,6 +2720,10 @@ static struct omap_ocp2scp_dev ocp2scp_dev_attr[] = { { } }; +static struct omap_hwmod_opt_clk ocp2scp_usb_phy_opt_clks[] = { + { .role = "48mhz", .clk = "ocp2scp_usb_phy_phy_48m" }, +}; + /* ocp2scp_usb_phy */ static struct omap_hwmod omap44xx_ocp2scp_usb_phy_hwmod = { .name = "ocp2scp_usb_phy", @@ -2734,6 +2738,8 @@ static struct omap_hwmod omap44xx_ocp2scp_usb_phy_hwmod = { }, }, .dev_attr = ocp2scp_dev_attr, + .opt_clks = ocp2scp_usb_phy_opt_clks, + .opt_clks_cnt = ARRAY_SIZE(ocp2scp_usb_phy_opt_clks), }; /*
Commit 17b7e7d33530e2bbd3bdc90f4db09b91cfdde2bb ("ARM: OMAP4: clock/hwmod data: start to remove some IP block control "clocks"") introduced a regression preventing the L3INIT clockdomain of OMAP4 systems from entering idle. This in turn prevented these systems from entering full chip clock-stop. The regression was caused by the incorrect removal of a so-called "optional functional clock" from the OMAP4 clock data. This wasn't caught for two reasons. First, I missed the retention entry failure in the branch test logs: http://www.pwsan.com/omap/testlogs/cleanup_a_3.9/20130126014242/pm/4460pandaes/4460pandaes_log.txt Second, the integration data for the OCP2SCP PHY IP block, added by commit 0c6688753f9912c6a7013549ec31c8844020bbc1 ("ARM: OMAP4: hwmod data: add remaining USB-related IP blocks"), should have associated this clock with the IP block, but did not. Fix by adding back the so-called "optional" functional clock to the clock data, and by linking that clock to the OCP2SCP PHY IP block integration hwmod data. The problem patch was discovered by J, Keerthy <j-keerthy@ti.com>. Cc: Keerthy <j-keerthy@ti.com> Cc: Benoît Cousson <b-cousson@ti.com> Signed-off-by: Paul Walmsley <paul@pwsan.com> --- It'd be nice, but certainly not necessary, to get this in for the v3.9 merge window. Otherwise, will send it for -rc1. arch/arm/mach-omap2/cclock44xx_data.c | 5 +++++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 6 ++++++ 2 files changed, 11 insertions(+)