Re: [PATCH v2 2/5] ARM: OMAP2+: DRA7: Add hwmod entries for PWMSS

["R, Vignesh" <vigneshr@ti.com>]

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 <vigneshr@ti.com>
>> ---
>>
>> 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