Hi Paul

On 02/18/2016 12:58 AM, Paul Walmsley wrote:

Hi Franklin

On Wed, 17 Feb 2016, Franklin S Cooper Jr. wrote:

quoted

On 08/31/2015 10:51 AM, Paul Walmsley wrote:

quoted

On Thu, 16 Jul 2015, R, Vignesh wrote:

quoted

On 07/16/2015 03:24 AM, Paul Walmsley wrote:

quoted

On Wed, 3 Jun 2015, Vignesh R wrote:

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+/* 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?

Pwm-tipwmss.c exports a function
pwmss_submodule_state_change which interacts with
PWMSS_CLKCONFIG and PWMSS_CLKSTATUS registers. Both
pwm-tiehrpwm.c and pwm-tiecap.c calls this exported function
to unlock the gate at probe time and lock the gate when the
driver is removed.

So when the gate fails to be unlocked after it previously is
locked there isn't a way to know this via the
PWMSS_CLKSTATUS registers. So the driver "believes" that the
gate is unlocked and when pwm_bl runs it calls the
set_polarity function. Set_polarity in the case of AM437x gp
evm maps to the ecap's  ecap_pwm_set_polarity function call.
This call then attempts to write to the ecap registers which
results in the external abort since the clock to the ecap is
still gated.

When the ecap and tihrpwm driver request to unlock the clock
gate it already checks the XXX_CLK_EN_ACK bitfields within
CLKSTATUS and it shows that the clocks "should" be unlocked.
So there is an issue with the IP.

quoted

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 

quoted

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 ?  

Yes, I plan on addressing the change Vignesh acknowledged
regarding changing RESETSTATUS to SOFTRESET. For your other
comments I believe he already explained why it has to be set
in that particular way. If there is anything that I missed
or something that isn't clear please let me know.

- Paul