Message-ID: <AM6PR0402MB3911FD60ACBF99217BF526B2F54A0@AM6PR0402MB3911.eurprd04.prod.outlook.com>
Date:   Wed, 13 Mar 2019 08:47:16 +0000
From:   Anson Huang <anson.huang@....com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        Leonard Crestez <leonard.crestez@....com>,
        "schnitzeltony@...il.com" <schnitzeltony@...il.com>,
        "jan.tuerk@...rion.com" <jan.tuerk@...rion.com>,
        Robin Gong <yibin.gong@....com>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 2/5] pwm: Add i.MX TPM PWM driver support

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@...gutronix.de]
> Sent: 2019年3月13日 16:32
> To: Anson Huang <anson.huang@....com>
> Cc: thierry.reding@...il.com; robh+dt@...nel.org; mark.rutland@....com;
> shawnguo@...nel.org; s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com; linux@...linux.org.uk; stefan@...er.ch;
> otavio@...ystems.com.br; Leonard Crestez <leonard.crestez@....com>;
> schnitzeltony@...il.com; jan.tuerk@...rion.com; Robin Gong
> <yibin.gong@....com>; linux-pwm@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello,
> 
> On Wed, Mar 13, 2019 at 07:28:24AM +0000, Anson Huang wrote:
> > > On Mon, Mar 11, 2019 at 07:16:16AM +0000, Anson Huang wrote:
> > > > +
> > > >  config PWM_JZ4740
> > > >  	tristate "Ingenic JZ47xx PWM support"
> > > >  	depends on MACH_INGENIC
> > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > > > 9c676a0..64e036c 100644
> > > > --- a/drivers/pwm/Makefile
> > > > +++ b/drivers/pwm/Makefile
> > > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-
> ftm.o
> > > >  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
> > > >  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> > > >  obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
> > > > +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> > > >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> > > >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> > > >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-
> tpm.c
> > > new
> > > > file mode 100644 index 0000000..a53256a
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-imx-tpm.c
> > > > @@ -0,0 +1,277 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright 2018-2019 NXP.
> > > > + */
> > >
> > > Please add a link to the reference manual to the header.
> >
> > I checked the NXP website, looks like i.MX7ULP reference manual is NOT
> > published yet, should be published very soon.
> 
> ok.
> 
> > > > +#include <linux/bitops.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/platform_device.h> #include <linux/pwm.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +#define TPM_GLOBAL	0x8
> > > > +#define TPM_SC		0x10
> > > > +#define TPM_CNT		0x14
> > > > +#define TPM_MOD		0x18
> > > > +#define TPM_C0SC	0x20
> > > > +#define TPM_C0V		0x24
> > > > +
> > > > +#define SC_CMOD		3
> > >
> > > This seems to be an offset, why not defining it as BIT(3) instead?
> >
> > SC_CMOD field as 2 bits, I will use below in V2 patch set:
> >
> > #define TPM_SC_CMOD_SHIFT       3
> > #define TPM_SC_CMOD_MASK        (0x3 << TPM_SC_CMOD_SHIFT)
> 
> I suggest
> 
> 	#define PWM_IMX_TPM_SC_CMOD	GENMASK(1, 0)
> 
> instead. See include/linux/bitfield.h for details.
> Also note that "TPM" isn't a great prefix given that we have:
> 
> 	$ git ls-files | grep -c tpm
> 	82
> 
> . Even though PWM_IMX_TPM is longer I think it's worth the extra bytes.
> Then if you use pwm_imx_tpm als prefix for your function it's obvious that
> they all belong together. For extra credits also adapt the driver name to
> match.

OK, I will improve it accordingly.

> 
> > > > +		ret = clk_prepare_enable(tpm->clk);
> > > > +		if (ret) {
> > > > +			dev_err(chip->dev,
> > > > +				"failed to prepare or enable clk %d\n", ret);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	val = readl(tpm->base + TPM_SC);
> > > > +	val &= ~TPM_SC_PS_MASK;
> > > > +	val |= div;
> > > > +	writel(val, tpm->base + TPM_SC);
> > >
> > > If the unit ran with (say) div == 5 and a high duty cycle before and
> > > for the new configuration you need div == 6 with a low duty cycle,
> > > can it happen here that the output uses the new div value already with
> the high duty cycle?
> > > If so, this is bad.
> >
> > As the TPM counter is shared between 2 channels, so I will make this
> > prescale setting to be initialized ONLY once, ONLY first channel will
> > config it, this makes things simple and the other channel should use
> > same prescale value as first channel and ONLY adjust its own duty cycle and
> polarity etc..
> 
> So the two channels have to share the period length? If so please note this in
> the header of the driver like it was done in
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.spinics.net%2Flists%2Flinux-
> pwm%2Fmsg09149.html&amp;data=02%7C01%7Canson.huang%40nxp.com
> %7Cf5187311ac734acb97f708d6a78e770c%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C636880627658454374&amp;sdata=bgBRXVABZJxNlKuw
> DZ5J7GtQA1cvmMfhHvlEzHldbSI%3D&amp;reserved=0 (search for
> "Limitations"). You also need some logic to assert that setting the 2nd
> channel doesn't modify the first (if the first is in use).
> 

OK, I will add limitations at top of file.

> > > > +	period_cycles = c;
> > > > +	c *= duty_ns;
> > > > +	do_div(c, period_ns);
> > > > +	duty_cycles = c;
> > > > +
> > > > +	writel(period_cycles & TPM_MOD_MOD_MASK, tpm->base +
> > > TPM_MOD);
> > >
> > > Don't you need to add pwm->hwpwm * TPM_CHn_ADDR_OFFSET to the
> > > register offset here? And if not, I assume this affects the other
> > > PWMs provided by this hardware unit which is bad.
> >
> > The MOD register is shared between different channels, ONLY 1 register
> there.
> > This register will be same as SC, ONLY configured ONCE by first channel.
> >
> > >
> > > > +	writel(duty_cycles & TPM_MOD_MOD_MASK, tpm->base +
> > > > +	       TPM_C0V + pwm->hwpwm * TPM_CHn_ADDR_OFFSET);
> > > > +
> > > > +	/* if pwm is not enabled, disable clk after setting */
> > > > +	if (!pwm_is_enabled(pwm))
> > > > +		clk_disable_unprepare(tpm->clk);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int tpm_pwm_enable(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > > +*pwm) {
> > > > +	struct tpm_pwm_chip *tpm = to_tpm_pwm_chip(chip);
> > > > +	int ret;
> > > > +	u32 val;
> > > > +
> > > > +	ret = clk_prepare_enable(tpm->clk);
> > > > +	if (ret) {
> > > > +		dev_err(chip->dev,
> > > > +			"failed to prepare or enable clk %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * To enable a tpm channel, CPWMS = 0, MSnB:MSnA = 0x0,
> > > > +	 * for TPM normal polarity ELSnB:ELSnA = 2b'10,
> > > > +	 * inverse ELSnB:ELSnA = 2b'01
> > > > +	 */
> > > > +	val = readl(tpm->base + TPM_C0SC + pwm->hwpwm *
> > > > +TPM_CHn_ADDR_OFFSET);
> 
> You could make it obvious which channels are shared and which have one
> instance per pwm by doing:
> 
> 	#define PWM_IMX_TPM_C0SC(hwid)	(0x20 + hwid * 4)

Looks good, I will use macro to improve it.

> 
> > > > +	val |= pwm->state.polarity ? ELSnA : ELSnB;
> > > > +
> > > > +	writel(val, tpm->base + TPM_C0SC + pwm->hwpwm *
> > > > +TPM_CHn_ADDR_OFFSET);
> > > > +
> > > > +	/* start the counter */
> > > > +	val = readl(tpm->base + TPM_SC);
> > > > +	val |= 0x1 << SC_CMOD;
> > > > +	writel(val, tpm->base + TPM_SC);
> > >
> > > If tpm_pwm_enable is called for the first PWM provided by the
> > > hardware, how does this writel affect the second one?
> >
> > I will make the TPM counter enabled for every channel enabled, if ONLY
> > disabled when both channels are disabled.
> 
> So you have to make sure that:
> 
>  - if you disable one channel while the other is still running, just set
>    the duty cycle to zero to not interfere with the other
>  - if you enable one channel while the other is still off/unused, make
>    sure that other channel doesn't start to wiggle.

ONLY setting duty cycle is NOT enough I think, I saw the .config will be called
and duty cycle is configured, ONCE duty cycle is configured, the PWM channel
will start output signals, the TPM hardware has polarity setting register, when
they are clear, the channel will be disabled, so below is what I did in V2:

if one channel is disabled, the polarity will be set to 0 to disable the channel, and
the config will be saved. And config will be restored if channel got enabled again.
And if all channels are disabled, the TPM counter will be stopped, if any channel
is active, TPM counter will remain running.

> 
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int tpm_pwm_remove(struct platform_device *pdev) {
> > > > +	struct tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> > > > +
> > > > +	return pwmchip_remove(&tpm->chip); }
> > > > +
> > > > +static int __maybe_unused tpm_pwm_suspend(struct device *dev) {
> > > > +	struct tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > > > +
> > > > +	clk_disable_unprepare(tpm->clk);
> > >
> > > If the PWM is in use, it shouldn't stop on suspend.
> >
> > I think PWM should be disabled when suspend, if a device is suspended,
> > but PWM is still enabled, we will see backlight is enabled. This is
> > weird. Unless the backlight driver will guarantee that pwm is disabled
> before suspend?
> 
> No, on suspend it's the responsibility of the backlight driver to disable the
> pwm. Otherwise the PWM changes its behaviour without the consumer's
> consent which depending on the purpose of the PWM is bad.

So, pwm's consumer will disable the PWM channel when suspend, since the PWM
clock is enabled after probed and kept always enabled, then disable it in PWM driver
also make sense? As all consumers will disable the PWM channels before PWM driver
suspend?

Making clock PWM clock always enabled after probed is to make register write/read simple,
otherwise, we have to add check and timeout for every TPM register write, if enable the clock,
do register write and disable the clock immediately, I saw register value is NOT changed at all,
I guess the TPM clock is too slow compared with ARM clock. 

Can you review the V2 patch set and provide comments, then I will fix them together and sending
A V3.

Thanks.
Anson

> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7
> Cf5187311ac734acb97f708d6a78e770c%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636880627658454374&amp;sdata=rpA%2F19njTiaYKcTTs
> MwzexpTnU7wg5j2PVjWC4OuOdE%3D&amp;reserved=0  |

Powered by blists - more mailing lists