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...
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
