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

[Vignesh R]

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 <vigneshr@xxxxxx>
>>>> ---
>>>>
>>>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/