Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755834AbbGPPdB (ORCPT ); Thu, 16 Jul 2015 11:33:01 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:50303 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752203AbbGPPc6 (ORCPT ); Thu, 16 Jul 2015 11:32:58 -0400 Message-ID: <55A7CE59.6010600@ti.com> Date: Thu, 16 Jul 2015 21:01:37 +0530 From: "R, Vignesh" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paul Walmsley CC: Tero Kristo , Thierry Reding , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Benoit Cousson , Tony Lindgren , Russell King , Mike Turquette , Stephen Boyd , , , , , , Subject: Re: [PATCH v2 2/5] ARM: OMAP2+: DRA7: Add hwmod entries for PWMSS References: <1433332284-10766-1-git-send-email-vigneshr@ti.com> <1433332284-10766-3-git-send-email-vigneshr@ti.com> In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4730 Lines: 139 Hi, On 07/16/2015 03:24 AM, Paul Walmsley wrote: > Hi, > > some comments. > > On Wed, 3 Jun 2015, Vignesh R wrote: > >> Add hwmod entries for the PWMSS on DRA7. >> >> Set l4_root_clk_div as the main_clk of PWMSS. It is fixed-factored clock >> equal to L4PER2_L3_GICLK/2(l3_iclk_div/2). >> As per AM57x TRM SPRUHZ6[1], October 2014, Section 29.1.3 Table 29-4, >> clock source to PWMSS is L4PER2_L3_GICLK. But it is actually >> L4PER2_L3_GICLK/2. The TRM does not show the division by 2. > > Is the divide-by-two coming from PWMSS_EPWM.EPWM_TBCTL[HSPCLKDIV]? Or is > HSPCLKDIV a separate divider after the divide-by-2 you mention above? No, it not related to HSPCLKDIV. The TRM wrongly states L4PER2_L3_GICLK as clock input for PWMSS. But actually L4PER2_L4_GICLK(=L3_GICLK/2) is the clock input for PWMSS. This will be updated in TRM soon. > >> [1] www.ti.com/lit/ug/spruhz6/spruhz6.pdf >> >> Signed-off-by: Vignesh R >> --- >> >> v2: >> * add TRM references. >> >> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 239 ++++++++++++++++++++++++++++++ >> 1 file changed, 239 insertions(+) >> >> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >> index 0e64c2fac0b5..86a7ac9a3138 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c >> @@ -362,6 +362,149 @@ static struct omap_hwmod dra7xx_dcan2_hwmod = { >> }, >> }; >> >> +/* pwmss */ >> +static struct omap_hwmod_class_sysconfig dra7xx_epwmss_sysc = { >> + .rev_offs = 0x0, >> + .sysc_offs = 0x4, >> + .sysc_flags = SYSC_HAS_SIDLEMODE | SYSC_HAS_RESET_STATUS, > > This doesn't match SPRUHZ6 Table 29-13 "PWMSS_SYSCONFIG". There's no > RESETSTATUS bit. There is a SOFTRESET bit. > > Could you please confirm whether this is intentional? sorry my bad... I will change this in v3. > >> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >> + .sysc_fields = &omap_hwmod_sysc_type2, >> +}; >> + >> +struct omap_hwmod_class dra7xx_epwmss_hwmod_class = { >> + .name = "epwmss", >> + .sysc = &dra7xx_epwmss_sysc, >> +}; >> + >> +static struct omap_hwmod_class dra7xx_ecap_hwmod_class = { >> + .name = "ecap", >> +}; >> + >> +static struct omap_hwmod_class dra7xx_eqep_hwmod_class = { >> + .name = "eqep", >> +}; >> + >> +struct omap_hwmod_class dra7xx_ehrpwm_hwmod_class = { >> + .name = "ehrpwm", >> +}; >> + >> +/* epwmss0 */ >> +struct omap_hwmod dra7xx_epwmss0_hwmod = { >> + .name = "epwmss0", >> + .class = &dra7xx_epwmss_hwmod_class, >> + .clkdm_name = "l4per2_clkdm", >> + .main_clk = "l4_root_clk_div", >> + .prcm = { >> + .omap4 = { >> + .modulemode = MODULEMODE_SWCTRL, >> + .clkctrl_offs = DRA7XX_CM_L4PER2_PWMSS1_CLKCTRL_OFFSET, >> + .context_offs = DRA7XX_RM_L4PER2_PWMSS1_CONTEXT_OFFSET, >> + }, >> + }, > > Per my comment on the previous patch, sounds like it might be better to > mark this as HWMOD_SWSUP_SIDLE? Then again, see the next comment below > re: main_clk. > >> +}; >> + >> +/* ecap0 */ >> +struct omap_hwmod dra7xx_ecap0_hwmod = { >> + .name = "ecap0", >> + .class = &dra7xx_ecap_hwmod_class, >> + .clkdm_name = "l4per2_clkdm", >> + .main_clk = "l4_root_clk_div", > > Looking at SPRUHZ6 Section 29.1.4.2 "PWMSS Modules Local Clock Gating", > there appears to be a local "mini-PRCM" for the PWMSS which implements > clock gating and reports back on the status of what I'd guess is the local > clock gating FSM. > > So from that point of view, you should probably create a clock driver that > manages both the clock gate request bit and the FSM status bit. It should > be something that can be reused for the other PWMSS IP blocks. Then > you'd create per-IP block clock nodes and set the main_clk to point to > that node. > Since, this register is within the config space of PWMSS, the individual gating and reporting for the modules within PWMSS (PWMSS_CLKCONFIG) is currently being taken care by pwm-tipwmss.c(almost the sole function this driver is doing). It has been the same since am335x. Adding new clock nodes will result in driver changes and also changes to am335x, am437x (and other platforms) hwmod files. It also involves adding new nodes to clocks.dtsi and it will be difficult to maintain backward compatibility for older platforms. Is it not better to keep this as is, in order to maintain consistency (with am335x, am437x etc) and also that these clock bits are within IP's config space? -- Regards Vignesh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/