| Message ID | 20240702083848.1947449-1-william.qiu@starfivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v13] pwm: opencores: Add PWM driver support | expand |
> -----Original Message----- > From: William Qiu > Sent: 2024年7月2日 16:39 > To: linux-kernel@vger.kernel.org; linux-pwm@vger.kernel.org > Cc: Uwe Kleine-König <ukleinek@kernel.org>; Hal Feng > <hal.feng@starfivetech.com>; Philipp Zabel <p.zabel@pengutronix.de>; William > Qiu <william.qiu@starfivetech.com> > Subject: [PATCH v13] pwm: opencores: Add PWM driver support > > Add driver for OpenCores PWM Controller. And add compatibility code which > based on StarFive SoC. > > Co-developed-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > --- > MAINTAINERS | 7 ++ > drivers/pwm/Kconfig | 12 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-ocores.c | 239 > +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 259 insertions(+) > create mode 100644 drivers/pwm/pwm-ocores.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3c4fdf74a3f9..3b547ede2ce5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16824,6 +16824,13 @@ F: Documentation/i2c/busses/i2c-ocores.rst > F: drivers/i2c/busses/i2c-ocores.c > F: include/linux/platform_data/i2c-ocores.h > > +OPENCORES PWM DRIVER > +M: William Qiu <william.qiu@starfivetech.com> > +M: Hal Feng <hal.feng@starfivetech.com> > +S: Supported > +F: Documentation/devicetree/bindings/pwm/opencores,pwm.yaml > +F: drivers/pwm/pwm-ocores.c > + > OPENRISC ARCHITECTURE > M: Jonas Bonn <jonas@southpole.se> > M: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > 1dd7921194f5..42158bc1c8bc 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -440,6 +440,18 @@ config PWM_NTXEC > controller found in certain e-book readers designed by the original > design manufacturer Netronix. > > +config PWM_OCORES > + tristate "OpenCores PTC PWM support" > + depends on HAS_IOMEM && OF > + depends on COMMON_CLK && RESET_CONTROLLER > + depends on ARCH_STARFIVE || COMPILE_TEST > + help > + If you say yes to this option, support will be included for the > + OpenCores PWM. For details see https://opencores.org/projects/ptc. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-ocores. > + > config PWM_OMAP_DMTIMER > tristate "OMAP Dual-Mode Timer PWM support" > depends on OF > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > 90913519f11a..7a44d8afe044 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE) += > pwm-microchip-core.o > obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o > obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o > +obj-$(CONFIG_PWM_OCORES) += pwm-ocores.o > obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o > obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c new file > mode 100644 index 000000000000..c8f08aa14e44 > --- /dev/null > +++ b/drivers/pwm/pwm-ocores.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * OpenCores PWM Driver > + * > + * https://opencores.org/projects/ptc > + * > + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd. > + * > + * Limitations: > + * - The hardware only supports inverted polarity. > + * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock > frequency) ns. > + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock > frequency) ns. > + * - The hardware is set to a low level immediately when disabledThe hardware > is set to > + * a low level immediately when disabled. > + * - The hardware will have a conversion cycle when reconfiguring. > + */ > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/reset.h> > +#include <linux/slab.h> > + > +/* OpenCores Register offsets */ > +#define REG_OCPWM_CNTR 0x0 > +#define REG_OCPWM_HRC 0x4 > +#define REG_OCPWM_LRC 0x8 > +#define REG_OCPWM_CTRL 0xC > + > +/* OCPWM_CTRL register bits*/ > +#define REG_OCPWM_CNTR_EN BIT(0) > +#define REG_OCPWM_CNTR_ECLK BIT(1) > +#define REG_OCPWM_CNTR_NEC BIT(2) > +#define REG_OCPWM_CNTR_OE BIT(3) > +#define REG_OCPWM_CNTR_SIGNLE BIT(4) > +#define REG_OCPWM_CNTR_INTE BIT(5) > +#define REG_OCPWM_CNTR_INT BIT(6) > +#define REG_OCPWM_CNTR_RST BIT(7) > +#define REG_OCPWM_CNTR_CAPTE BIT(8) > + > +struct ocores_pwm_device { > + const struct ocores_pwm_data *data; > + void __iomem *regs; > + u32 clk_rate; /* PWM APB clock frequency */ }; > + > +struct ocores_pwm_data { > + void __iomem *(*get_ch_base)(void __iomem *base, unsigned int > +channel); }; > + > +static inline u32 ocores_pwm_readl(struct ocores_pwm_device *ddata, > + unsigned int channel, > + unsigned int offset) > +{ > + void __iomem *base = ddata->data->get_ch_base ? > + ddata->data->get_ch_base(ddata->regs, channel) : > ddata->regs; > + > + return readl(base + offset); > +} > + > +static inline void ocores_pwm_writel(struct ocores_pwm_device *ddata, > + unsigned int channel, > + unsigned int offset, u32 val) > +{ > + void __iomem *base = ddata->data->get_ch_base ? > + ddata->data->get_ch_base(ddata->regs, channel) : > ddata->regs; > + > + writel(val, base + offset); > +} > + > +static inline struct ocores_pwm_device *chip_to_ocores(struct pwm_chip > +*chip) { > + return pwmchip_get_drvdata(chip); > +} > + > +static void __iomem *starfive_get_ch_base(void __iomem *base, > + unsigned int channel) > +{ > + unsigned int offset = (channel & 4) << 13 | (channel & 3) << 4; > + > + return base + offset; > +} > + > +static int ocores_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct ocores_pwm_device *ddata = chip_to_ocores(chip); > + u32 period_data, duty_data, ctrl_data; > + > + period_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_LRC); > + duty_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_HRC); > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > + > + state->period = DIV_ROUND_UP_ULL((u64)period_data * NSEC_PER_SEC, > ddata->clk_rate); > + state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty_data * > NSEC_PER_SEC, ddata->clk_rate); > + state->polarity = PWM_POLARITY_INVERSED; > + state->enabled = (ctrl_data & REG_OCPWM_CNTR_EN) ? true : false; > + > + return 0; > +} > + > +static int ocores_pwm_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct ocores_pwm_device *ddata = chip_to_ocores(chip); > + u32 ctrl_data = 0; > + u64 period_data, duty_data; > + > + if (state->polarity != PWM_POLARITY_INVERSED) > + return -EINVAL; > + > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, > NSEC_PER_SEC); > + if (!period_data) > + return -EINVAL; > + > + if (period_data > U32_MAX) > + period_data = U32_MAX; > + > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > + > + duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, > NSEC_PER_SEC); > + if (duty_data <= U32_MAX) > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, > (u32)duty_data); > + else if (duty_data > U32_MAX) > + duty_data = U32_MAX; > + else > + return -EINVAL; > + > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > + if (state->enabled) > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data | REG_OCPWM_CNTR_EN | > REG_OCPWM_CNTR_OE); > + else > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data & ~(REG_OCPWM_CNTR_EN | > REG_OCPWM_CNTR_OE)); > + > + return 0; > +} > + > +static const struct pwm_ops ocores_pwm_ops = { > + .get_state = ocores_pwm_get_state, > + .apply = ocores_pwm_apply, > +}; > + > +static const struct ocores_pwm_data starfive_pwm_data = { > + .get_ch_base = starfive_get_ch_base, > +}; > + > +static const struct of_device_id ocores_pwm_of_match[] = { > + { .compatible = "opencores,pwm-v1" }, > + { .compatible = "starfive,jh7100-pwm", .data = &starfive_pwm_data}, > + { .compatible = "starfive,jh7110-pwm", .data = &starfive_pwm_data}, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, ocores_pwm_of_match); > + > +static void ocores_pwm_reset_control_assert(void *data) { > + reset_control_assert(data); > +} > + > +static int ocores_pwm_probe(struct platform_device *pdev) { > + const struct of_device_id *id; > + struct device *dev = &pdev->dev; > + struct ocores_pwm_device *ddata; > + struct pwm_chip *chip; > + struct clk *clk; > + struct reset_control *rst; > + int ret; > + > + id = of_match_device(ocores_pwm_of_match, dev); > + if (!id) > + return -EINVAL; > + > + chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata)); > + if (IS_ERR(chip)) > + return -ENOMEM; > + > + ddata = chip_to_ocores(chip); > + ddata->data = id->data; > + chip->ops = &ocores_pwm_ops; > + > + ddata->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(ddata->regs)) > + return dev_err_probe(dev, PTR_ERR(ddata->regs), > + "Unable to map IO resources\n"); > + > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), > + "Unable to get pwm's clock\n"); > + > + ret = devm_clk_rate_exclusive_get(dev, clk); > + if (ret) > + return ret; > + > + rst = devm_reset_control_get_optional_exclusive(dev, NULL); > + if (IS_ERR(rst)) > + return dev_err_probe(dev, PTR_ERR(rst), > + "Unable to get pwm's reset\n"); > + > + reset_control_deassert(rst); > + > + ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, > rst); > + if (ret) > + return ret; > + > + ddata->clk_rate = clk_get_rate(clk); > + if (ddata->clk_rate > NSEC_PER_SEC) > + return dev_err_probe(dev, ddata->clk_rate, > + "Unable to get clock's rate\n"); > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Could not register PWM chip\n"); > + > + return 0; > +} > + > +static struct platform_driver ocores_pwm_driver = { > + .probe = ocores_pwm_probe, > + .driver = { > + .name = "ocores-pwm", > + .of_match_table = ocores_pwm_of_match, > + }, > +}; > +module_platform_driver(ocores_pwm_driver); > + > +MODULE_AUTHOR("Jieqin Chen"); > +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>"); > +MODULE_DESCRIPTION("OpenCores PTC PWM driver"); > MODULE_LICENSE("GPL"); > -- > 2.34.1 Hi Uwe, Could you please help me review this patch series to see if there is anything that needs to be modified? Thanks for taking time to review this patch series. Best Regards, William
Hello William, On Tue, Jul 02, 2024 at 04:38:48PM +0800, William Qiu wrote: > Add driver for OpenCores PWM Controller. And add compatibility code > which based on StarFive SoC. > > Co-developed-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com> > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > --- > MAINTAINERS | 7 ++ > drivers/pwm/Kconfig | 12 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-ocores.c | 239 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 259 insertions(+) > create mode 100644 drivers/pwm/pwm-ocores.c > [...] > diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c > new file mode 100644 > index 000000000000..c8f08aa14e44 > --- /dev/null > +++ b/drivers/pwm/pwm-ocores.c > @@ -0,0 +1,239 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * OpenCores PWM Driver > + * > + * https://opencores.org/projects/ptc > + * > + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd. > + * > + * Limitations: > + * - The hardware only supports inverted polarity. > + * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock frequency) ns. > + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock frequency) ns. Nitpick: s/ ns// > + * - The hardware is set to a low level immediately when disabledThe hardware is set to > + * a low level immediately when disabled. This sentence is duplicated somehow. Maybe also: s/hardware/output/ ? > + * - The hardware will have a conversion cycle when reconfiguring. I don't understand that. > + */ > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/reset.h> > +#include <linux/slab.h> > + > +/* OpenCores Register offsets */ > +#define REG_OCPWM_CNTR 0x0 > +#define REG_OCPWM_HRC 0x4 > +#define REG_OCPWM_LRC 0x8 > +#define REG_OCPWM_CTRL 0xC > + > +/* OCPWM_CTRL register bits*/ > +#define REG_OCPWM_CNTR_EN BIT(0) > +#define REG_OCPWM_CNTR_ECLK BIT(1) > +#define REG_OCPWM_CNTR_NEC BIT(2) > +#define REG_OCPWM_CNTR_OE BIT(3) > +#define REG_OCPWM_CNTR_SIGNLE BIT(4) > +#define REG_OCPWM_CNTR_INTE BIT(5) > +#define REG_OCPWM_CNTR_INT BIT(6) > +#define REG_OCPWM_CNTR_RST BIT(7) > +#define REG_OCPWM_CNTR_CAPTE BIT(8) > + > +struct ocores_pwm_device { > + const struct ocores_pwm_data *data; I admit I didn't try to compile this, but I wonder if this doesn't result in a compiler warning given that struct ocores_pwm_data is only defined below. > + void __iomem *regs; > + u32 clk_rate; /* PWM APB clock frequency */ > +}; > + > +struct ocores_pwm_data { > + void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel); > +}; > [...] > +static int ocores_pwm_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct ocores_pwm_device *ddata = chip_to_ocores(chip); > + u32 ctrl_data = 0; > + u64 period_data, duty_data; > + > + if (state->polarity != PWM_POLARITY_INVERSED) > + return -EINVAL; > + > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC); > + if (!period_data) > + return -EINVAL; > + > + if (period_data > U32_MAX) > + period_data = U32_MAX; > + > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); s/0x8/REG_OCPWM_LRC/ ? That cast can be dropped. > + duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, NSEC_PER_SEC); > + if (duty_data <= U32_MAX) > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, (u32)duty_data); > + else if (duty_data > U32_MAX) > + duty_data = U32_MAX; > + else > + return -EINVAL; That looks wrong. If duty_data > U32_MAX you assign duty_data but don't reuse this variable later. > + > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); > + if (state->enabled) > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data | REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE); > + else > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data & ~(REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE)); > + > + return 0; > +} > [...] > +static int ocores_pwm_probe(struct platform_device *pdev) > +{ > + const struct of_device_id *id; > + struct device *dev = &pdev->dev; > + struct ocores_pwm_device *ddata; > + struct pwm_chip *chip; > + struct clk *clk; > + struct reset_control *rst; > + int ret; > + > + id = of_match_device(ocores_pwm_of_match, dev); > + if (!id) > + return -EINVAL; > + > + chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata)); > + if (IS_ERR(chip)) > + return -ENOMEM; > + > + ddata = chip_to_ocores(chip); > + ddata->data = id->data; > + chip->ops = &ocores_pwm_ops; > + > + ddata->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(ddata->regs)) > + return dev_err_probe(dev, PTR_ERR(ddata->regs), > + "Unable to map IO resources\n"); > + > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), > + "Unable to get pwm's clock\n"); > + > + ret = devm_clk_rate_exclusive_get(dev, clk); > + if (ret) > + return ret; > + > + rst = devm_reset_control_get_optional_exclusive(dev, NULL); > + if (IS_ERR(rst)) > + return dev_err_probe(dev, PTR_ERR(rst), > + "Unable to get pwm's reset\n"); > + > + reset_control_deassert(rst); I think you want to check the return value of reset_control_deassert(). > + ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, rst); > + if (ret) > + return ret; > + > + ddata->clk_rate = clk_get_rate(clk); > + if (ddata->clk_rate > NSEC_PER_SEC) > + return dev_err_probe(dev, ddata->clk_rate, > + "Unable to get clock's rate\n"); Best regards Uwe
> -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > Sent: 2024年8月7日 1:19 > To: William Qiu <william.qiu@starfivetech.com> > Cc: linux-kernel@vger.kernel.org; linux-pwm@vger.kernel.org; Hal Feng > <hal.feng@starfivetech.com>; Philipp Zabel <p.zabel@pengutronix.de> > Subject: Re: [PATCH v13] pwm: opencores: Add PWM driver support > > Hello William, > > On Tue, Jul 02, 2024 at 04:38:48PM +0800, William Qiu wrote: > > Add driver for OpenCores PWM Controller. And add compatibility code > > which based on StarFive SoC. > > > > Co-developed-by: Hal Feng <hal.feng@starfivetech.com> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com> > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > > --- > > MAINTAINERS | 7 ++ > > drivers/pwm/Kconfig | 12 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-ocores.c | 239 > > +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 259 insertions(+) > > create mode 100644 drivers/pwm/pwm-ocores.c [...] diff --git > > a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c new file mode > > 100644 index 000000000000..c8f08aa14e44 > > --- /dev/null > > +++ b/drivers/pwm/pwm-ocores.c > > @@ -0,0 +1,239 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * OpenCores PWM Driver > > + * > > + * https://opencores.org/projects/ptc > > + * > > + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd. > > + * > > + * Limitations: > > + * - The hardware only supports inverted polarity. > > + * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock > frequency) ns. > > + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb > clock frequency) ns. > > Nitpick: s/ ns// > Will update. > > + * - The hardware is set to a low level immediately when disabledThe > hardware is set to > > + * a low level immediately when disabled. > > This sentence is duplicated somehow. Maybe also: s/hardware/output/ ? > Will update. > > + * - The hardware will have a conversion cycle when reconfiguring. > > I don't understand that. > . For example, after the PWM duty cycle is changed from 50% to 80%, it is not a direct change, but there is a conversion period. The waveform during the conversion period will vary depending on whether it is high or low when reconfigured. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/reset.h> > > +#include <linux/slab.h> > > + > > +/* OpenCores Register offsets */ > > +#define REG_OCPWM_CNTR 0x0 > > +#define REG_OCPWM_HRC 0x4 > > +#define REG_OCPWM_LRC 0x8 > > +#define REG_OCPWM_CTRL 0xC > > + > > +/* OCPWM_CTRL register bits*/ > > +#define REG_OCPWM_CNTR_EN BIT(0) > > +#define REG_OCPWM_CNTR_ECLK BIT(1) > > +#define REG_OCPWM_CNTR_NEC BIT(2) > > +#define REG_OCPWM_CNTR_OE BIT(3) > > +#define REG_OCPWM_CNTR_SIGNLE BIT(4) > > +#define REG_OCPWM_CNTR_INTE BIT(5) > > +#define REG_OCPWM_CNTR_INT BIT(6) > > +#define REG_OCPWM_CNTR_RST BIT(7) > > +#define REG_OCPWM_CNTR_CAPTE BIT(8) > > + > > +struct ocores_pwm_device { > > + const struct ocores_pwm_data *data; > > I admit I didn't try to compile this, but I wonder if this doesn't result in a > compiler warning given that struct ocores_pwm_data is only defined below. > Will update. > > + void __iomem *regs; > > + u32 clk_rate; /* PWM APB clock frequency */ }; > > + > > +struct ocores_pwm_data { > > + void __iomem *(*get_ch_base)(void __iomem *base, unsigned int > > +channel); }; > > [...] > > +static int ocores_pwm_apply(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_state *state) { > > + struct ocores_pwm_device *ddata = chip_to_ocores(chip); > > + u32 ctrl_data = 0; > > + u64 period_data, duty_data; > > + > > + if (state->polarity != PWM_POLARITY_INVERSED) > > + return -EINVAL; > > + > > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, > NSEC_PER_SEC); > > + if (!period_data) > > + return -EINVAL; > > + > > + if (period_data > U32_MAX) > > + period_data = U32_MAX; > > + > > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > > s/0x8/REG_OCPWM_LRC/ ? > Will update. > That cast can be dropped. > > > + duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, > NSEC_PER_SEC); > > + if (duty_data <= U32_MAX) > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, > (u32)duty_data); > > + else if (duty_data > U32_MAX) > > + duty_data = U32_MAX; > > + else > > + return -EINVAL; > > That looks wrong. If duty_data > U32_MAX you assign duty_data but don't > reuse this variable later. > Will update. > > + > > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > > + if (state->enabled) > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > > + ctrl_data | REG_OCPWM_CNTR_EN | > REG_OCPWM_CNTR_OE); > > + else > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > > + ctrl_data & ~(REG_OCPWM_CNTR_EN | > REG_OCPWM_CNTR_OE)); > > + > > + return 0; > > +} > > [...] > > +static int ocores_pwm_probe(struct platform_device *pdev) { > > + const struct of_device_id *id; > > + struct device *dev = &pdev->dev; > > + struct ocores_pwm_device *ddata; > > + struct pwm_chip *chip; > > + struct clk *clk; > > + struct reset_control *rst; > > + int ret; > > + > > + id = of_match_device(ocores_pwm_of_match, dev); > > + if (!id) > > + return -EINVAL; > > + > > + chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata)); > > + if (IS_ERR(chip)) > > + return -ENOMEM; > > + > > + ddata = chip_to_ocores(chip); > > + ddata->data = id->data; > > + chip->ops = &ocores_pwm_ops; > > + > > + ddata->regs = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(ddata->regs)) > > + return dev_err_probe(dev, PTR_ERR(ddata->regs), > > + "Unable to map IO resources\n"); > > + > > + clk = devm_clk_get_enabled(dev, NULL); > > + if (IS_ERR(clk)) > > + return dev_err_probe(dev, PTR_ERR(clk), > > + "Unable to get pwm's clock\n"); > > + > > + ret = devm_clk_rate_exclusive_get(dev, clk); > > + if (ret) > > + return ret; > > + > > + rst = devm_reset_control_get_optional_exclusive(dev, NULL); > > + if (IS_ERR(rst)) > > + return dev_err_probe(dev, PTR_ERR(rst), > > + "Unable to get pwm's reset\n"); > > + > > + reset_control_deassert(rst); > > I think you want to check the return value of reset_control_deassert(). > Will add. > > + ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, > rst); > > + if (ret) > > + return ret; > > + > > + ddata->clk_rate = clk_get_rate(clk); > > + if (ddata->clk_rate > NSEC_PER_SEC) > > + return dev_err_probe(dev, ddata->clk_rate, > > + "Unable to get clock's rate\n"); > > Best regards > Uwe
Hello William, On Wed, Aug 07, 2024 at 03:41:15AM +0000, William Qiu wrote: > > -----Original Message----- > > From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > [...] > > On Tue, Jul 02, 2024 at 04:38:48PM +0800, William Qiu wrote: > > > + * - The hardware will have a conversion cycle when reconfiguring. > > > > I don't understand that. > > . > For example, after the PWM duty cycle is changed from 50% to 80%, > it is not a direct change, but there is a conversion period. The waveform > during the conversion period will vary depending on whether it is high or > low when reconfigured. Is it "just" that the new settings become active immediately and so if you change the relative duty_cycle from 50% to 80% and the current period is "done" between 50% and 80% you get three instead of one level change during that period? If it's that, I'd describe that as: When configuration changes are done, they get active immediately without resetting the counter. This might result in one period affected by both old and new settings. Best regards Uwe
> -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > Sent: 2024年8月7日 13:57 > To: William Qiu <william.qiu@starfivetech.com> > Cc: linux-kernel@vger.kernel.org; linux-pwm@vger.kernel.org; Hal Feng > <hal.feng@starfivetech.com>; Philipp Zabel <p.zabel@pengutronix.de> > Subject: Re: [PATCH v13] pwm: opencores: Add PWM driver support > > Hello William, > > On Wed, Aug 07, 2024 at 03:41:15AM +0000, William Qiu wrote: > > > -----Original Message----- > > > From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> [...] On Tue, > > > Jul 02, 2024 at 04:38:48PM +0800, William Qiu wrote: > > > > + * - The hardware will have a conversion cycle when reconfiguring. > > > > > > I don't understand that. > > > . > > For example, after the PWM duty cycle is changed from 50% to 80%, it > > is not a direct change, but there is a conversion period. The waveform > > during the conversion period will vary depending on whether it is high > > or low when reconfigured. > > Is it "just" that the new settings become active immediately and so if you > change the relative duty_cycle from 50% to 80% and the current period is > "done" between 50% and 80% you get three instead of one level change during > that period? > > If it's that, I'd describe that as: When configuration changes are done, they get > active immediately without resetting the counter. This might result in one > period affected by both old and new settings. > > Best regards > Uwe Fine, I'll describe just like that. Best Regards, William
diff --git a/MAINTAINERS b/MAINTAINERS index 3c4fdf74a3f9..3b547ede2ce5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16824,6 +16824,13 @@ F: Documentation/i2c/busses/i2c-ocores.rst F: drivers/i2c/busses/i2c-ocores.c F: include/linux/platform_data/i2c-ocores.h +OPENCORES PWM DRIVER +M: William Qiu <william.qiu@starfivetech.com> +M: Hal Feng <hal.feng@starfivetech.com> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <jonas@southpole.se> M: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 1dd7921194f5..42158bc1c8bc 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -440,6 +440,18 @@ config PWM_NTXEC controller found in certain e-book readers designed by the original design manufacturer Netronix. +config PWM_OCORES + tristate "OpenCores PTC PWM support" + depends on HAS_IOMEM && OF + depends on COMMON_CLK && RESET_CONTROLLER + depends on ARCH_STARFIVE || COMPILE_TEST + help + If you say yes to this option, support will be included for the + OpenCores PWM. For details see https://opencores.org/projects/ptc. + + To compile this driver as a module, choose M here: the module + will be called pwm-ocores. + config PWM_OMAP_DMTIMER tristate "OMAP Dual-Mode Timer PWM support" depends on OF diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 90913519f11a..7a44d8afe044 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o +obj-$(CONFIG_PWM_OCORES) += pwm-ocores.o obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c new file mode 100644 index 000000000000..c8f08aa14e44 --- /dev/null +++ b/drivers/pwm/pwm-ocores.c @@ -0,0 +1,239 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * OpenCores PWM Driver + * + * https://opencores.org/projects/ptc + * + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd. + * + * Limitations: + * - The hardware only supports inverted polarity. + * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock frequency) ns. + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock frequency) ns. + * - The hardware is set to a low level immediately when disabledThe hardware is set to + * a low level immediately when disabled. + * - The hardware will have a conversion cycle when reconfiguring. + */ + +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/reset.h> +#include <linux/slab.h> + +/* OpenCores Register offsets */ +#define REG_OCPWM_CNTR 0x0 +#define REG_OCPWM_HRC 0x4 +#define REG_OCPWM_LRC 0x8 +#define REG_OCPWM_CTRL 0xC + +/* OCPWM_CTRL register bits*/ +#define REG_OCPWM_CNTR_EN BIT(0) +#define REG_OCPWM_CNTR_ECLK BIT(1) +#define REG_OCPWM_CNTR_NEC BIT(2) +#define REG_OCPWM_CNTR_OE BIT(3) +#define REG_OCPWM_CNTR_SIGNLE BIT(4) +#define REG_OCPWM_CNTR_INTE BIT(5) +#define REG_OCPWM_CNTR_INT BIT(6) +#define REG_OCPWM_CNTR_RST BIT(7) +#define REG_OCPWM_CNTR_CAPTE BIT(8) + +struct ocores_pwm_device { + const struct ocores_pwm_data *data; + void __iomem *regs; + u32 clk_rate; /* PWM APB clock frequency */ +}; + +struct ocores_pwm_data { + void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel); +}; + +static inline u32 ocores_pwm_readl(struct ocores_pwm_device *ddata, + unsigned int channel, + unsigned int offset) +{ + void __iomem *base = ddata->data->get_ch_base ? + ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs; + + return readl(base + offset); +} + +static inline void ocores_pwm_writel(struct ocores_pwm_device *ddata, + unsigned int channel, + unsigned int offset, u32 val) +{ + void __iomem *base = ddata->data->get_ch_base ? + ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs; + + writel(val, base + offset); +} + +static inline struct ocores_pwm_device *chip_to_ocores(struct pwm_chip *chip) +{ + return pwmchip_get_drvdata(chip); +} + +static void __iomem *starfive_get_ch_base(void __iomem *base, + unsigned int channel) +{ + unsigned int offset = (channel & 4) << 13 | (channel & 3) << 4; + + return base + offset; +} + +static int ocores_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct ocores_pwm_device *ddata = chip_to_ocores(chip); + u32 period_data, duty_data, ctrl_data; + + period_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_LRC); + duty_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_HRC); + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); + + state->period = DIV_ROUND_UP_ULL((u64)period_data * NSEC_PER_SEC, ddata->clk_rate); + state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty_data * NSEC_PER_SEC, ddata->clk_rate); + state->polarity = PWM_POLARITY_INVERSED; + state->enabled = (ctrl_data & REG_OCPWM_CNTR_EN) ? true : false; + + return 0; +} + +static int ocores_pwm_apply(struct pwm_chip *chip, + struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct ocores_pwm_device *ddata = chip_to_ocores(chip); + u32 ctrl_data = 0; + u64 period_data, duty_data; + + if (state->polarity != PWM_POLARITY_INVERSED) + return -EINVAL; + + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC); + if (!period_data) + return -EINVAL; + + if (period_data > U32_MAX) + period_data = U32_MAX; + + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); + + duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, NSEC_PER_SEC); + if (duty_data <= U32_MAX) + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, (u32)duty_data); + else if (duty_data > U32_MAX) + duty_data = U32_MAX; + else + return -EINVAL; + + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); + if (state->enabled) + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, + ctrl_data | REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE); + else + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, + ctrl_data & ~(REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE)); + + return 0; +} + +static const struct pwm_ops ocores_pwm_ops = { + .get_state = ocores_pwm_get_state, + .apply = ocores_pwm_apply, +}; + +static const struct ocores_pwm_data starfive_pwm_data = { + .get_ch_base = starfive_get_ch_base, +}; + +static const struct of_device_id ocores_pwm_of_match[] = { + { .compatible = "opencores,pwm-v1" }, + { .compatible = "starfive,jh7100-pwm", .data = &starfive_pwm_data}, + { .compatible = "starfive,jh7110-pwm", .data = &starfive_pwm_data}, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ocores_pwm_of_match); + +static void ocores_pwm_reset_control_assert(void *data) +{ + reset_control_assert(data); +} + +static int ocores_pwm_probe(struct platform_device *pdev) +{ + const struct of_device_id *id; + struct device *dev = &pdev->dev; + struct ocores_pwm_device *ddata; + struct pwm_chip *chip; + struct clk *clk; + struct reset_control *rst; + int ret; + + id = of_match_device(ocores_pwm_of_match, dev); + if (!id) + return -EINVAL; + + chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata)); + if (IS_ERR(chip)) + return -ENOMEM; + + ddata = chip_to_ocores(chip); + ddata->data = id->data; + chip->ops = &ocores_pwm_ops; + + ddata->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(ddata->regs)) + return dev_err_probe(dev, PTR_ERR(ddata->regs), + "Unable to map IO resources\n"); + + clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "Unable to get pwm's clock\n"); + + ret = devm_clk_rate_exclusive_get(dev, clk); + if (ret) + return ret; + + rst = devm_reset_control_get_optional_exclusive(dev, NULL); + if (IS_ERR(rst)) + return dev_err_probe(dev, PTR_ERR(rst), + "Unable to get pwm's reset\n"); + + reset_control_deassert(rst); + + ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, rst); + if (ret) + return ret; + + ddata->clk_rate = clk_get_rate(clk); + if (ddata->clk_rate > NSEC_PER_SEC) + return dev_err_probe(dev, ddata->clk_rate, + "Unable to get clock's rate\n"); + + ret = devm_pwmchip_add(dev, chip); + if (ret < 0) + return dev_err_probe(dev, ret, "Could not register PWM chip\n"); + + return 0; +} + +static struct platform_driver ocores_pwm_driver = { + .probe = ocores_pwm_probe, + .driver = { + .name = "ocores-pwm", + .of_match_table = ocores_pwm_of_match, + }, +}; +module_platform_driver(ocores_pwm_driver); + +MODULE_AUTHOR("Jieqin Chen"); +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>"); +MODULE_DESCRIPTION("OpenCores PTC PWM driver"); +MODULE_LICENSE("GPL");