| Message ID | 20191103203334.10539-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for H6 PWM | expand |
Hello, adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm not stripping the uncommented parts of the patch. On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > H6 PWM core needs deasserted reset line in order to work. > > Add an optional probe for it. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index 6f5840a1a82d..d194b8ebdb00 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -16,6 +16,7 @@ > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > +#include <linux/reset.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/time.h> > @@ -78,6 +79,7 @@ struct sun4i_pwm_data { > struct sun4i_pwm_chip { > struct pwm_chip chip; > struct clk *clk; > + struct reset_control *rst; > void __iomem *base; > spinlock_t ctrl_lock; > const struct sun4i_pwm_data *data; > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > if (IS_ERR(pwm->clk)) > return PTR_ERR(pwm->clk); > > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > + if (IS_ERR(pwm->rst)) { > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > + return PTR_ERR(pwm->rst); > + dev_info(&pdev->dev, "no reset control found\n"); I would degrade this to a dev_dbg. Otherwise this spams the log for all unaffected machines. devm_reset_control_get_optional() is defined in a section that has a comment "These inline function calls will be removed once all consumers have been moved over to the new explicit API.", so I guess you want devm_reset_control_get_optional_exclusive or even devm_reset_control_get_optional_shared here. @Philipp: maybe a check in checkpatch that warns about introduction of such new instances would be good?! > + } > + > + /* Deassert reset */ > + ret = reset_control_deassert(pwm->rst); > + if (ret) { > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > + return ret; > + } > + > pwm->chip.dev = &pdev->dev; > pwm->chip.ops = &sun4i_pwm_ops; > pwm->chip.base = -1; > @@ -377,19 +393,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > ret = pwmchip_add(&pwm->chip); > if (ret < 0) { > dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); > - return ret; > + goto err_pwm_add; > } > > platform_set_drvdata(pdev, pwm); > > return 0; > + > +err_pwm_add: > + reset_control_assert(pwm->rst); > + > + return ret; > } > > static int sun4i_pwm_remove(struct platform_device *pdev) > { > struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev); > + int ret; > + > + ret = pwmchip_remove(&pwm->chip); > + if (ret) > + return ret; > + > + reset_control_assert(pwm->rst); > > - return pwmchip_remove(&pwm->chip); > + return 0; > } > > static struct platform_driver sun4i_pwm_driver = { Best regards Uwe
Hello, On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > H6 PWM core needs bus clock to be enabled in order to work. > > Add an optional probe for it and a fallback for previous > bindings without name on module clock. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index d194b8ebdb00..b5e7ac364f59 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > struct sun4i_pwm_chip { > struct pwm_chip chip; > + struct clk *bus_clk; > struct clk *clk; > struct reset_control *rst; > void __iomem *base; > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev) Adding more context here: | pwm->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pwm->clk)) > return PTR_ERR(pwm->clk); > > + /* Get all clocks and reset line */ > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > + if (IS_ERR(pwm->clk)) { > + dev_err(&pdev->dev, "get clock failed %ld\n", > + PTR_ERR(pwm->clk)); > + return PTR_ERR(pwm->clk); > + } I guess you want to drop the first assignment to pwm->clk. > + /* Fallback for old dtbs with a single clock and no name */ > + if (!pwm->clk) { > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pwm->clk)) { > + dev_err(&pdev->dev, "get clock failed %ld\n", > + PTR_ERR(pwm->clk)); > + return PTR_ERR(pwm->clk); > + } > + } There is a slight change of behaviour if I'm not mistaken. If you have this: clocks = <&clk1>; clock-names = "mod"; pwm { compatible = "allwinner,sun4i-a10-pwm" clocks = <&clk2>; } you now use clk1 instead of clk2 before. Assuming this is only a theoretical problem, at least pointing this out in the commit log would be good I think. > + pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus"); > + if (IS_ERR(pwm->bus_clk)) { > + dev_err(&pdev->dev, "get bus_clock failed %ld\n", > + PTR_ERR(pwm->bus_clk)); > + return PTR_ERR(pwm->bus_clk); > + } > + > pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > if (IS_ERR(pwm->rst)) { > if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > return ret; > } > > + /* Enable bus clock */ > + ret = clk_prepare_enable(pwm->bus_clk); > + if (ret) { > + dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n"); I'd do s/prepare_enable/prepare and enable/ here. > + goto err_bus; > + } > + > pwm->chip.dev = &pdev->dev; > pwm->chip.ops = &sun4i_pwm_ops; > pwm->chip.base = -1; > @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > return 0; > > err_pwm_add: > + clk_disable_unprepare(pwm->bus_clk); > +err_bus: > reset_control_assert(pwm->rst); > > return ret; What is that clock used for? Is it required to access the hardware registers? Or is it only required while the PWM is enabled? If so you could enable the clock more finegrainded. Best regards Uwe
On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote: > From: Jernej Skrabec <jernej.skrabec@siol.net> > > PWM core has an option to bypass whole logic and output unchanged source > clock as PWM output. This is achieved by enabling bypass bit. > > Note that when bypass is enabled, no other setting has any meaning, not > even enable bit. > > This mode of operation is needed to achieve high enough frequency to > serve as clock source for AC200 chip, which is integrated into same > package as H6 SoC. I think the , should be dropped. > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index b5e7ac364f59..2441574674d9 100644 > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -3,6 +3,10 @@ > * Driver for Allwinner sun4i Pulse Width Modulation Controller > * > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com> > + * > + * Limitations: > + * - When outputing the source clock directly, the PWM logic will be bypassed > + * and the currently running period is not guaranted to be completed Typo: guaranted -> guaranteed > */ > > #include <linux/bitops.h> > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = { > > struct sun4i_pwm_data { > bool has_prescaler_bypass; > + bool has_direct_mod_clk_output; > unsigned int npwm; > }; > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > + /* > + * PWM chapter in H6 manual has a diagram which explains that if bypass > + * bit is set, no other setting has any meaning. Even more, experiment > + * proved that also enable bit is ignored in this case. > + */ > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) && > + data->has_direct_mod_clk_output) { > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > + state->duty_cycle = state->period / 2; > + state->polarity = PWM_POLARITY_NORMAL; > + state->enabled = true; > + return; > + } Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer to let .get_state() round up which together with .apply_state() rounding down yields sound behaviour. > + > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > sun4i_pwm->data->has_prescaler_bypass) > prescaler = 1; > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > { > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > struct pwm_state cstate; > - u32 ctrl; > + u32 ctrl, clk_rate; > + bool bypass; > int ret; > unsigned int delay_us; > unsigned long now; > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > } > } > > + /* > + * Although it would make much more sense to check for bypass in > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > + * Period is allowed to be rounded up or down. > + */ > + clk_rate = clk_get_rate(sun4i_pwm->clk); > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > + state->enabled); I guess the compiler is smart enough here, but checking for state->enabled is cheaper than the other checks, so putting this at the start of the expression seems sensible. The comment doesn't match the code. You don't round up state->period. (This is good, please fix the comment.) I think dropping the check state->period * clk_rate < NSEC_PER_SEC + clk_rate would be fine, too. I'd like to have a check for state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 && state->duty_cycle * clk_rate < NSEC_PER_SEC here. If this isn't true rather disable the PWM or output a 100% duty cycle with a larger period. > + > spin_lock(&sun4i_pwm->ctrl_lock); > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > Best regards Uwe
On Mon, 4 Nov 2019 at 09:11, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm > not stripping the uncommented parts of the patch. > > On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 PWM core needs deasserted reset line in order to work. > > > > Add an optional probe for it. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 6f5840a1a82d..d194b8ebdb00 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -16,6 +16,7 @@ > > #include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/pwm.h> > > +#include <linux/reset.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > #include <linux/time.h> > > @@ -78,6 +79,7 @@ struct sun4i_pwm_data { > > struct sun4i_pwm_chip { > > struct pwm_chip chip; > > struct clk *clk; > > + struct reset_control *rst; > > void __iomem *base; > > spinlock_t ctrl_lock; > > const struct sun4i_pwm_data *data; > > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > if (IS_ERR(pwm->clk)) > > return PTR_ERR(pwm->clk); > > > > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > > + if (IS_ERR(pwm->rst)) { > > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > > + return PTR_ERR(pwm->rst); > > + dev_info(&pdev->dev, "no reset control found\n"); > > I would degrade this to a dev_dbg. Otherwise this spams the log for all > unaffected machines. devm_reset_control_get_optional() is defined in a > section that has a comment "These inline function calls will be removed > once all consumers have been moved over to the new explicit API.", so I > guess you want devm_reset_control_get_optional_exclusive or even > devm_reset_control_get_optional_shared here. Thanks for pointing this, I will change it. > > @Philipp: maybe a check in checkpatch that warns about introduction of > such new instances would be good?! > > > + } > > + > > + /* Deassert reset */ > > + ret = reset_control_deassert(pwm->rst); > > + if (ret) { > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > + return ret; > > + } > > + > > pwm->chip.dev = &pdev->dev; > > pwm->chip.ops = &sun4i_pwm_ops; > > pwm->chip.base = -1; > > @@ -377,19 +393,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > ret = pwmchip_add(&pwm->chip); > > if (ret < 0) { > > dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); > > - return ret; > > + goto err_pwm_add; > > } > > > > platform_set_drvdata(pdev, pwm); > > > > return 0; > > + > > +err_pwm_add: > > + reset_control_assert(pwm->rst); > > + > > + return ret; > > } > > > > static int sun4i_pwm_remove(struct platform_device *pdev) > > { > > struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = pwmchip_remove(&pwm->chip); > > + if (ret) > > + return ret; > > + > > + reset_control_assert(pwm->rst); > > > > - return pwmchip_remove(&pwm->chip); > > + return 0; > > } > > > > static struct platform_driver sun4i_pwm_driver = { > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi, On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 PWM core needs bus clock to be enabled in order to work. > > > > Add an optional probe for it and a fallback for previous > > bindings without name on module clock. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index d194b8ebdb00..b5e7ac364f59 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > > > struct sun4i_pwm_chip { > > struct pwm_chip chip; > > + struct clk *bus_clk; > > struct clk *clk; > > struct reset_control *rst; > > void __iomem *base; > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > Adding more context here: > > | pwm->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(pwm->clk)) > > return PTR_ERR(pwm->clk); > > > > + /* Get all clocks and reset line */ > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > + if (IS_ERR(pwm->clk)) { > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > + PTR_ERR(pwm->clk)); > > + return PTR_ERR(pwm->clk); > > + } > > I guess you want to drop the first assignment to pwm->clk. devm_clk_get_optional will return NULL if there is no entry, I don't get where I need to drop it assignment. > > > + /* Fallback for old dtbs with a single clock and no name */ > > + if (!pwm->clk) { > > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(pwm->clk)) { > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > + PTR_ERR(pwm->clk)); > > + return PTR_ERR(pwm->clk); > > + } > > + } > > There is a slight change of behaviour if I'm not mistaken. If you have > this: > > clocks = <&clk1>; > clock-names = "mod"; > > pwm { > compatible = "allwinner,sun4i-a10-pwm" > clocks = <&clk2>; > } > > you now use clk1 instead of clk2 before. > > Assuming this is only a theoretical problem, at least pointing this out > in the commit log would be good I think. Yes it's correct and as you said the driver don't check for a correct device tree, that why it's now optional probe. Let's assume that's the device-tree is correct, I will add a comment in the commit log. > > > + pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus"); > > + if (IS_ERR(pwm->bus_clk)) { > > + dev_err(&pdev->dev, "get bus_clock failed %ld\n", > > + PTR_ERR(pwm->bus_clk)); > > + return PTR_ERR(pwm->bus_clk); > > + } > > + > > pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > > if (IS_ERR(pwm->rst)) { > > if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > > @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > return ret; > > } > > > > + /* Enable bus clock */ > > + ret = clk_prepare_enable(pwm->bus_clk); > > + if (ret) { > > + dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n"); > > I'd do s/prepare_enable/prepare and enable/ here. Ok > > > + goto err_bus; > > + } > > + > > pwm->chip.dev = &pdev->dev; > > pwm->chip.ops = &sun4i_pwm_ops; > > pwm->chip.base = -1; > > @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > return 0; > > > > err_pwm_add: > > + clk_disable_unprepare(pwm->bus_clk); > > +err_bus: > > reset_control_assert(pwm->rst); > > > > return ret; > > What is that clock used for? Is it required to access the hardware > registers? Or is it only required while the PWM is enabled? If so you > could enable the clock more finegrainded. Regarding the datasheet it's required to access the hardware. page 261 : https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf Regards, Clément > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Clément, On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote: > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > H6 PWM core needs bus clock to be enabled in order to work. > > > > > > Add an optional probe for it and a fallback for previous > > > bindings without name on module clock. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index d194b8ebdb00..b5e7ac364f59 100644 > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > > > > > struct sun4i_pwm_chip { > > > struct pwm_chip chip; > > > + struct clk *bus_clk; > > > struct clk *clk; > > > struct reset_control *rst; > > > void __iomem *base; > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > > > Adding more context here: > > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > if (IS_ERR(pwm->clk)) > > > return PTR_ERR(pwm->clk); > > > > > > + /* Get all clocks and reset line */ > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > > + if (IS_ERR(pwm->clk)) { > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > + PTR_ERR(pwm->clk)); > > > + return PTR_ERR(pwm->clk); > > > + } > > > > I guess you want to drop the first assignment to pwm->clk. > > devm_clk_get_optional will return NULL if there is no entry, I don't > get where I need to drop it assignment. With your patch the code looks as follows: pwm->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pwm->clk)) return PTR_ERR(pwm->clk); /* Get all clocks and reset line */ pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); ... The assignment to pwm->clk above the comment is the one I suggested to drop. > > > + /* Fallback for old dtbs with a single clock and no name */ > > > + if (!pwm->clk) { > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > + if (IS_ERR(pwm->clk)) { > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > + PTR_ERR(pwm->clk)); > > > + return PTR_ERR(pwm->clk); > > > + } > > > + } > > > > There is a slight change of behaviour if I'm not mistaken. If you have > > this: > > > > clocks = <&clk1>; > > clock-names = "mod"; > > > > pwm { > > compatible = "allwinner,sun4i-a10-pwm" > > clocks = <&clk2>; > > } > > > > you now use clk1 instead of clk2 before. > > > > Assuming this is only a theoretical problem, at least pointing this out > > in the commit log would be good I think. > > Yes it's correct and as you said the driver don't check for a correct > device tree, that why it's now optional probe. > Let's assume that's the device-tree is correct, I will add a comment > in the commit log. If the mod clock was shared by all peripherals on the bus this would be IMHO quite elegant. Probably it depends on what you mean by saying "incorrect" if this snippet is incorrect. (It can be part of a valid dtb that even complies to the binding documentation. However that's not how any existing allwinner hardware looks like.) But let's stop arguing as we agree it's a corner case and if you mention it in the commit log we're both happy. > > What is that clock used for? Is it required to access the hardware > > registers? Or is it only required while the PWM is enabled? If so you > > could enable the clock more finegrainded. > > Regarding the datasheet it's required to access the hardware. > page 261 : https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf So enabling the bus clock is called "open APB1 Bus gating" in that manual? If I understand that correctly the bus clock then only need to be on while accessing the registers and could be disabled once the hardware is programmed and running. Can you please describe that in a comment. Something like: /* * We're keeping the bus clock on for the sake of simplicity. * Actually it only needs to be on for hardware register * accesses. */ should be fine. This way it's at least obvious that the handling could be improved. Best regards Uwe
Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König napisal(a): > Hello Clément, > > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote: > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > H6 PWM core needs bus clock to be enabled in order to work. > > > > > > > > Add an optional probe for it and a fallback for previous > > > > bindings without name on module clock. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > --- > > > > > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 36 insertions(+) > > > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > > index d194b8ebdb00..b5e7ac364f59 100644 > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > > > > > > > struct sun4i_pwm_chip { > > > > > > > > struct pwm_chip chip; > > > > > > > > + struct clk *bus_clk; > > > > > > > > struct clk *clk; > > > > struct reset_control *rst; > > > > void __iomem *base; > > > > > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device > > > > *pdev)> > > > > Adding more context here: > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > | > > > > if (IS_ERR(pwm->clk)) > > > > > > > > return PTR_ERR(pwm->clk); > > > > > > > > + /* Get all clocks and reset line */ > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > > > + if (IS_ERR(pwm->clk)) { > > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > > + PTR_ERR(pwm->clk)); > > > > + return PTR_ERR(pwm->clk); > > > > + } > > > > > > I guess you want to drop the first assignment to pwm->clk. > > > > devm_clk_get_optional will return NULL if there is no entry, I don't > > get where I need to drop it assignment. > > With your patch the code looks as follows: > > pwm->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pwm->clk)) > return PTR_ERR(pwm->clk); > > /* Get all clocks and reset line */ > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); Actually, it's the other way around, e.g. "mod" clock is checked first. > ... > > The assignment to pwm->clk above the comment is the one I suggested to > drop. Neither can be dropped. DT files for other SoCs don't have clock-names property, so search for "mod" clock will fail and then fallback option without name is used. Best regards, Jernej > > > > > + /* Fallback for old dtbs with a single clock and no name */ > > > > + if (!pwm->clk) { > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > + if (IS_ERR(pwm->clk)) { > > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > > + PTR_ERR(pwm->clk)); > > > > + return PTR_ERR(pwm->clk); > > > > + } > > > > + } > > > > > > There is a slight change of behaviour if I'm not mistaken. If you have > > > > > > this: > > > clocks = <&clk1>; > > > clock-names = "mod"; > > > > > > pwm { > > > > > > compatible = "allwinner,sun4i-a10-pwm" > > > clocks = <&clk2>; > > > > > > } > > > > > > you now use clk1 instead of clk2 before. > > > > > > Assuming this is only a theoretical problem, at least pointing this out > > > in the commit log would be good I think. > > > > Yes it's correct and as you said the driver don't check for a correct > > device tree, that why it's now optional probe. > > Let's assume that's the device-tree is correct, I will add a comment > > in the commit log. > > If the mod clock was shared by all peripherals on the bus this would be > IMHO quite elegant. Probably it depends on what you mean by saying > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb > that even complies to the binding documentation. However that's not how > any existing allwinner hardware looks like.) But let's stop arguing as > we agree it's a corner case and if you mention it in the commit log > we're both happy. > > > > What is that clock used for? Is it required to access the hardware > > > registers? Or is it only required while the PWM is enabled? If so you > > > could enable the clock more finegrainded. > > > > Regarding the datasheet it's required to access the hardware. > > page 261 : > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf > So enabling the bus clock is called "open APB1 Bus gating" in that > manual? If I understand that correctly the bus clock then only need to > be on while accessing the registers and could be disabled once the > hardware is programmed and running. > > Can you please describe that in a comment. Something like: > > /* > * We're keeping the bus clock on for the sake of simplicity. > * Actually it only needs to be on for hardware register > * accesses. > */ > > should be fine. This way it's at least obvious that the handling could > be improved. > > Best regards > Uwe
Hi, On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König > napisal(a): > > Hello Clément, > > > > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote: > > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > H6 PWM core needs bus clock to be enabled in order to work. > > > > > > > > > > Add an optional probe for it and a fallback for previous > > > > > bindings without name on module clock. > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > --- > > > > > > > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 36 insertions(+) > > > > > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > > > index d194b8ebdb00..b5e7ac364f59 100644 > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > > > > > > > > > struct sun4i_pwm_chip { > > > > > > > > > > struct pwm_chip chip; > > > > > > > > > > + struct clk *bus_clk; > > > > > > > > > > struct clk *clk; > > > > > struct reset_control *rst; > > > > > void __iomem *base; > > > > > > > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device > > > > > *pdev)> > > > > > Adding more context here: > > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > | > > > > > if (IS_ERR(pwm->clk)) > > > > > > > > > > return PTR_ERR(pwm->clk); > > > > > > > > > > + /* Get all clocks and reset line */ > > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > > > > + if (IS_ERR(pwm->clk)) { > > > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > > > + PTR_ERR(pwm->clk)); > > > > > + return PTR_ERR(pwm->clk); > > > > > + } > > > > > > > > I guess you want to drop the first assignment to pwm->clk. > > > > > > devm_clk_get_optional will return NULL if there is no entry, I don't > > > get where I need to drop it assignment. > > > > With your patch the code looks as follows: > > > > pwm->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(pwm->clk)) > > return PTR_ERR(pwm->clk); > > > > /* Get all clocks and reset line */ > > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > Actually, it's the other way around, e.g. "mod" clock is checked first. The first devm_clk_get is indeed wrong, I will remove it! > > > ... > > > > The assignment to pwm->clk above the comment is the one I suggested to > > drop. > > Neither can be dropped. DT files for other SoCs don't have clock-names > property, so search for "mod" clock will fail and then fallback option without > name is used. > > Best regards, > Jernej > > > > > > > > + /* Fallback for old dtbs with a single clock and no name */ > > > > > + if (!pwm->clk) { > > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > > + if (IS_ERR(pwm->clk)) { > > > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > > > + PTR_ERR(pwm->clk)); > > > > > + return PTR_ERR(pwm->clk); > > > > > + } > > > > > + } > > > > > > > > There is a slight change of behaviour if I'm not mistaken. If you have > > > > > > > > this: > > > > clocks = <&clk1>; > > > > clock-names = "mod"; > > > > > > > > pwm { > > > > > > > > compatible = "allwinner,sun4i-a10-pwm" > > > > clocks = <&clk2>; > > > > > > > > } > > > > > > > > you now use clk1 instead of clk2 before. > > > > > > > > Assuming this is only a theoretical problem, at least pointing this out > > > > in the commit log would be good I think. > > > > > > Yes it's correct and as you said the driver don't check for a correct > > > device tree, that why it's now optional probe. > > > Let's assume that's the device-tree is correct, I will add a comment > > > in the commit log. > > > > If the mod clock was shared by all peripherals on the bus this would be > > IMHO quite elegant. Probably it depends on what you mean by saying > > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb > > that even complies to the binding documentation. However that's not how > > any existing allwinner hardware looks like.) But let's stop arguing as > > we agree it's a corner case and if you mention it in the commit log > > we're both happy. > > > > > > What is that clock used for? Is it required to access the hardware > > > > registers? Or is it only required while the PWM is enabled? If so you > > > > could enable the clock more finegrainded. > > > > > > Regarding the datasheet it's required to access the hardware. > > > page 261 : > > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf > > So enabling the bus clock is called "open APB1 Bus gating" in that > > manual? If I understand that correctly the bus clock then only need to > > be on while accessing the registers and could be disabled once the > > hardware is programmed and running. > > > > Can you please describe that in a comment. Something like: > > > > /* > > * We're keeping the bus clock on for the sake of simplicity. > > * Actually it only needs to be on for hardware register > > * accesses. > > */ > > > > should be fine. This way it's at least obvious that the handling could > > be improved. > > > > Best regards > > Uwe > > > >
Dne ponedeljek, 04. november 2019 ob 21:27:04 CET je Clément Péron napisal(a): > Hi, > > On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König > > > > napisal(a): > > > Hello Clément, > > > > > > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote: > > > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König > > > > > > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote: > > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > > > > > H6 PWM core needs bus clock to be enabled in order to work. > > > > > > > > > > > > Add an optional probe for it and a fallback for previous > > > > > > bindings without name on module clock. > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > > > --- > > > > > > > > > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 36 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > > > > index d194b8ebdb00..b5e7ac364f59 100644 > > > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data { > > > > > > > > > > > > struct sun4i_pwm_chip { > > > > > > > > > > > > struct pwm_chip chip; > > > > > > > > > > > > + struct clk *bus_clk; > > > > > > > > > > > > struct clk *clk; > > > > > > struct reset_control *rst; > > > > > > void __iomem *base; > > > > > > > > > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct > > > > > > platform_device > > > > > > *pdev)> > > > > > > > > > > > Adding more context here: > > > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > > | > > > > > > if (IS_ERR(pwm->clk)) > > > > > > > > > > > > return PTR_ERR(pwm->clk); > > > > > > > > > > > > + /* Get all clocks and reset line */ > > > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > > > > > + if (IS_ERR(pwm->clk)) { > > > > > > + dev_err(&pdev->dev, "get clock failed %ld\n", > > > > > > + PTR_ERR(pwm->clk)); > > > > > > + return PTR_ERR(pwm->clk); > > > > > > + } > > > > > > > > > > I guess you want to drop the first assignment to pwm->clk. > > > > > > > > devm_clk_get_optional will return NULL if there is no entry, I don't > > > > get where I need to drop it assignment. > > > > > > With your patch the code looks as follows: > > > pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > if (IS_ERR(pwm->clk)) > > > > > > return PTR_ERR(pwm->clk); > > > > > > /* Get all clocks and reset line */ > > > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod"); > > > > Actually, it's the other way around, e.g. "mod" clock is checked first. > > The first devm_clk_get is indeed wrong, I will remove it! Sorry, I missed that too. Yeah, it should be removed. Best regards, Jernej > > > > ... > > > > > > The assignment to pwm->clk above the comment is the one I suggested to > > > drop. > > > > Neither can be dropped. DT files for other SoCs don't have clock-names > > property, so search for "mod" clock will fail and then fallback option > > without name is used. > > > > Best regards, > > Jernej > > > > > > > > + /* Fallback for old dtbs with a single clock and no name */ > > > > > > + if (!pwm->clk) { > > > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > > > + if (IS_ERR(pwm->clk)) { > > > > > > + dev_err(&pdev->dev, "get clock failed > > > > > > %ld\n", > > > > > > + PTR_ERR(pwm->clk)); > > > > > > + return PTR_ERR(pwm->clk); > > > > > > + } > > > > > > + } > > > > > > > > > > There is a slight change of behaviour if I'm not mistaken. If you > > > > > have > > > > > > > > > > this: > > > > > clocks = <&clk1>; > > > > > clock-names = "mod"; > > > > > > > > > > pwm { > > > > > > > > > > compatible = "allwinner,sun4i-a10-pwm" > > > > > clocks = <&clk2>; > > > > > > > > > > } > > > > > > > > > > you now use clk1 instead of clk2 before. > > > > > > > > > > Assuming this is only a theoretical problem, at least pointing this > > > > > out > > > > > in the commit log would be good I think. > > > > > > > > Yes it's correct and as you said the driver don't check for a correct > > > > device tree, that why it's now optional probe. > > > > Let's assume that's the device-tree is correct, I will add a comment > > > > in the commit log. > > > > > > If the mod clock was shared by all peripherals on the bus this would be > > > IMHO quite elegant. Probably it depends on what you mean by saying > > > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb > > > that even complies to the binding documentation. However that's not how > > > any existing allwinner hardware looks like.) But let's stop arguing as > > > we agree it's a corner case and if you mention it in the commit log > > > we're both happy. > > > > > > > > What is that clock used for? Is it required to access the hardware > > > > > registers? Or is it only required while the PWM is enabled? If so > > > > > you > > > > > could enable the clock more finegrainded. > > > > > > > > Regarding the datasheet it's required to access the hardware. > > > > page 261 : > > > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf > > > > > > So enabling the bus clock is called "open APB1 Bus gating" in that > > > manual? If I understand that correctly the bus clock then only need to > > > be on while accessing the registers and could be disabled once the > > > hardware is programmed and running. > > > > > > Can you please describe that in a comment. Something like: > > > /* > > > > > > * We're keeping the bus clock on for the sake of simplicity. > > > * Actually it only needs to be on for hardware register > > > * accesses. > > > */ > > > > > > should be fine. This way it's at least obvious that the handling could > > > be improved. > > > > > > Best regards > > > Uwe
Hi Uwe On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > PWM core has an option to bypass whole logic and output unchanged source > > clock as PWM output. This is achieved by enabling bypass bit. > > > > Note that when bypass is enabled, no other setting has any meaning, not > > even enable bit. > > > > This mode of operation is needed to achieve high enough frequency to > > serve as clock source for AC200 chip, which is integrated into same > > package as H6 SoC. > > I think the , should be dropped. > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index b5e7ac364f59..2441574674d9 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -3,6 +3,10 @@ > > * Driver for Allwinner sun4i Pulse Width Modulation Controller > > * > > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com> > > + * > > + * Limitations: > > + * - When outputing the source clock directly, the PWM logic will be bypassed > > + * and the currently running period is not guaranted to be completed > > Typo: guaranted -> guaranteed > > > */ > > > > #include <linux/bitops.h> > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = { > > > > struct sun4i_pwm_data { > > bool has_prescaler_bypass; > > + bool has_direct_mod_clk_output; > > unsigned int npwm; > > }; > > > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > + /* > > + * PWM chapter in H6 manual has a diagram which explains that if bypass > > + * bit is set, no other setting has any meaning. Even more, experiment > > + * proved that also enable bit is ignored in this case. > > + */ > > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) && > > + data->has_direct_mod_clk_output) { > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > > + state->duty_cycle = state->period / 2; > > + state->polarity = PWM_POLARITY_NORMAL; > > + state->enabled = true; > > + return; > > + } > > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer > to let .get_state() round up which together with .apply_state() rounding > down yields sound behaviour. Ok > > > + > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > > sun4i_pwm->data->has_prescaler_bypass) > > prescaler = 1; > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > { > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > struct pwm_state cstate; > > - u32 ctrl; > > + u32 ctrl, clk_rate; > > + bool bypass; > > int ret; > > unsigned int delay_us; > > unsigned long now; > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > } > > } > > > > + /* > > + * Although it would make much more sense to check for bypass in > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > > + * Period is allowed to be rounded up or down. > > + */ > > + clk_rate = clk_get_rate(sun4i_pwm->clk); > > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > > + state->enabled); > > I guess the compiler is smart enough here, but checking for > state->enabled is cheaper than the other checks, so putting this at the > start of the expression seems sensible. > > The comment doesn't match the code. You don't round up state->period. > (This is good, please fix the comment.) I think dropping the check > > state->period * clk_rate < NSEC_PER_SEC + clk_rate > > would be fine, too. Ok > > I'd like to have a check for > > state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 && > state->duty_cycle * clk_rate < NSEC_PER_SEC > > here. If this isn't true rather disable the PWM or output a 100% duty > cycle with a larger period. Why not just having the duty_cycle is 50% only ? state->duty_cycle * 2 == state->period; Regards, Clement > > > + > > spin_lock(&sun4i_pwm->ctrl_lock); > > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Nov 04, 2019 at 09:11:57AM +0100, Uwe Kleine-König wrote: > Hello, > > adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm > not stripping the uncommented parts of the patch. > > On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote: > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > H6 PWM core needs deasserted reset line in order to work. > > > > Add an optional probe for it. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 6f5840a1a82d..d194b8ebdb00 100644 > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -16,6 +16,7 @@ > > #include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/pwm.h> > > +#include <linux/reset.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > #include <linux/time.h> > > @@ -78,6 +79,7 @@ struct sun4i_pwm_data { > > struct sun4i_pwm_chip { > > struct pwm_chip chip; > > struct clk *clk; > > + struct reset_control *rst; > > void __iomem *base; > > spinlock_t ctrl_lock; > > const struct sun4i_pwm_data *data; > > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > if (IS_ERR(pwm->clk)) > > return PTR_ERR(pwm->clk); > > > > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > > + if (IS_ERR(pwm->rst)) { > > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > > + return PTR_ERR(pwm->rst); > > + dev_info(&pdev->dev, "no reset control found\n"); > > I would degrade this to a dev_dbg. Otherwise this spams the log for all > unaffected machines. The _optional variants return NULL if the reset is not specified in the device tree, so this is not "no reset control found", but a real error that should be returned. > devm_reset_control_get_optional() is defined in a section that has a > comment "These inline function calls will be removed once all > consumers have been moved over to the new explicit API.", so I guess > you want devm_reset_control_get_optional_exclusive or even > devm_reset_control_get_optional_shared here. Correct. If this driver deasserts in probe() and asserts the reset in remove(), this can use the refcounting _shared variant. > @Philipp: maybe a check in checkpatch that warns about introduction of > such new instances would be good?! Yes, that would be helpful. regards Philipp
Hi Clément, On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote: > On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > PWM core has an option to bypass whole logic and output unchanged source > > > clock as PWM output. This is achieved by enabling bypass bit. > > > > > > Note that when bypass is enabled, no other setting has any meaning, not > > > even enable bit. > > > > > > This mode of operation is needed to achieve high enough frequency to > > > serve as clock source for AC200 chip, which is integrated into same > > > package as H6 SoC. > > > > I think the , should be dropped. > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index b5e7ac364f59..2441574674d9 100644 > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -3,6 +3,10 @@ > > > * Driver for Allwinner sun4i Pulse Width Modulation Controller > > > * > > > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > + * > > > + * Limitations: > > > + * - When outputing the source clock directly, the PWM logic will be bypassed > > > + * and the currently running period is not guaranted to be completed > > > > Typo: guaranted -> guaranteed > > > > > */ > > > > > > #include <linux/bitops.h> > > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = { > > > > > > struct sun4i_pwm_data { > > > bool has_prescaler_bypass; > > > + bool has_direct_mod_clk_output; > > > unsigned int npwm; > > > }; > > > > > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, > > > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > > + /* > > > + * PWM chapter in H6 manual has a diagram which explains that if bypass > > > + * bit is set, no other setting has any meaning. Even more, experiment > > > + * proved that also enable bit is ignored in this case. > > > + */ > > > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) && > > > + data->has_direct_mod_clk_output) { > > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > > > + state->duty_cycle = state->period / 2; > > > + state->polarity = PWM_POLARITY_NORMAL; > > > + state->enabled = true; > > > + return; > > > + } > > > > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer > > to let .get_state() round up which together with .apply_state() rounding > > down yields sound behaviour. > Ok > > > > > + > > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > > > sun4i_pwm->data->has_prescaler_bypass) > > > prescaler = 1; > > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > { > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > > struct pwm_state cstate; > > > - u32 ctrl; > > > + u32 ctrl, clk_rate; > > > + bool bypass; > > > int ret; > > > unsigned int delay_us; > > > unsigned long now; > > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > } > > > } > > > > > > + /* > > > + * Although it would make much more sense to check for bypass in > > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > > > + * Period is allowed to be rounded up or down. > > > + */ > > > + clk_rate = clk_get_rate(sun4i_pwm->clk); > > > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > > > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > > > + state->enabled); > > > > I guess the compiler is smart enough here, but checking for > > state->enabled is cheaper than the other checks, so putting this at the > > start of the expression seems sensible. > > > > The comment doesn't match the code. You don't round up state->period. > > (This is good, please fix the comment.) I think dropping the check > > > > state->period * clk_rate < NSEC_PER_SEC + clk_rate > > > > would be fine, too. > Ok > > > > > I'd like to have a check for > > > > state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 && > > state->duty_cycle * clk_rate < NSEC_PER_SEC > > > > here. If this isn't true rather disable the PWM or output a 100% duty > > cycle with a larger period. > > Why not just having the duty_cycle is 50% only ? > state->duty_cycle * 2 == state->period; Yeah, for the bypass case you can only provide a 50% duty cycle. The problem you have to address is that you cannot rely on your consumer to request only 50% duty cycles. So you have to implement some behaviour if your consumer requests period = 1 / clk_rate and 20% duty cycle. Where I want to get the pwm framework as a whole is to let lowlevel drivers round down both duty_cycle and period to the next possible values in their .apply callback to be able to provide a more uniform behaviour for consumers. So here this would mean: - 1 / clk_rate <= state->period < smallest value without bypass && 0 <= state->duty_cycle < state->period / 2 => provide a constant 0 - 1 / clk_rate <= state->period < smallest value without bypass && state->period / 2 <= state->duty_cycle < state->period => use bypass mode providing 50% duty cycle - 1 / clk_rate <= state->period < smallest value without bypass && state->period == state->duty_cycle => provide a constant 1 Best regards Uwe
Hi Uwe, On Tue, 5 Nov 2019 at 08:29, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hi Clément, > > On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote: > > On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote: > > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > > PWM core has an option to bypass whole logic and output unchanged source > > > > clock as PWM output. This is achieved by enabling bypass bit. > > > > > > > > Note that when bypass is enabled, no other setting has any meaning, not > > > > even enable bit. > > > > > > > > This mode of operation is needed to achieve high enough frequency to > > > > serve as clock source for AC200 chip, which is integrated into same > > > > package as H6 SoC. > > > > > > I think the , should be dropped. > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > > --- > > > > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > > index b5e7ac364f59..2441574674d9 100644 > > > > --- a/drivers/pwm/pwm-sun4i.c > > > > +++ b/drivers/pwm/pwm-sun4i.c > > > > @@ -3,6 +3,10 @@ > > > > * Driver for Allwinner sun4i Pulse Width Modulation Controller > > > > * > > > > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com> > > > > + * > > > > + * Limitations: > > > > + * - When outputing the source clock directly, the PWM logic will be bypassed > > > > + * and the currently running period is not guaranted to be completed > > > > > > Typo: guaranted -> guaranteed > > > > > > > */ > > > > > > > > #include <linux/bitops.h> > > > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = { > > > > > > > > struct sun4i_pwm_data { > > > > bool has_prescaler_bypass; > > > > + bool has_direct_mod_clk_output; > > > > unsigned int npwm; > > > > }; > > > > > > > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip, > > > > > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > > > > + /* > > > > + * PWM chapter in H6 manual has a diagram which explains that if bypass > > > > + * bit is set, no other setting has any meaning. Even more, experiment > > > > + * proved that also enable bit is ignored in this case. > > > > + */ > > > > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) && > > > > + data->has_direct_mod_clk_output) { > > > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate); > > > > + state->duty_cycle = state->period / 2; > > > > + state->polarity = PWM_POLARITY_NORMAL; > > > > + state->enabled = true; > > > > + return; > > > > + } > > > > > > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer > > > to let .get_state() round up which together with .apply_state() rounding > > > down yields sound behaviour. > > Ok > > > > > > > + > > > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) && > > > > sun4i_pwm->data->has_prescaler_bypass) > > > > prescaler = 1; > > > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > { > > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > > > struct pwm_state cstate; > > > > - u32 ctrl; > > > > + u32 ctrl, clk_rate; > > > > + bool bypass; > > > > int ret; > > > > unsigned int delay_us; > > > > unsigned long now; > > > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > } > > > > } > > > > > > > > + /* > > > > + * Although it would make much more sense to check for bypass in > > > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled". > > > > + * Period is allowed to be rounded up or down. > > > > + */ > > > > + clk_rate = clk_get_rate(sun4i_pwm->clk); > > > > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC && > > > > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) && > > > > + state->enabled); > > > > > > I guess the compiler is smart enough here, but checking for > > > state->enabled is cheaper than the other checks, so putting this at the > > > start of the expression seems sensible. > > > > > > The comment doesn't match the code. You don't round up state->period. > > > (This is good, please fix the comment.) I think dropping the check > > > > > > state->period * clk_rate < NSEC_PER_SEC + clk_rate > > > > > > would be fine, too. > > Ok > > > > > > > > I'd like to have a check for > > > > > > state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 && > > > state->duty_cycle * clk_rate < NSEC_PER_SEC > > > > > > here. If this isn't true rather disable the PWM or output a 100% duty > > > cycle with a larger period. > > > > Why not just having the duty_cycle is 50% only ? > > state->duty_cycle * 2 == state->period; > > Yeah, for the bypass case you can only provide a 50% duty cycle. The > problem you have to address is that you cannot rely on your consumer to > request only 50% duty cycles. So you have to implement some behaviour if > your consumer requests period = 1 / clk_rate and 20% duty cycle. So you request to add a new patch in this series for fixing the actual PWM behavior at corner case? This series just want to add a new device and a new bypass functionality and I can't measure the output of PWM and testing it properly. Can this be done in another patch/series ? Regards, Clément > > Where I want to get the pwm framework as a whole is to let lowlevel > drivers round down both duty_cycle and period to the next possible values > in their .apply callback to be able to provide a more uniform behaviour > for consumers. So here this would mean: > > - 1 / clk_rate <= state->period < smallest value without bypass && > 0 <= state->duty_cycle < state->period / 2 > => provide a constant 0 > > - 1 / clk_rate <= state->period < smallest value without bypass && > state->period / 2 <= state->duty_cycle < state->period > => use bypass mode providing 50% duty cycle > > - 1 / clk_rate <= state->period < smallest value without bypass && > state->period == state->duty_cycle > => provide a constant 1 > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Philipp, On Tue, 5 Nov 2019 at 08:01, Philipp Zabel <pza@pengutronix.de> wrote: > > On Mon, Nov 04, 2019 at 09:11:57AM +0100, Uwe Kleine-König wrote: > > Hello, > > > > adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm > > not stripping the uncommented parts of the patch. > > > > On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > H6 PWM core needs deasserted reset line in order to work. > > > > > > Add an optional probe for it. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > > --- > > > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++-- > > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index 6f5840a1a82d..d194b8ebdb00 100644 > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/of_device.h> > > > #include <linux/platform_device.h> > > > #include <linux/pwm.h> > > > +#include <linux/reset.h> > > > #include <linux/slab.h> > > > #include <linux/spinlock.h> > > > #include <linux/time.h> > > > @@ -78,6 +79,7 @@ struct sun4i_pwm_data { > > > struct sun4i_pwm_chip { > > > struct pwm_chip chip; > > > struct clk *clk; > > > + struct reset_control *rst; > > > void __iomem *base; > > > spinlock_t ctrl_lock; > > > const struct sun4i_pwm_data *data; > > > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > > if (IS_ERR(pwm->clk)) > > > return PTR_ERR(pwm->clk); > > > > > > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL); > > > + if (IS_ERR(pwm->rst)) { > > > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER) > > > + return PTR_ERR(pwm->rst); > > > + dev_info(&pdev->dev, "no reset control found\n"); > > > > I would degrade this to a dev_dbg. Otherwise this spams the log for all > > unaffected machines. > > The _optional variants return NULL if the reset is not specified in the > device tree, so this is not "no reset control found", but a real error > that should be returned. Correct, Thanks for the catch, Clément > > > devm_reset_control_get_optional() is defined in a section that has a > > comment "These inline function calls will be removed once all > > consumers have been moved over to the new explicit API.", so I guess > > you want devm_reset_control_get_optional_exclusive or even > > devm_reset_control_get_optional_shared here. > > Correct. If this driver deasserts in probe() and asserts the reset in > remove(), this can use the refcounting _shared variant. > > > @Philipp: maybe a check in checkpatch that warns about introduction of > > such new instances would be good?! > > Yes, that would be helpful. > > regards > Philipp
Hello Clément, On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote: > This series just want to add a new device and a new bypass > functionality and I can't measure the output of PWM and testing it > properly. > Can this be done in another patch/series ? I'm fine if you implement the bypass stuff with this logic and keep the other stuff as is. Best regards Uwe
On Tue, 5 Nov 2019 at 14:12, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Clément, > > On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote: > > This series just want to add a new device and a new bypass > > functionality and I can't measure the output of PWM and testing it > > properly. > > Can this be done in another patch/series ? > > I'm fine if you implement the bypass stuff with this logic and keep the > other stuff as is. Thanks, Clément > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |