Message-ID: <AM6PR0402MB391160F820907ABE8ECA4C26F54A0@AM6PR0402MB3911.eurprd04.prod.outlook.com>
Date:   Wed, 13 Mar 2019 07:28:24 +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月11日 17:27
> 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 Mon, Mar 11, 2019 at 07:16:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@....com>
> > ---
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-imx-tpm.c | 277
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 287 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..23839ad 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,15 @@ config PWM_IMX
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > +	tristate "i.MX TPM PWM support"
> > +	depends on ARCH_MXC
> > +	help
> > +	  Generic PWM framework driver for i.MX TPM.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-imx-tpm.
> 
> On which imx platforms does this hardware unit exist? I'd say it makes sense
> to mention this in the help text and even restrict availability to enable this
> driver to these platforms. This ensures that the help text stays helpful
> (because a reviewer will spot it if the restriction is lifted but the help text isn't
> adapted accordingly).

Agreed, I will mentioned the TPM is for i.MX7ULP in help txt.

> 
> > +
> >  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.

> 
> > +
> > +#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)

> 
> > +#define SC_CPWMS	BIT(5)
> > +#define MSnB		BIT(5)
> > +#define MSnA		BIT(4)
> > +#define ELSnB		BIT(3)
> > +#define ELSnA		BIT(2)
> > +
> > +#define TPM_SC_PS_MASK		0x7
> > +#define TPM_MOD_MOD_MASK	0xffff
> > +
> > +#define PERIOD_PERIOD_MAX	0x10000
> 
> I bet the maximum period is 0xffff, so this name is irritating.

It is actually for setting max count for TPM counter, will use below in V2 patch set:

#define TPM_COUNT_MAX           0xffff

> 
> > +#define PERIOD_DIV_MAX		8
> > +
> > +#define TPM_CHn_ADDR_OFFSET	0x8
> > +#define DEFAULT_PWM_CHANNEL_NUM	2
> 
> Please add a common prefix to all these defines. Also it should be obvious to
> which register a certain bit mask belongs to.

OK, I will use TPM_ as prefix for all definition.

> 
> > +struct tpm_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +};
> > +
> > +static const unsigned int prediv[8] = {
> > +	1, 2, 4, 8, 16, 32, 64, 128
> > +};
> 
> Given that prediv[i] == 1 << i I wonder if we really need this array.

Yes, removed in V2 patch set.

> 
> > +#define to_tpm_pwm_chip(_chip)	container_of(_chip, struct
> tpm_pwm_chip, chip)
> > +
> > +static int tpm_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +			  int duty_ns, int period_ns)
> > +{
> > +	struct tpm_pwm_chip *tpm = to_tpm_pwm_chip(chip);
> > +	unsigned int period_cycles, duty_cycles;
> > +	unsigned long rate;
> > +	u32 val, div = 0;
> > +	u64 c;
> > +	int ret;
> > +
> > +	rate = clk_get_rate(tpm->clk);
> 
> You must not call clk_get_rate if the clock is off.

I will keep clock enabled after driver probed and ONLY be disabled after suspend,
since many of TPM registers need clock to be ON for writing register and sync, disabling
clock too soon could cause register writing NOT completed, and adding check to make sure
each register write introduced too many ugly code. 

> 
> > +	/* calculate the period_cycles and duty_cycles */
> > +	while (1) {
> > +		c = rate / prediv[div];
> > +		c = c * period_ns;
> 
> You're loosing precision here. Consider:
> 
> 	div = 2
> 	rate = 33333335
> 	period_ns = 30000000
> 
> Then you have c == 249999990000000 while the exact value is
> 250000012500000 which you achieve if you multiply with period_ns before
> dividing by prediv[div].
> 
> > +		do_div(c, 1000000000);
> 
> NSEC_PER_SEC instead of 1000000000?
> 
> > +		if (c < PERIOD_PERIOD_MAX)
> > +			break;
> > +		div++;
> > +		if (div >= 8)
> > +			return -EINVAL;
> > +	}
> 
> I'm sure div can be calculated without a loop.

Yes, ilog2(roundup_pow_of_two(val)) can be used for this case, thanks for reminder.

> 
> > +	/* enable the clock before writing the register */
> > +	if (!pwm_is_enabled(pwm)) {
> 
> If the unit is disabled, there is no need to actually configure period and
> duty_cycle. Also if I understand correctly this might start the PWM with
> whatever configuration was applied before which shouldn't happen.

Will remove all these check.

> 
> > +		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..

> 
> > +	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);
> > +	val &= ~(MSnB | MSnA | ELSnB | ELSnA);
> > +	val |= MSnB;
> 
> If you set MSnB unconditionally, you don't need to clear it first.

OK.

> 
> > +	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.

> 
> > +	return 0;
> > +}
> > +
> > +static void tpm_pwm_disable(struct pwm_chip *chip, struct pwm_device
> > +*pwm) {
> > +	struct tpm_pwm_chip *tpm = to_tpm_pwm_chip(chip);
> > +
> > +	clk_disable_unprepare(tpm->clk);
> 
> I assume this interrupts the currently running period immediately?! If so, this
> is wrong. What is the resulting pin state if the clk is disabled? Does it freeze
> (i.e. just stops on the level where it happend to be) or does it get inactive? If
> the latter: Does it get inactive for both possible polarities?

Will make clock enabled after probed and ONLY disabled when suspend, as read/write
registers need clock to be ON for acknowledge.

> 
> > +}
> > +
> > +static int tpm_pwm_set_polarity(struct pwm_chip *chip,
> > +				    struct pwm_device *pwm,
> > +				    enum pwm_polarity polarity)
> > +{
> > +	struct tpm_pwm_chip *tpm = to_tpm_pwm_chip(chip);
> > +	int ret;
> > +	u32 val;
> > +
> > +	/* enable the clock before writing the register */
> > +	if (!pwm_is_enabled(pwm)) {
> > +		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_C0SC + pwm->hwpwm *
> TPM_CHn_ADDR_OFFSET);
> > +	val &= ~(ELSnB | ELSnA);
> > +	val |= pwm->state.polarity ? ELSnA : ELSnB;
> > +	writel(val, tpm->base + TPM_C0SC + pwm->hwpwm *
> > +TPM_CHn_ADDR_OFFSET);
> > +
> > +	/* disable the clock after writing the register */
> > +	if (!pwm_is_enabled(pwm))
> > +		clk_disable_unprepare(tpm->clk);
> > +
> > +	return  0;
> > +}
> > +
> > +static const struct pwm_ops tpm_pwm_ops = {
> > +	.config		= tpm_pwm_config,
> > +	.enable		= tpm_pwm_enable,
> > +	.disable	= tpm_pwm_disable,
> > +	.set_polarity	= tpm_pwm_set_polarity,
> > +	.owner		= THIS_MODULE,
> 
> Please implement .apply instead of .config, .enable, .disable and .set_polarity.
> 
> Also you align the = here but in other structs (e.g.
> tpm_pwm_driver.driver) you don't. It's a bit a question of personal
> preference what you do, but it should be consistent in a driver. If you ask me,
> just use a single space before the assignment. Otherwise you either have to
> touch the whole struct when a new and longer member is added, or it looks
> really ugly afterwards because only some members have a commonly aligned
> assignment.

Fixed.

> 
> > +};
> > +
> > +static int tpm_pwm_probe(struct platform_device *pdev) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct tpm_pwm_chip *tpm;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> > +	if (!tpm)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	tpm->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(tpm->base))
> > +		return PTR_ERR(tpm->base);
> > +
> > +	tpm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(tpm->clk))
> > +		return PTR_ERR(tpm->clk);
> 
> Please add a dev_err if devm_ioremap_resource or devm_clk_get fails,
> unless the problem is EPROBE_DEFER.

OK, will add check and print.

> 
> > +	tpm->chip.dev = &pdev->dev;
> > +	tpm->chip.ops = &tpm_pwm_ops;
> > +	tpm->chip.base = -1;
> > +	tpm->chip.npwm = DEFAULT_PWM_CHANNEL_NUM;
> > +
> > +	/* init pwm channel number if "fsl,pwm-number" is found in DT */
> > +	ret = of_property_read_u32(np, "fsl,pwm-number", &tpm-
> >chip.npwm);
> > +	if (ret)
> > +		dev_warn(&pdev->dev, "two pwm channels by default\n");
> 
> @Thierry: Can we please agree on a non-vendor specific property here?

After further check, the i.MX7ULP TPM ONLY has 2 channels, so I will make it
to be 2 by default, so no need to read it from DT now.

> 
> > +	ret = pwmchip_add(&tpm->chip);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, tpm);
> 
> If you do this earlier---just after allocating the memory is fine---you can
> simplify this a bit to:
> 
> 	ret = pwmchip_add(&tpm->chip);
> 	if (ret)
> 		dev_err(&pdev->dev, "Failed to add pwm chip (%d)\n", ret);
> 
> 	return ret;
> 

Thanks, it is nice.

> > +
> > +	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?

> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tpm_pwm_resume(struct device *dev) {
> > +	struct tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(tpm->clk);
> > +	if (ret) {
> > +		dev_err(dev, "could not prepare or enable tpm clock\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +};
> > +
> > +static SIMPLE_DEV_PM_OPS(tpm_pwm_pm,
> > +		 tpm_pwm_suspend, tpm_pwm_resume);
> > +
> > +static const struct of_device_id tpm_pwm_dt_ids[] = {
> > +	{ .compatible = "fsl,imx-tpm-pwm", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, tpm_pwm_dt_ids);
> > +
> > +static struct platform_driver tpm_pwm_driver = {
> > +	.driver = {
> > +		.name = "tpm-pwm",
> > +		.of_match_table = tpm_pwm_dt_ids,
> > +		.pm = &tpm_pwm_pm,
> > +	},
> > +	.probe	= tpm_pwm_probe,
> > +	.remove = tpm_pwm_remove,
> > +};
> > +module_platform_driver(tpm_pwm_driver);
> 
> The filename is pwm-imx-tpm. It would be great if this would relate to the
> driver name and the used function prefix.

Will add imx_ prefix for all function name.

> 
> > +
> > +MODULE_AUTHOR("Jacky Bai <ping.bai@....com>");
> 
> Should Jacky Bai the author of this patch?

Now, I almost re-write this driver for sending out, so I think I can change it to
my name, Jacky is the original owner of PWM module in NXP's internal tree, but
the code is NOT good enough for upstream, so I re-write it now.

Thanks for detail review, will send out a V2 patch set which is big different from V1.

Anson.

> 
> > +MODULE_DESCRIPTION("i.MX TPM PWM Driver"); MODULE_LICENSE("GPL
> v2");
> 
> 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
> C829e60eef7f14301581208d6a603b7ef%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636878932244461230&amp;sdata=C68Yz8qcz0iu7rw%2Fw
> uKCYB6cc3rMCjoUcOcEfzvJ%2BMM%3D&amp;reserved=0  |

Powered by blists - more mailing lists