[2/5] pwm: Add support for the MSTAR MSC313 PWM

From: Daniel Palmer <daniel@0x0f.com>

This adds support for the PWM block on the Mstar MSC313e SoCs and newer.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
Co-developed-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |  10 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-msc313e.c | 242 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+)
 create mode 100644 drivers/pwm/pwm-msc313e.c

Hello,

On Wed, Jun 15, 2022 at 09:08:10AM +0200, Romain Perier wrote:
> From: Daniel Palmer <daniel@0x0f.com>
> 
> This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> Co-developed-by: Romain Perier <romain.perier@gmail.com>
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-msc313e.c | 242 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 254 insertions(+)
>  create mode 100644 drivers/pwm/pwm-msc313e.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2316278d9db9..45d001643b93 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2389,6 +2389,7 @@ F:	arch/arm/mach-mstar/
>  F:	drivers/clk/mstar/
>  F:	drivers/clocksource/timer-msc313e.c
>  F:	drivers/gpio/gpio-msc313.c
> +F:	drivers/pwm/pwm-msc313e.c
>  F:	drivers/rtc/rtc-msc313.c
>  F:	drivers/watchdog/msc313e_wdt.c
>  F:	include/dt-bindings/clock/mstar-*
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 904de8d61828..802573122b25 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -651,6 +651,16 @@ config PWM_VT8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-vt8500.
>  
> +config PWM_MSC313E
> +	tristate "MStar MSC313e PWM support"
> +	depends on ARCH_MSTARV7 || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for MSTAR MSC313e.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-msc313e.
> +
> +

only one empty line between entries, and also please stick to alphabetic
ordering.

>  config PWM_XILINX
>  	tristate "Xilinx AXI Timer PWM support"
>  	depends on OF_ADDRESS
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 5c08bdb817b4..e24a48c78335 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -61,4 +61,5 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_MSC313E)	+= pwm-msc313e.o
>  obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> new file mode 100644
> index 000000000000..f20419c6b9be
> --- /dev/null
> +++ b/drivers/pwm/pwm-msc313e.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define DRIVER_NAME "msc313e-pwm"
> +
> +#define CHANNEL_OFFSET	0x80
> +#define REG_DUTY	0x8
> +#define REG_PERIOD	0x10
> +#define REG_DIV		0x18
> +#define REG_CTRL	0x1c
> +#define REG_SWRST	0x1fc
> +
> +struct msc313e_pwm_channel {
> +	struct regmap_field *clkdiv;
> +	struct regmap_field *polarity;
> +	struct regmap_field *dutyl;
> +	struct regmap_field *dutyh;
> +	struct regmap_field *periodl;
> +	struct regmap_field *periodh;
> +	struct regmap_field *swrst;
> +};
> +
> +struct msc313e_pwm {
> +	struct regmap *regmap;
> +	struct pwm_chip pwmchip;
> +	struct clk *clk;
> +	struct msc313e_pwm_channel channels[];
> +};
> +
> +struct msc313e_pwm_info {
> +	unsigned int channels;
> +};
> +
> +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> +
> +static const struct regmap_config msc313e_pwm_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 4,
> +};
> +
> +static const struct msc313e_pwm_info msc313e_data = {
> +	.channels = 8,
> +};
> +
> +static const struct msc313e_pwm_info ssd20xd_data = {
> +	.channels = 4,
> +};
> +
> +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> +{
> +	regmap_field_write(low, value);
> +	regmap_field_write(high, value >> 16);

Is this racy? E.g. if the hw is running and the low register overflows
before the high register is updated?

> +}
> +
> +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> +			      int duty_ns, int period_ns)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned long long nspertick = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> +	unsigned long long div = 1;
> +
> +	/* fit the period into the period register by prescaling the clk */
> +	while (DIV_ROUND_CLOSEST_ULL(period_ns, (nspertick = DIV_ROUND_CLOSEST_ULL(nspertick, div)))
> +	      > 0x3ffff){

Strange line breaking.

Dividing by a division is inexact, also please round down, not
round-closest.

Please test your driver with PWM_DEBUG enabled, and use something like I
proposed in
https://lore.kernel.org/linux-pwm/20220607200755.tgsrwe4ten5inw27@pengutronix.de .


> +		div++;
> +		if (div > (0xffff + 1)) {
> +			dev_err(chip->dev, "Can't fit period into period register\n");
> +			return -EINVAL;
> +		}

If the requested period is too big, please configure the biggest
possible period.

Also .apply() shouldn't emit error messages as this might flood the
kernel log.

> +	}
> +
> +	regmap_field_write(channel->clkdiv, div - 1);
> +	msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
> +				 DIV_ROUND_CLOSEST_ULL(duty_ns, nspertick));
> +	msc313e_pwm_writecounter(channel->periodl, channel->periodh,
> +				 DIV_ROUND_CLOSEST_ULL(period_ns, nspertick));
> +
> +	return 0;
> +};
> +
> +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
> +				    enum pwm_polarity polarity)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned int pol = 0;
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		pol = 1;
> +	regmap_field_update_bits(channel->polarity, 1, pol);
> +
> +	return 0;
> +}
> +
> +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	int ret;
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret)
> +		return ret;
> +
> +	regmap_field_write(channel->swrst, 0);
> +
> +	return 0;
> +}
> +
> +static void msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +
> +	regmap_field_write(channel->swrst, 1);
> +	clk_disable(pwm->clk);

how does the hardware behave on disable? Does it emit the inactive
level? Or 0? Or does it freeze? Or high-Z? Please document that like
it's done e.g. in drivers/pwm/pwm-sl28cpld.c. Stick to the format used
there. (i.e. "Limitations:" + a list of hardware properties in the
toplevel comment.)

Does setting swrst ("software reset"?) reset the other registers?

> +}
> +
> +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	if (state->enabled) {
> +		msc313e_pwm_enable(chip, pwm);
> +		msc313e_pwm_set_polarity(chip, pwm, state->polarity);
> +		msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);

If you enable at the end, you might prevent a glitch. I assume the
glitch isn't preventable in general?

Is the currently running period completed when a new configuration is
written to the registers?

As msc313e_pwm_enable calls clk_prepare_enable() unconditionally, and
it's valid to call pwm_apply several times in a row with state->enabled
= true, the clk calls are not balanced.

> +	} else {
> +		msc313e_pwm_disable(chip, pwm);
> +	}
> +	return 0;
> +}
> +
> +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,
> +			      struct pwm_state *state)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned int pol = 0;
> +
> +	regmap_field_read(channel->polarity, &pol);
> +	state->polarity = pol;

I'd prefer something like:

state->polarity = pol ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;

to not hardcode the values of the PWM constants in the driver.

Also this is incomplete, you need to handle .duty_cycle, .period and
.enabled, too.

> +}
> +
> +static const struct pwm_ops msc313e_pwm_ops = {
> +	.config = msc313e_pwm_config,
> +	.set_polarity = msc313e_pwm_set_polarity,
> +	.enable = msc313e_pwm_enable,
> +	.disable = msc313e_pwm_disable,

Please drop these. If there is an apply functions, these are all unused.

> +	.apply = msc313e_apply,
> +	.get_state = msc313e_get_state,
> +	.owner = THIS_MODULE
> +};
> +
> +static int msc313e_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct msc313e_pwm_info *match_data;
> +	struct device *dev = &pdev->dev;
> +	struct msc313e_pwm *pwm;
> +	__iomem void *base;
> +	int i;
> +
> +	match_data = of_device_get_match_data(dev);
> +	if (!match_data)
> +		return -EINVAL;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
> +
> +	pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
> +	if (IS_ERR(pwm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
> +
> +	for (i = 0; i < match_data->channels; i++) {
> +		unsigned int offset = CHANNEL_OFFSET * i;
> +		struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
> +		struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
> +		struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
> +		struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
> +		struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
> +		struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
> +		struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
> +
> +		pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
> +								  div_clkdiv_field);
> +		pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
> +								    ctrl_polarity_field);
> +		pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
> +		pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
> +		pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
> +		pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
> +		pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);

Huh, never saw something like that. Is that really easier than using
regmap_write()?

> +	}
> +
> +	pwm->pwmchip.dev = dev;
> +	pwm->pwmchip.ops = &msc313e_pwm_ops;
> +	pwm->pwmchip.base = -1;

Please drop this line

> +	pwm->pwmchip.npwm = match_data->channels;
> +	pwm->pwmchip.of_xlate = of_pwm_xlate_with_flags;

You can drop this, this is assigned by default in the pwmchip_add
function.

> +	pwm->pwmchip.of_pwm_n_cells = 3;

I didn't double check, but if the dtb has #pwm-cells = <3> this isn't
needed.

> +
> +	platform_set_drvdata(pdev, pwm);

This is unused -> drop.

> +	return devm_pwmchip_add(dev, &pwm->pwmchip);
> +}
> +
> +static const struct of_device_id msc313e_pwm_dt_ids[] = {
> +	{ .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
> +	{ .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
> +
> +static struct platform_driver msc313e_pwm_driver = {
> +	.probe = msc313e_pwm_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = msc313e_pwm_dt_ids,
> +	},
> +};
> +module_platform_driver(msc313e_pwm_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
> -- 

Best regards
Uwe

Hi,

Le dim. 19 juin 2022 à 23:35, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> a écrit :
> > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > new file mode 100644
> > index 000000000000..f20419c6b9be
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-msc313e.c
> > @@ -0,0 +1,242 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define DRIVER_NAME "msc313e-pwm"
> > +
> > +#define CHANNEL_OFFSET       0x80
> > +#define REG_DUTY     0x8
> > +#define REG_PERIOD   0x10
> > +#define REG_DIV              0x18
> > +#define REG_CTRL     0x1c
> > +#define REG_SWRST    0x1fc
> > +
> > +struct msc313e_pwm_channel {
> > +     struct regmap_field *clkdiv;
> > +     struct regmap_field *polarity;
> > +     struct regmap_field *dutyl;
> > +     struct regmap_field *dutyh;
> > +     struct regmap_field *periodl;
> > +     struct regmap_field *periodh;
> > +     struct regmap_field *swrst;
> > +};
> > +
> > +struct msc313e_pwm {
> > +     struct regmap *regmap;
> > +     struct pwm_chip pwmchip;
> > +     struct clk *clk;
> > +     struct msc313e_pwm_channel channels[];
> > +};
> > +
> > +struct msc313e_pwm_info {
> > +     unsigned int channels;
> > +};
> > +
> > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> > +
> > +static const struct regmap_config msc313e_pwm_regmap_config = {
> > +     .reg_bits = 16,
> > +     .val_bits = 16,
> > +     .reg_stride = 4,
> > +};
> > +
> > +static const struct msc313e_pwm_info msc313e_data = {
> > +     .channels = 8,
> > +};
> > +
> > +static const struct msc313e_pwm_info ssd20xd_data = {
> > +     .channels = 4,
> > +};
> > +
> > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > +{
> > +     regmap_field_write(low, value);
> > +     regmap_field_write(high, value >> 16);
>
> Is this racy? E.g. if the hw is running and the low register overflows
> before the high register is updated?
>

Ack, I am re-working most of the stuff you noticed. The problem with
this IP blocks (and others...) is we have close registers
but we only need to write or read 16 bits in each of these (it is
mainly reverse engineered from vendor source or runtime most of the
time) . You cannot really do a single read (except by doing an obscur
thing via readq ? ...)

. We had exactly the same issue with the rtc driver see [1]

1. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/rtc/rtc-msc313.c#n50
.

Regards,
Romain

Hello Romain,

On Fri, Aug 19, 2022 at 03:12:56PM +0200, Romain Perier wrote:
> Le dim. 19 juin 2022 à 23:35, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > > +{
> > > +     regmap_field_write(low, value);
> > > +     regmap_field_write(high, value >> 16);
> >
> > Is this racy? E.g. if the hw is running and the low register overflows
> > before the high register is updated?
> >
> 
> Ack, I am re-working most of the stuff you noticed. The problem with
> this IP blocks (and others...) is we have close registers
> but we only need to write or read 16 bits in each of these (it is
> mainly reverse engineered from vendor source or runtime most of the
> time) . You cannot really do a single read (except by doing an obscur
> thing via readq ? ...)

This is fine, but pleast document that in a comment.

Best regards
Uwe