Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753073AbbG2Gdv (ORCPT ); Wed, 29 Jul 2015 02:33:51 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:35435 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752999AbbG2Gdt (ORCPT ); Wed, 29 Jul 2015 02:33:49 -0400 Message-ID: <55B87390.5010809@ti.com> Date: Wed, 29 Jul 2015 12:02:48 +0530 From: Vignesh R User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.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> <55A7CE59.6010600@ti.com> <55B109C1.80901@ti.com> In-Reply-To: <55B109C1.80901@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: 5520 Lines: 156 On 07/23/2015 09:05 PM, R, Vignesh wrote: > > > 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... > I really want PWMSS support to go into v4.3... I see two options: 1. Drop pwm-tipwmss.c that is currently managing "PWMSS Modules Local Clock Gating" and change am335x, am437x and dra7 to use new main_clk and update clocks.dtsi (Breaks DT backward compatibility for am335x, am437x) 2. Or stick with $subject approach to maintain consistency. I prefer second option, Shall I send a v3 fixing other comments for this patch? -- 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/