Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Wed, Dec 12, 2018 at 03:19:40AM +0000, Yoshihiro Shimoda wrote:
> Hi Uwe,
> 
> > From: Uwe Kleine-Konig, Sent: Monday, December 10, 2018 5:11 PM
> > 
> > Hello,
> > 
> > On Mon, Dec 10, 2018 at 04:49:17AM +0000, Yoshihiro Shimoda wrote:
> > > > From: Uwe Kleine-Konig, Sent: Friday, December 7, 2018 6:14 PM
> > > >
> > > > On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> > > <snip>
> > > > > +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> > > > > +{
> > > > > +	/*
> > > > > +	 * This PWM Timer cannot output low because setting 0x000 is
> > > > > +	 * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> > > > > +	 * the prohibited, this function changes the value from 0 to 1 as
> > > > > +	 * pseudo low level.
> > > > > +	 *
> > > > > +	 * TODO: Add GPIO handling to output low level.
> > > > > +	 */
> > > > > +	if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> > > > > +		rp->pwmcnt |= 1;
> > > >
> > > > In my eyes this is too broken to do. Not sure I have the complete
> > > > picture, but given a small period (say 2) this 1 cycle might result in
> > > > 50 % duty cycle. Depending on how the hardware behaves if you disable
> > > > it, better do this instead.
> > >
> > > You're right.
> > 
> > But in the meantime I learned that the pwm gets active on disable, so
> > this won't help.
> > 
> > > > Are you aware of the series adding such gpio support to the imx driver?
> > >
> > > I didn't know that.
> > >
> > > > @Thierry: So there are three drivers now that could benefit for a
> > > > generic approach.
> > >
> > > Should I wait for Thierry's opinion whether PWM subsystem will have
> > > a generic approach or not?
> > 
> > Not sure how to preceed here. The needed procedure would be:
> > 
> > 	set duty_cycle to 0%
> > 	delay long enough to be sure the duty cycle is active
> > 	switch to gpio
> > 	disable the hardware
> > 
> > The additional blocker for rcar is that it doesn't support duty_cycle
> > 0%.
> > 
> > So unless your hardware guys confirm that 0% works even though not
> > supported according to the hardware manual I have no good idea.
> > 
> > In the past I suggested to weaken the requirements after pwm_disable,
> > but Thierry didn't like it.
> 
> I read the following discussion once:
> https://patchwork.ozlabs.org/patch/959776/

Yeah, that's (part of) the discussion I meant. Thierry doesn't agree
though, so for now that's a dead end. My plan is to watch the pwm list
for a while to get a better picture about the different pwm
implementations because just one or two problematic cases are not
enough to justify a generic solution in the core in his eyes.
 
> I could not understand all this yet, but I think I should try to add a special gpio handling
> to the pwm-rcar.c driver instead of a generic approach because as you mentioned above,
> such special handling needs for the hardware.

Being able to set PHO to zero would be still better, so I hope you
follow up on the question to your hardware guys if this is really
forbidden. (If I had access to such hardware, I'd bluntly try what
happens.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |