diff mbox series

[v1,2/2] pwm: sophgo: add pwm support for Sophgo CV1800 SoC

Message ID 20240207060913.672554-1-qiujingbao.dlmu@gmail.com
State Changes Requested
Headers show
Series riscv: pwm: sophgo: add pwm support for CV1800 | expand

Commit Message

Jingbao Qiu Feb. 7, 2024, 6:09 a.m. UTC
Implement the PWM driver for CV1800.

Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
---
 drivers/pwm/Kconfig      |  10 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+)
 create mode 100644 drivers/pwm/pwm-cv1800.c

Comments

Krzysztof Kozlowski Feb. 7, 2024, 8:08 a.m. UTC | #1
On 07/02/2024 07:09, Jingbao Qiu wrote:
> Implement the PWM driver for CV1800.
> 
> Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> ---


> +
> +static struct platform_driver cv1800_pwm_driver = {
> +	.driver = {
> +		.name = "cv1800-pwm",
> +		.of_match_table = cv1800_pwm_dt_ids,
> +	},
> +	.probe = cv1800_pwm_probe,
> +};
> +module_platform_driver(cv1800_pwm_driver);
> +
> +MODULE_ALIAS("platform:cv1800-pwm");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


Best regards,
Krzysztof
Uwe Kleine-König Feb. 7, 2024, 8:39 a.m. UTC | #2
Hello Jingbao,

On Wed, Feb 07, 2024 at 02:09:13PM +0800, Jingbao Qiu wrote:
> Implement the PWM driver for CV1800.
> 
> Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> ---
>  drivers/pwm/Kconfig      |  10 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 229 insertions(+)
>  create mode 100644 drivers/pwm/pwm-cv1800.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..455f07af94f7 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -186,6 +186,16 @@ config PWM_CROS_EC
>  	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
>  	  Controller.
>  
> +config PWM_CV1800
> +	tristate "Sophgo CV1800 PWM driver"
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for the Sophgo CV1800 series
> +	  SoCs.
> +
> +	  To compile this driver as a module, build the dependecies
> +	  as modules, this will be called pwm-cv1800.
> +
>  config PWM_DWC_CORE
>  	tristate
>  	depends on HAS_IOMEM
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..6c3c4a07a316 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
>  obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>  obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
> +obj-$(CONFIG_PWM_CV1800)	+= pwm-cv1800.o
>  obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
>  obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
> diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
> new file mode 100644
> index 000000000000..4d4f233c9087
> --- /dev/null
> +++ b/drivers/pwm/pwm-cv1800.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
> + *
> + * Author: Jingbao Qiu <qiujingbao.dlmu@gmail.com>

Please document the behaviour of the PWM here at the top of the driver.
Things to mention are:

 - How does the hardware behave on disable? (Usual behaviours include:
   output of inactive state; freeze where it just happens to be; high-z)

 - If you reconfigure the hardware, does it complete the previously
   running period before emitting the new wave form?

 - Are there possible glitches in .apply()? (i.e. can it happen, that
   for a short moment a wave form is emitted that has the new period but
   the old duty_cycle?)

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define HLPERIOD_BASE  0x00
> +#define PERIOD_BASE    0x04
> +#define POLARITY       0x040
> +#define PWMSTART       0x044
> +#define PWMDONE        0x048
> +#define PWMUPDATE      0x4c
> +#define PWM_OE         0xd0
> +#define HLPERIOD_SHIFT 0x08
> +#define PERIOD_SHIFT   0x08
> +
> +#define HLPERIOD(n)    (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
> +#define PERIOD(n)      (PERIOD_BASE + ((n) * PERIOD_SHIFT))
> +#define UPDATE(n)      (BIT(0) << (n))
> +#define OE_MASK(n)     (BIT(0) << (n))
> +#define START_MASK(n)  (BIT(0) << (n))
> +
> +#define PERIOD_RESET   0x02
> +#define HLPERIOD_RESET 0x1
> +#define REG_DISABLE    0x0U
> +#define REG_ENABLE     BIT(0)

Please use a driver specific prefix for all these defines.

> +struct soc_info {
> +	unsigned int num_pwms;
> +};
> +
> +struct cv1800_pwm {
> +	struct pwm_chip chip;
> +	struct regmap *map;
> +	struct clk *clk;
> +};
> +
> +static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct cv1800_pwm, chip);
> +}
> +
> +static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     u32 enable)

This is called with the enable parameter set to state->enabled which is
a bool. Please use a bool here, too, instead of a u32.

> +{
> +	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> +	u32 pwm_enable;
> +
> +	regmap_read(priv->map, PWMSTART, &pwm_enable);
> +	pwm_enable >>= pwm->hwpwm;

I don't understand why this value is shifted here. I guess this misses a
mask?

> +	if (enable)
> +		clk_prepare_enable(priv->clk);
> +	else
> +		clk_disable_unprepare(priv->clk);

This is broken. It might well happen that the first call to .apply() has
state->enabled = false. Please make sure to properly balance clk
enabling.

> +	/*
> +	 * If the parameters are changed during runtime, Register needs
> +	 * to be updated to take effect.
> +	 */
> +	if (pwm_enable) {
> +		regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> +				   REG_ENABLE << pwm->hwpwm);
> +		regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> +				   REG_DISABLE << pwm->hwpwm);

REG_DISABLE is zero. Why is this shifted? Very strange.

> +	} else {
> +		regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
> +				   enable << pwm->hwpwm);
> +		regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
> +				   enable << pwm->hwpwm);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> +	u64 period_ns, duty_ns;
> +	u32 period_val, hlperiod_val;
> +	unsigned long long rate, div;
> +
> +	period_ns = state->period;
> +	duty_ns = state->duty_cycle;
> +
> +	rate = (unsigned long long)clk_get_rate(priv->clk);

This cast has no effect, so please drop it. To prevent that the clock
changes rate while the PWM is running, please use
clk_rate_exclusive_get().

> +	div = rate * period_ns;

This might overflow. Please use mul_u64_u64_div_u64() or one of its
variants and error out on rate > NSEC_PER_SEC. (There are other pwm
drivers that make it right, took a look into these if this is unclear.)

> +	do_div(div, NSEC_PER_SEC);
> +	period_val = div;
> +
> +	div = rate * (period_ns - duty_ns);
> +	do_div(div, NSEC_PER_SEC);
> +	hlperiod_val = div;

I think it can happen here, that div it too big to fit into this u32.

> +	regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
> +	regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
> +
> +	cv1800_pwm_enable(chip, pwm, state->enabled);
> +
> +	return 0;
> +}
> +
> +static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> +	u32 period_val, hlperiod_val, tem;
> +	u64 rate;
> +	u64 period_ns = 0;
> +	u64 duty_ns = 0;
> +	u32 enable = 0;
> +
> +	regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
> +	regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
> +
> +	if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
> +		rate = (u64)clk_get_rate(priv->clk);
> +
> +		tem = NSEC_PER_SEC * period_val;

This might overflow.

> +		do_div(tem, rate);
> +		period_ns = tem;

This uses wrong rounding. If you enabled PWM_DEBUG during your tests,
this should be catched and logged.

> +
> +		tem = period_val * period_ns;
> +		do_div(tem, hlperiod_val);
> +		duty_ns = tem;
> +
> +		regmap_read(priv->map, PWMSTART, &enable);
> +		enable >>= pwm->hwpwm;

Again a missing mask??

> +	}
> +
> +	state->period = period_ns;
> +	state->duty_cycle = duty_ns;
> +	state->enabled = enable;
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops cv1800_pwm_ops = {
> +	.apply = cv1800_pwm_apply,
> +	.get_state = cv1800_pwm_get_state,
> +};
> +
> +static const struct regmap_config cv1800_pwm_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int cv1800_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cv1800_pwm *cv_pwm;

Please pick always the same name for your driver private variable. In
the other functions it is called "priv".

> +	void __iomem *base;
> +	const struct soc_info *info;
> +
> +	info = device_get_match_data(dev);
> +	if (!info)

error message please.

> +		return -EINVAL;
> +
> +	cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
> +	if (!cv_pwm)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
> +					    &cv1800_pwm_regmap_config);
> +	if (IS_ERR(cv_pwm->map))
> +		return PTR_ERR(cv_pwm->map);
> +
> +	cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(cv_pwm->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
> +				     "clk not found\n");
> +
> +	cv_pwm->chip.dev = dev;
> +	cv_pwm->chip.ops = &cv1800_pwm_ops;
> +	cv_pwm->chip.npwm = info->num_pwms;

Missing clk handling here. Please all clk_prepare_enable once for each
running channel.

> +	return devm_pwmchip_add(dev, &cv_pwm->chip);
> +}
> +
> +static const struct soc_info cv1800b_soc_info = {
> +	.num_pwms = 4,
> +};

Until you support more variants, I suggest to just hardcode npwm to
four.

> +static const struct of_device_id cv1800_pwm_dt_ids[] = {
> +	{ .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
> +
> +static struct platform_driver cv1800_pwm_driver = {
> +	.driver = {
> +		.name = "cv1800-pwm",
> +		.of_match_table = cv1800_pwm_dt_ids,
> +	},
> +	.probe = cv1800_pwm_probe,
> +};
> +module_platform_driver(cv1800_pwm_driver);
> +
> +MODULE_ALIAS("platform:cv1800-pwm");

I'm with Krzysztof here, this shouldn't be needed.

> +MODULE_AUTHOR("Jingbao Qiu");
> +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe
Jingbao Qiu Feb. 7, 2024, 8:43 a.m. UTC | #3
On Wed, Feb 7, 2024 at 4:09 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 07/02/2024 07:09, Jingbao Qiu wrote:
> > Implement the PWM driver for CV1800.
> >
> > Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> > ---
>
>
> > +
> > +static struct platform_driver cv1800_pwm_driver = {
> > +     .driver = {
> > +             .name = "cv1800-pwm",
> > +             .of_match_table = cv1800_pwm_dt_ids,
> > +     },
> > +     .probe = cv1800_pwm_probe,
> > +};
> > +module_platform_driver(cv1800_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:cv1800-pwm");
>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
>

you're right, I will drop it.

Best regards,
Jingbao Qiu
Jingbao Qiu Feb. 7, 2024, 8:59 a.m. UTC | #4
On Wed, Feb 7, 2024 at 4:39 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Jingbao,
>
> On Wed, Feb 07, 2024 at 02:09:13PM +0800, Jingbao Qiu wrote:
> > Implement the PWM driver for CV1800.
> >
> > Signed-off-by: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
> > ---
> >  drivers/pwm/Kconfig      |  10 ++
> >  drivers/pwm/Makefile     |   1 +
> >  drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 229 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-cv1800.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..455f07af94f7 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -186,6 +186,16 @@ config PWM_CROS_EC
> >         PWM driver for exposing a PWM attached to the ChromeOS Embedded
> >         Controller.
> >
> > +config PWM_CV1800
> > +     tristate "Sophgo CV1800 PWM driver"
> > +     depends on ARCH_SOPHGO || COMPILE_TEST
> > +     help
> > +       Generic PWM framework driver for the Sophgo CV1800 series
> > +       SoCs.
> > +
> > +       To compile this driver as a module, build the dependecies
> > +       as modules, this will be called pwm-cv1800.
> > +
> >  config PWM_DWC_CORE
> >       tristate
> >       depends on HAS_IOMEM
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c5ec9e168ee7..6c3c4a07a316 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLK)               += pwm-clk.o
> >  obj-$(CONFIG_PWM_CLPS711X)   += pwm-clps711x.o
> >  obj-$(CONFIG_PWM_CRC)                += pwm-crc.o
> >  obj-$(CONFIG_PWM_CROS_EC)    += pwm-cros-ec.o
> > +obj-$(CONFIG_PWM_CV1800)     += pwm-cv1800.o
> >  obj-$(CONFIG_PWM_DWC_CORE)   += pwm-dwc-core.o
> >  obj-$(CONFIG_PWM_DWC)                += pwm-dwc.o
> >  obj-$(CONFIG_PWM_EP93XX)     += pwm-ep93xx.o
> > diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
> > new file mode 100644
> > index 000000000000..4d4f233c9087
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-cv1800.c
> > @@ -0,0 +1,218 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
> > + *
> > + * Author: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
>
> Please document the behaviour of the PWM here at the top of the driver.
> Things to mention are:
>
>  - How does the hardware behave on disable? (Usual behaviours include:
>    output of inactive state; freeze where it just happens to be; high-z)
>
>  - If you reconfigure the hardware, does it complete the previously
>    running period before emitting the new wave form?
>
>  - Are there possible glitches in .apply()? (i.e. can it happen, that
>    for a short moment a wave form is emitted that has the new period but
>    the old duty_cycle?)
>

Thanks, I will do that.

> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define HLPERIOD_BASE  0x00
> > +#define PERIOD_BASE    0x04
> > +#define POLARITY       0x040
> > +#define PWMSTART       0x044
> > +#define PWMDONE        0x048
> > +#define PWMUPDATE      0x4c
> > +#define PWM_OE         0xd0
> > +#define HLPERIOD_SHIFT 0x08
> > +#define PERIOD_SHIFT   0x08
> > +
> > +#define HLPERIOD(n)    (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
> > +#define PERIOD(n)      (PERIOD_BASE + ((n) * PERIOD_SHIFT))
> > +#define UPDATE(n)      (BIT(0) << (n))
> > +#define OE_MASK(n)     (BIT(0) << (n))
> > +#define START_MASK(n)  (BIT(0) << (n))
> > +
> > +#define PERIOD_RESET   0x02
> > +#define HLPERIOD_RESET 0x1
> > +#define REG_DISABLE    0x0U
> > +#define REG_ENABLE     BIT(0)
>
> Please use a driver specific prefix for all these defines.
>

I will do that.

> > +struct soc_info {
> > +     unsigned int num_pwms;
> > +};
> > +
> > +struct cv1800_pwm {
> > +     struct pwm_chip chip;
> > +     struct regmap *map;
> > +     struct clk *clk;
> > +};
> > +
> > +static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
> > +{
> > +     return container_of(chip, struct cv1800_pwm, chip);
> > +}
> > +
> > +static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                          u32 enable)
>
> This is called with the enable parameter set to state->enabled which is
> a bool. Please use a bool here, too, instead of a u32.
>

Thanks, I will use bool instead of a u32.

> > +{
> > +     struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > +     u32 pwm_enable;
> > +
> > +     regmap_read(priv->map, PWMSTART, &pwm_enable);
> > +     pwm_enable >>= pwm->hwpwm;
>
> I don't understand why this value is shifted here. I guess this misses a
> mask?

The nth bit of this register represents the running state, so a shift
representation is required.
I will use macro definitions to represent masks.

>
> > +     if (enable)
> > +             clk_prepare_enable(priv->clk);
> > +     else
> > +             clk_disable_unprepare(priv->clk);
>
> This is broken. It might well happen that the first call to .apply() has
> state->enabled = false. Please make sure to properly balance clk
> enabling.
>

I will fix it.

> > +     /*
> > +      * If the parameters are changed during runtime, Register needs
> > +      * to be updated to take effect.
> > +      */
> > +     if (pwm_enable) {
> > +             regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> > +                                REG_ENABLE << pwm->hwpwm);
> > +             regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> > +                                REG_DISABLE << pwm->hwpwm);
>
> REG_DISABLE is zero. Why is this shifted? Very strange.

The data manual states that for dynamic updates, for the nth bit of
this register
write one first and then write 0.
you're right, just 0 or BIT(n) is ok.

>
> > +     } else {
> > +             regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
> > +                                enable << pwm->hwpwm);
> > +             regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
> > +                                enable << pwm->hwpwm);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                         const struct pwm_state *state)
> > +{
> > +     struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > +     u64 period_ns, duty_ns;
> > +     u32 period_val, hlperiod_val;
> > +     unsigned long long rate, div;
> > +
> > +     period_ns = state->period;
> > +     duty_ns = state->duty_cycle;
> > +
> > +     rate = (unsigned long long)clk_get_rate(priv->clk);
>
> This cast has no effect, so please drop it. To prevent that the clock
> changes rate while the PWM is running, please use
> clk_rate_exclusive_get().

I will fix it.

>
> > +     div = rate * period_ns;
>
> This might overflow. Please use mul_u64_u64_div_u64() or one of its
> variants and error out on rate > NSEC_PER_SEC. (There are other pwm
> drivers that make it right, took a look into these if this is unclear.)
>

Thanks, I will do that.

> > +     do_div(div, NSEC_PER_SEC);
> > +     period_val = div;
> > +
> > +     div = rate * (period_ns - duty_ns);
> > +     do_div(div, NSEC_PER_SEC);
> > +     hlperiod_val = div;
>
> I think it can happen here, that div it too big to fit into this u32.

you're right, I will fix it.

>
> > +     regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
> > +     regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
> > +
> > +     cv1800_pwm_enable(chip, pwm, state->enabled);
> > +
> > +     return 0;
> > +}
> > +
> > +static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                              struct pwm_state *state)
> > +{
> > +     struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > +     u32 period_val, hlperiod_val, tem;
> > +     u64 rate;
> > +     u64 period_ns = 0;
> > +     u64 duty_ns = 0;
> > +     u32 enable = 0;
> > +
> > +     regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
> > +     regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
> > +
> > +     if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
> > +             rate = (u64)clk_get_rate(priv->clk);
> > +
> > +             tem = NSEC_PER_SEC * period_val;
>
> This might overflow.

I will fix it.

>
> > +             do_div(tem, rate);
> > +             period_ns = tem;
>
> This uses wrong rounding. If you enabled PWM_DEBUG during your tests,
> this should be catched and logged.

I will fix it.

>
> > +
> > +             tem = period_val * period_ns;
> > +             do_div(tem, hlperiod_val);
> > +             duty_ns = tem;
> > +
> > +             regmap_read(priv->map, PWMSTART, &enable);
> > +             enable >>= pwm->hwpwm;
>
> Again a missing mask??

Yes, I will use a mask.

>
> > +     }
> > +
> > +     state->period = period_ns;
> > +     state->duty_cycle = duty_ns;
> > +     state->enabled = enable;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct pwm_ops cv1800_pwm_ops = {
> > +     .apply = cv1800_pwm_apply,
> > +     .get_state = cv1800_pwm_get_state,
> > +};
> > +
> > +static const struct regmap_config cv1800_pwm_regmap_config = {
> > +     .reg_bits = 32,
> > +     .val_bits = 32,
> > +     .reg_stride = 4,
> > +};
> > +
> > +static int cv1800_pwm_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cv1800_pwm *cv_pwm;
>
> Please pick always the same name for your driver private variable. In
> the other functions it is called "priv".

I will fix it.

>
> > +     void __iomem *base;
> > +     const struct soc_info *info;
> > +
> > +     info = device_get_match_data(dev);
> > +     if (!info)
>
> error message please.

I will fix it.

>
> > +             return -EINVAL;
> > +
> > +     cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
> > +     if (!cv_pwm)
> > +             return -ENOMEM;
> > +
> > +     base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
> > +                                         &cv1800_pwm_regmap_config);
> > +     if (IS_ERR(cv_pwm->map))
> > +             return PTR_ERR(cv_pwm->map);
> > +
> > +     cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > +     if (IS_ERR(cv_pwm->clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
> > +                                  "clk not found\n");
> > +
> > +     cv_pwm->chip.dev = dev;
> > +     cv_pwm->chip.ops = &cv1800_pwm_ops;
> > +     cv_pwm->chip.npwm = info->num_pwms;
>
> Missing clk handling here. Please all clk_prepare_enable once for each
> running channel.

I will fix it.

>
> > +     return devm_pwmchip_add(dev, &cv_pwm->chip);
> > +}
> > +
> > +static const struct soc_info cv1800b_soc_info = {
> > +     .num_pwms = 4,
> > +};
>
> Until you support more variants, I suggest to just hardcode npwm to
> four.

I will do that.

>
> > +static const struct of_device_id cv1800_pwm_dt_ids[] = {
> > +     { .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
> > +
> > +static struct platform_driver cv1800_pwm_driver = {
> > +     .driver = {
> > +             .name = "cv1800-pwm",
> > +             .of_match_table = cv1800_pwm_dt_ids,
> > +     },
> > +     .probe = cv1800_pwm_probe,
> > +};
> > +module_platform_driver(cv1800_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:cv1800-pwm");
>
> I'm with Krzysztof here, this shouldn't be needed.

I will drop it.

>
> > +MODULE_AUTHOR("Jingbao Qiu");
> > +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> > +MODULE_LICENSE("GPL");
>

Best regards
Jingbao Qiu
kernel test robot Feb. 8, 2024, 2:20 a.m. UTC | #5
Hi Jingbao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 7afc0e7f681e6efd6b826f003fc14c17b5093643]

url:    https://github.com/intel-lab-lkp/linux/commits/Jingbao-Qiu/dt-bindings-pwm-sophgo-add-pwm-for-Sophgo-CV1800-series-SoC/20240207-141135
base:   7afc0e7f681e6efd6b826f003fc14c17b5093643
patch link:    https://lore.kernel.org/r/20240207060913.672554-1-qiujingbao.dlmu%40gmail.com
patch subject: [PATCH v1 2/2] pwm: sophgo: add pwm support for Sophgo CV1800 SoC
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20240208/202402081005.iOYVJ6KI-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7dd790db8b77c4a833c06632e903dc4f13877a64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402081005.iOYVJ6KI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402081005.iOYVJ6KI-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/pwm/pwm-cv1800.c:131:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
   drivers/pwm/pwm-cv1800.c:131:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                                      ^
   drivers/pwm/pwm-cv1800.c:135:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
   drivers/pwm/pwm-cv1800.c:135:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                                      ^
>> drivers/pwm/pwm-cv1800.c:131:3: warning: shift count >= width of type [-Wshift-count-overflow]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   drivers/pwm/pwm-cv1800.c:135:3: warning: shift count >= width of type [-Wshift-count-overflow]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   10 warnings and 2 errors generated.


vim +131 drivers/pwm/pwm-cv1800.c

   113	
   114	static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
   115					 struct pwm_state *state)
   116	{
   117		struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
   118		u32 period_val, hlperiod_val, tem;
   119		u64 rate;
   120		u64 period_ns = 0;
   121		u64 duty_ns = 0;
   122		u32 enable = 0;
   123	
   124		regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
   125		regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
   126	
   127		if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
   128			rate = (u64)clk_get_rate(priv->clk);
   129	
   130			tem = NSEC_PER_SEC * period_val;
 > 131			do_div(tem, rate);
   132			period_ns = tem;
   133	
   134			tem = period_val * period_ns;
   135			do_div(tem, hlperiod_val);
   136			duty_ns = tem;
   137	
   138			regmap_read(priv->map, PWMSTART, &enable);
   139			enable >>= pwm->hwpwm;
   140		}
   141	
   142		state->period = period_ns;
   143		state->duty_cycle = duty_ns;
   144		state->enabled = enable;
   145	
   146		return 0;
   147	}
   148
kernel test robot Feb. 8, 2024, 2:53 a.m. UTC | #6
Hi Jingbao,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7afc0e7f681e6efd6b826f003fc14c17b5093643]

url:    https://github.com/intel-lab-lkp/linux/commits/Jingbao-Qiu/dt-bindings-pwm-sophgo-add-pwm-for-Sophgo-CV1800-series-SoC/20240207-141135
base:   7afc0e7f681e6efd6b826f003fc14c17b5093643
patch link:    https://lore.kernel.org/r/20240207060913.672554-1-qiujingbao.dlmu%40gmail.com
patch subject: [PATCH v1 2/2] pwm: sophgo: add pwm support for Sophgo CV1800 SoC
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240208/202402081059.e1PYAFcY-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7dd790db8b77c4a833c06632e903dc4f13877a64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402081059.e1PYAFcY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402081059.e1PYAFcY-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/pwm/pwm-cv1800.c:14:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:337:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   drivers/pwm/pwm-cv1800.c:131:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
>> drivers/pwm/pwm-cv1800.c:131:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                                      ^
   drivers/pwm/pwm-cv1800.c:135:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
   drivers/pwm/pwm-cv1800.c:135:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                                      ^
   drivers/pwm/pwm-cv1800.c:131:3: warning: shift count >= width of type [-Wshift-count-overflow]
     131 |                 do_div(tem, rate);
         |                 ^~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   drivers/pwm/pwm-cv1800.c:135:3: warning: shift count >= width of type [-Wshift-count-overflow]
     135 |                 do_div(tem, hlperiod_val);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   10 warnings and 2 errors generated.


vim +131 drivers/pwm/pwm-cv1800.c

   113	
   114	static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
   115					 struct pwm_state *state)
   116	{
   117		struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
   118		u32 period_val, hlperiod_val, tem;
   119		u64 rate;
   120		u64 period_ns = 0;
   121		u64 duty_ns = 0;
   122		u32 enable = 0;
   123	
   124		regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
   125		regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
   126	
   127		if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
   128			rate = (u64)clk_get_rate(priv->clk);
   129	
   130			tem = NSEC_PER_SEC * period_val;
 > 131			do_div(tem, rate);
   132			period_ns = tem;
   133	
   134			tem = period_val * period_ns;
   135			do_div(tem, hlperiod_val);
   136			duty_ns = tem;
   137	
   138			regmap_read(priv->map, PWMSTART, &enable);
   139			enable >>= pwm->hwpwm;
   140		}
   141	
   142		state->period = period_ns;
   143		state->duty_cycle = duty_ns;
   144		state->enabled = enable;
   145	
   146		return 0;
   147	}
   148
diff mbox series

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..455f07af94f7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -186,6 +186,16 @@  config PWM_CROS_EC
 	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
 	  Controller.
 
+config PWM_CV1800
+	tristate "Sophgo CV1800 PWM driver"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Generic PWM framework driver for the Sophgo CV1800 series
+	  SoCs.
+
+	  To compile this driver as a module, build the dependecies
+	  as modules, this will be called pwm-cv1800.
+
 config PWM_DWC_CORE
 	tristate
 	depends on HAS_IOMEM
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..6c3c4a07a316 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
+obj-$(CONFIG_PWM_CV1800)	+= pwm-cv1800.o
 obj-$(CONFIG_PWM_DWC_CORE)	+= pwm-dwc-core.o
 obj-$(CONFIG_PWM_DWC)		+= pwm-dwc.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
new file mode 100644
index 000000000000..4d4f233c9087
--- /dev/null
+++ b/drivers/pwm/pwm-cv1800.c
@@ -0,0 +1,218 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
+ *
+ * Author: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define HLPERIOD_BASE  0x00
+#define PERIOD_BASE    0x04
+#define POLARITY       0x040
+#define PWMSTART       0x044
+#define PWMDONE        0x048
+#define PWMUPDATE      0x4c
+#define PWM_OE         0xd0
+#define HLPERIOD_SHIFT 0x08
+#define PERIOD_SHIFT   0x08
+
+#define HLPERIOD(n)    (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
+#define PERIOD(n)      (PERIOD_BASE + ((n) * PERIOD_SHIFT))
+#define UPDATE(n)      (BIT(0) << (n))
+#define OE_MASK(n)     (BIT(0) << (n))
+#define START_MASK(n)  (BIT(0) << (n))
+
+#define PERIOD_RESET   0x02
+#define HLPERIOD_RESET 0x1
+#define REG_DISABLE    0x0U
+#define REG_ENABLE     BIT(0)
+
+struct soc_info {
+	unsigned int num_pwms;
+};
+
+struct cv1800_pwm {
+	struct pwm_chip chip;
+	struct regmap *map;
+	struct clk *clk;
+};
+
+static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
+{
+	return container_of(chip, struct cv1800_pwm, chip);
+}
+
+static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+			     u32 enable)
+{
+	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+	u32 pwm_enable;
+
+	regmap_read(priv->map, PWMSTART, &pwm_enable);
+	pwm_enable >>= pwm->hwpwm;
+
+	if (enable)
+		clk_prepare_enable(priv->clk);
+	else
+		clk_disable_unprepare(priv->clk);
+
+	/*
+	 * If the parameters are changed during runtime, Register needs
+	 * to be updated to take effect.
+	 */
+	if (pwm_enable) {
+		regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
+				   REG_ENABLE << pwm->hwpwm);
+		regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
+				   REG_DISABLE << pwm->hwpwm);
+	} else {
+		regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
+				   enable << pwm->hwpwm);
+		regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
+				   enable << pwm->hwpwm);
+	}
+
+	return 0;
+}
+
+static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+	u64 period_ns, duty_ns;
+	u32 period_val, hlperiod_val;
+	unsigned long long rate, div;
+
+	period_ns = state->period;
+	duty_ns = state->duty_cycle;
+
+	rate = (unsigned long long)clk_get_rate(priv->clk);
+
+	div = rate * period_ns;
+	do_div(div, NSEC_PER_SEC);
+	period_val = div;
+
+	div = rate * (period_ns - duty_ns);
+	do_div(div, NSEC_PER_SEC);
+	hlperiod_val = div;
+
+	regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
+	regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
+
+	cv1800_pwm_enable(chip, pwm, state->enabled);
+
+	return 0;
+}
+
+static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+	u32 period_val, hlperiod_val, tem;
+	u64 rate;
+	u64 period_ns = 0;
+	u64 duty_ns = 0;
+	u32 enable = 0;
+
+	regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
+	regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
+
+	if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
+		rate = (u64)clk_get_rate(priv->clk);
+
+		tem = NSEC_PER_SEC * period_val;
+		do_div(tem, rate);
+		period_ns = tem;
+
+		tem = period_val * period_ns;
+		do_div(tem, hlperiod_val);
+		duty_ns = tem;
+
+		regmap_read(priv->map, PWMSTART, &enable);
+		enable >>= pwm->hwpwm;
+	}
+
+	state->period = period_ns;
+	state->duty_cycle = duty_ns;
+	state->enabled = enable;
+
+	return 0;
+}
+
+static const struct pwm_ops cv1800_pwm_ops = {
+	.apply = cv1800_pwm_apply,
+	.get_state = cv1800_pwm_get_state,
+};
+
+static const struct regmap_config cv1800_pwm_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int cv1800_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cv1800_pwm *cv_pwm;
+	void __iomem *base;
+	const struct soc_info *info;
+
+	info = device_get_match_data(dev);
+	if (!info)
+		return -EINVAL;
+
+	cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
+	if (!cv_pwm)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
+					    &cv1800_pwm_regmap_config);
+	if (IS_ERR(cv_pwm->map))
+		return PTR_ERR(cv_pwm->map);
+
+	cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(cv_pwm->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
+				     "clk not found\n");
+
+	cv_pwm->chip.dev = dev;
+	cv_pwm->chip.ops = &cv1800_pwm_ops;
+	cv_pwm->chip.npwm = info->num_pwms;
+
+	return devm_pwmchip_add(dev, &cv_pwm->chip);
+}
+
+static const struct soc_info cv1800b_soc_info = {
+	.num_pwms = 4,
+};
+
+static const struct of_device_id cv1800_pwm_dt_ids[] = {
+	{ .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
+
+static struct platform_driver cv1800_pwm_driver = {
+	.driver = {
+		.name = "cv1800-pwm",
+		.of_match_table = cv1800_pwm_dt_ids,
+	},
+	.probe = cv1800_pwm_probe,
+};
+module_platform_driver(cv1800_pwm_driver);
+
+MODULE_ALIAS("platform:cv1800-pwm");
+MODULE_AUTHOR("Jingbao Qiu");
+MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
+MODULE_LICENSE("GPL");