| Message ID | 20240429075140.56867-1-william.qiu@starfivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v12] pwm: opencores: Add PWM driver support | expand |
> -----Original Message----- > From: William Qiu > Sent: 2024年4月29日 15:52 > To: linux-kernel@vger.kernel.org; linux-pwm@vger.kernel.org > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Hal Feng > <hal.feng@starfivetech.com>; Philipp Zabel <p.zabel@pengutronix.de>; William > Qiu <william.qiu@starfivetech.com> > Subject: [PATCH v12] 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 | 240 > +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 260 insertions(+) > create mode 100644 drivers/pwm/pwm-ocores.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ebf03f5f0619..eb3ff739998f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16548,6 +16548,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 > 4b956d661755..92acce23298d 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -444,6 +444,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 > c5ec9e168ee7..517c4f643058 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -40,6 +40,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..039fb3c526a7 > --- /dev/null > +++ b/drivers/pwm/pwm-ocores.c > @@ -0,0 +1,240 @@ > +// 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 do 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. > + */ > + > +#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_EN BIT(0) > +#define REG_OCPWM_ECLK BIT(1) > +#define REG_OCPWM_NEC BIT(2) > +#define REG_OCPWM_OE BIT(3) > +#define REG_OCPWM_SIGNLE BIT(4) > +#define REG_OCPWM_INTE BIT(5) > +#define REG_OCPWM_INT BIT(6) > +#define REG_OCPWM_CNTRRST BIT(7) > +#define REG_OCPWM_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_jh71x0_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_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; > + > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, 0); > + > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, > NSEC_PER_SEC); > + if (period_data > U32_MAX) > + period_data = U32_MAX; > + else if (period_data > 0) > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > + else > + return -EINVAL; > + > + 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 > + return -EINVAL; > + > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CNTR, 0); > + > + if (state->enabled) { > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data | REG_OCPWM_EN | REG_OCPWM_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 jh7100_pwm_data = { > + .get_ch_base = starfive_jh71x0_get_ch_base, }; > + > +static const struct ocores_pwm_data jh7110_pwm_data = { > + .get_ch_base = starfive_jh71x0_get_ch_base, }; > + > +static const struct of_device_id ocores_pwm_of_match[] = { > + { .compatible = "opencores,pwm-v1" }, > + { .compatible = "starfive,jh7100-pwm", .data = &jh7100_pwm_data}, > + { .compatible = "starfive,jh7110-pwm", .data = &jh7110_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 <= 0 || 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? If not, could you help me integrate this patch into the mainline? Thanks. Thanks for taking time to review this patch series. Best Regards, William
Hello William, thanks for your patience and sorry for taking so long until I came around to review this. On Mon, Apr 29, 2024 at 03:51:40PM +0800, William Qiu wrote: > diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c > new file mode 100644 > index 000000000000..039fb3c526a7 > --- /dev/null > +++ b/drivers/pwm/pwm-ocores.c > @@ -0,0 +1,240 @@ > +// 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 do inverted polarity. s/do/does/ > + * - 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. How does the hardware behave on disable? Does it complete the currently running period when reconfiguring or disabling? Are glitches expected in .apply()? Please answer these questions in the Limitations paragraph. > + */ > + > +#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_EN BIT(0) I would prefer this one to be called REG_OCPWM_CNTR_EN. Ditto for the following definitions. > +#define REG_OCPWM_ECLK BIT(1) > +#define REG_OCPWM_NEC BIT(2) > +#define REG_OCPWM_OE BIT(3) > +#define REG_OCPWM_SIGNLE BIT(4) > +#define REG_OCPWM_INTE BIT(5) > +#define REG_OCPWM_INT BIT(6) > +#define REG_OCPWM_CNTRRST BIT(7) > +#define REG_OCPWM_CAPTE BIT(8) > + > [...] > +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; > + > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, 0); > + > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC); > + if (period_data > U32_MAX) > + period_data = U32_MAX; This assignment is useless, the value of period_data isn't used later, I think you want: period_data = ... if (!period_data) return -EINVAL if (period_data > U32_MAX) period_data = U32_MAX; ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > + else if (period_data > 0) > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > + else > + return -EINVAL; > + > + 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 > + return -EINVAL; duty_data > U32_MAX should be handled in the same way as period_data > U32_MAX. > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CNTR, 0); > + > + if (state->enabled) { > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > + ctrl_data | REG_OCPWM_EN | REG_OCPWM_OE); > + } Wouldn't it make sense to unset REG_OCPWM_EN | REG_OCPWM_OE if (!state->enabled)? > + return 0; > +} > + > +static const struct pwm_ops ocores_pwm_ops = { > + .get_state = ocores_pwm_get_state, > + .apply = ocores_pwm_apply, In other structs you're using a single space before =. I'd prefer that here, too. > +}; > + > +static const struct ocores_pwm_data jh7100_pwm_data = { > + .get_ch_base = starfive_jh71x0_get_ch_base, > +}; > + > +static const struct ocores_pwm_data jh7110_pwm_data = { > + .get_ch_base = starfive_jh71x0_get_ch_base, > +}; These two are identical. Does it make sense to use only one instance of these? > [...] > +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 <= 0 || ddata->clk_rate > NSEC_PER_SEC) clk_rate is an u32. So ddata->clk_rate <= 0 will never be true. Also on 64bit archs clk_get_rate() might return 4294967297 which results in ddata->clk_rate being assigned 1 and then passing this test. > + 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; > +} Best regards Uwe
> -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > Sent: 2024年6月14日 18:37 > 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 v12] pwm: opencores: Add PWM driver support > > Hello William, > > thanks for your patience and sorry for taking so long until I came around to > review this. > > On Mon, Apr 29, 2024 at 03:51:40PM +0800, William Qiu wrote: > > diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c new > > file mode 100644 index 000000000000..039fb3c526a7 > > --- /dev/null > > +++ b/drivers/pwm/pwm-ocores.c > > @@ -0,0 +1,240 @@ > > +// 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 do inverted polarity. > > s/do/does/ > Will update. > > + * - 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. > > How does the hardware behave on disable? Does it complete the currently > running period when reconfiguring or disabling? Are glitches expected in .apply()? > Please answer these questions in the Limitations paragraph. > Will update. > > + */ > > + > > +#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_EN BIT(0) > > I would prefer this one to be called REG_OCPWM_CNTR_EN. Ditto for the > following definitions. > Will update. > > +#define REG_OCPWM_ECLK BIT(1) > > +#define REG_OCPWM_NEC BIT(2) > > +#define REG_OCPWM_OE BIT(3) > > +#define REG_OCPWM_SIGNLE BIT(4) > > +#define REG_OCPWM_INTE BIT(5) > > +#define REG_OCPWM_INT BIT(6) > > +#define REG_OCPWM_CNTRRST BIT(7) > > +#define REG_OCPWM_CAPTE BIT(8) > > + > > [...] > > +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; > > + > > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, 0); > > + > > + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, > NSEC_PER_SEC); > > + if (period_data > U32_MAX) > > + period_data = U32_MAX; > > This assignment is useless, the value of period_data isn't used later, > > I think you want: > > period_data = ... > > if (!period_data) > return -EINVAL > > if (period_data > U32_MAX) > period_data = U32_MAX; > > ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > > I'll try it then. > > + else if (period_data > 0) > > + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); > > + else > > + return -EINVAL; > > + > > + 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 > > + return -EINVAL; > > duty_data > U32_MAX should be handled in the same way as period_data > > U32_MAX. > > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CNTR, 0); > > + > > + if (state->enabled) { > > + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, > REG_OCPWM_CTRL); > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, > > + ctrl_data | REG_OCPWM_EN | REG_OCPWM_OE); > > + } > > Wouldn't it make sense to unset REG_OCPWM_EN | REG_OCPWM_OE if > (!state->enabled)? > Will update. > > + return 0; > > +} > > + > > +static const struct pwm_ops ocores_pwm_ops = { > > + .get_state = ocores_pwm_get_state, > > + .apply = ocores_pwm_apply, > > In other structs you're using a single space before =. I'd prefer that here, too. > Will add. > > +}; > > + > > +static const struct ocores_pwm_data jh7100_pwm_data = { > > + .get_ch_base = starfive_jh71x0_get_ch_base, }; > > + > > +static const struct ocores_pwm_data jh7110_pwm_data = { > > + .get_ch_base = starfive_jh71x0_get_ch_base, }; > > These two are identical. Does it make sense to use only one instance of these? > Will update. > > [...] > > +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 <= 0 || ddata->clk_rate > NSEC_PER_SEC) > > clk_rate is an u32. So ddata->clk_rate <= 0 will never be true. Also on 64bit > archs clk_get_rate() might return 4294967297 which results in > ddata->clk_rate being assigned 1 and then passing this test. > Will fix it. > > + 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; > > +} > > Best regards > Uwe Thank you for taking time to review this patch. Best Regards William
diff --git a/MAINTAINERS b/MAINTAINERS index ebf03f5f0619..eb3ff739998f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16548,6 +16548,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 4b956d661755..92acce23298d 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -444,6 +444,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 c5ec9e168ee7..517c4f643058 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -40,6 +40,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..039fb3c526a7 --- /dev/null +++ b/drivers/pwm/pwm-ocores.c @@ -0,0 +1,240 @@ +// 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 do 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. + */ + +#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_EN BIT(0) +#define REG_OCPWM_ECLK BIT(1) +#define REG_OCPWM_NEC BIT(2) +#define REG_OCPWM_OE BIT(3) +#define REG_OCPWM_SIGNLE BIT(4) +#define REG_OCPWM_INTE BIT(5) +#define REG_OCPWM_INT BIT(6) +#define REG_OCPWM_CNTRRST BIT(7) +#define REG_OCPWM_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_jh71x0_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_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; + + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, 0); + + period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC); + if (period_data > U32_MAX) + period_data = U32_MAX; + else if (period_data > 0) + ocores_pwm_writel(ddata, pwm->hwpwm, 0x8, (u32)period_data); + else + return -EINVAL; + + 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 + return -EINVAL; + + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CNTR, 0); + + if (state->enabled) { + ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL); + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL, + ctrl_data | REG_OCPWM_EN | REG_OCPWM_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 jh7100_pwm_data = { + .get_ch_base = starfive_jh71x0_get_ch_base, +}; + +static const struct ocores_pwm_data jh7110_pwm_data = { + .get_ch_base = starfive_jh71x0_get_ch_base, +}; + +static const struct of_device_id ocores_pwm_of_match[] = { + { .compatible = "opencores,pwm-v1" }, + { .compatible = "starfive,jh7100-pwm", .data = &jh7100_pwm_data}, + { .compatible = "starfive,jh7110-pwm", .data = &jh7110_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 <= 0 || 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");