Hello,
On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum PWM support, which provides maximum 4
> channels.
>
> Signed-off-by: Neo Hou <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> Changes from v1:
> - Add depending on HAS_IOMEM.
> - Rename parameters' names.
> - Implement .apply() instead of .config(), .enable() and .disable().
> - Use NSEC_PER_SEC instead of 1000000000ULL.
> - Add some comments to make code more readable.
> - Remove some redundant operation.
> - Use standard clock properties to set clock parent.
> - Other coding style optimization.
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sprd.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
> create mode 100644 drivers/pwm/pwm-sprd.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e5751..31dfc88 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -423,6 +423,17 @@ config PWM_SPEAR
> To compile this driver as a module, choose M here: the module
> will be called pwm-spear.
>
> +config PWM_SPRD
> + tristate "Spreadtrum PWM support"
> + depends on ARCH_SPRD || COMPILE_TEST
> + depends on HAS_IOMEM
> + help
> + Generic PWM framework driver for the PWM controller on
> + Spreadtrum SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sprd.
> +
> config PWM_STI
> tristate "STiH4xx PWM support"
> depends on ARCH_STI
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 76b555b..26326ad 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> +obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
> obj-$(CONFIG_PWM_STI) += pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
> diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> new file mode 100644
> index 0000000..067e711
> --- /dev/null
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Spreadtrum Communications Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define SPRD_PWM_PRESCALE 0x0
> +#define SPRD_PWM_MOD 0x4
> +#define SPRD_PWM_DUTY 0x8
> +#define SPRD_PWM_ENABLE 0x18
> +
> +#define SPRD_PWM_MOD_MAX GENMASK(7, 0)
> +#define SPRD_PWM_DUTY_MSK GENMASK(15, 0)
> +#define SPRD_PWM_PRESCALE_MSK GENMASK(7, 0)
> +#define SPRD_PWM_ENABLE_BIT BIT(0)
> +
> +#define SPRD_PWM_NUM 4
> +#define SPRD_PWM_REGS_SHIFT 5
> +#define SPRD_PWM_NUM_CLKS 2
> +#define SPRD_PWM_OUTPUT_CLK 1
These definitions could benefit from some explaining comments. Just from
looking at the names it is for example not obvious what is the
difference between SPRD_PWM_NUM and SPRD_PWM_NUM_CLKS is.
> +struct sprd_pwm_chn {
> + struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> + u32 clk_rate;
> +};
> +
> +struct sprd_pwm_chip {
> + void __iomem *base;
> + struct device *dev;
> + struct pwm_chip chip;
> + int num_pwms;
> + struct sprd_pwm_chn chn[SPRD_PWM_NUM];
> +};
> +
> +/*
> + * The list of clocks required by PWM channels, and each channel has 2 clocks:
> + * enable clock and pwm clock.
> + */
> +static const char * const sprd_pwm_clks[] = {
> + "enable0", "pwm0",
> + "enable1", "pwm1",
> + "enable2", "pwm2",
> + "enable3", "pwm3",
> +};
> +
> +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
> +{
> + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> + return readl_relaxed(spc->base + offset);
> +}
> +
> +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> + u32 reg, u32 val)
> +{
> + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> + writel_relaxed(val, spc->base + offset);
> +}
> +
> +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct sprd_pwm_chip *spc =
> + container_of(chip, struct sprd_pwm_chip, chip);
> + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> + u32 val, duty, prescale;
> + u64 tmp;
> + int ret;
> +
> + /*
> + * The clocks to PWM channel has to be enabled first before
> + * reading to the registers.
> + */
> + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> + if (ret) {
> + dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> + pwm->hwpwm);
> + return;
> + }
> +
> + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
> + if (val & SPRD_PWM_ENABLE_BIT)
> + state->enabled = true;
> + else
> + state->enabled = false;
> +
> + /*
> + * The hardware provides a counter that is feed by the source clock.
> + * The period length is (PRESCALE + 1) * MOD counter steps.
> + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> + * Thus the period_ns and duty_ns calculation formula should be:
> + * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> + * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> + */
> + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> + prescale = val & SPRD_PWM_PRESCALE_MSK;
> + tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> + duty = val & SPRD_PWM_DUTY_MSK;
> + tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> + /* Disable PWM clocks if the PWM channel is not in enable state. */
> + if (!state->enabled)
> + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> +}
> +
> +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> + u64 div, tmp;
> + u32 prescale, duty;
> +
> + /*
> + * The hardware provides a counter that is feed by the source clock.
> + * The period length is (PRESCALE + 1) * MOD counter steps.
> + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> + *
> + * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> + * The value for PRESCALE is selected such that the resulting period
> + * gets the maximal length not bigger than the requested one with the
> + * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> + */
> + duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> +
> + tmp = (u64)chn->clk_rate * period_ns;
> + div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> + prescale = div64_u64(tmp, div) - 1;
> + if (prescale > SPRD_PWM_PRESCALE_MSK)
> + prescale = SPRD_PWM_PRESCALE_MSK;
This isn't the inverse of .get_state(). Consider:
clk_rate = 3333333
SPRD_PWM_PRESCALE = 15
then you calculate in .get_state():
period = 1224000
If you then call apply with this value you calulate:
prescale = 14
.
> +
> + /*
> + * Note: The MOD must be configured before DUTY, and the hardware can
> + * ensure current running period is completed before changing a new
> + * configuration to avoid mixed settings.
You write "the hardware can ensure ..". Does that actually means "The
hardware ensures that ..." or is there some additional condition? Maybe
you mean:
/*
* Writing DUTY triggers the hardware to actually apply the
* values written to MOD and DUTY to the output. So write DUTY
* last.
*/
> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
If writing DUTY triggers the hardware to sample DUTY and MOD, what about
PRESCALE?
> + return 0;
> +}
> +
> +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct sprd_pwm_chip *spc =
> + container_of(chip, struct sprd_pwm_chip, chip);
> + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> + struct pwm_state cstate;
> + int ret;
> +
> + pwm_get_state(pwm, &cstate);
I don't like it when pwm drivers call pwm_get_state(). If ever
pwm_get_state would take a lock, this would deadlock as the lock is
probably already taken when your .apply() callback is running. Moreover
the (expensive) calculations are not used appropriately. See below.
> + if (state->enabled) {
> + if (!cstate.enabled) {
To just know the value of cstate.enabled you only need to read the
register with the ENABLE flag. That is cheaper than calling get_state.
> + /*
> + * The clocks to PWM channel has to be enabled first
> + * before writing to the registers.
> + */
> + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> + chn->clks);
> + if (ret) {
> + dev_err(spc->dev,
> + "failed to enable pwm%u clocks\n",
> + pwm->hwpwm);
> + return ret;
> + }
> + }
> +
> + if (state->period != cstate.period ||
> + state->duty_cycle != cstate.duty_cycle) {
This is a coarse check. If state->period and cstate.period only differ
by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
state->period) probably results in a noop. So you're doing an expensive
division to get an unreliable check. It would be better to calculate the
register values from the requested state and compare the register
values. The costs are more or less the same than calling .get_state and
the check is reliable. And you don't need to spend another division to
calculate the new register values.
> + ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> + state->period);
> + if (ret)
> + return ret;
> + }
> +
> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> + } else if (cstate.enabled) {
> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> +
> + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
currently running period and the write doesn't block that long: Does
disabling the clocks interfere with completing the period?
> + }
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops sprd_pwm_ops = {
> + .apply = sprd_pwm_apply,
> + .get_state = sprd_pwm_get_state,
> + .owner = THIS_MODULE,
> +};
> +
> +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> +{
> + struct clk *clk_pwm;
> + int ret, i, clk_index = 0;
> +
> + for (i = 0; i < SPRD_PWM_NUM; i++) {
> + struct sprd_pwm_chn *chn = &spc->chn[i];
> + int j;
> +
> + for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> + chn->clks[j].id = sprd_pwm_clks[clk_index++];
I think this would be more understandable when written as:
for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
chn->clks[j].id = sprd_pwm_clks[i * SPRD_PWM_NUM_CLKS + j];
but I'm not sure I'm objective here.
> +
> + ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> + if (ret) {
> + if (ret == -ENOENT)
> + break;
> +
> + dev_err(spc->dev, "failed to get channel clocks\n");
if ret == -EPROBE_DEFER you shouldn't issue an error message.
> + return ret;
> + }
> +
> + clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk;
> + chn->clk_rate = clk_get_rate(clk_pwm);
> + }
> +
> + if (!i) {
> + dev_err(spc->dev, "no available PWM channels\n");
> + return -EINVAL;
ENODEV?
> + }
> +
> + spc->num_pwms = i;
> +
> + return 0;
> +}
> +
> +static int sprd_pwm_probe(struct platform_device *pdev)
> +{
> + struct sprd_pwm_chip *spc;
> + int ret;
> +
> + spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
> + if (!spc)
> + return -ENOMEM;
> +
> + spc->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(spc->base))
> + return PTR_ERR(spc->base);
> +
> + spc->dev = &pdev->dev;
> + platform_set_drvdata(pdev, spc);
> +
> + ret = sprd_pwm_clk_init(spc);
> + if (ret)
> + return ret;
> +
> + spc->chip.dev = &pdev->dev;
> + spc->chip.ops = &sprd_pwm_ops;
> + spc->chip.base = -1;
> + spc->chip.npwm = spc->num_pwms;
> +
> + ret = pwmchip_add(&spc->chip);
> + if (ret)
> + dev_err(&pdev->dev, "failed to add PWM chip\n");
> +
> + return ret;
> +}
> +
> +static int sprd_pwm_remove(struct platform_device *pdev)
> +{
> + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> + int ret, i;
> +
> + ret = pwmchip_remove(&spc->chip);
> +
> + for (i = 0; i < spc->num_pwms; i++) {
> + struct sprd_pwm_chn *chn = &spc->chn[i];
> +
> + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
If a PWM was still running you're effectively stopping it here, right?
Are you sure you don't disable once more than you enabled?
> + }
> +
> + return ret;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
<[email protected]> wrote:
>
> Hello,
>
> On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > This patch adds the Spreadtrum PWM support, which provides maximum 4
> > channels.
> >
> > Signed-off-by: Neo Hou <[email protected]>
> > Signed-off-by: Baolin Wang <[email protected]>
> > ---
> > Changes from v1:
> > - Add depending on HAS_IOMEM.
> > - Rename parameters' names.
> > - Implement .apply() instead of .config(), .enable() and .disable().
> > - Use NSEC_PER_SEC instead of 1000000000ULL.
> > - Add some comments to make code more readable.
> > - Remove some redundant operation.
> > - Use standard clock properties to set clock parent.
> > - Other coding style optimization.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-sprd.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 319 insertions(+)
> > create mode 100644 drivers/pwm/pwm-sprd.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a7e5751..31dfc88 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -423,6 +423,17 @@ config PWM_SPEAR
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-spear.
> >
> > +config PWM_SPRD
> > + tristate "Spreadtrum PWM support"
> > + depends on ARCH_SPRD || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + help
> > + Generic PWM framework driver for the PWM controller on
> > + Spreadtrum SoCs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-sprd.
> > +
> > config PWM_STI
> > tristate "STiH4xx PWM support"
> > depends on ARCH_STI
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 76b555b..26326ad 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> > obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> > +obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
> > obj-$(CONFIG_PWM_STI) += pwm-sti.o
> > obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> > obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
> > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > new file mode 100644
> > index 0000000..067e711
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sprd.c
> > @@ -0,0 +1,307 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Spreadtrum Communications Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#define SPRD_PWM_PRESCALE 0x0
> > +#define SPRD_PWM_MOD 0x4
> > +#define SPRD_PWM_DUTY 0x8
> > +#define SPRD_PWM_ENABLE 0x18
> > +
> > +#define SPRD_PWM_MOD_MAX GENMASK(7, 0)
> > +#define SPRD_PWM_DUTY_MSK GENMASK(15, 0)
> > +#define SPRD_PWM_PRESCALE_MSK GENMASK(7, 0)
> > +#define SPRD_PWM_ENABLE_BIT BIT(0)
> > +
> > +#define SPRD_PWM_NUM 4
> > +#define SPRD_PWM_REGS_SHIFT 5
> > +#define SPRD_PWM_NUM_CLKS 2
> > +#define SPRD_PWM_OUTPUT_CLK 1
>
> These definitions could benefit from some explaining comments. Just from
> looking at the names it is for example not obvious what is the
> difference between SPRD_PWM_NUM and SPRD_PWM_NUM_CLKS is.
Sure, I will optimize the macros' names to make them understandable.
>
> > +struct sprd_pwm_chn {
> > + struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> > + u32 clk_rate;
> > +};
> > +
> > +struct sprd_pwm_chip {
> > + void __iomem *base;
> > + struct device *dev;
> > + struct pwm_chip chip;
> > + int num_pwms;
> > + struct sprd_pwm_chn chn[SPRD_PWM_NUM];
> > +};
> > +
> > +/*
> > + * The list of clocks required by PWM channels, and each channel has 2 clocks:
> > + * enable clock and pwm clock.
> > + */
> > +static const char * const sprd_pwm_clks[] = {
> > + "enable0", "pwm0",
> > + "enable1", "pwm1",
> > + "enable2", "pwm2",
> > + "enable3", "pwm3",
> > +};
> > +
> > +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
> > +{
> > + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> > +
> > + return readl_relaxed(spc->base + offset);
> > +}
> > +
> > +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> > + u32 reg, u32 val)
> > +{
> > + u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> > +
> > + writel_relaxed(val, spc->base + offset);
> > +}
> > +
> > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct sprd_pwm_chip *spc =
> > + container_of(chip, struct sprd_pwm_chip, chip);
> > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > + u32 val, duty, prescale;
> > + u64 tmp;
> > + int ret;
> > +
> > + /*
> > + * The clocks to PWM channel has to be enabled first before
> > + * reading to the registers.
> > + */
> > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > + if (ret) {
> > + dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > + pwm->hwpwm);
> > + return;
> > + }
> > +
> > + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
> > + if (val & SPRD_PWM_ENABLE_BIT)
> > + state->enabled = true;
> > + else
> > + state->enabled = false;
> > +
> > + /*
> > + * The hardware provides a counter that is feed by the source clock.
> > + * The period length is (PRESCALE + 1) * MOD counter steps.
> > + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > + * Thus the period_ns and duty_ns calculation formula should be:
> > + * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > + * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > + */
> > + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > + prescale = val & SPRD_PWM_PRESCALE_MSK;
> > + tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > + val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > + duty = val & SPRD_PWM_DUTY_MSK;
> > + tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > + /* Disable PWM clocks if the PWM channel is not in enable state. */
> > + if (!state->enabled)
> > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > +}
> > +
> > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > + u64 div, tmp;
> > + u32 prescale, duty;
> > +
> > + /*
> > + * The hardware provides a counter that is feed by the source clock.
> > + * The period length is (PRESCALE + 1) * MOD counter steps.
> > + * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > + *
> > + * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > + * The value for PRESCALE is selected such that the resulting period
> > + * gets the maximal length not bigger than the requested one with the
> > + * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > + */
> > + duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > +
> > + tmp = (u64)chn->clk_rate * period_ns;
> > + div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > + prescale = div64_u64(tmp, div) - 1;
> > + if (prescale > SPRD_PWM_PRESCALE_MSK)
> > + prescale = SPRD_PWM_PRESCALE_MSK;
>
> This isn't the inverse of .get_state(). Consider:
>
> clk_rate = 3333333
> SPRD_PWM_PRESCALE = 15
>
> then you calculate in .get_state():
>
> period = 1224000
>
> If you then call apply with this value you calulate:
>
> prescale = 14
OK. I will optimize the logics to fix this issue in next version.
> > +
> > + /*
> > + * Note: The MOD must be configured before DUTY, and the hardware can
> > + * ensure current running period is completed before changing a new
> > + * configuration to avoid mixed settings.
>
> You write "the hardware can ensure ..". Does that actually means "The
> hardware ensures that ..." or is there some additional condition? Maybe
Yes, will update the comments.
> you mean:
>
> /*
> * Writing DUTY triggers the hardware to actually apply the
> * values written to MOD and DUTY to the output. So write DUTY
> * last.
> */
Not really, our hardware's method is, when you changed a new
configuration (MOD or duty is changed) , the hardware will wait for a
while to complete current period, then change to the new period.
>
> > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
>
> If writing DUTY triggers the hardware to sample DUTY and MOD, what about
> PRESCALE?
See above comments.
>
> > + return 0;
> > +}
> > +
> > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct sprd_pwm_chip *spc =
> > + container_of(chip, struct sprd_pwm_chip, chip);
> > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > + struct pwm_state cstate;
> > + int ret;
> > +
> > + pwm_get_state(pwm, &cstate);
>
> I don't like it when pwm drivers call pwm_get_state(). If ever
> pwm_get_state would take a lock, this would deadlock as the lock is
> probably already taken when your .apply() callback is running. Moreover
> the (expensive) calculations are not used appropriately. See below.
I do not think so, see:
static inline void pwm_get_state(const struct pwm_device *pwm, struct
pwm_state *state)
{
*state = pwm->state;
}
>
> > + if (state->enabled) {
> > + if (!cstate.enabled) {
>
> To just know the value of cstate.enabled you only need to read the
> register with the ENABLE flag. That is cheaper than calling get_state.
>
> > + /*
> > + * The clocks to PWM channel has to be enabled first
> > + * before writing to the registers.
> > + */
> > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > + chn->clks);
> > + if (ret) {
> > + dev_err(spc->dev,
> > + "failed to enable pwm%u clocks\n",
> > + pwm->hwpwm);
> > + return ret;
> > + }
> > + }
> > +
> > + if (state->period != cstate.period ||
> > + state->duty_cycle != cstate.duty_cycle) {
>
> This is a coarse check. If state->period and cstate.period only differ
> by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> state->period) probably results in a noop. So you're doing an expensive
> division to get an unreliable check. It would be better to calculate the
> register values from the requested state and compare the register
> values. The costs are more or less the same than calling .get_state and
> the check is reliable. And you don't need to spend another division to
> calculate the new register values.
Same as above comment.
>
> > + ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > + state->period);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > + } else if (cstate.enabled) {
> > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > +
> > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>
> Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> currently running period and the write doesn't block that long: Does
> disabling the clocks interfere with completing the period?
Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
waiting for completing the currently period.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops sprd_pwm_ops = {
> > + .apply = sprd_pwm_apply,
> > + .get_state = sprd_pwm_get_state,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > +{
> > + struct clk *clk_pwm;
> > + int ret, i, clk_index = 0;
> > +
> > + for (i = 0; i < SPRD_PWM_NUM; i++) {
> > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > + int j;
> > +
> > + for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> > + chn->clks[j].id = sprd_pwm_clks[clk_index++];
>
> I think this would be more understandable when written as:
>
> for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> chn->clks[j].id = sprd_pwm_clks[i * SPRD_PWM_NUM_CLKS + j];
>
> but I'm not sure I'm objective here.
Sure.
>
> > +
> > + ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> > + if (ret) {
> > + if (ret == -ENOENT)
> > + break;
> > +
> > + dev_err(spc->dev, "failed to get channel clocks\n");
>
> if ret == -EPROBE_DEFER you shouldn't issue an error message.
Yes.
>
> > + return ret;
> > + }
> > +
> > + clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk;
> > + chn->clk_rate = clk_get_rate(clk_pwm);
> > + }
> > +
> > + if (!i) {
> > + dev_err(spc->dev, "no available PWM channels\n");
> > + return -EINVAL;
>
> ENODEV?
Yes
>
> > + }
> > +
> > + spc->num_pwms = i;
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct sprd_pwm_chip *spc;
> > + int ret;
> > +
> > + spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
> > + if (!spc)
> > + return -ENOMEM;
> > +
> > + spc->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(spc->base))
> > + return PTR_ERR(spc->base);
> > +
> > + spc->dev = &pdev->dev;
> > + platform_set_drvdata(pdev, spc);
> > +
> > + ret = sprd_pwm_clk_init(spc);
> > + if (ret)
> > + return ret;
> > +
> > + spc->chip.dev = &pdev->dev;
> > + spc->chip.ops = &sprd_pwm_ops;
> > + spc->chip.base = -1;
> > + spc->chip.npwm = spc->num_pwms;
> > +
> > + ret = pwmchip_add(&spc->chip);
> > + if (ret)
> > + dev_err(&pdev->dev, "failed to add PWM chip\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int sprd_pwm_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > + int ret, i;
> > +
> > + ret = pwmchip_remove(&spc->chip);
> > +
> > + for (i = 0; i < spc->num_pwms; i++) {
> > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > +
> > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>
> If a PWM was still running you're effectively stopping it here, right?
> Are you sure you don't disable once more than you enabled?
Yes, you are right. I should check current enable status of the PWM channel.
Thanks for your comments.
--
Baolin Wang
Best Regards
Hello Baolin,
On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > [...]
> Not really, our hardware's method is, when you changed a new
> configuration (MOD or duty is changed) , the hardware will wait for a
> while to complete current period, then change to the new period.
Can you describe that in more detail? This doesn't explain why MOD must be
configured before DUTY. Is there another reason for that?
> > > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct sprd_pwm_chip *spc =
> > > + container_of(chip, struct sprd_pwm_chip, chip);
> > > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > + struct pwm_state cstate;
> > > + int ret;
> > > +
> > > + pwm_get_state(pwm, &cstate);
> >
> > I don't like it when pwm drivers call pwm_get_state(). If ever
> > pwm_get_state would take a lock, this would deadlock as the lock is
> > probably already taken when your .apply() callback is running. Moreover
> > the (expensive) calculations are not used appropriately. See below.
>
> I do not think so, see:
>
> static inline void pwm_get_state(const struct pwm_device *pwm, struct
> pwm_state *state)
> {
> *state = pwm->state;
> }
OK, the PWM framework currently caches this for you. Still I would
prefer if you didn't call PWM API functions in your lowlevel driver.
There is (up to now) nothing bad that will happen if you do it anyhow,
but when the PWM framework evolves it might (and I want to work on such
an evolution). You must not call clk_get_rate() in a .set_rate()
callback of a clock either.
> > > + if (state->enabled) {
> > > + if (!cstate.enabled) {
> >
> > To just know the value of cstate.enabled you only need to read the
> > register with the ENABLE flag. That is cheaper than calling get_state.
> >
> > > + /*
> > > + * The clocks to PWM channel has to be enabled first
> > > + * before writing to the registers.
> > > + */
> > > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > > + chn->clks);
> > > + if (ret) {
> > > + dev_err(spc->dev,
> > > + "failed to enable pwm%u clocks\n",
> > > + pwm->hwpwm);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + if (state->period != cstate.period ||
> > > + state->duty_cycle != cstate.duty_cycle) {
> >
> > This is a coarse check. If state->period and cstate.period only differ
> > by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> > state->period) probably results in a noop. So you're doing an expensive
> > division to get an unreliable check. It would be better to calculate the
> > register values from the requested state and compare the register
> > values. The costs are more or less the same than calling .get_state and
> > the check is reliable. And you don't need to spend another division to
> > calculate the new register values.
>
> Same as above comment.
When taking the caching into account (which I wouldn't) the check is
still incomplete. OK, you could argue avoiding the recalculation in 90%
(to just say some number) of the cases where it is unnecessary is good.
> >
> > > + ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > + state->period);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > > + } else if (cstate.enabled) {
> > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > > +
> > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> > currently running period and the write doesn't block that long: Does
> > disabling the clocks interfere with completing the period?
>
> Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
> waiting for completing the currently period.
The currently active period is supposed to be completed. If you cannot
implement this please point this out as limitation at the top of the
driver.
Honestly I think most pwm users won't mind and they should get the
possibility to tell they prefer pwm_apply_state to return immediately
even if this could result in a spike. But we're not there yet.
(Actually there are three things a PWM consumer might want:
a) stop immediately;
b) complete the currently running period, then stop and return only
when the period is completed; or
c) complete the currently running period and then stop, return immediately if
possible.
Currently the expected semantic is b).
> > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > +{
> > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > + int ret, i;
> > > +
> > > + ret = pwmchip_remove(&spc->chip);
> > > +
> > > + for (i = 0; i < spc->num_pwms; i++) {
> > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > +
> > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > If a PWM was still running you're effectively stopping it here, right?
> > Are you sure you don't disable once more than you enabled?
>
> Yes, you are right. I should check current enable status of the PWM channel.
> Thanks for your comments.
I didn't recheck, but I think the right approach is to not fiddle with
the clocks at all and rely on the PWM framework to not let someone call
sprd_pwm_remove when a PWM is still in use.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
<[email protected]> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > <[email protected]> wrote:
> > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > [...]
> > Not really, our hardware's method is, when you changed a new
> > configuration (MOD or duty is changed) , the hardware will wait for a
> > while to complete current period, then change to the new period.
>
> Can you describe that in more detail? This doesn't explain why MOD must be
> configured before DUTY. Is there another reason for that?
Sorry, I did not explain this explicitly. When we change a new PWM
configuration, the PWM controller will make sure the current period is
completed before changing to a new period. Once setting the MOD
register (since we always set MOD firstly), that will tell the
hardware that a new period need to change.
The reason MOD must be configured before DUTY is that, if we
configured DUTY firstly, the PWM can work abnormally if the current
DUTY is larger than previous MOD. That is also our hardware's
limitation.
>
> > > > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct sprd_pwm_chip *spc =
> > > > + container_of(chip, struct sprd_pwm_chip, chip);
> > > > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > + struct pwm_state cstate;
> > > > + int ret;
> > > > +
> > > > + pwm_get_state(pwm, &cstate);
> > >
> > > I don't like it when pwm drivers call pwm_get_state(). If ever
> > > pwm_get_state would take a lock, this would deadlock as the lock is
> > > probably already taken when your .apply() callback is running. Moreover
> > > the (expensive) calculations are not used appropriately. See below.
> >
> > I do not think so, see:
> >
> > static inline void pwm_get_state(const struct pwm_device *pwm, struct
> > pwm_state *state)
> > {
> > *state = pwm->state;
> > }
>
> OK, the PWM framework currently caches this for you. Still I would
> prefer if you didn't call PWM API functions in your lowlevel driver.
> There is (up to now) nothing bad that will happen if you do it anyhow,
> but when the PWM framework evolves it might (and I want to work on such
> an evolution). You must not call clk_get_rate() in a .set_rate()
> callback of a clock either.
Make sense, I will change to:
struct pwm_state *cstate = pwm->state;
>
> > > > + if (state->enabled) {
> > > > + if (!cstate.enabled) {
> > >
> > > To just know the value of cstate.enabled you only need to read the
> > > register with the ENABLE flag. That is cheaper than calling get_state.
> > >
> > > > + /*
> > > > + * The clocks to PWM channel has to be enabled first
> > > > + * before writing to the registers.
> > > > + */
> > > > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > > > + chn->clks);
> > > > + if (ret) {
> > > > + dev_err(spc->dev,
> > > > + "failed to enable pwm%u clocks\n",
> > > > + pwm->hwpwm);
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + if (state->period != cstate.period ||
> > > > + state->duty_cycle != cstate.duty_cycle) {
> > >
> > > This is a coarse check. If state->period and cstate.period only differ
> > > by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > state->period) probably results in a noop. So you're doing an expensive
> > > division to get an unreliable check. It would be better to calculate the
> > > register values from the requested state and compare the register
> > > values. The costs are more or less the same than calling .get_state and
> > > the check is reliable. And you don't need to spend another division to
> > > calculate the new register values.
> >
> > Same as above comment.
>
> When taking the caching into account (which I wouldn't) the check is
> still incomplete. OK, you could argue avoiding the recalculation in 90%
> (to just say some number) of the cases where it is unnecessary is good.
Yes :)
>
> > >
> > > > + ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > > + state->period);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > > > + } else if (cstate.enabled) {
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > > > +
> > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> > > currently running period and the write doesn't block that long: Does
> > > disabling the clocks interfere with completing the period?
> >
> > Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
> > waiting for completing the currently period.
>
> The currently active period is supposed to be completed. If you cannot
> implement this please point this out as limitation at the top of the
> driver.
Sure.
>
> Honestly I think most pwm users won't mind and they should get the
> possibility to tell they prefer pwm_apply_state to return immediately
> even if this could result in a spike. But we're not there yet.
> (Actually there are three things a PWM consumer might want:
>
> a) stop immediately;
> b) complete the currently running period, then stop and return only
> when the period is completed; or
> c) complete the currently running period and then stop, return immediately if
> possible.
>
> Currently the expected semantic is b).
Make sense.
>
> > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > + int ret, i;
> > > > +
> > > > + ret = pwmchip_remove(&spc->chip);
> > > > +
> > > > + for (i = 0; i < spc->num_pwms; i++) {
> > > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > +
> > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > If a PWM was still running you're effectively stopping it here, right?
> > > Are you sure you don't disable once more than you enabled?
> >
> > Yes, you are right. I should check current enable status of the PWM channel.
> > Thanks for your comments.
>
> I didn't recheck, but I think the right approach is to not fiddle with
> the clocks at all and rely on the PWM framework to not let someone call
> sprd_pwm_remove when a PWM is still in use.
So you mean just return pwmchip_remove(&spc->chip); ?
--
Baolin Wang
Best Regards
Hello Baolin,
On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-K?nig
> > > <[email protected]> wrote:
> > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > [...]
> > > Not really, our hardware's method is, when you changed a new
> > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > while to complete current period, then change to the new period.
> >
> > Can you describe that in more detail? This doesn't explain why MOD must be
> > configured before DUTY. Is there another reason for that?
>
> Sorry, I did not explain this explicitly. When we change a new PWM
> configuration, the PWM controller will make sure the current period is
> completed before changing to a new period. Once setting the MOD
> register (since we always set MOD firstly), that will tell the
> hardware that a new period need to change.
So if the current period just ended after you reconfigured MOD but
before you wrote to DUTY we'll see a bogus period, right? I assume the
same holds true for writing the prescale value?
> The reason MOD must be configured before DUTY is that, if we
> configured DUTY firstly, the PWM can work abnormally if the current
> DUTY is larger than previous MOD. That is also our hardware's
> limitation.
OK, so you must not get into a situation where DUTY > MOD, right?
Now if the hardware was configured for
period = 8s, duty = 4s
and now you are supposed to change to
period = 2s, duty = 1s
you'd need to write DUTY first, don't you?
> > > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > > +{
> > > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > > + int ret, i;
> > > > > +
> > > > > + ret = pwmchip_remove(&spc->chip);
> > > > > +
> > > > > + for (i = 0; i < spc->num_pwms; i++) {
> > > > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > > +
> > > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > >
> > > > If a PWM was still running you're effectively stopping it here, right?
> > > > Are you sure you don't disable once more than you enabled?
> > >
> > > Yes, you are right. I should check current enable status of the PWM channel.
> > > Thanks for your comments.
> >
> > I didn't recheck, but I think the right approach is to not fiddle with
> > the clocks at all and rely on the PWM framework to not let someone call
> > sprd_pwm_remove when a PWM is still in use.
>
> So you mean just return pwmchip_remove(&spc->chip); ?
right.
I just rechecked: If there is still a pwm in use, pwmchip_remove returns
-EBUSY. So this should be safe.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On Wed, 14 Aug 2019 at 18:55, Uwe Kleine-König
<[email protected]> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> > On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
> > <[email protected]> wrote:
> > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > > > <[email protected]> wrote:
> > > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > > [...]
> > > > Not really, our hardware's method is, when you changed a new
> > > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > > while to complete current period, then change to the new period.
> > >
> > > Can you describe that in more detail? This doesn't explain why MOD must be
> > > configured before DUTY. Is there another reason for that?
> >
> > Sorry, I did not explain this explicitly. When we change a new PWM
> > configuration, the PWM controller will make sure the current period is
> > completed before changing to a new period. Once setting the MOD
> > register (since we always set MOD firstly), that will tell the
> > hardware that a new period need to change.
>
> So if the current period just ended after you reconfigured MOD but
> before you wrote to DUTY we'll see a bogus period, right? I assume the
> same holds true for writing the prescale value?
I confirmed again, I am sorry I missed something before. Yes, like you
said before, writing DUTY triggers the hardware to actually apply the
values written to MOD and DUTY to the output. So write DUTY last. I
will update the comments and change the PWM configure like:
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
>
> > The reason MOD must be configured before DUTY is that, if we
> > configured DUTY firstly, the PWM can work abnormally if the current
> > DUTY is larger than previous MOD. That is also our hardware's
> > limitation.
>
> OK, so you must not get into a situation where DUTY > MOD, right?
>
> Now if the hardware was configured for
>
> period = 8s, duty = 4s
>
> and now you are supposed to change to
>
> period = 2s, duty = 1s
>
> you'd need to write DUTY first, don't you?
>
> > > > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > > > + int ret, i;
> > > > > > +
> > > > > > + ret = pwmchip_remove(&spc->chip);
> > > > > > +
> > > > > > + for (i = 0; i < spc->num_pwms; i++) {
> > > > > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > > > +
> > > > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > >
> > > > > If a PWM was still running you're effectively stopping it here, right?
> > > > > Are you sure you don't disable once more than you enabled?
> > > >
> > > > Yes, you are right. I should check current enable status of the PWM channel.
> > > > Thanks for your comments.
> > >
> > > I didn't recheck, but I think the right approach is to not fiddle with
> > > the clocks at all and rely on the PWM framework to not let someone call
> > > sprd_pwm_remove when a PWM is still in use.
> >
> > So you mean just return pwmchip_remove(&spc->chip); ?
>
> right.
>
> I just rechecked: If there is still a pwm in use, pwmchip_remove returns
> -EBUSY. So this should be safe.
Yes. Thanks for your comments.
--
Baolin Wang
Best Regards
Hello,
On Wed, Aug 14, 2019 at 08:23:37PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 18:55, Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> > > On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-K?nig
> > > <[email protected]> wrote:
> > > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-K?nig
> > > > > <[email protected]> wrote:
> > > > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > > > [...]
> > > > > Not really, our hardware's method is, when you changed a new
> > > > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > > > while to complete current period, then change to the new period.
> > > >
> > > > Can you describe that in more detail? This doesn't explain why MOD must be
> > > > configured before DUTY. Is there another reason for that?
> > >
> > > Sorry, I did not explain this explicitly. When we change a new PWM
> > > configuration, the PWM controller will make sure the current period is
> > > completed before changing to a new period. Once setting the MOD
> > > register (since we always set MOD firstly), that will tell the
> > > hardware that a new period need to change.
> >
> > So if the current period just ended after you reconfigured MOD but
> > before you wrote to DUTY we'll see a bogus period, right? I assume the
> > same holds true for writing the prescale value?
>
> I confirmed again, I am sorry I missed something before. Yes, like you
> said before, writing DUTY triggers the hardware to actually apply the
> values written to MOD and DUTY to the output. So write DUTY last. I
> will update the comments and change the PWM configure like:
>
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
So PRESCALE is independent and it can still happen that writing PRESCALE
affects the output before MOD and DUTY do?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 14. 08. 19 11:23, Uwe Kleine-König wrote:
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
>> On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
>> <[email protected]> wrote:
>>> On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
>>> [...]
>> Not really, our hardware's method is, when you changed a new
>> configuration (MOD or duty is changed) , the hardware will wait for a
>> while to complete current period, then change to the new period.
>
> Can you describe that in more detail? This doesn't explain why MOD must be
> configured before DUTY. Is there another reason for that?
>
>>>> +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> + struct pwm_state *state)
>>>> +{
>>>> + struct sprd_pwm_chip *spc =
>>>> + container_of(chip, struct sprd_pwm_chip, chip);
>>>> + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
>>>> + struct pwm_state cstate;
>>>> + int ret;
>>>> +
>>>> + pwm_get_state(pwm, &cstate);
>>>
>>> I don't like it when pwm drivers call pwm_get_state(). If ever
>>> pwm_get_state would take a lock, this would deadlock as the lock is
>>> probably already taken when your .apply() callback is running. Moreover
>>> the (expensive) calculations are not used appropriately. See below.
>>
>> I do not think so, see:
>>
>> static inline void pwm_get_state(const struct pwm_device *pwm, struct
>> pwm_state *state)
>> {
>> *state = pwm->state;
>> }
>
> OK, the PWM framework currently caches this for you. Still I would
> prefer if you didn't call PWM API functions in your lowlevel driver.
> There is (up to now) nothing bad that will happen if you do it anyhow,
> but when the PWM framework evolves it might (and I want to work on such
> an evolution). You must not call clk_get_rate() in a .set_rate()
> callback of a clock either.
>
>>>> + if (state->enabled) {
>>>> + if (!cstate.enabled) {
>>>
>>> To just know the value of cstate.enabled you only need to read the
>>> register with the ENABLE flag. That is cheaper than calling get_state.
>>>
>>>> + /*
>>>> + * The clocks to PWM channel has to be enabled first
>>>> + * before writing to the registers.
>>>> + */
>>>> + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
>>>> + chn->clks);
>>>> + if (ret) {
>>>> + dev_err(spc->dev,
>>>> + "failed to enable pwm%u clocks\n",
>>>> + pwm->hwpwm);
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + if (state->period != cstate.period ||
>>>> + state->duty_cycle != cstate.duty_cycle) {
>>>
>>> This is a coarse check. If state->period and cstate.period only differ
>>> by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
>>> state->period) probably results in a noop. So you're doing an expensive
>>> division to get an unreliable check. It would be better to calculate the
>>> register values from the requested state and compare the register
>>> values. The costs are more or less the same than calling .get_state and
>>> the check is reliable. And you don't need to spend another division to
>>> calculate the new register values.
>>
>> Same as above comment.
>
> When taking the caching into account (which I wouldn't) the check is
> still incomplete. OK, you could argue avoiding the recalculation in 90%
> (to just say some number) of the cases where it is unnecessary is good.
>
>>>
>>>> + ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
>>>> + state->period);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
>>>> + } else if (cstate.enabled) {
>>>> + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
>>>> +
>>>> + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>>>
>>> Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
>>> currently running period and the write doesn't block that long: Does
>>> disabling the clocks interfere with completing the period?
>>
>> Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
>> waiting for completing the currently period.
>
> The currently active period is supposed to be completed. If you cannot
> implement this please point this out as limitation at the top of the
> driver.
>
> Honestly I think most pwm users won't mind and they should get the
> possibility to tell they prefer pwm_apply_state to return immediately
> even if this could result in a spike. But we're not there yet.
> (Actually there are three things a PWM consumer might want:
>
> a) stop immediately;
> b) complete the currently running period, then stop and return only
> when the period is completed; or
> c) complete the currently running period and then stop, return immediately if
> possible.
>
> Currently the expected semantic is b).
Hi Uwe et all,
I am sorry for interrupting your discussion. From my (last year or so)
observation of the PWM mailing list I see this as a common pattern in
almost every submission of a new PWM driver. That is PWM driver authors
keep submitting solution a) and you tell them the expected behavior
is b).
Why is that? I hope that the fact that implementing a) is simpler
than b) is not the main reason. I believe that people think a)
is the correct solution.
I agree that smooth transition from one period/duty setting to
different setting makes sense. But I also believe the expected
behavior of setting enabled=0 is different. That is to stop
immediately the technology that is fed by the PWM signal.
Otherwise the concept of enabled/disabled does not make sense
because setting duty=0 would have the same effect.
So I still wonder where the expectation for b) is coming from.
What would be the physical/electrical reasoning for b)?
Why/how it can be dangerous/harmful for any device to stop the
PWM signal immediately?
Would be great to finally reach consensus on that so the expected
behavior can be documented. I know you already attempted that [1].
I think you have done a great job but I still consider the part
about state changes little controversial and unresolved.
Best regards,
Michal
[1] http://patchwork.ozlabs.org/patch/1021566/
Hello Michal,
[adapted the Subject to the actual topic of this mail]
On Wed, Aug 14, 2019 at 04:07:21PM +0200, Michal Vokáč wrote:
> > Honestly I think most pwm users won't mind and they should get the
> > possibility to tell they prefer pwm_apply_state to return immediately
> > even if this could result in a spike. But we're not there yet.
> > (Actually there are three things a PWM consumer might want:
> >
> > a) stop immediately;
> > b) complete the currently running period, then stop and return
> > only
> > when the period is completed; or
> > c) complete the currently running period and then stop, return
> > immediately if
> > possible.
> >
> > Currently the expected semantic is b).
>
> I am sorry for interrupting your discussion. From my (last year or so)
> observation of the PWM mailing list I see this as a common pattern in
> almost every submission of a new PWM driver. That is PWM driver authors
> keep submitting solution a) and you tell them the expected behavior
> is b).
The history is that there was a discussion about something related to
the pwm-imx27 or the pwm-mxs driver between Thierry and me. I don't
remember the exact issue though. My impression back then was that
Thierry had a conception of how a PWM should work that was idealised and
hard to implement with a big part of the actually available hardware
implementations. In the end Thierry ended the discussion saying
something like: "I'm not aware that the available drivers don't fulfill
the requirements so I must assume they are right. If you know otherwise,
please tell us. I'm not willing to change the API's requirements just
because of one or two drivers having problems." (I didn't look up the
mail thread, so this is not an exact quote.)
One of these requirements was b) from above. So if you notice a common
pattern here on the list that most new drivers have problems to
implement b) I'm happy. I let you decide what this probably means for
the existing drivers.
> Why is that? I hope that the fact that implementing a) is simpler
> than b) is not the main reason. I believe that people think a)
> is the correct solution.
Honestly I think people are not aware of the requirements and/or don't
care enough about these details instead of actively being convinced that
a) is right. Their test cases are most of the time quite limited and if
the LED's brightness reacts according to the expectations the driver is
considered done.
And I agree with Thierry that in some situations it is sane to require
that periods are completed to prevent spikes. (I'm not a hardware guy,
but I think controlling a step motor is such a situation where you'd want
to prevent spikes.)
> I agree that smooth transition from one period/duty setting to
> different setting makes sense. But I also believe the expected
> behavior of setting enabled=0 is different. That is to stop
> immediately the technology that is fed by the PWM signal.
> Otherwise the concept of enabled/disabled does not make sense
> because setting duty=0 would have the same effect.
That's approximately my argumentation from back then. I asked several
times what was the difference between duty=0 and enabled=0. Thierry was
unable to come up with a better explanation but insisted there was a
difference. (Later I became aware that there is a difference, i.e. with
duty=0 the inactive time still has to be a multiple of the configured
period. So disabled is actually equivalent to duty=0 with an
infinitesimal period. Unless I missed something of course.)
I even questioned if consumers really care about a PWM going to it's
inactive level on disable and suggested to introduce something to the
PWM API to let consumers express they don't care. But the discussion
led nowhere. In my eyes the PWM framework would become better if
consumers were able to specify their intentions in a more detailed way.
Thierry didn't agree.
I think allowing lowlevel drivers to act immediately on enabled=0 would
be fine. Consumers who'd want to disable only after completion of the
currently running period could then do:
pwm_apply(pwm, duty=0, period=dontcare, enabled=1);
pwm_apply(pwm, duty=0, period=dontcare, enabled=0);
to achieve this completion. So after all consumers are converted the API
is more powerful than before because consumers have the choice between
the above and a single
pwm_apply(pwm, duty=0, period=dontcare, enabled=0);
The net benefit is that consumers don't have to wait if they don't
actually care about completing the period. The downside is that all
consumers must be reviewed and maybe fixed and maybe that people's
expectations are not met anymore. (But I doubt the latter a bit because
I'm not sure that most people actually expect that pwm_apply() blocks
until the output reached its disabled state.)
And we could even go further and tell consumers who care about the
output's level should not disable the pwm but instead do
pwm_apply(pwm, duty=0, period=1, enabled=1);
which has effectively the same semantic as enabled=0 today. This would
give at least two (probably more) PWM drivers the freedom to really
disable the hardware which might result in something different than
"inactive level" without taking the possibility from consumers to
actually request what they want.
> So I still wonder where the expectation for b) is coming from.
> What would be the physical/electrical reasoning for b)?
> Why/how it can be dangerous/harmful for any device to stop the
> PWM signal immediately?
Maybe you can get Thierry to answer this. He is the source of this
requirement.
> Would be great to finally reach consensus on that so the expected
> behavior can be documented. I know you already attempted that [1].
> I think you have done a great job but I still consider the part
> about state changes little controversial and unresolved.
I'm completely open to document the expectations. I'm also open to shift
the expectations to something that is saner and/or gives consumers more
possibilities. But it's Thierry's responsibility to go forward here or
at least stop the blockade.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |