[PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM.

[Philip, Avinash]

On Tue, Jul 24, 2012 at 13:37:24, Thierry Reding wrote:
> On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote:
> > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote:
> > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote:
> > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote:
> > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote:
> > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote:
> > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote:
> > > > > > > > +	/*
> > > > > > > > +	 * Enable 'Free run Time stamp counter mode' to start counter
> > > > > > > > +	 * and  'APWM mode' to enable APWM output
> > > > > > > > +	 */
> > > > > > > > +	reg_val = readw(pc->mmio_base + ECCTL2);
> > > > > > > > +	reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE;
> > > > > > > 
> > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here
> > > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it
> > > > > > > clear in .config() is the better option.
> > > > > > 
> > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low 
> > > > > > in idle state).
> > > > > > 
> > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2)
> > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode 
> > > > > > should be set.
> > > > > > The same is done in .config().
> > > > > 
> > > > > My point was that if you do it in .enable(), then why do you still set
> > > > > it in .config()? Since the sequence is typically .config() followed by
> > > > > .enable(), setting the bit in both seems redundant. It should be enough
> > > > > to load the registers during .enable(), right?
> > > > 
> > > > Consider scenarios where .enable() can be called without calling .config().
> > > > Example I just need to stop the pwm signal momentarily and re-enable.
> > > > In this case, .config() need not be called. So, APWM mode bit needs to be 
> > > > set in .enable()
> > > > 
> > > > Now, considering .config() followed by .enable().
> > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow 
> > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM
> > > > mode bit to be set. 
> > > > 
> > > > The same can be done in .enable() also. However, we again need to pass 
> > > > the pwm parameters (period & duty cycle values) to the .enable(). 
> > > > We don't want to duplicate this parameter passing. 
> > > > To avoid this we enable the APWM mode bit in both .config() & .enable().
> > > 
> > > That's weird. I assumed that the values written to the shadow registers
> > > would automatically be copied to the active registers on each new PWM
> > > period.
> > 
> > This is correct in case if PWM is running.
> > 
> > > If I understand correctly what you're saying, however, the eCAP
> > > requires the APWM bit to be set in order to write the shadow registers
> > > at all.
> > 
> > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the
> > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates
> > immediate mode. 
> > 
> > Writing directly to the shadow registers CAP3/CAP4 will invoke the
> > shadow mode.
> > 
> > If PWM is not running, cycle & duty values written to active registers, not
> > to shadow registers. To copy these value to shadow registers APWM mode to be
> > set. This way reloading from shadow registers can be ensured on next PWM
> > period/cycle.
> 
> I think this is the part I don't understand. If you wrote cycle and duty
> values to the active registers already, then the shadow registers should
> be ignored by the hardware. There should be no need to reload the active
> registers.

ECAP PWM hardware always loads active registers from shadow registers at
counter = period value (starting of next period). So on every next cycle
active registers being updated from shadow registers. So shadow registers
acting as a backup.

> 
> > If PWM is running, cycle & duty values written to shadow registers. So that
> > it won't disturb current PWM period. On the next PWM period, new values will
> > be reloaded from shadow registers to active registers.
> > 
> > So APWM mode to be set to copy shadow registers from active registers
> > (immediate mode).
> 
> I realize that I'm just talking from a general understanding of shadow
> registers and maybe you're hardware doesn't work quite that way. If this
> is really the only way that you can make the hardware work then I will
> no longer object.

Ok.

Thanks
Avinash

> 
> Thierry
>