Re: [PATCH v3 3/6] pwm: imx: support output polarity inversion

[Boris Brezillon]

On Fri, 21 Oct 2016 23:49:39 +0200
Lukasz Majewski <l.majewski@xxxxxxxxx> wrote:

> Hi Stefan,
> 
> > On 2016-10-20 01:30, Lukasz Majewski wrote:  
> > > Hi Stefan,
> > >   
> > >> Hi Stefan,
> > >>  
> > >> > On 2016-10-12 15:15, Lukasz Majewski wrote:  
> > >> > > Hi Stefan,
> > >> > >  
> > >> > >> On 2016-10-07 08:11, Bhuvanchandra DV wrote:  
> > >> > >> > From: Lothar Wassmann <LW@xxxxxxxxxxxxxxxxxxx>
> > >> > >> >
> > >> > >> > The i.MX pwm unit on i.MX27 and newer SoCs provides a
> > >> > >> > configurable output polarity. This patch adds support to
> > >> > >> > utilize this feature where available.
> > >> > >> >
> > >> > >> > Signed-off-by: Lothar WaÃmann <LW@xxxxxxxxxxxxxxxxxxx>
> > >> > >> > Signed-off-by: Lukasz Majewski <l.majewski@xxxxxxxxxxx>
> > >> > >> > Signed-off-by: Bhuvanchandra DV
> > >> > >> > <bhuvanchandra.dv@xxxxxxxxxxx> Acked-by: Shawn Guo
> > >> > >> > <shawn.guo@xxxxxxxxxx> Reviewed-by: Sascha Hauer
> > >> > >> > <s.hauer@xxxxxxxxxxxxxx> ---
> > >> > >> >  Documentation/devicetree/bindings/pwm/imx-pwm.txt |  6 +--
> > >> > >> >  drivers/pwm/pwm-imx.c                             | 51
> > >> > >> > +++++++++++++++++++++-- 2 files changed, 51 insertions(+), 6
> > >> > >> > deletions(-)
> > >> > >> >
> > >> > >> > diff --git
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt index
> > >> > >> > e00c2e9..c61bdf8 100644 ---
> > >> > >> > a/Documentation/devicetree/bindings/pwm/imx-pwm.txt +++
> > >> > >> > b/Documentation/devicetree/bindings/pwm/imx-pwm.txt @@ -6,8
> > >> > >> > +6,8 @@ Required properties:
> > >> > >> >    - "fsl,imx1-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX1
> > >> > >> >    - "fsl,imx27-pwm" for PWM compatible with the one
> > >> > >> > integrated on i.MX27
> > >> > >> >  - reg: physical base address and length of the controller's
> > >> > >> > registers -- #pwm-cells: should be 2. See pwm.txt in this
> > >> > >> > directory for a description of
> > >> > >> > -  the cells format.
> > >> > >> > +- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs.
> > >> > >> > See pwm.txt
> > >> > >> > +  in this directory for a description of the cells format.
> > >> > >> >  - clocks : Clock specifiers for both ipg and per clocks.
> > >> > >> >  - clock-names : Clock names should include both "ipg" and
> > >> > >> > "per" See the clock consumer binding,
> > >> > >> > @@ -17,7 +17,7 @@ See the clock consumer binding,
> > >> > >> >  Example:
> > >> > >> >
> > >> > >> >  pwm1: pwm@53fb4000 {
> > >> > >> > -	#pwm-cells = <2>;
> > >> > >> > +	#pwm-cells = <3>;
> > >> > >> >  	compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
> > >> > >> >  	reg = <0x53fb4000 0x4000>;
> > >> > >> >  	clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
> > >> > >> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > >> > >> > index d600fd5..c37d223 100644
> > >> > >> > --- a/drivers/pwm/pwm-imx.c
> > >> > >> > +++ b/drivers/pwm/pwm-imx.c
> > >> > >> > @@ -38,6 +38,7 @@
> > >> > >> >  #define MX3_PWMCR_DOZEEN		(1 << 24)
> > >> > >> >  #define MX3_PWMCR_WAITEN		(1 << 23)
> > >> > >> >  #define MX3_PWMCR_DBGEN			(1 << 22)
> > >> > >> > +#define MX3_PWMCR_POUTC			(1 << 18)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG_HIGH	(2 << 16)
> > >> > >> >  #define MX3_PWMCR_CLKSRC_IPG		(1 << 16)
> > >> > >> >  #define MX3_PWMCR_SWR			(1 << 3)
> > >> > >> > @@ -180,6 +181,9 @@ static int imx_pwm_config_v2(struct
> > >> > >> > pwm_chip *chip, if (enable)
> > >> > >> >  		cr |= MX3_PWMCR_EN;
> > >> > >> >
> > >> > >> > +	if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > >> > >> > +		cr |= MX3_PWMCR_POUTC;
> > >> > >> > +  
> > >> > >>
> > >> > >> This seems wrong to me, the config callback is meant for
> > >> > >> period/duty cycle only.  
> > > 
> > > Unfortunately, it also resets the PWM IP block and setups it again
> > > (by writing to PWMCR register). In that function we setup for
> > > example MX3_PWMCR_DOZEEN
> > > and MX3_PWMCR_DBGEN. Why cannot we setup polarity as well?
> > > 
> > > 
> > > I've double checked the backlight and pwm code flow.
> > > 
> > > Please find following snippet:
> > > 
> > > [    0.135545] ######### imx_pwm_probe
> > > [    0.135581] PWM supports output inversion
> > > [    0.136864] ######### pwm_backlight_probe
> > > [    0.136913] backlight supply power not found, using dummy
> > > regulator [    0.136984] ######### imx_pwm_set_polarity 1
> > > [    0.136995] imx_pwm_set_polarity: polarity set to inverted cr:
> > > 0x40000 0xf08f8000
> > > [    0.137005] #########0 imx_pwm_config_v2 cr: 0x40000 
> > > [    0.137683] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> > > [    0.137693] #########2 imx_pwm_config_v2 cr: 0x1c20050
> > > [    0.137702] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> > > [    0.137711] @@@@@@@@@@ pwm_apply_state  
> 
> Maybe a bit more logs:
> 
> [    0.135451] ######### imx_pwm_probe
> [    0.135488] PWM supports output inversion
> [    0.136777] ######### pwm_backlight_probe
> [    0.136826] backlight supply power not found, using dummy regulator
> [    0.136893] ********* pwm_apply_state state->enabled: 0
> [    0.136902] ######### imx_pwm_set_polarity 1
> [    0.136913] imx_pwm_set_polarity: polarity set to inverted cr: 0x40000 0xf08f8000
> [    0.136923] #########0 imx_pwm_config_v2 cr: 0x40000
> [    0.137692] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.137701] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.137710] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.137720] @@@@@@@@@@ pwm_apply_state
> [    0.137856] ********* pwm_apply_state state->enabled: 0
> [    0.137869] #########0 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138904] #########1 imx_pwm_config_v2 cr: 0x0 0xf08f8000
> [    0.138913] #########2 imx_pwm_config_v2 cr: 0x1c20050
> [    0.138921] #########3 imx_pwm_config_v2 cr: 0x1c20050 0xf08f8000
> [    0.138928] @@@@@@@@@@ pwm_apply_state
> [    0.138940] ********* pwm_apply_state state->enabled: 1
> 					 ^^^^^^^^^^^^^^^^^^ this is called from
> 			pwm_backlight_power_on() from pwm_bl probe function
> 
> The problem here is not the lack of ->apply() callback, but the requirement to
> perform software reset on the pwm_v2 fifo when the pwm_v2 is NOT enabled (state->enabled: 0).
> 
> As fair as I can see the pwm_state has following members: period, duty cycle, polarity and enabled.
> I'm fine to implement ->apply() callback, which would change above values.
> 
> However, there is a problem with ->config() (imx_pwm_config_v2 @ pwm-imx.c) and imx pwm_v2 software
> FIFO reset.
> We can set polarity in any other kernel subsystem, which uses PWM (backlight in this example) and 
> then this setting would disappear when we call pwm_apply_state with state->enabled = 0 (as presented
> in the log). This imposes setting polarity at ->config when we enable the PWM (as this patch does).
> 
> 
> 
> > > 
> > > Here the pwm_backlight_probe calls set_polarity callback available
> > > in pwm - the polarity is set (the 0x40000 value).
> > > 
> > > The above operation is performed in pwm_apply_state (@
> > > drivers/pwm/core.c). In the same function, latter we call the
> > > pwm->chip->ops->config(), which is the pointer to config_v2.
> > > Since the PWM is not yet enabled, this function performs SW reset
> > > and PWM inversion setting is cleared.  
> > 
> > That function should not do that.   
> 
> I do agree that it shouldn't. Correct me if I'm wrong, but it seems like an 
> PWM HW requirement to perform the reset.
> 
> >It was probably already problematic
> > in the old times, it is definitely now with the atomic PWM stuff.  
> 
> The "atomic"[*] code (with ->apply() provided) will not solve this issue.
> 
> >   
> > > 
> > > Possible solutions:
> > > 
> > > 1. Leave the original patch from Bhuvanchandra as it was (I'm for
> > > this option)  
> > 
> > That really seems like a hack to me, and makes transition to the
> > atomic PWM API more complex.  
> 
> Could you be more specific here? 
> 
> As I mentioned before, the problem is not with the lack of
> "atomic" API.

Below is a quick and dirty I made on top of this patch to show you how
atomic update can be implemented in this driver. It's not tested, and
probably not working, but it should give you a better idea of what is
expected.

--->8---