diff mbox series

[2/2] pwm: add T-HEAD PWM driver

Message ID 20230928170254.413-3-jszhang@kernel.org (mailing list archive)
State Superseded
Headers show
Series pwm: add driver for T-THEAD TH1520 SoC | expand

Commit Message

Jisheng Zhang Sept. 28, 2023, 5:02 p.m. UTC
T-HEAD SoCs such as the TH1520 contain a PWM controller used
among other things to control the LCD backlight, fan and so on.
Add driver for it.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 MAINTAINERS             |   1 +
 drivers/pwm/Kconfig     |  11 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 302 insertions(+)
 create mode 100644 drivers/pwm/pwm-thead.c

Comments

Emil Renner Berthing Sept. 29, 2023, 11:26 a.m. UTC | #1
Jisheng Zhang wrote:
> T-HEAD SoCs such as the TH1520 contain a PWM controller used
> among other things to control the LCD backlight, fan and so on.
> Add driver for it.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  MAINTAINERS             |   1 +
>  drivers/pwm/Kconfig     |  11 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 302 insertions(+)
>  create mode 100644 drivers/pwm/pwm-thead.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d55e40060c46..86cf0926dbfc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18482,6 +18482,7 @@ L:	linux-riscv@lists.infradead.org
>  S:	Maintained
>  F:	arch/riscv/boot/dts/thead/
>  F:	drivers/usb/dwc3/dwc3-thead.c
> +F:	drivers/pwm/pwm-thead.c
>
>  RNBD BLOCK DRIVERS
>  M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..428fa365a19a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -637,6 +637,17 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>
> +config PWM_THEAD
> +	tristate "T-HEAD PWM support"
> +	depends on ARCH_THEAD || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for the PWFM controller found on THEAD
> +	  SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-thead.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c822389c2a24..4c317e6316e8 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_THEAD)		+= pwm-thead.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> diff --git a/drivers/pwm/pwm-thead.c b/drivers/pwm/pwm-thead.c
> new file mode 100644
> index 000000000000..8339f5617b6f
> --- /dev/null
> +++ b/drivers/pwm/pwm-thead.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * T-HEAD PWM driver
> + *
> + * Copyright (C) 2021 Alibaba Group Holding Limited.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define MAX_PWM_NUM	6
> +
> +#define LIGHT_PWM_CHN_BASE(n)		((n) * 0x20)
> +#define LIGHT_PWM_CTRL(n)		(LIGHT_PWM_CHN_BASE(n) + 0x00)
> +#define LIGHT_PWM_RPT(n)		(LIGHT_PWM_CHN_BASE(n) + 0x04)
> +#define LIGHT_PWM_PER(n)		(LIGHT_PWM_CHN_BASE(n) + 0x08)
> +#define LIGHT_PWM_FP(n)			(LIGHT_PWM_CHN_BASE(n) + 0x0c)
> +#define LIGHT_PWM_STATUS(n)		(LIGHT_PWM_CHN_BASE(n) + 0x10)
> +
> +/* bit definition PWM_CTRL */
> +#define PWM_START				BIT(0)
> +#define PWM_SOFT_RST				BIT(1)
> +#define PWM_CFG_UPDATE				BIT(2)
> +#define PWM_INT_EN				BIT(3)
> +#define PWM_ONE_SHOT_MODE			BIT(4)
> +#define PWM_CONTINUOUS_MODE			BIT(5)
> +#define PWM_EVT_RISING_TRIG_UNDER_ONE_SHOT	BIT(6)
> +#define PWM_EVT_FALLING_TRIG_UNDER_ONE_SHOT	BIT(7)
> +#define PWM_FPOUT				BIT(8)
> +#define PWM_INFACTOUT				BIT(9)

Hi Jisheng,

I'd be worried that these defines one day clash with the PWM framework. Maybe
just keep the LIGHT_PWM_ prefix?

> +
> +struct thead_pwm_chip {
> +	struct clk *clk;
> +	void __iomem *mmio_base;
> +	struct pwm_chip chip;
> +};
> +
> +#define to_thead_pwm_chip(chip)		container_of(chip, struct thead_pwm_chip, chip)
> +
> +static int thead_pwm_clk_prepare_enable(struct thead_pwm_chip *pc)
> +{
> +	return clk_prepare_enable(pc->clk);
> +}
> +
> +static void thead_pwm_clk_disable_unprepare(struct thead_pwm_chip *pc)
> +{
> +	clk_disable_unprepare(pc->clk);
> +}

These two wrappers don't seem to add a lot of value compared to just writing
clk_*(pc->clk) directly and they're not used as callbacks.

> +
> +static int thead_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(chip->dev);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	value |= PWM_START;
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	return 0;
> +}
> +
> +static void thead_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value;
> +
> +	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	value &= ~PWM_START;
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	pm_runtime_put_sync(chip->dev);
> +}
> +
> +static int thead_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    int duty_ns, int period_ns)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	unsigned long rate = clk_get_rate(pc->clk);
> +	unsigned long duty_cycle, period_cycle;
> +	u32 pwm_cfg = PWM_INFACTOUT | PWM_FPOUT | PWM_CONTINUOUS_MODE | PWM_INT_EN;
> +	int ret;
> +
> +	if (duty_ns > period_ns) {
> +		dev_err(chip->dev, "invalid pwm configure\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = pm_runtime_get_sync(chip->dev);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	period_cycle = period_ns * rate;
> +	do_div(period_cycle, NSEC_PER_SEC);

I thought do_div() was only needed to do 64bit by 32bit divisions on
32bit hardware, but here
period_cycle is an unsigned long, so only 32bit on 32bit hardware.

> +	writel(period_cycle, pc->mmio_base + LIGHT_PWM_PER(pwm->hwpwm));
> +
> +	duty_cycle = duty_ns * rate;
> +	do_div(duty_cycle, NSEC_PER_SEC);

Same as above.

> +	writel(duty_cycle, pc->mmio_base + LIGHT_PWM_FP(pwm->hwpwm));
> +
> +	pwm_cfg = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	pwm_cfg |= PWM_CFG_UPDATE;
> +	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	pm_runtime_put_sync(chip->dev);
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_set_polarity(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  enum pwm_polarity polarity)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(chip->dev);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		value |= PWM_FPOUT;
> +	else
> +		value &= ~PWM_FPOUT;
> +
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	pm_runtime_put_sync(chip->dev);
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	int err;
> +	bool enabled = pwm->state.enabled;
> +
> +	if (state->polarity != pwm->state.polarity)
> +		thead_pwm_set_polarity(chip, pwm, state->polarity);
> +
> +	if (!state->enabled) {
> +		if (enabled)
> +			thead_pwm_disable(chip, pwm);
> +		return 0;
> +	}
> +
> +	err = thead_pwm_config(chip, pwm, state->duty_cycle, state->period);
> +	if (err)
> +		return err;
> +
> +	if (!enabled)
> +		return thead_pwm_enable(chip, pwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops thead_pwm_ops = {
> +	.apply = thead_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int __maybe_unused thead_pwm_runtime_suspend(struct device *dev)
> +{
> +	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
> +
> +	thead_pwm_clk_disable_unprepare(pc);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused thead_pwm_runtime_resume(struct device *dev)
> +{
> +	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = thead_pwm_clk_prepare_enable(pc);
> +	if (ret) {
> +		dev_err(dev, "failed to enable pwm clock(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_probe(struct platform_device *pdev)
> +{
> +	struct thead_pwm_chip *pc;
> +	int ret;
> +
> +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pc->mmio_base))
> +		return PTR_ERR(pc->mmio_base);
> +
> +	pc->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pc->clk))
> +		return PTR_ERR(pc->clk);

A lot of other drivers have something like
return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk), "failed to get clock\n");
when failing to get clocks.

> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +	ret = thead_pwm_clk_prepare_enable(pc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable pwm clock(%d)\n", ret);
> +		goto err_pm_disable;
> +	}
> +
> +	pc->chip.ops = &thead_pwm_ops;
> +	pc->chip.dev = &pdev->dev;
> +	pc->chip.npwm = MAX_PWM_NUM;
> +
> +	ret = pwmchip_add(&pc->chip);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	pm_runtime_put(&pdev->dev);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	thead_pwm_clk_disable_unprepare(pc);
> +err_pm_disable:
> +	pm_runtime_disable(&pdev->dev);

Here you disable the clock and then pm_runtime_disable()..

> +	return ret;
> +}
> +
> +static void thead_pwm_remove(struct platform_device *pdev)
> +{
> +	struct thead_pwm_chip *pc = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	thead_pwm_clk_disable_unprepare(pc);

..but here it's the other way around. It may not make a difference, but let's
be consistent anyway.

> +	pwmchip_remove(&pc->chip);
> +}
> +
> +static const struct of_device_id thead_pwm_dt_ids[] = {
> +	{.compatible = "thead,th1520-pwm",},
> +	{/* sentinel */}
> +};
> +
> +static const struct dev_pm_ops thead_pwm_pm_ops = {
> +	SET_RUNTIME_PM_OPS(thead_pwm_runtime_suspend, thead_pwm_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver thead_pwm_driver = {
> +	.driver = {
> +		.name = "thead-pwm",
> +		.of_match_table = thead_pwm_dt_ids,
> +		.pm = &thead_pwm_pm_ops,
> +	},
> +	.probe = thead_pwm_probe,
> +	.remove_new = thead_pwm_remove,
> +};
> +module_platform_driver(thead_pwm_driver);
> +
> +MODULE_AUTHOR("wei.liu <lw312886@linux.alibaba.com>");

wei.liu is a very uncommon name, do you mean Wei Liu?

> +MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
> +MODULE_DESCRIPTION("T-HEAD pwm driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.40.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Uwe Kleine-König Sept. 29, 2023, 3:40 p.m. UTC | #2
Hello,

On Fri, Sep 29, 2023 at 01:02:54AM +0800, Jisheng Zhang wrote:
> T-HEAD SoCs such as the TH1520 contain a PWM controller used
> among other things to control the LCD backlight, fan and so on.
> Add driver for it.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  MAINTAINERS             |   1 +
>  drivers/pwm/Kconfig     |  11 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-thead.c | 289 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 302 insertions(+)
>  create mode 100644 drivers/pwm/pwm-thead.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d55e40060c46..86cf0926dbfc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18482,6 +18482,7 @@ L:	linux-riscv@lists.infradead.org
>  S:	Maintained
>  F:	arch/riscv/boot/dts/thead/
>  F:	drivers/usb/dwc3/dwc3-thead.c
> +F:	drivers/pwm/pwm-thead.c
>  
>  RNBD BLOCK DRIVERS
>  M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..428fa365a19a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -637,6 +637,17 @@ config PWM_TEGRA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tegra.
>  
> +config PWM_THEAD
> +	tristate "T-HEAD PWM support"
> +	depends on ARCH_THEAD || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for the PWFM controller found on THEAD
> +	  SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-thead.
> +
>  config PWM_TIECAP
>  	tristate "ECAP PWM support"
>  	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c822389c2a24..4c317e6316e8 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> +obj-$(CONFIG_PWM_THEAD)		+= pwm-thead.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> diff --git a/drivers/pwm/pwm-thead.c b/drivers/pwm/pwm-thead.c
> new file mode 100644
> index 000000000000..8339f5617b6f
> --- /dev/null
> +++ b/drivers/pwm/pwm-thead.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * T-HEAD PWM driver
> + *
> + * Copyright (C) 2021 Alibaba Group Holding Limited.

If you have a public link to a reference manual, please mention this
here. Also please add a section that describes hardware (and software)
limitations sticking to the format in other drivers. The idea is that

	sed -rn '/Limitations:/,/\*\/?$/p'

tells you about (at least) about the following properties:

 - Can the PWM be updated atomically?
 - Does it complete the current running period before switching to the
   new configuration?
 - How does the output behave when disabled?

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define MAX_PWM_NUM	6
> +
> +#define LIGHT_PWM_CHN_BASE(n)		((n) * 0x20)
> +#define LIGHT_PWM_CTRL(n)		(LIGHT_PWM_CHN_BASE(n) + 0x00)
> +#define LIGHT_PWM_RPT(n)		(LIGHT_PWM_CHN_BASE(n) + 0x04)
> +#define LIGHT_PWM_PER(n)		(LIGHT_PWM_CHN_BASE(n) + 0x08)
> +#define LIGHT_PWM_FP(n)			(LIGHT_PWM_CHN_BASE(n) + 0x0c)
> +#define LIGHT_PWM_STATUS(n)		(LIGHT_PWM_CHN_BASE(n) + 0x10)
> +
> +/* bit definition PWM_CTRL */
> +#define PWM_START				BIT(0)
> +#define PWM_SOFT_RST				BIT(1)
> +#define PWM_CFG_UPDATE				BIT(2)
> +#define PWM_INT_EN				BIT(3)
> +#define PWM_ONE_SHOT_MODE			BIT(4)
> +#define PWM_CONTINUOUS_MODE			BIT(5)
> +#define PWM_EVT_RISING_TRIG_UNDER_ONE_SHOT	BIT(6)
> +#define PWM_EVT_FALLING_TRIG_UNDER_ONE_SHOT	BIT(7)
> +#define PWM_FPOUT				BIT(8)
> +#define PWM_INFACTOUT				BIT(9)

Emil already criticised the naming here (Thanks btw for the review!). I
also want you to use a consistent prefix, but I wonder about "LIGHT_", I
would have expected "THEAD_" matching the driver name?!

> +struct thead_pwm_chip {
> +	struct clk *clk;
> +	void __iomem *mmio_base;
> +	struct pwm_chip chip;
> +};
> +
> +#define to_thead_pwm_chip(chip)		container_of(chip, struct thead_pwm_chip, chip)

This is wrong. Try:

	struct pwm_chip *mychip;
	struct thead_pwm_chip *pc = to_thead_pwm_chip(mychip);

. (Of course you shouldn't name a pwm_chip "mychip", "chip" is fine.
Still this is too ugly.) I suggest a static inline here, ideally one
that has a name starting with "thead_pwm_". thead_pwm_from_chip() comes
to mind.
	
> +static int thead_pwm_clk_prepare_enable(struct thead_pwm_chip *pc)
> +{
> +	return clk_prepare_enable(pc->clk);
> +}
> +
> +static void thead_pwm_clk_disable_unprepare(struct thead_pwm_chip *pc)
> +{
> +	clk_disable_unprepare(pc->clk);
> +}

I agree to Emil here that these wrappers are too thin. (Also you might
get rid of them completely, see below.)

> +static int thead_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(chip->dev);

This function is typically called directly after thead_pwm_config()
which ended with calling pm_runtime_put_sync(). If you put the logic of
.apply() in a single function (or move runtime pm handling in there) you
can prevent some put/get ping-pong.

> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);

Please use %pe for better readable error messages.

> +		return ret;
> +	}
> +
> +	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	value |= PWM_START;
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	return 0;
> +}
> +
> +static void thead_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value;
> +
> +	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	value &= ~PWM_START;
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	pm_runtime_put_sync(chip->dev);
> +}
> +
> +static int thead_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    int duty_ns, int period_ns)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	unsigned long rate = clk_get_rate(pc->clk);
> +	unsigned long duty_cycle, period_cycle;
> +	u32 pwm_cfg = PWM_INFACTOUT | PWM_FPOUT | PWM_CONTINUOUS_MODE | PWM_INT_EN;
> +	int ret;
> +
> +	if (duty_ns > period_ns) {
> +		dev_err(chip->dev, "invalid pwm configure\n");
> +		return -EINVAL;

You can assume that won't happen (once you fixed the implicit type cast
in the call to thead_pwm_config pointed out below).

> +	}
> +
> +	ret = pm_runtime_get_sync(chip->dev);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +
> +	period_cycle = period_ns * rate;

This might overflow. Please use mul_u64_u64_div_u64 here together with
making sure that rate <= NSEC_PER_SEC.

> +	do_div(period_cycle, NSEC_PER_SEC);
> +	writel(period_cycle, pc->mmio_base + LIGHT_PWM_PER(pwm->hwpwm));
> +	duty_cycle = duty_ns * rate;
> +	do_div(duty_cycle, NSEC_PER_SEC);
> +	writel(duty_cycle, pc->mmio_base + LIGHT_PWM_FP(pwm->hwpwm));
> +
> +	pwm_cfg = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	pwm_cfg |= PWM_CFG_UPDATE;
> +	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));

I assume this means period and duty_cycle are updated atomically (i.e.
at no time the hardware is configured to emit a wave that has the new
period but the old duty_cycle)?

Is this needed even if the PWM is currently off and you write PWM_START
a moment later? Can writing PWM_CFG_UPDATE and PWM_START be done in a
single step?

> +	pm_runtime_put_sync(chip->dev);
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_set_polarity(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  enum pwm_polarity polarity)
> +{
> +	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
> +	u32 value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(chip->dev);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);

Please don't make .apply() emit an error message.

> +		return ret;
> +	}
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		value |= PWM_FPOUT;
> +	else
> +		value &= ~PWM_FPOUT;
> +
> +	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));

Oh, no PWM_CFG_UPDATE here? So writing the polarity takes effect
immediately? If so, please note this is the Limitations section I
asked for above.

> +	pm_runtime_put_sync(chip->dev);
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	int err;
> +	bool enabled = pwm->state.enabled;
> +
> +	if (state->polarity != pwm->state.polarity)
> +		thead_pwm_set_polarity(chip, pwm, state->polarity);
> +
> +	if (!state->enabled) {
> +		if (enabled)
> +			thead_pwm_disable(chip, pwm);
> +		return 0;
> +	}
> +
> +	err = thead_pwm_config(chip, pwm, state->duty_cycle, state->period);

state->duty_cycle is an u64, but thead_pwm_config takes an int ...

> +	if (err)
> +		return err;
> +
> +	if (!enabled)
> +		return thead_pwm_enable(chip, pwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops thead_pwm_ops = {
> +	.apply = thead_pwm_apply,

Please implement .get_state() and test with PWM_DEBUG enabled.

> +	.owner = THIS_MODULE,
> +};
> +
> +static int __maybe_unused thead_pwm_runtime_suspend(struct device *dev)
> +{
> +	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
> +
> +	thead_pwm_clk_disable_unprepare(pc);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused thead_pwm_runtime_resume(struct device *dev)
> +{
> +	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = thead_pwm_clk_prepare_enable(pc);
> +	if (ret) {
> +		dev_err(dev, "failed to enable pwm clock(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thead_pwm_probe(struct platform_device *pdev)
> +{
> +	struct thead_pwm_chip *pc;
> +	int ret;
> +
> +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pc->mmio_base))
> +		return PTR_ERR(pc->mmio_base);
> +
> +	pc->clk = devm_clk_get(&pdev->dev, NULL);

devm_clk_get_enabled()

> +	if (IS_ERR(pc->clk))
> +		return PTR_ERR(pc->clk);
> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +	ret = thead_pwm_clk_prepare_enable(pc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable pwm clock(%d)\n", ret);
> +		goto err_pm_disable;
> +	}
> +
> +	pc->chip.ops = &thead_pwm_ops;
> +	pc->chip.dev = &pdev->dev;
> +	pc->chip.npwm = MAX_PWM_NUM;
> +
> +	ret = pwmchip_add(&pc->chip);

devm_pwmchip_add()

> +	if (ret)
> +		goto err_clk_disable;
> +
> +	pm_runtime_put(&pdev->dev);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	thead_pwm_clk_disable_unprepare(pc);
> +err_pm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
> +}

Best regards
Uwe
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d55e40060c46..86cf0926dbfc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18482,6 +18482,7 @@  L:	linux-riscv@lists.infradead.org
 S:	Maintained
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/usb/dwc3/dwc3-thead.c
+F:	drivers/pwm/pwm-thead.c
 
 RNBD BLOCK DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..428fa365a19a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -637,6 +637,17 @@  config PWM_TEGRA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tegra.
 
+config PWM_THEAD
+	tristate "T-HEAD PWM support"
+	depends on ARCH_THEAD || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWFM controller found on THEAD
+	  SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-thead.
+
 config PWM_TIECAP
 	tristate "ECAP PWM support"
 	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c822389c2a24..4c317e6316e8 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -59,6 +59,7 @@  obj-$(CONFIG_PWM_STMPE)		+= pwm-stmpe.o
 obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_SUNPLUS)	+= pwm-sunplus.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
+obj-$(CONFIG_PWM_THEAD)		+= pwm-thead.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
diff --git a/drivers/pwm/pwm-thead.c b/drivers/pwm/pwm-thead.c
new file mode 100644
index 000000000000..8339f5617b6f
--- /dev/null
+++ b/drivers/pwm/pwm-thead.c
@@ -0,0 +1,289 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * T-HEAD PWM driver
+ *
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define MAX_PWM_NUM	6
+
+#define LIGHT_PWM_CHN_BASE(n)		((n) * 0x20)
+#define LIGHT_PWM_CTRL(n)		(LIGHT_PWM_CHN_BASE(n) + 0x00)
+#define LIGHT_PWM_RPT(n)		(LIGHT_PWM_CHN_BASE(n) + 0x04)
+#define LIGHT_PWM_PER(n)		(LIGHT_PWM_CHN_BASE(n) + 0x08)
+#define LIGHT_PWM_FP(n)			(LIGHT_PWM_CHN_BASE(n) + 0x0c)
+#define LIGHT_PWM_STATUS(n)		(LIGHT_PWM_CHN_BASE(n) + 0x10)
+
+/* bit definition PWM_CTRL */
+#define PWM_START				BIT(0)
+#define PWM_SOFT_RST				BIT(1)
+#define PWM_CFG_UPDATE				BIT(2)
+#define PWM_INT_EN				BIT(3)
+#define PWM_ONE_SHOT_MODE			BIT(4)
+#define PWM_CONTINUOUS_MODE			BIT(5)
+#define PWM_EVT_RISING_TRIG_UNDER_ONE_SHOT	BIT(6)
+#define PWM_EVT_FALLING_TRIG_UNDER_ONE_SHOT	BIT(7)
+#define PWM_FPOUT				BIT(8)
+#define PWM_INFACTOUT				BIT(9)
+
+struct thead_pwm_chip {
+	struct clk *clk;
+	void __iomem *mmio_base;
+	struct pwm_chip chip;
+};
+
+#define to_thead_pwm_chip(chip)		container_of(chip, struct thead_pwm_chip, chip)
+
+static int thead_pwm_clk_prepare_enable(struct thead_pwm_chip *pc)
+{
+	return clk_prepare_enable(pc->clk);
+}
+
+static void thead_pwm_clk_disable_unprepare(struct thead_pwm_chip *pc)
+{
+	clk_disable_unprepare(pc->clk);
+}
+
+static int thead_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
+	u32 value;
+	int ret;
+
+	ret = pm_runtime_get_sync(chip->dev);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
+		return ret;
+	}
+
+	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+	value |= PWM_START;
+	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+
+	return 0;
+}
+
+static void thead_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
+	u32 value;
+
+	value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+	value &= ~PWM_START;
+	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+
+	pm_runtime_put_sync(chip->dev);
+}
+
+static int thead_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    int duty_ns, int period_ns)
+{
+	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
+	unsigned long rate = clk_get_rate(pc->clk);
+	unsigned long duty_cycle, period_cycle;
+	u32 pwm_cfg = PWM_INFACTOUT | PWM_FPOUT | PWM_CONTINUOUS_MODE | PWM_INT_EN;
+	int ret;
+
+	if (duty_ns > period_ns) {
+		dev_err(chip->dev, "invalid pwm configure\n");
+		return -EINVAL;
+	}
+
+	ret = pm_runtime_get_sync(chip->dev);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
+		return ret;
+	}
+
+	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+
+	period_cycle = period_ns * rate;
+	do_div(period_cycle, NSEC_PER_SEC);
+	writel(period_cycle, pc->mmio_base + LIGHT_PWM_PER(pwm->hwpwm));
+
+	duty_cycle = duty_ns * rate;
+	do_div(duty_cycle, NSEC_PER_SEC);
+	writel(duty_cycle, pc->mmio_base + LIGHT_PWM_FP(pwm->hwpwm));
+
+	pwm_cfg = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+	pwm_cfg |= PWM_CFG_UPDATE;
+	writel(pwm_cfg, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+
+	pm_runtime_put_sync(chip->dev);
+
+	return 0;
+}
+
+static int thead_pwm_set_polarity(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  enum pwm_polarity polarity)
+{
+	struct thead_pwm_chip *pc = to_thead_pwm_chip(chip);
+	u32 value = readl(pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+	int ret;
+
+	ret = pm_runtime_get_sync(chip->dev);
+	if (ret < 0) {
+		dev_err(chip->dev, "failed to clock on the pwm device(%d)\n", ret);
+		return ret;
+	}
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		value |= PWM_FPOUT;
+	else
+		value &= ~PWM_FPOUT;
+
+	writel(value, pc->mmio_base + LIGHT_PWM_CTRL(pwm->hwpwm));
+
+	pm_runtime_put_sync(chip->dev);
+
+	return 0;
+}
+
+static int thead_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			   const struct pwm_state *state)
+{
+	int err;
+	bool enabled = pwm->state.enabled;
+
+	if (state->polarity != pwm->state.polarity)
+		thead_pwm_set_polarity(chip, pwm, state->polarity);
+
+	if (!state->enabled) {
+		if (enabled)
+			thead_pwm_disable(chip, pwm);
+		return 0;
+	}
+
+	err = thead_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (err)
+		return err;
+
+	if (!enabled)
+		return thead_pwm_enable(chip, pwm);
+
+	return 0;
+}
+
+static const struct pwm_ops thead_pwm_ops = {
+	.apply = thead_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static int __maybe_unused thead_pwm_runtime_suspend(struct device *dev)
+{
+	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
+
+	thead_pwm_clk_disable_unprepare(pc);
+
+	return 0;
+}
+
+static int __maybe_unused thead_pwm_runtime_resume(struct device *dev)
+{
+	struct thead_pwm_chip *pc = dev_get_drvdata(dev);
+	int ret;
+
+	ret = thead_pwm_clk_prepare_enable(pc);
+	if (ret) {
+		dev_err(dev, "failed to enable pwm clock(%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int thead_pwm_probe(struct platform_device *pdev)
+{
+	struct thead_pwm_chip *pc;
+	int ret;
+
+	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
+	if (!pc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pc);
+
+	pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pc->mmio_base))
+		return PTR_ERR(pc->mmio_base);
+
+	pc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pc->clk))
+		return PTR_ERR(pc->clk);
+
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	ret = thead_pwm_clk_prepare_enable(pc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable pwm clock(%d)\n", ret);
+		goto err_pm_disable;
+	}
+
+	pc->chip.ops = &thead_pwm_ops;
+	pc->chip.dev = &pdev->dev;
+	pc->chip.npwm = MAX_PWM_NUM;
+
+	ret = pwmchip_add(&pc->chip);
+	if (ret)
+		goto err_clk_disable;
+
+	pm_runtime_put(&pdev->dev);
+
+	return 0;
+
+err_clk_disable:
+	thead_pwm_clk_disable_unprepare(pc);
+err_pm_disable:
+	pm_runtime_disable(&pdev->dev);
+	return ret;
+}
+
+static void thead_pwm_remove(struct platform_device *pdev)
+{
+	struct thead_pwm_chip *pc = platform_get_drvdata(pdev);
+
+	pm_runtime_disable(&pdev->dev);
+	thead_pwm_clk_disable_unprepare(pc);
+	pwmchip_remove(&pc->chip);
+}
+
+static const struct of_device_id thead_pwm_dt_ids[] = {
+	{.compatible = "thead,th1520-pwm",},
+	{/* sentinel */}
+};
+
+static const struct dev_pm_ops thead_pwm_pm_ops = {
+	SET_RUNTIME_PM_OPS(thead_pwm_runtime_suspend, thead_pwm_runtime_resume, NULL)
+};
+
+static struct platform_driver thead_pwm_driver = {
+	.driver = {
+		.name = "thead-pwm",
+		.of_match_table = thead_pwm_dt_ids,
+		.pm = &thead_pwm_pm_ops,
+	},
+	.probe = thead_pwm_probe,
+	.remove_new = thead_pwm_remove,
+};
+module_platform_driver(thead_pwm_driver);
+
+MODULE_AUTHOR("wei.liu <lw312886@linux.alibaba.com>");
+MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
+MODULE_DESCRIPTION("T-HEAD pwm driver");
+MODULE_LICENSE("GPL v2");