| Message ID | 1562576868-8124-7-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | treewide: modify sh-pfc and add support pwm duty zero | expand |
Hello, On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote: > The R-Car SoCs PWM Timer cannot output duty zero. So, this patch > adds gpio support to output it. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/pwm/pwm-rcar.c | 36 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) I'd like to see a paragraph at the top of the driver describing the limitations of this driver similar to what pwm-sifive.c does. Something like: diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c index 5b2b8ecc354c..b67ac84db834 100644 --- a/drivers/pwm/pwm-rcar.c +++ b/drivers/pwm/pwm-rcar.c @@ -3,6 +3,9 @@ * R-Car PWM Timer driver * * Copyright (C) 2015 Renesas Electronics Corporation + * + * Limitations: + * - The hardware cannot generate a 0% duty cycle. */ #include <linux/clk.h> While at it: If there is a publicly available reference manual adding a line: Reference Manual: https://... would be great, too. > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > index c8cd43f..1c19a8b 100644 > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -7,6 +7,7 @@ > > #include <linux/clk.h> > #include <linux/err.h> > +#include <linux/gpio/consumer.h> > #include <linux/io.h> > #include <linux/log2.h> > #include <linux/math64.h> > @@ -38,6 +39,7 @@ struct rcar_pwm_chip { > struct pwm_chip chip; > void __iomem *base; > struct clk *clk; > + struct gpio_desc *gpio; > }; > > static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip) > @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns, > ph = tmp & RCAR_PWMCNT_PH0_MASK; > > /* Avoid prohibited setting */ > - if (cyc == 0 || ph == 0) > + if (cyc == 0) > return -EINVAL; > + /* Try to use GPIO to output duty zero */ > + if (ph == 0) > + return -EAGAIN; If there is no gpio requesting cyc=0 should still yield an error. > rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT); > > @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp) > rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); > } > > +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp) > +{ > + if (rp->gpio) > + return 0; > + > + rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW); > + if (!IS_ERR(rp->gpio)) > + return 0; > + > + rp->gpio = NULL; > + return -EINVAL; Please use gpiod_get_optional() instead of open coding it. Does getting the gpio automatically switch the pinmuxing? If yes, this is IMHO a really surprising mis-feature of the gpio subsystem. I'd prefer to "get" the gpio at probe time and only switch the pinmuxing in .apply(). This makes .apply() quicker, ensures that all resources necessary for pwm operation are available, handles -EPROBE_DEFER (and maybe other errors) correctly. Note you're introducing a bug here because switching to gpio doesn't ensure that the currently running period is completed. > +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp) > +{ > + if (!rp->gpio) > + return; > + > + gpiod_put(rp->gpio); > + rp->gpio = NULL; > +} > + > static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_state *state) > { > @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > if (!state->enabled) { > rcar_pwm_disable(rp); > + rcar_pwm_gpiod_put(rp); From the framework's POV disabling a PWM is quite similar to duty cycle 0. Assuming disabling the PWM completes the currently running period[1] it might be better and easier to disable instead of switching to gpio. (Further assuming that disable really yields the inactive level which is should and is another limitation if not.) > return 0; > } > > @@ -187,8 +215,12 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ > rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); > > - if (!ret) > + if (!ret) { > ret = rcar_pwm_enable(rp); > + rcar_pwm_gpiod_put(rp); > + } else if (ret == -EAGAIN) { > + ret = rcar_pwm_gpiod_get(rp); > + } > > return ret; > } Best regards Uwe [1] if not, please add "Disabling doesn't complete the currently running period" to the list of limitations.
Hi Uwe, Thank you for your review! > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM > > Hello, > > On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote: > > The R-Car SoCs PWM Timer cannot output duty zero. So, this patch > > adds gpio support to output it. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/pwm/pwm-rcar.c | 36 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 34 insertions(+), 2 deletions(-) > > I'd like to see a paragraph at the top of the driver describing the > limitations of this driver similar to what pwm-sifive.c does. > > Something like: > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > index 5b2b8ecc354c..b67ac84db834 100644 > --- a/drivers/pwm/pwm-rcar.c > +++ b/drivers/pwm/pwm-rcar.c > @@ -3,6 +3,9 @@ > * R-Car PWM Timer driver > * > * Copyright (C) 2015 Renesas Electronics Corporation > + * > + * Limitations: > + * - The hardware cannot generate a 0% duty cycle. > */ I'll add this. > #include <linux/clk.h> > > While at it: If there is a publicly available reference manual adding a line: > > Reference Manual: https://... > > would be great, too. Unfortunately, the document is not public... > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c > > index c8cd43f..1c19a8b 100644 > > --- a/drivers/pwm/pwm-rcar.c > > +++ b/drivers/pwm/pwm-rcar.c > > @@ -7,6 +7,7 @@ > > > > #include <linux/clk.h> > > #include <linux/err.h> > > +#include <linux/gpio/consumer.h> > > #include <linux/io.h> > > #include <linux/log2.h> > > #include <linux/math64.h> > > @@ -38,6 +39,7 @@ struct rcar_pwm_chip { > > struct pwm_chip chip; > > void __iomem *base; > > struct clk *clk; > > + struct gpio_desc *gpio; > > }; > > > > static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip) > > @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns, > > ph = tmp & RCAR_PWMCNT_PH0_MASK; > > > > /* Avoid prohibited setting */ > > - if (cyc == 0 || ph == 0) > > + if (cyc == 0) > > return -EINVAL; > > + /* Try to use GPIO to output duty zero */ > > + if (ph == 0) > > + return -EAGAIN; > > If there is no gpio requesting cyc=0 should still yield an error. I'm sorry, I cannot understand this. > > rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT); > > > > @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp) > > rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); > > } > > > > +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp) > > +{ > > + if (rp->gpio) > > + return 0; > > + > > + rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW); > > + if (!IS_ERR(rp->gpio)) > > + return 0; > > + > > + rp->gpio = NULL; > > + return -EINVAL; > > Please use gpiod_get_optional() instead of open coding it. I got it. > Does getting the gpio automatically switch the pinmuxing? > > If yes, this is IMHO a really surprising mis-feature of the gpio > subsystem. I'd prefer to "get" the gpio at probe time and only switch > the pinmuxing in .apply(). This makes .apply() quicker, ensures that all > resources necessary for pwm operation are available, handles > -EPROBE_DEFER (and maybe other errors) correctly. The current pinctrl subsystem only has .set_mux(). I checked the pinctrl subsystem history and the commit 2243a87d90b42eb38bc281957df3e57c712b5e56 removed the ".disable()" ops. So, IIUC, we cannot such a handling. > Note you're introducing a bug here because switching to gpio doesn't > ensure that the currently running period is completed. Umm, the hardware doesn't have such a condition so that the driver cannot manage it. So, I'll add this into the "Limitations" too. > > +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp) > > +{ > > + if (!rp->gpio) > > + return; > > + > > + gpiod_put(rp->gpio); > > + rp->gpio = NULL; > > +} > > + > > static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > struct pwm_state *state) > > { > > @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > if (!state->enabled) { > > rcar_pwm_disable(rp); > > + rcar_pwm_gpiod_put(rp); > > From the framework's POV disabling a PWM is quite similar to duty cycle > 0. Assuming disabling the PWM completes the currently running period[1] > it might be better and easier to disable instead of switching to gpio. > (Further assuming that disable really yields the inactive level which is > should and is another limitation if not.) If we disable the hardware, the duty cycle is 100% unfortunately. So, I think I should describe it as one of "Limitations". > > return 0; > > } > > > > @@ -187,8 +215,12 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ > > rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); > > > > - if (!ret) > > + if (!ret) { > > ret = rcar_pwm_enable(rp); > > + rcar_pwm_gpiod_put(rp); > > + } else if (ret == -EAGAIN) { > > + ret = rcar_pwm_gpiod_get(rp); > > + } > > > > return ret; > > } > > Best regards > Uwe > > [1] if not, please add "Disabling doesn't complete the currently running > period" to the list of limitations. Yeah, the hardware will complete the currently running period, but as I said, the hardware doesn't have such a condition, so that the driver's .apply() returns immediately without the completion. So, I'll add it as a Limitation. Best regards, Yoshihiro Shimoda > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Shimoda-san, On Thu, Aug 8, 2019 at 5:53 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM > > While at it: If there is a publicly available reference manual adding a line: > > > > Reference Manual: https://... > > > > would be great, too. > > Unfortunately, the document is not public... RZ/G1 has the same hardware block, right? Its Hardware User's Manual is publicly available, e.g. for RZ/G1M: https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Thursday, August 8, 2019 3:49 PM > > Hi Shimoda-san, > > On Thu, Aug 8, 2019 at 5:53 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM > > > While at it: If there is a publicly available reference manual adding a line: > > > > > > Reference Manual: https://... > > > > > > would be great, too. > > > > Unfortunately, the document is not public... > > RZ/G1 has the same hardware block, right? > Its Hardware User's Manual is publicly available, e.g. for RZ/G1M: > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents You're right. # Since I could get RZ/G2 series hardware user's manual from public website yesterday, # I was thinking any manuals are not public... Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hello, On Thu, Aug 08, 2019 at 03:52:52AM +0000, Yoshihiro Shimoda wrote: > > From: Uwe Kleine-König, Sent: Wednesday, August 7, 2019 4:03 PM > > On Mon, Jul 08, 2019 at 06:07:47PM +0900, Yoshihiro Shimoda wrote: > > > @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns, > > > ph = tmp & RCAR_PWMCNT_PH0_MASK; > > > > > > /* Avoid prohibited setting */ > > > - if (cyc == 0 || ph == 0) > > > + if (cyc == 0) > > > return -EINVAL; > > > + /* Try to use GPIO to output duty zero */ > > > + if (ph == 0) > > > + return -EAGAIN; > > > > If there is no gpio requesting cyc=0 should still yield an error. > > I'm sorry, I cannot understand this. I meant that if getting the gpio failed but it's needed to yield the right level this should result in an error. I thought I removed that comment because this is taken care of further below. > > > rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT); > > > > > > @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp) > > > rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); > > > } > > > > > > +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp) > > > +{ > > > + if (rp->gpio) > > > + return 0; > > > + > > > + rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW); > > > + if (!IS_ERR(rp->gpio)) > > > + return 0; > > > + > > > + rp->gpio = NULL; > > > + return -EINVAL; > > > > Does getting the gpio automatically switch the pinmuxing? > > > > If yes, this is IMHO a really surprising mis-feature of the gpio > > subsystem. I'd prefer to "get" the gpio at probe time and only switch > > the pinmuxing in .apply(). This makes .apply() quicker, ensures that all > > resources necessary for pwm operation are available, handles > > -EPROBE_DEFER (and maybe other errors) correctly. > > The current pinctrl subsystem only has .set_mux(). I checked the pinctrl subsystem > history and the commit 2243a87d90b42eb38bc281957df3e57c712b5e56 removed the ".disable()" ops. > So, IIUC, we cannot such a handling. You'd need to reactivate the pwm setting when changing from 0% to something bigger of course. But as I understand it "getting" the gpio already implies that it is muxed which is bad here. So it would be great if we could opt-out. Linus? The pwm-imx driver has a similar problem[1] where we already considered using a gpio. There auto-muxing would be in the way, too. (Maybe it isn't because it isn't implmented on i.MX?) > > Note you're introducing a bug here because switching to gpio doesn't > > ensure that the currently running period is completed. > > Umm, the hardware doesn't have such a condition so that the driver cannot manage it. > So, I'll add this into the "Limitations" too. yes please. > > > +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp) > > > +{ > > > + if (!rp->gpio) > > > + return; > > > + > > > + gpiod_put(rp->gpio); > > > + rp->gpio = NULL; > > > +} > > > + > > > static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > struct pwm_state *state) > > > { > > > @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > if (!state->enabled) { > > > rcar_pwm_disable(rp); > > > + rcar_pwm_gpiod_put(rp); > > > > From the framework's POV disabling a PWM is quite similar to duty cycle > > 0. Assuming disabling the PWM completes the currently running period[1] > > it might be better and easier to disable instead of switching to gpio. > > (Further assuming that disable really yields the inactive level which is > > should and is another limitation if not.) > > If we disable the hardware, the duty cycle is 100% unfortunately. So, > I think I should describe it as one of "Limitations". It seems you got the idea .. :-) Best regards Uwe [1] it goes to 0 on disable even if the polarity is inverted
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c index c8cd43f..1c19a8b 100644 --- a/drivers/pwm/pwm-rcar.c +++ b/drivers/pwm/pwm-rcar.c @@ -7,6 +7,7 @@ #include <linux/clk.h> #include <linux/err.h> +#include <linux/gpio/consumer.h> #include <linux/io.h> #include <linux/log2.h> #include <linux/math64.h> @@ -38,6 +39,7 @@ struct rcar_pwm_chip { struct pwm_chip chip; void __iomem *base; struct clk *clk; + struct gpio_desc *gpio; }; static inline struct rcar_pwm_chip *to_rcar_pwm_chip(struct pwm_chip *chip) @@ -119,8 +121,11 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns, ph = tmp & RCAR_PWMCNT_PH0_MASK; /* Avoid prohibited setting */ - if (cyc == 0 || ph == 0) + if (cyc == 0) return -EINVAL; + /* Try to use GPIO to output duty zero */ + if (ph == 0) + return -EAGAIN; rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT); @@ -157,6 +162,28 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp) rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR); } +static int rcar_pwm_gpiod_get(struct rcar_pwm_chip *rp) +{ + if (rp->gpio) + return 0; + + rp->gpio = gpiod_get(rp->chip.dev, "renesas,duty-zero", GPIOD_OUT_LOW); + if (!IS_ERR(rp->gpio)) + return 0; + + rp->gpio = NULL; + return -EINVAL; +} + +static void rcar_pwm_gpiod_put(struct rcar_pwm_chip *rp) +{ + if (!rp->gpio) + return; + + gpiod_put(rp->gpio); + rp->gpio = NULL; +} + static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { @@ -171,6 +198,7 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (!state->enabled) { rcar_pwm_disable(rp); + rcar_pwm_gpiod_put(rp); return 0; } @@ -187,8 +215,12 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */ rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR); - if (!ret) + if (!ret) { ret = rcar_pwm_enable(rp); + rcar_pwm_gpiod_put(rp); + } else if (ret == -EAGAIN) { + ret = rcar_pwm_gpiod_get(rp); + } return ret; }
The R-Car SoCs PWM Timer cannot output duty zero. So, this patch adds gpio support to output it. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pwm/pwm-rcar.c | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)