Hello Uwe,
Thank you for taking the time to review this patch set.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Sunday, May 24, 2020 5:41 AM
> 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>; Shevchenko,
> Andriy <andriy.shevchenko@xxxxxxxxx>
> Subject: Re: [PATCH 2/3] pwm: Add PWM driver for Intel Keem Bay
>
> Hello,
>
> On Sun, May 17, 2020 at 09:52:39PM +0800,
> vineetha.g.jaya.kumaran@xxxxxxxxx wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@xxxxxxxxx>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
> >
> > 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 | 308
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 318 insertions(+)
> > create mode 100644 drivers/pwm/pwm-keembay.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > c13d146..5311975 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -569,4 +569,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
>
> Support for COMPILE_TEST would be nice here.
>
I will add this in v2.
> > + 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.
> > +
> > endif
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > a59c710..0c84ff2 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -55,3 +55,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
> > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c new
> > file mode 100644 index 0000000..39c7310
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-keembay.c
> > @@ -0,0 +1,308 @@
> > +// 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>
>
> Is there publically available documentation? If so, please add a link here.
>
Will check, and add it here if any are available.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define TOTAL_PWM_CHANNELS 6
> > +#define LEAD_IN_DEFAULT 0
> > +#define PWM_COUNT_MAX 65535
>
> please use a unique prefix for all defines in your driver. The names as they are
> are too generic.
>
I'll update the naming in v2, to match the mask defines.
> > +
> > +#define KEEMBAY_PWM_EN_BIT 31
> > +
> > +/* Mask */
> > +#define KEEMBAY_PWM_RPT_CNT_MASK GENMASK(15, 0)
> > +#define KEEMBAY_PWM_LEAD_IN_MASK GENMASK(30, 16)
> > +#define KEEMBAY_PWM_HIGH_MASK GENMASK(31, 16)
> > +#define KEEMBAY_PWM_LOW_MASK GENMASK(15, 0)
> > +
> > +/* PWM Register offset */
> > +#define PWM_LEADIN0_OFFSET 0x00
> > +#define PWM_LEADIN1_OFFSET 0x04
> > +#define PWM_LEADIN2_OFFSET 0x08
> > +#define PWM_LEADIN3_OFFSET 0x0c
> > +#define PWM_LEADIN4_OFFSET 0x10
> > +#define PWM_LEADIN5_OFFSET 0x14
> > +
> > +#define PWM_HIGHLOW0_OFFSET 0x20
> > +#define PWM_HIGHLOW1_OFFSET 0x24
> > +#define PWM_HIGHLOW2_OFFSET 0x28
> > +#define PWM_HIGHLOW3_OFFSET 0x2c
> > +#define PWM_HIGHLOW4_OFFSET 0x30
> > +#define PWM_HIGHLOW5_OFFSET 0x34
> > +
> > +struct keembay_pwm {
> > + struct pwm_chip chip;
> > + struct device *dev;
> > + struct clk *clk;
> > + void __iomem *regmap;
>
> I'd call this member "base" instead of "regmap" as the latter has a different
> meaning in the kernel.
>
OK, will update the naming.
> > +};
> > +
> > +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_enable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > + u32 buff, offset;
> > + void __iomem *address;
> > +
> > + offset = PWM_LEADIN0_OFFSET + ch * 4;
> > + address = priv->regmap + offset;
> > + buff = readl(address);
> > + buff |= BIT(KEEMBAY_PWM_EN_BIT);
> > + writel(buff, address);
> > +}
> > +
> > +static inline void keembay_pwm_disable_channel(struct keembay_pwm
> > +*priv, int ch) {
> > + u32 buff, offset;
> > + void __iomem *address;
> > +
> > + offset = PWM_LEADIN0_OFFSET + ch * 4;
> > + address = priv->regmap + offset;
> > + buff = readl(address);
> > + buff &= ~BIT(KEEMBAY_PWM_EN_BIT);
> > + writel(buff, address);
> > +}
>
> The two functions above could make use of keembay_pwm_update_bits().
>
Agreed, will update them to reflect this.
> > +
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32
> mask,
> > + u32 val, u32 reg, int ch)
> > +{
> > + u32 buff, offset, tmp;
> > + void __iomem *address;
> > +
> > + offset = reg + ch * 4;
> > + address = priv->regmap + offset;
> > + buff = readl(address);
> > + tmp = buff & ~mask;
> > + tmp |= FIELD_PREP(mask, val);
> > + writel(tmp, address);
> > +}
> > +
> > +static inline u32 keembay_pwm_config_min(struct keembay_pwm *priv) {
> > + unsigned long long divd, divs;
> > +
> > + divd = NSEC_PER_SEC;
> > + divs = clk_get_rate(priv->clk);
> > + do_div(divd, divs);
>
> Given that both NSEC_PER_SEC and the return value of clk_get_rate fit into an
> unsigned long, this seems too much.
>
Noted, will modify to use a different data type.
> > + return (u32)divd;
> > +}
> > +
> > +static inline u16 keembay_pwm_config_duty_cycle(struct keembay_pwm
> *priv,
> > + int duty_ns, u32 ns_min)
> > +{
> > + unsigned long long divd;
> > +
> > + divd = duty_ns;
> > + do_div(divd, ns_min);
> > + if ((u16)divd == 0)
> > + return 0;
> > +
> > + return (u16)divd - 1;
>
> missing range checking.
>
Will add this in.
> > +}
> > +
> > +static inline u16 keembay_pwm_config_period(struct keembay_pwm *priv,
> > + int period_ns,
> > + int duty_ns,
> > + u32 ns_min)
> > +{
> > + unsigned long long divd;
> > +
> > + divd = period_ns - duty_ns;
> > + do_div(divd, ns_min);
> > + if ((u16)divd == 0)
> > + return 0;
> > +
> > + return (u16)divd - 1;
>
> I wonder if both keembay_pwm_config_duty_cycle() and
> keembay_pwm_config_period() would be easier to understand if they were not
> separate functions but unrolled into their only user.
>
> As above there is no range checking.
>
OK - will move this into keembay_pwm_apply, and add the range check as well.
> > +}
> > +
> > +/*
> > + * For calculating "high time" register value:
> > + * High time (quotient only) = duty_cycle / ns_min
> > + *
> > + * For calculating "low time" register value:
> > + * Low time (quotient only) = (period - duty_cycle) / ns_min
> > + *
> > + * All values used are in nanoseconds for calculation.
> > + */
> > +static int keembay_pwm_config(struct keembay_pwm *priv, int ch,
> > + int duty_ns, int period_ns, int count)
>
> this function is only called once, I wonder if it can better be folded into its only
> user.
>
Will move this into keembay_pwm_apply.
> > +{
> > + u32 ns_min;
> > + u16 pwm_h_count, pwm_l_count;
> > +
> > + /* Write to lead in */
> > + keembay_pwm_update_bits(priv, KEEMBAY_PWM_LEAD_IN_MASK,
> > + LEAD_IN_DEFAULT,
> > + PWM_LEADIN0_OFFSET, ch);
>
> What is the effect of this "lead in"?
>
It is used to specify a low period between the trigger(enable bit set) and start of the waveform.
So the PWM output remains low for the number of clock cycles specified by this LEAD_IN value.
> > + /* Write the number of PWM pulse repetition */
> > + keembay_pwm_update_bits(priv, KEEMBAY_PWM_RPT_CNT_MASK,
> count,
> > + PWM_LEADIN0_OFFSET, ch);
>
> Is this racy? E.g. if the PWM is already running and just after you update the
> repetition count completes a period?
>
The repetition count will only take effect once the channel is disabled and reenabled again.
So, it will not affect the currently running waveform.
> This writes to the same register as the lead in above. Can this be done in a single
> register access?
>
Yes -- will change this to single access instead.
> > + /* Calculate min */
> > + ns_min = keembay_pwm_config_min(priv);
>
> What is "min"?
>
This refers to the ns equivalent of 1 clock cycle.
However, since the calculation for the high/low time will be changed in v2 (based on further comments below),
I will remove this.
> > + /* For duty cycle */
> > + pwm_h_count = keembay_pwm_config_duty_cycle(priv, duty_ns,
> ns_min);
>
> this is ineffective and less exact as possible. ns_min is the result of a division and
> in keembay_pwm_config_duty_cycle you divide by it.
>
Understood - will change the formula used to get the high time/low time to something like below instead:
value = clk_rate * state->duty_cycle;
pwm_h_count = do_div(value, NSEC_PER_SEC);
> > + /* Write to high registers */
> > + keembay_pwm_update_bits(priv, KEEMBAY_PWM_HIGH_MASK,
> pwm_h_count,
> > + PWM_HIGHLOW0_OFFSET, ch);
> > +
> > + /* For period */
> > + pwm_l_count = keembay_pwm_config_period(priv, period_ns, duty_ns,
> > + ns_min);
> > +
> > + /* Write to low registers */
> > + keembay_pwm_update_bits(priv, KEEMBAY_PWM_LOW_MASK,
> pwm_l_count,
> > + PWM_HIGHLOW0_OFFSET, ch);
>
> Can you please explain in a comment what's the resulting wave form for a given
> value of this register?
>
Okay, I will add a comment in the driver explaining this.
In summary, the upper/lower 16 bits of the register represent the waveform's high/low time in number of clock cycles - 1.
e.g. for a period of 70000ns, duty cycle of 35000ns and clock rate of 500MHz:
The resulting register value would be 0x445B445B.
> Can writing h_count and l_count be combined in a single register access?
> (And if not, what happens if a period completes between the two
> updates?)
>
> How does the hardware behave on a change here? Is the currently running
> period completed before the new values take effect?
>
The write can be combined, I will make this change for v2.
As for the HW behaviour, yes, the current period will
be completed before the new configuration takes effect.
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_pwm_enable(struct keembay_pwm *priv, int ch) {
> > + int ret;
> > +
> > + ret = clk_enable(priv->clk);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enable channel */
> > + keembay_pwm_enable_channel(priv, ch);
> > +
> > + return 0;
> > +}
> > +
> > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) {
> > + /* Disable channel */
> > + keembay_pwm_disable_channel(priv, ch);
> > +
> > + clk_disable(priv->clk);
> > +}
> > +
> > +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);
> > +
> > + if (!state->enabled && pwm_is_enabled(pwm)) {
>
> Please don't call API functions in the driver.
>
Noted, will change this in v2.
> > + keembay_pwm_disable(priv, pwm->hwpwm);
>
> Is a currently running period completed here? How does the output behave on
> disable? (i.e. does it go in its inactive state?)
>
Upon disable (the "enable" bit is cleared), the output signal is stopped/inactive.
The currently running period will not be completed.
> > + return 0;
> > + }
> > +
> > + if (state->count > PWM_COUNT_MAX)
> > + return -EINVAL;
> > +
> > + if (state->polarity != pwm_get_polarity(pwm))
>
> Using:
>
> if (state->polarity != PWM_POLARITY_NORMAL)
>
> seems both more robust and easier to understand.
>
Will update this.
> > + return -ENOSYS;
> > +
> > + keembay_pwm_config(priv, pwm->hwpwm, state->duty_cycle,
> > + state->period, state->count);
> > +
> > + if (state->enabled && !pwm_is_enabled(pwm))
> > + return keembay_pwm_enable(priv, pwm->hwpwm);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops keembay_pwm_ops = {
> > + .owner = THIS_MODULE,
> > + .apply = keembay_pwm_apply,
>
> Please implement .get_state().
>
Will add this in v2.
> > +};
> > +
> > +static int keembay_pwm_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct keembay_pwm *priv;
> > + int ret;
> > +
> > + 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))
>
> Error message please. (Something like:
>
> int ret = PTR_ERR(ret);
>
> if (ret != -EPROBE_DEFER)
> dev_err(pdev->dev, "Failed to get clk: %pE\n", priv-
> >clk);
>
> return ret;
> )
>
Noted, I will add this in.
> > + return PTR_ERR(priv->clk);
> > +
> > + /*
> > + * Prepare clock here, and carry out clock enabling/disabling
> > + * during channel enablement/disablement.
> > + * The clock will not be unprepared due to shared usage with GPIO.
>
> What has this clock to do with GPIO? If the GPIO drivers gets and enables this
> clock itself there should be no problem.
>
The PWM outputs are actually part of the GPIO block, hence use the same clock as it.
The current behaviour is unpreparing causes the system to hang - I will cross-check regarding this.
> > + */
> > + ret = clk_prepare(priv->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to prepare PWM clock\n");
>
> Please include the error code in the message (preferably using %pE as above).
>
Noted, will update.
> > + return ret;
> > + }
> > +
> > + priv->regmap = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->regmap))
> > + return PTR_ERR(priv->regmap);
> > +
> > + priv->chip.base = -1;
> > + priv->chip.dev = dev;
> > + priv->chip.ops = &keembay_pwm_ops;
> > + priv->chip.npwm = TOTAL_PWM_CHANNELS;
> > +
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to add PWM chip: %d\n", ret);
>
> %pE please.
>
Will update.
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > + struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > + unsigned int i;
> > +
> > + for (i = 0; i < priv->chip.npwm; i++)
> > + pwm_disable(&priv->chip.pwms[i]);
>
> That's wrong. It's the responsibility of the consumer to disable the clock.
>
Noted, will remove this.
> > +
> > + pwmchip_remove(&priv->chip);
>
> clk_unprepare missing here.
>
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id keembay_pwm_of_match[] = {
> > + { .compatible = "intel,keembay-pwm" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match);
> > +
> > +static struct platform_driver keembay_pwm_driver = {
> > + .probe = keembay_pwm_probe,
> > + .remove = keembay_pwm_remove,
> > + .driver = {
> > + .name = "pwm-keembay",
> > + .of_match_table = keembay_pwm_of_match,
> > + },
> > +};
> > +module_platform_driver(keembay_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:keembay");
> > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
> MODULE_LICENSE("GPL
> > +v2");
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
