[2/5] pwm: Add i.MX TPM PWM driver support

Commit Message

Anson Huang March 11, 2019, 7:16 a.m. UTC

i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
inside, add TPM PWM driver support.

Signed-off-by: Anson Huang <[e-mail]>
---
 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

Comments

Claudiu Beznea March 11, 2019, 8:25 a.m. UTC | #1

On 11.03.2019 09:16, Anson Huang wrote:
> +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,
> +};

The new way is to use atomic PWM APIs, actually to implement pwm_ops::apply

Uwe Kleine-König March 11, 2019, 9:26 a.m. UTC | #2

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 <[e-mail]>
> ---
>  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).

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> +
> +MODULE_AUTHOR("Jacky Bai <[e-mail]>");

Should Jacky Bai the author of this patch?

> +MODULE_DESCRIPTION("i.MX TPM PWM Driver");
> +MODULE_LICENSE("GPL v2");

Best regards
Uwe

Anson Huang March 13, 2019, 7:28 a.m. UTC | #3

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:[e-mail]]
> Sent: 2019年3月11日 17:27
> To: Anson Huang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leonard Crestez <[e-mail]>;
> [e-mail]; [e-mail]; Robin Gong
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; dl-linux-imx <[e-mail]>
> 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 <[e-mail]>
> > ---
> >  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 <[e-mail]>");
> 
> 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  |

Uwe Kleine-König March 13, 2019, 8:32 a.m. UTC | #4

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.

> > > +		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://www.spinics.net/lists/linux-pwm/msg09149.html (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).

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

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

> > > +	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.
 
Best regards
Uwe

Anson Huang March 13, 2019, 8:47 a.m. UTC | #5

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:[e-mail]]
> Sent: 2019年3月13日 16:32
> To: Anson Huang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leonard Crestez <[e-mail]>;
> [e-mail]; [e-mail]; Robin Gong
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; dl-linux-imx <[e-mail]>
> 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  |

Patch

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.
+
 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.
+ */
+
+#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
+#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
+#define PERIOD_DIV_MAX		8
+
+#define TPM_CHn_ADDR_OFFSET	0x8
+#define DEFAULT_PWM_CHANNEL_NUM	2
+
+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
+};
+
+#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);
+	/* calculate the period_cycles and duty_cycles */
+	while (1) {
+		c = rate / prediv[div];
+		c = c * period_ns;
+		do_div(c, 1000000000);
+		if (c < PERIOD_PERIOD_MAX)
+			break;
+		div++;
+		if (div >= 8)
+			return -EINVAL;
+	}
+
+	/* 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_SC);
+	val &= ~TPM_SC_PS_MASK;
+	val |= div;
+	writel(val, tpm->base + TPM_SC);
+
+	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);
+	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;
+	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);
+
+	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);
+}
+
+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,
+};
+
+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);
+
+	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");
+
+	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);
+
+	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);
+
+	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);
+
+MODULE_AUTHOR("Jacky Bai <[e-mail]>");
+MODULE_DESCRIPTION("i.MX TPM PWM Driver");
+MODULE_LICENSE("GPL v2");