Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753748AbbGWPge (ORCPT ); Thu, 23 Jul 2015 11:36:34 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:46078 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751025AbbGWPg0 (ORCPT ); Thu, 23 Jul 2015 11:36:26 -0400 Subject: Re: [PATCH v2 2/5] ARM: OMAP2+: DRA7: Add hwmod entries for PWMSS To: Paul Walmsley References: <1433332284-10766-1-git-send-email-vigneshr@ti.com> <1433332284-10766-3-git-send-email-vigneshr@ti.com> <55A7CE59.6010600@ti.com> 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 , , , , , , From: "R, Vignesh" Message-ID: <55B109C1.80901@ti.com> Date: Thu, 23 Jul 2015 21:05:29 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <55A7CE59.6010600@ti.com> 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: 4914 Lines: 141 On 7/16/2015 9:01 PM, R, Vignesh wrote: > 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? > ping... 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/