Hi Franklin
On Wed, 17 Feb 2016, Franklin S Cooper Jr. wrote:
> On 08/31/2015 10:51 AM, Paul Walmsley wrote:
> > On Thu, 16 Jul 2015, R, Vignesh wrote:
> >> On 07/16/2015 03:24 AM, Paul Walmsley wrote:
> >>> 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.
> > OK
> >
> >>>> --- 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.
> > OK
> >
> >>>> +/* 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?
> > It's certainly possible that we as maintainers didn't look closely enough
> > at the AM33xx data for the PWMSS when we merged it. But if that's
> > incorrect too, then now is the time to fix this. Otherwise it will never
> > get fixed, since each new group of people patching this code will keep
> > punting it off to the indeterminate future.
> >
> > In terms of hwmod data: based on the register maps in sections 29.4.3,
> > 29.3.3, and 29.2.3 of SPRUHZ6, none of these subdevices are hwmod devices.
> > They don't support the Highlander OCP registers, they have no individual
> > PRCM registers or register bitfields, and all of the idle and status
> > reporting is to the PWMSS top-level IP block itself. So it looks to me
> > like the eCAP, eQEP, and ePWM modules should be registered via DT, rather
> > than via hwmod. It looks like you can get away with using the
> > "simple-bus" abstraction, but you might ultimately have to define
> > something custom here. However, the PWMSS top level subsystem, described
> > in section 29.1, does have the OCP registers, sideband signals, etc., and
> > thus should remain a hwmod-registered device (via DT).
> >
> > In terms of the clock data: based on section 29.1.4, section 29.1.5.2,
> > figure 29-3, and table 29-4, there are several clock gating control bits.
> > These should be modeled as clock nodes in the Linux common clock
> > framework. Furthermore these registers are located inside the PWMSS
> > subsystem itself, and are only accessible when the PWMSS IP block is
> > functional in a PM runtime point of view: i.e., when the block is mapped
> > into memory space, clocked and out of reset, etc. So the clock types for
> > the PWMSS_CFG IP block should most likely be implemented in the PWMSS
> > driver. I think you'll still be able to define the clock node data itself
> > in DT, but this will probably need a closer look. Ideally, since the
> > clock node register address data is the same for all three subsystem
> > instances, one would be able to simply include a DT include file three
> > times; but the DT binding data format may ultimately make this
> > impractical.
> >
> > In terms of the transition from the old approach to this approach, it ooks
> > to me like the first thing to do would be to convert
> > drivers/pwm/pwm-tipwmss.c to define some clk_ops and register some clock
> > nodes with the CCF. You've got the meat of the clock gating control code
> > there already. Then the next thing to do would be to to get rid of
> > pwmss_submodule_state_change() and use
> > clk_{prepare,enable,disable,unprepare}*() in the drivers/pwm/pwm-ti*.c
> > subdrivers instead. All of that looks like it should be possible to
> > implement in a backwards-compatible way. Then you'd convert the eCAP,
> > eQEP, and ePWM code to probe as platform_devices, driven from a simple-bus
> > or pwmss-bus or whatever in the DT data. Naturally, the AM33xx/43xx data
> > should be fixed also.
>
> I am working on updating this patchset on behalf of Vignesh
> based on your suggestion. During the process of converting
> the pwm-tipwmss.c to be a clock provider I discovered that
> there is an issue with the PWMSS local clock gates. For
> example on AM437x if you run the below commands you will get
> a " Custom Error: MASTER M2 (64-bit) TARGET L4_PER_0 (Read):
> Data Access in User mode during Functional access" error.
>
> rmmod pwm_tiecap
> rmmod pwm_bl
> rmmod pwm_tiecap
> modprobe pwm_tiecap
> modprobe pwm_bl
>
> Full Log: http://pastebin.com/sEyy52HV
>
> On device powerup, the various PWMSS local clock gates are
> "unlocked". The remove call in the ecap driver gates the
> local PWMSS clock gate for the ecap. The probe of the ecap
> driver "unlocks" the clock gate for the ecap. However, once
> you try to access any of the ecap registers which is
> indirectly accessed by the pwm_bl driver you will get an
> error. This is because the ecap clock is still being gated.
OK I'm not sure I understand what's going on, particularly the part about
locking and unlocking. Are you saying that the pwm_bl driver calls into
the pwm_tiecap module to write to the PWMSS_CLKCONFIG registers to ungate
the ECAP clock, and then the hardware silently ignores the write? If
that's the case, shouldn't we be seeing some warning messages from a
failure to ungate the clock from a subsequent PWMSS_CLKSTATUS poll? Or am
I misunderstanding what's going on here?
> This has been verified by hardware folks on our side. Its
> been determined these registers are not needed since they
> were designed for devices that didn't have a PRCM.
> Therefore, the current plan is to update the trms of
> AM335x,AM437x,DRA7 and AM57x to make it clear that these
> registers shouldn't be touched.
OK
> I plan on sending a patch for the pwm-tipwmss.c driver
> essentially removing anything that touches the
> PWMSS_CLKCONFIG and PWMSS_CLKSTATUS registers. I will also
> be sending a v3 version of this patch since I believe the
> struct omap_hwmod dra7xx_ecap0_hwmod and similar entries you
> had comments about before are ok to leave as is based on the
> above findings.
Including the comments regarding dra7xx_epwmss_sysc ?
- Paul
--
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
