Hi Uwe,
> -----Original Message-----
> From: linux-pwm-owner@xxxxxxxxxxxxxxx <linux-pwm-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Uwe Kleine-König
> Sent: Wednesday, September 2, 2020 2:26 PM
> To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@xxxxxxxxx>
> Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; linux-
> pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Wan Mohamad, Wan
> Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>;
> andriy.shevchenko@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@xxxxxxxxx>
> Subject: Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay
>
> Hello,
>
> On Wed, Aug 26, 2020 at 06:25:58PM +0800,
> vineetha.g.jaya.kumaran@xxxxxxxxx wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@xxxxxxxxx>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
> >
> > Co-developed-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@xxxxxxxxx>
> > Signed-off-by: Lai, Poey Seng <poey.seng.lai@xxxxxxxxx>
> > Signed-off-by: Vineetha G. Jaya Kumaran
> > <vineetha.g.jaya.kumaran@xxxxxxxxx>
> > ---
> > drivers/pwm/Kconfig | 9 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-keembay.c | 228
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 238 insertions(+)
> > create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 7dbcf69..0a68a167 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -560,4 +560,13 @@ config PWM_ZX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-zx.
> >
> > +config PWM_KEEMBAY
> > + tristate "Intel Keem Bay PWM driver"
> > + depends on ARM64 || COMPILE_TEST
> > + help
> > + The platform driver for Intel Keem Bay PWM controller.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-keembay.
>
> The symbols in drivers/pwm/Kconfig are ordered alphabetically.
>
OK, I will reorder this.
> > +
> > endif
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 2c2ba0a..293e48f 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -54,3 +54,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
> > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> > obj-$(CONFIG_PWM_ZX) += pwm-zx.o
> > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o
>
> ditto.
>
> > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new
> > file mode 100644 index 00000000..3c7481f
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,228 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Keem Bay PWM driver
> > + *
> > + * Copyright (C) 2020 Intel Corporation
> > + * Authors: Lai Poey Seng <poey.seng.lai@xxxxxxxxx>
> > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@xxxxxxxxx>
> > + *
> > + * Limitation:
>
> Please make this "Limitations:" for consitency with other drivers. This allows
> something like
>
> for f in drivers/pwm/pwm-*; do echo $f; sed -nr
> '/Limitations:/,/\*\/?$/p' $f; done
>
> to work nicely.
>
OK, will update this to "Limitations:"
> > + * - Upon disabling a channel, the currently running
> > + * period will not be completed. However, upon
> > + * reconfiguration of the duty cycle/period, the
> > + * currently running period will be completed first.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define KMB_TOTAL_PWM_CHANNELS 6
> > +#define KMB_PWM_COUNT_MAX 0xffff
> > +#define KMB_PWM_EN_BIT BIT(31)
> > +
> > +/* Mask */
> > +#define KMB_PWM_HIGH_MASK GENMASK(31, 16)
> > +#define KMB_PWM_LOW_MASK GENMASK(15, 0)
> > +#define KMB_PWM_COUNT_MASK GENMASK(31, 0)
> > +
> > +/* PWM Register offset */
> > +#define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch))
> > +#define KMB_PWM_HIGHLOW_OFFSET(ch) (0x20 + 4 * (ch))
> > +
> > +struct keembay_pwm {
> > + struct pwm_chip chip;
> > + struct device *dev;
> > + struct clk *clk;
> > + void __iomem *base;
> > +};
> > +
> > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct
> pwm_chip
> > +*chip) {
> > + return container_of(chip, struct keembay_pwm, chip); }
> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm
> *priv, u32 mask,
> > + u32 val, u32 offset)
> > +{
> > + u32 buff = readl(priv->base + offset);
> > +
> > + buff = u32_replace_bits(buff, val, mask);
> > + writel(buff, priv->base + offset);
> > +}
> > +
> > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> > + KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> > + KMB_PWM_LEADIN_OFFSET(ch));
> > +}
> > +
> > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > + unsigned long long pwm_h_count, pwm_l_count;
> > + unsigned long clk_rate;
> > + u32 buff;
> > +
> > + clk_rate = clk_get_rate(priv->clk);
>
> clk_get_rate() must only be called when the clock is enabled. Unless I miss
> something this isn't ensured here.
>
My understanding is this would not be a problem, as according to databook, the GPIO block clock is auto-enabled,
and also we are not doing any disabling in the driver for it.
> > +
> > + /* Read channel enabled status */
> > + buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm-
> >hwpwm));
> > + if (buff & KMB_PWM_EN_BIT)
> > + state->enabled = true;
> > + else
> > + state->enabled = false;
> > +
> > + /* Read period and duty cycle */
> > + buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm-
> >hwpwm));
> > + pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) *
> NSEC_PER_SEC;
> > + pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) *
> NSEC_PER_SEC;
> > + state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> > + state->period = DIV_ROUND_UP_ULL(pwm_h_count +
> pwm_l_count,
> > +clk_rate); }
> > +
> > +static int keembay_pwm_apply(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > + const struct pwm_state *state) {
> > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> > + struct pwm_state current_state;
> > + u16 pwm_h_count, pwm_l_count;
> > + unsigned long long div;
> > + unsigned long clk_rate;
> > + u32 pwm_count = 0;
> > +
> > + keembay_pwm_get_state(chip, pwm, ¤t_state);
>
> This can be moved after the test for state->polarity.
>
OK, will move this.
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -ENOSYS;
> > +
> > + if (!state->enabled && current_state.enabled) {
> > + keembay_pwm_disable(priv, pwm->hwpwm);
> > + return 0;
> > + }
>
> if (!state->enabled) {
> if (current_state.enabled)
> keembay_pwm_disable(priv, pwm->hwpwm);
> return 0;
> }
>
OK, I will update this.
> > +
> > + /*
> > + * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register
> contain
> > + * the high time of the waveform, while the last 16 bits contain
> > + * the low time of the waveform, in terms of clock cycles.
> > + *
> > + * high time = clock rate * duty cycle
> > + * low time = clock rate * (period - duty cycle)
> > + *
> > + * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> > + * high time = 500MHz * 30us = 0x3A98
> > + * low time = 500MHz * 20us = 0x2710
> > + * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> > + */
> > +
> > + clk_rate = clk_get_rate(priv->clk);
> > +
> > + /* Configure waveform high time */
> > + div = clk_rate * state->duty_cycle;
>
> Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578)
> state->duty_cycle is a 64 bit type. So div being unsigned long isn't
> big enough on some platforms.
>
div is 64-bit here, so I guess I can keep it as is?
> > + div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > + if (div > KMB_PWM_COUNT_MAX)
> > + return -ERANGE;
> > +
> > + pwm_h_count = div;
> > +
> > + /* Configure waveform low time */
> > + div = clk_rate * (state->period - state->duty_cycle);
> > + div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> > + if (div > KMB_PWM_COUNT_MAX)
> > + return -ERANGE;
> > +
> > + pwm_l_count = div;
> > +
> > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK,
> pwm_h_count) |
> > + FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> > +
> > + writel(pwm_count, priv->base +
> KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> > +
> > + if (state->enabled && !current_state.enabled)
> > + keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > + .owner = THIS_MODULE,
> > + .apply = keembay_pwm_apply,
> > + .get_state = keembay_pwm_get_state,
> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct keembay_pwm *priv;
> > + int ret, ch;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(priv->clk))
> > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to
> get
> > +clock\n");
> > +
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base))
> > + return PTR_ERR(priv->base);
> > +
> > + priv->chip.base = -1;
> > + priv->chip.dev = dev;
> > + priv->chip.ops = &keembay_pwm_ops;
> > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> > +
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret) {
> > + dev_err(dev, "Failed to add PWM chip: %pe\n",
> ERR_PTR(ret));
> > + return ret;
> > + }
> > +
> > + /* Ensure enable bit for each channel is cleared at boot */
> > + for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++)
> > + keembay_pwm_disable(priv, ch);
>
> .probe() is not supposed to change the state of the PWM.
>
Sorry, I think misunderstood one of your comments in V2 and added this.
The reset value of the enable bit (and all other bits) in the LEADIN register is 0, so this may not be needed.
If it's ok, I'll remove it.
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Thank you,
Vineetha
