* [PATCH v2 0/2] Add PWM support for Intel Keem Bay SoC @ 2020-07-18 14:47 vineetha.g.jaya.kumaran 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran 2020-07-18 14:47 ` [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vineetha.g.jaya.kumaran 0 siblings, 2 replies; 10+ messages in thread From: vineetha.g.jaya.kumaran @ 2020-07-18 14:47 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com> Hi, This patch set enables support for PWM on the Intel Keem Bay SoC. Keem Bay is an ARM based SoC, and the GPIO module allows configuration of 6 PWM outputs. Patch 1 adds the PWM driver and Patch 2 is for the required Device Tree bindings documentation. This driver was tested on the Keem Bay evaluation module board. Thank you. Best regards, Vineetha Changes since v1: -Updated licensing info, "clocks" property and example in DT bindings -Updated name of DT bindings document to match compatible string -Removed 1 patch for addition of new sysfs attribute "count" -Added support for COMPILE_TEST in Kconfig -Updated naming of defines and regmap attribute -Updated calculation of waveform high time and low time -Added range checking for waveform high/low time -Implemented .get_state -Removed register writes for lead-in and count values (left to default) -Updated register access to single-access -Folded keembay_pwm_enable/disable_channel, keembay_pwm_config_period/duty_cycle, and keembay_pwm_config into keembay_pwm_apply -Updated error messages/error codes -Removed pwm_disable from keembay_pwm_remove -Removed clk_prepare/clk_enable/clk_disable from driver Lai, Poey Seng (1): pwm: Add PWM driver for Intel Keem Bay Vineetha G. Jaya Kumaran (1): dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM .../devicetree/bindings/pwm/intel,keembay-pwm.yaml | 45 ++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-keembay.c | 239 +++++++++++++++++++++ 4 files changed, 294 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml create mode 100644 drivers/pwm/pwm-keembay.c -- 1.9.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-18 14:47 [PATCH v2 0/2] Add PWM support for Intel Keem Bay SoC vineetha.g.jaya.kumaran @ 2020-07-18 14:47 ` vineetha.g.jaya.kumaran 2020-07-18 20:41 ` kernel test robot ` (2 more replies) 2020-07-18 14:47 ` [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vineetha.g.jaya.kumaran 1 sibling, 3 replies; 10+ messages in thread From: vineetha.g.jaya.kumaran @ 2020-07-18 14:47 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko From: "Lai, Poey Seng" <poey.seng.lai@intel.com> Enable PWM support for the Intel Keem Bay SoC. Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> --- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-keembay.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 drivers/pwm/pwm-keembay.c diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index cb8d739..2b0419b 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 || 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. + 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 00000000..fa5fe95 --- /dev/null +++ b/drivers/pwm/pwm-keembay.c @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Keem Bay PWM driver + * + * Copyright (C) 2020 Intel Corporation + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> + */ + +#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 KMB_TOTAL_PWM_CHANNELS 6 +#define KMB_PWM_COUNT_MAX 65535 +#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_LEADIN0_OFFSET 0x00 +#define KMB_PWM_LEADIN1_OFFSET 0x04 +#define KMB_PWM_LEADIN2_OFFSET 0x08 +#define KMB_PWM_LEADIN3_OFFSET 0x0c +#define KMB_PWM_LEADIN4_OFFSET 0x10 +#define KMB_PWM_LEADIN5_OFFSET 0x14 + +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 + +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 reg, int ch) +{ + u32 buff, offset, tmp; + void __iomem *address; + + offset = reg + ch * 4; + address = priv->base + offset; + buff = readl(address); + tmp = buff & ~mask; + tmp |= FIELD_PREP(mask, val); + writel(tmp, address); +} + +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) +{ + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, + KMB_PWM_LEADIN0_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_LEADIN0_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); + + /* Read channel enabled status */ + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm->hwpwm * 4); + if (buff & KMB_PWM_EN_BIT) + state->enabled = true; + else + state->enabled = false; + + /* Read period and duty cycle*/ + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm->hwpwm * 4); + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * NSEC_PER_SEC; + state->duty_cycle = pwm_h_count / clk_rate; + state->period = (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; + + pwm_get_state(pwm, ¤t_state); + + if (!state->enabled && current_state.enabled) { + keembay_pwm_disable(priv, pwm->hwpwm); + return 0; + } + + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOSYS; + + /* + * The upper 16 bits of the KMB_PWM_HIGHLOWx_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 / NSEC_PER_SEC + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC + * + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 + * low time = (500000000 * 20000) / 1000000000 = 0x2710 + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 + */ + + clk_rate = clk_get_rate(priv->clk); + + /* Configure waveform high time */ + div = clk_rate * state->duty_cycle; + do_div(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_h_count = (u16)div; + + /* Configure waveform low time */ + div = clk_rate * (state->period - state->duty_cycle); + do_div(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_l_count = (u16)div; + + pwm_count |= pwm_h_count << 16; + pwm_count |= pwm_l_count; + + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, + pwm_count, KMB_PWM_HIGHLOW0_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; + + 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)) { + if (PTR_ERR(priv->clk) != -EPROBE_DEFER) + dev_err(dev, "Failed to get clock: %pe", priv->clk); + + return PTR_ERR(priv->clk); + } + + 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 < 0) { + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); + 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); + + return pwmchip_remove(&priv->chip); +} + +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"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran @ 2020-07-18 20:41 ` kernel test robot 2020-07-18 21:17 ` kernel test robot 2020-07-21 9:13 ` Uwe Kleine-König 2 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2020-07-18 20:41 UTC (permalink / raw) To: vineetha.g.jaya.kumaran, thierry.reding, u.kleine-koenig, robh+dt Cc: kbuild-all, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko [-- Attachment #1: Type: text/plain, Size: 1388 bytes --] Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on pwm/for-next] [also build test ERROR on robh/for-next v5.8-rc5 next-20200717] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/vineetha-g-jaya-kumaran-intel-com/Add-PWM-support-for-Intel-Keem-Bay-SoC/20200718-225115 base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next config: m68k-allyesconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): m68k-linux-ld: drivers/pwm/pwm-keembay.o: in function `keembay_pwm_get_state': >> pwm-keembay.c:(.text+0x8a): undefined reference to `__udivdi3' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 57523 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran 2020-07-18 20:41 ` kernel test robot @ 2020-07-18 21:17 ` kernel test robot 2020-07-21 9:13 ` Uwe Kleine-König 2 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2020-07-18 21:17 UTC (permalink / raw) To: vineetha.g.jaya.kumaran, thierry.reding, u.kleine-koenig, robh+dt Cc: kbuild-all, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko [-- Attachment #1: Type: text/plain, Size: 1406 bytes --] Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on pwm/for-next] [also build test ERROR on robh/for-next v5.8-rc5 next-20200717] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/vineetha-g-jaya-kumaran-intel-com/Add-PWM-support-for-Intel-Keem-Bay-SoC/20200718-225115 base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "__divdi3" [drivers/pwm/pwm-keembay.ko] undefined! >> ERROR: modpost: "__udivdi3" [drivers/pwm/pwm-keembay.ko] undefined! --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 57215 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran 2020-07-18 20:41 ` kernel test robot 2020-07-18 21:17 ` kernel test robot @ 2020-07-21 9:13 ` Uwe Kleine-König 2020-07-27 5:12 ` G Jaya Kumaran, Vineetha 2 siblings, 1 reply; 10+ messages in thread From: Uwe Kleine-König @ 2020-07-21 9:13 UTC (permalink / raw) To: vineetha.g.jaya.kumaran Cc: thierry.reding, robh+dt, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko [-- Attachment #1: Type: text/plain, Size: 9169 bytes --] Hello, On Sat, Jul 18, 2020 at 10:47:07PM +0800, vineetha.g.jaya.kumaran@intel.com wrote: > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > Enable PWM support for the Intel Keem Bay SoC. > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-keembay.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 249 insertions(+) > create mode 100644 drivers/pwm/pwm-keembay.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index cb8d739..2b0419b 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 || 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. > + > 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 00000000..fa5fe95 > --- /dev/null > +++ b/drivers/pwm/pwm-keembay.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Intel Keem Bay PWM driver > + * > + * Copyright (C) 2020 Intel Corporation > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > + */ If possible, please add a link here to documentation for this chip. > + > +#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 KMB_TOTAL_PWM_CHANNELS 6 > +#define KMB_PWM_COUNT_MAX 65535 If you write this as hexadecimal constant it is more obvious. > +#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_LEADIN0_OFFSET 0x00 > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > +#define KMB_PWM_LEADIN5_OFFSET 0x14 All but ..LEADIN0.. are unused. Is this maybe more useful to write as: #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) ? > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 ditto. > +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 reg, int ch) > +{ > + u32 buff, offset, tmp; > + void __iomem *address; > + > + offset = reg + ch * 4; > + address = priv->base + offset; > + buff = readl(address); > + tmp = buff & ~mask; > + tmp |= FIELD_PREP(mask, val); > + writel(tmp, address); > +} > + > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > + KMB_PWM_LEADIN0_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_LEADIN0_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); > + > + /* Read channel enabled status */ > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm->hwpwm * 4); > + if (buff & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle*/ Missing ' ' before closing */ > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm->hwpwm * 4); > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * NSEC_PER_SEC; pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC > + state->duty_cycle = pwm_h_count / clk_rate; > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; Please round up both values. > +} > + > +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; > + > + pwm_get_state(pwm, ¤t_state); Please check the hardware state, not the value cached in the PWM framework. > + if (!state->enabled && current_state.enabled) { > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; This must be checked before .enabled. That's because the expectation on .enabled = false .polarity = PWM_POLARITY_INVERSED is that the output gets constant high. > + /* > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_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. Just to be sure: Each period starts with the high time, right? > + * high time = clock rate * duty cycle / NSEC_PER_SEC > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > + * > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz you have to configure: high = 7999 low = 5334 > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Configure waveform high time */ > + div = clk_rate * state->duty_cycle; > + do_div(div, NSEC_PER_SEC); Can this overflow? > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_h_count = (u16)div; No need for this cast. > + /* Configure waveform low time */ > + div = clk_rate * (state->period - state->duty_cycle); > + do_div(div, NSEC_PER_SEC); Here the rounding is wrong. (See above example, currently you use low = 5333 here) > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_l_count = (u16)div; > + > + pwm_count |= pwm_h_count << 16; > + pwm_count |= pwm_l_count; pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > + > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > + pwm_count, KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); here the rwm-procedure is not necessary as all 32 bits are written anyhow. > + if (state->enabled && !current_state.enabled) > + keembay_pwm_enable(priv, pwm->hwpwm); > + > + return 0; > +} > [...] > +MODULE_ALIAS("platform:keembay"); This has to match your driver's name, so use: MODULE_ALIAS("platform:pwm-keembay"); > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > +MODULE_LICENSE("GPL v2"); In v1 you told that on reconfiguration the hardware completes the currently running period. Please document this in the driver, similar to e.g. pwm-sifive.c. You only ever write the enable bit in the leadin register. If there is something != 0 at boot this influences the correct behaviour of the driver, right? Further things to note there: - What is the behaviour on disable (usual candidates: freezes at current value, completes period and emits low, changes to High-Z)? - Can it do 0% and 100% duty ratio? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-21 9:13 ` Uwe Kleine-König @ 2020-07-27 5:12 ` G Jaya Kumaran, Vineetha 2020-07-29 6:20 ` G Jaya Kumaran, Vineetha 0 siblings, 1 reply; 10+ messages in thread From: G Jaya Kumaran, Vineetha @ 2020-07-27 5:12 UTC (permalink / raw) To: Uwe Kleine Konig Cc: thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, Shevchenko, Andriy Hi Uwe, Thank you for the feedback, will work on the changes for V3. > -----Original Message----- > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > owner@vger.kernel.org> On Behalf Of Uwe Kleine-König > Sent: Tuesday, July 21, 2020 5:14 PM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > Hello, > > On Sat, Jul 18, 2020 at 10:47:07PM +0800, > vineetha.g.jaya.kumaran@intel.com wrote: > > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > > > Enable PWM support for the Intel Keem Bay SoC. > > > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > > Signed-off-by: Vineetha G. Jaya Kumaran > > <vineetha.g.jaya.kumaran@intel.com> > > --- > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-keembay.c | 239 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 249 insertions(+) > > create mode 100644 drivers/pwm/pwm-keembay.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > cb8d739..2b0419b 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 || 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. > > + > > 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 00000000..fa5fe95 > > --- /dev/null > > +++ b/drivers/pwm/pwm-keembay.c > > @@ -0,0 +1,239 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Intel Keem Bay PWM driver > > + * > > + * Copyright (C) 2020 Intel Corporation > > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > > + */ > > If possible, please add a link here to documentation for this chip. > There is no documentation released publicly (yet), but once there is, I will add it in. > > + > > +#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 KMB_TOTAL_PWM_CHANNELS 6 > > +#define KMB_PWM_COUNT_MAX 65535 > > If you write this as hexadecimal constant it is more obvious. > > > +#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_LEADIN0_OFFSET 0x00 > > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > > +#define KMB_PWM_LEADIN5_OFFSET 0x14 > > All but ..LEADIN0.. are unused. Is this maybe more useful to write as: > > #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > > ? > Agreed, will change this for both KMB_PWM_HIGHLOW_OFFSET and KMB_PWM_LEADIN_OFFSET. > > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 > > ditto. > > > +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 reg, int ch) > > +{ > > + u32 buff, offset, tmp; > > + void __iomem *address; > > + > > + offset = reg + ch * 4; > > + address = priv->base + offset; > > + buff = readl(address); > > + tmp = buff & ~mask; > > + tmp |= FIELD_PREP(mask, val); > > + writel(tmp, address); > > +} > > + > > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) { > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > > + KMB_PWM_LEADIN0_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_LEADIN0_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); > > + > > + /* Read channel enabled status */ > > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm- > >hwpwm * 4); > > + if (buff & KMB_PWM_EN_BIT) > > + state->enabled = true; > > + else > > + state->enabled = false; > > + > > + /* Read period and duty cycle*/ > > Missing ' ' before closing */ > > > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm- > >hwpwm * 4); > > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * > NSEC_PER_SEC; > > pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * > NSEC_PER_SEC > > > + state->duty_cycle = pwm_h_count / clk_rate; > > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; > > Please round up both values. > > > +} > > + > > +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; > > + > > + pwm_get_state(pwm, ¤t_state); > > Please check the hardware state, not the value cached in the PWM > framework. > > > + if (!state->enabled && current_state.enabled) { > > + keembay_pwm_disable(priv, pwm->hwpwm); > > + return 0; > > + } > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -ENOSYS; > > This must be checked before .enabled. That's because the expectation on > > .enabled = false > .polarity = PWM_POLARITY_INVERSED > > is that the output gets constant high. > > > + /* > > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_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. > > Just to be sure: Each period starts with the high time, right? > Yes, each period starts with the high time. > > + * high time = clock rate * duty cycle / NSEC_PER_SEC > > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > > + * > > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 > > For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz you > have to configure: > > high = 7999 > low = 5334 > > > + */ > > + > > + clk_rate = clk_get_rate(priv->clk); > > + > > + /* Configure waveform high time */ > > + div = clk_rate * state->duty_cycle; > > + do_div(div, NSEC_PER_SEC); > > Can this overflow? > > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + pwm_h_count = (u16)div; > > No need for this cast. > > > + /* Configure waveform low time */ > > + div = clk_rate * (state->period - state->duty_cycle); > > + do_div(div, NSEC_PER_SEC); > > Here the rounding is wrong. (See above example, currently you use low = > 5333 here) > > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + pwm_l_count = (u16)div; > > + > > + pwm_count |= pwm_h_count << 16; > > + pwm_count |= pwm_l_count; > > pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > > > + > > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > > + pwm_count, > KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); > > here the rwm-procedure is not necessary as all 32 bits are written anyhow. > > > + if (state->enabled && !current_state.enabled) > > + keembay_pwm_enable(priv, pwm->hwpwm); > > + > > + return 0; > > +} > > [...] > > +MODULE_ALIAS("platform:keembay"); > > This has to match your driver's name, so use: > > MODULE_ALIAS("platform:pwm-keembay"); > > > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > MODULE_LICENSE("GPL > > +v2"); > > In v1 you told that on reconfiguration the hardware completes the currently > running period. Please document this in the driver, similar to e.g. pwm- > sifive.c. > OK, I will refer to that example and add this information in. > You only ever write the enable bit in the leadin register. If there is something > != 0 at boot this influences the correct behaviour of the driver, right? > That's right, I will add a check during probe to ensure the bit is set to 0 as default. > Further things to note there: > > - What is the behaviour on disable (usual candidates: freezes at > current value, completes period and emits low, changes to High-Z)? > For this case, the period does not get completed. As soon as the channel is disabled, the output goes low. > - Can it do 0% and 100% duty ratio? > Yes, we can set duty cycles of 0% and 100%. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-07-27 5:12 ` G Jaya Kumaran, Vineetha @ 2020-07-29 6:20 ` G Jaya Kumaran, Vineetha 0 siblings, 0 replies; 10+ messages in thread From: G Jaya Kumaran, Vineetha @ 2020-07-29 6:20 UTC (permalink / raw) To: Uwe Kleine Konig Cc: thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, Shevchenko, Andriy > -----Original Message----- > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > owner@vger.kernel.org> On Behalf Of G Jaya Kumaran, Vineetha > Sent: Monday, July 27, 2020 1:12 PM > To: Uwe Kleine Konig <u.kleine-koenig@pengutronix.de> > Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: RE: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > Hi Uwe, > > Thank you for the feedback, will work on the changes for V3. > > > -----Original Message----- > > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > > owner@vger.kernel.org> On Behalf Of Uwe Kleine-König > > Sent: Tuesday, July 21, 2020 5:14 PM > > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > > Cc: thierry.reding@gmail.com; robh+dt@kernel.org; linux- > > pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan > > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; > Shevchenko, > > Andriy <andriy.shevchenko@intel.com> > > Subject: Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > > > Hello, > > > > On Sat, Jul 18, 2020 at 10:47:07PM +0800, > > vineetha.g.jaya.kumaran@intel.com wrote: > > > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > > > > > Enable PWM support for the Intel Keem Bay SoC. > > > > > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > > > Signed-off-by: Vineetha G. Jaya Kumaran > > > <vineetha.g.jaya.kumaran@intel.com> > > > --- > > > drivers/pwm/Kconfig | 9 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-keembay.c | 239 > > > ++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 249 insertions(+) > > > create mode 100644 drivers/pwm/pwm-keembay.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > > cb8d739..2b0419b 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 || 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. > > > + > > > 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 00000000..fa5fe95 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-keembay.c > > > @@ -0,0 +1,239 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Intel Keem Bay PWM driver > > > + * > > > + * Copyright (C) 2020 Intel Corporation > > > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > > > + * Vineetha G. Jaya Kumaran > <vineetha.g.jaya.kumaran@intel.com> > > > + */ > > > > If possible, please add a link here to documentation for this chip. > > > > There is no documentation released publicly (yet), but once there is, I will > add it in. > > > > + > > > +#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 KMB_TOTAL_PWM_CHANNELS 6 > > > +#define KMB_PWM_COUNT_MAX 65535 > > > > If you write this as hexadecimal constant it is more obvious. > > > > > +#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_LEADIN0_OFFSET 0x00 > > > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > > > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > > > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > > > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > > > +#define KMB_PWM_LEADIN5_OFFSET 0x14 > > > > All but ..LEADIN0.. are unused. Is this maybe more useful to write as: > > > > #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > > > > ? > > > > Agreed, will change this for both KMB_PWM_HIGHLOW_OFFSET and > KMB_PWM_LEADIN_OFFSET. > > > > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > > > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > > > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > > > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > > > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > > > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 > > > > ditto. > > > > > +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 reg, int ch) > > > +{ > > > + u32 buff, offset, tmp; > > > + void __iomem *address; > > > + > > > + offset = reg + ch * 4; > > > + address = priv->base + offset; > > > + buff = readl(address); > > > + tmp = buff & ~mask; > > > + tmp |= FIELD_PREP(mask, val); > > > + writel(tmp, address); > > > +} > > > + > > > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) { > > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > > > + KMB_PWM_LEADIN0_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_LEADIN0_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); > > > + > > > + /* Read channel enabled status */ > > > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm- > > >hwpwm * 4); > > > + if (buff & KMB_PWM_EN_BIT) > > > + state->enabled = true; > > > + else > > > + state->enabled = false; > > > + > > > + /* Read period and duty cycle*/ > > > > Missing ' ' before closing */ > > > > > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm- > > >hwpwm * 4); > > > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > > > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * > > NSEC_PER_SEC; > > > > pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * > NSEC_PER_SEC > > > > > + state->duty_cycle = pwm_h_count / clk_rate; > > > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; > > > > Please round up both values. > > > > > +} > > > + > > > +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; > > > + > > > + pwm_get_state(pwm, ¤t_state); > > > > Please check the hardware state, not the value cached in the PWM > > framework. > > > > > + if (!state->enabled && current_state.enabled) { > > > + keembay_pwm_disable(priv, pwm->hwpwm); > > > + return 0; > > > + } > > > + > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > + return -ENOSYS; > > > > This must be checked before .enabled. That's because the expectation > > on > > > > .enabled = false > > .polarity = PWM_POLARITY_INVERSED > > > > is that the output gets constant high. > > > > > + /* > > > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_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. > > > > Just to be sure: Each period starts with the high time, right? > > > > Yes, each period starts with the high time. > > > > + * high time = clock rate * duty cycle / NSEC_PER_SEC > > > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > > > + * > > > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > > > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > > > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > > > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 > > > > For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz > > you have to configure: > > > > high = 7999 > > low = 5334 > > > > > + */ > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + > > > + /* Configure waveform high time */ > > > + div = clk_rate * state->duty_cycle; > > > + do_div(div, NSEC_PER_SEC); > > > > Can this overflow? > > Hi Uwe, sorry for missing this in my previous reply. Yes, it may exceed the 16-bit limit for the high/low time, depending on the selected duty cycle. There is a range check after, and -ERANGE will be returned if the high/low time range is exceeded. > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_h_count = (u16)div; > > > > No need for this cast. > > > > > + /* Configure waveform low time */ > > > + div = clk_rate * (state->period - state->duty_cycle); > > > + do_div(div, NSEC_PER_SEC); > > > > Here the rounding is wrong. (See above example, currently you use low > > = > > 5333 here) > > Will fix the rounding to the nearest integer for the high/low time. > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_l_count = (u16)div; > > > + > > > + pwm_count |= pwm_h_count << 16; > > > + pwm_count |= pwm_l_count; > > > > pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > > FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > > > > > + > > > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > > > + pwm_count, > > KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); > > > > here the rwm-procedure is not necessary as all 32 bits are written anyhow. > > > > > + if (state->enabled && !current_state.enabled) > > > + keembay_pwm_enable(priv, pwm->hwpwm); > > > + > > > + return 0; > > > +} > > > [...] > > > +MODULE_ALIAS("platform:keembay"); > > > > This has to match your driver's name, so use: > > > > MODULE_ALIAS("platform:pwm-keembay"); > > > > > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > > MODULE_LICENSE("GPL > > > +v2"); > > > > In v1 you told that on reconfiguration the hardware completes the > > currently running period. Please document this in the driver, similar > > to e.g. pwm- sifive.c. > > > > OK, I will refer to that example and add this information in. > > > You only ever write the enable bit in the leadin register. If there is > > something != 0 at boot this influences the correct behaviour of the driver, > right? > > > > That's right, I will add a check during probe to ensure the bit is set to 0 as > default. > > > Further things to note there: > > > > - What is the behaviour on disable (usual candidates: freezes at > > current value, completes period and emits low, changes to High-Z)? > > > > For this case, the period does not get completed. > As soon as the channel is disabled, the output goes low. > > > - Can it do 0% and 100% duty ratio? > > > > Yes, we can set duty cycles of 0% and 100%. > > > Best regards > > Uwe > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | https://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-07-18 14:47 [PATCH v2 0/2] Add PWM support for Intel Keem Bay SoC vineetha.g.jaya.kumaran 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran @ 2020-07-18 14:47 ` vineetha.g.jaya.kumaran 2020-07-23 17:47 ` Rob Herring 1 sibling, 1 reply; 10+ messages in thread From: vineetha.g.jaya.kumaran @ 2020-07-18 14:47 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC. Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> --- .../devicetree/bindings/pwm/intel,keembay-pwm.yaml | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml diff --git a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml new file mode 100644 index 00000000..e9388a7 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml @@ -0,0 +1,45 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2020 Intel Corporation +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Keem Bay PWM Device Tree Bindings + +maintainers: + - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + enum: + - intel,keembay-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + "#pwm-cells": + const: 2 + +required: + - compatible + - reg + - clocks + - '#pwm-cells' + +examples: + - | + #define KEEM_BAY_A53_GPIO + + pwm@203200a0 { + compatible = "intel,keembay-pwm"; + reg = <0x203200a0 0xe8>; + clocks = <&scmi_clk KEEM_BAY_A53_GPIO>; + #pwm-cells = <2>; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-07-18 14:47 ` [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vineetha.g.jaya.kumaran @ 2020-07-23 17:47 ` Rob Herring 2020-07-27 5:14 ` G Jaya Kumaran, Vineetha 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2020-07-23 17:47 UTC (permalink / raw) To: vineetha.g.jaya.kumaran Cc: thierry.reding, u.kleine-koenig, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko On Sat, Jul 18, 2020 at 10:47:08PM +0800, vineetha.g.jaya.kumaran@intel.com wrote: > From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com> > > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC. > > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > --- > .../devicetree/bindings/pwm/intel,keembay-pwm.yaml | 45 ++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > new file mode 100644 > index 00000000..e9388a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > @@ -0,0 +1,45 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2020 Intel Corporation > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Intel Keem Bay PWM Device Tree Bindings > + > +maintainers: > + - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + enum: > + - intel,keembay-pwm > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + "#pwm-cells": > + const: 2 > + > +required: > + - compatible > + - reg > + - clocks > + - '#pwm-cells' Add: additionalProperties: false With that, Reviewed-by: Rob Herring <robh@kernel.org> > + > +examples: > + - | > + #define KEEM_BAY_A53_GPIO > + > + pwm@203200a0 { > + compatible = "intel,keembay-pwm"; > + reg = <0x203200a0 0xe8>; > + clocks = <&scmi_clk KEEM_BAY_A53_GPIO>; > + #pwm-cells = <2>; > + }; > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-07-23 17:47 ` Rob Herring @ 2020-07-27 5:14 ` G Jaya Kumaran, Vineetha 0 siblings, 0 replies; 10+ messages in thread From: G Jaya Kumaran, Vineetha @ 2020-07-27 5:14 UTC (permalink / raw) To: Rob Herring Cc: thierry.reding, u.kleine-koenig, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, Shevchenko, Andriy > -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Friday, July 24, 2020 1:47 AM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: Re: [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for > Intel Keem Bay PWM > > On Sat, Jul 18, 2020 at 10:47:08PM +0800, > vineetha.g.jaya.kumaran@intel.com wrote: > > From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@intel.com> > > > > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC. > > > > Signed-off-by: Vineetha G. Jaya Kumaran > > <vineetha.g.jaya.kumaran@intel.com> > > --- > > .../devicetree/bindings/pwm/intel,keembay-pwm.yaml | 45 > > ++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > > b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > > new file mode 100644 > > index 00000000..e9388a7 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/intel,keembay- > pwm.yaml > > @@ -0,0 +1,45 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright > > +(C) 2020 Intel Corporation %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Intel Keem Bay PWM Device Tree Bindings > > + > > +maintainers: > > + - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > > + > > +allOf: > > + - $ref: pwm.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - intel,keembay-pwm > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + "#pwm-cells": > > + const: 2 > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - '#pwm-cells' > > Add: > > additionalProperties: false > > With that, > > Reviewed-by: Rob Herring <robh@kernel.org> > Thank you for reviewing, I will add this in V3. > > + > > +examples: > > + - | > > + #define KEEM_BAY_A53_GPIO > > + > > + pwm@203200a0 { > > + compatible = "intel,keembay-pwm"; > > + reg = <0x203200a0 0xe8>; > > + clocks = <&scmi_clk KEEM_BAY_A53_GPIO>; > > + #pwm-cells = <2>; > > + }; > > -- > > 1.9.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-29 6:20 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-18 14:47 [PATCH v2 0/2] Add PWM support for Intel Keem Bay SoC vineetha.g.jaya.kumaran 2020-07-18 14:47 ` [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay vineetha.g.jaya.kumaran 2020-07-18 20:41 ` kernel test robot 2020-07-18 21:17 ` kernel test robot 2020-07-21 9:13 ` Uwe Kleine-König 2020-07-27 5:12 ` G Jaya Kumaran, Vineetha 2020-07-29 6:20 ` G Jaya Kumaran, Vineetha 2020-07-18 14:47 ` [PATCH v2 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vineetha.g.jaya.kumaran 2020-07-23 17:47 ` Rob Herring 2020-07-27 5:14 ` G Jaya Kumaran, Vineetha
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).