Message-ID: <20190130153921.met3kipvdsoctjoa@pengutronix.de>
Date:   Wed, 30 Jan 2019 16:39:21 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Michal Vokáč <michal.vokac@...ft.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lukasz Majewski <l.majewski@...ess.pl>,
        Fabio Estevam <fabio.estevam@....com>,
        Lothar Waßmann <LW@...o-electronics.de>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [RFC PATCH v3 2/2] pwm: imx: Configure output to GPIO in
 disabled state

Hello Michal,

On Wed, Jan 30, 2019 at 03:42:29PM +0100, Michal Vokáč wrote:
> On 24.1.2019 11:44, Uwe Kleine-König wrote:
> > On Thu, Jan 24, 2019 at 11:12:12AM +0100, Michal Vokáč wrote:
> > > On 24.1.2019 10:22, Uwe Kleine-König wrote:
> > > > I think it might be beneficial to allow (or require) that disable acts
> > > > immediately. But this is not how it used to be and in my discussion with
> > > > Thierry (IIRC) he required to complete the currently running period.
> > > 
> > > I am confused here. IFAIK it always used to be that:
> > > 
> > > 	pwm_apply_state(pwm, { .enabled = 0 });
> > > 
> > > immediately stops the PWM with:
> > > 
> > > 	writel(0, imx->mmio_base + MX3_PWMCR);
> > > 
> > > regardless of the period (for pwm-imx).
> > 
> > Then is is a bug since forever (well, or a fact that resulted from the
> > intended semantic not being documented and the driver author not caring
> > or knowing better).
> 
> Hi Uwe, I skimmed all the PWM drivers trying to find out how others are
> waiting for the end of a period before disabling PWM.
> 
> There is 50 PWM drivers in total and I could find only two drivers that
> do something that resembles waiting for an end of a period:
> 
>  - pwm-atmel.c
>    https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-atmel.c#L176
> 	
>  - pwm-sun4i.c
>    https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-sun4i.c#L284

There are quite some drivers known (to me) being buggy here. My feeling
is that Thierry doesn't share that impression and I think the only
reasonable path forward is to first fix the requirements for drivers and
when this is done, look at the implementations and fix them or conclude
that the requirements are not practical and shift them accordingly.

>From my POV the situation is as follows: I suggested a few changes that
seemed reasonable to me (e.g.: don't rely on the pin state to be
inactive after disable; don't rely on a driver to block in .apply until
the new configuration is active; thought about dropping .disable
altogether) and Thierry shot them down indicating that he is not willing
to change the API and all affected users for only one or a few
implementations. My consequence is to be strict about the requirements
because that's the only way to show the resulting pain (or see there
isn't so much pain).

> I did not delve into that too much but I believe there are some HW
> requirements on those platforms to stop the PWM that way. Others
> simply stop the PWM straight away. So I am confused even more
> and wonder where your requirements came from?

I'm aware that many drivers don't adhere to these requirements. IMHO
this is related to the poor situation regarding documentation for
pwm-driver authors and lack of time for in-deep reviews by Thierry.
Never the less: The requirement to complete the currently running period
comes from Thierry. There is a patch by me waiting for review that
targets improving the documentation and you already have to suffer from
my plan to spend some time on pwm-reviews :-)

> Anyway, as I could not find any real example I tried to solve it
> according to your earlier suggestion. That is configure duty_cycle=0
> and some time later disable PWM.
> 
> It generates additional problems. The PWM block has a FIFO with four
> slots. Before adding the sample with duty cycle=0 into the FIFO it
> is wise to wait for a free slot in the FIFO. Then when the sample is
> added it may actually happen that the sample is the fourth in the
> FIFO. So it may take up to four periods until we can disable the PWM.
> These four periods can take up to 16s. Hmmm :(

No it cannot. Because if you put a new configuration into the FIFO you
have to block until the requested configuration is active, so it must
not happen that you hit the FIFO with 4 busy entries. (Unless a user
calls pwm_apply_state() in four contexts in parallel, which should not
happen. And if it does, we should implement serialization in the
pwm-framework such that pwm-drivers doesn't need to care.)

> I agree there are bugs in the driver and it is far from providing
> complete support of the i.MX PWM HW. Still, I believe those are totally
> independent problems from the pinctrl stuff and so should not block
> the discussion/inclusion of this series.

I think while there are people who care about a driver, the known
problems should be addressed before a change is "sneaked" in that makes
the contributor happy and care about other stuff.
 
Best regards
Uwe

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

Powered by blists - more mailing lists