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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
