| Message ID | 20240809103831.156436-1-william.qiu@starfivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v14] pwm: opencores: Add PWM driver support | expand |
Le 09/08/2024 à 12:38, William Qiu a écrit : > 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> > --- Hi, ... > +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, REG_OCPWM_LRC, (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; > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, (u32)duty_data); I guess that some {} are missing. BTW, does it even compile without it??? > + else > + return -EINVAL; What is the use of this else? duty_data is either <= U32_MAX or > U32_MAX. Or I missed something? Maybe just simplify things and write something as done with period_data just a few lines above? > + > + 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"); > + > + ret = reset_control_deassert(rst); > + if (ret) > + return ret; > + > + 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, Does it work? I would expect -EINVAL. > + "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; > +} ... CJ
> -----Original Message----- > From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Sent: 2024年8月9日 20:28 > To: William Qiu <william.qiu@starfivetech.com>; 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> > Subject: Re: [PATCH v14] pwm: opencores: Add PWM driver support > > Le 09/08/2024 à 12:38, William Qiu a écrit : > > 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> > > --- > > Hi, > > ... > > > +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, REG_OCPWM_LRC, > > +(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; > > + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, > > +(u32)duty_data); > > I guess that some {} are missing. > BTW, does it even compile without it??? > It does miss {}; Will add it. > > + else > > + return -EINVAL; > > What is the use of this else? > duty_data is either <= U32_MAX or > U32_MAX. > Or I missed something? > > Maybe just simplify things and write something as done with period_data just a > few lines above? > 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"); > > + > > + ret = reset_control_deassert(rst); > > + if (ret) > > + return ret; > > + > > + 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, > > Does it work? > I would expect -EINVAL. > Will update. > > + "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; > > +} > > ... > > CJ Thanks for taking time for review this patch. Best Regards, William
diff --git a/MAINTAINERS b/MAINTAINERS index 8766f3e5e87e..e79bd16cfab8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17109,6 +17109,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 3e53838990f5..7682936484aa 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -464,6 +464,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 + 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 0be4f3e6dd43..5d87811e8537 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -41,6 +41,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..860ee414e8fa --- /dev/null +++ b/drivers/pwm/pwm-ocores.c @@ -0,0 +1,242 @@ +// 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). + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock frequency). + * - The output is set to a low level immediately when disabled. + * - 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. + */ + +#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_data { + void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel); +}; + +struct ocores_pwm_device { + const struct ocores_pwm_data *data; + void __iomem *regs; + u32 clk_rate; /* PWM APB clock frequency */ +}; + +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, REG_OCPWM_LRC, (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; + ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, (u32)duty_data); + 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"); + + ret = reset_control_deassert(rst); + if (ret) + return ret; + + 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");