| Message ID | 1562576868-8124-4-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | treewide: modify sh-pfc and add support pwm duty zero | expand |
Hi Shimoda-san, On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > R-Car PWM controller requires the gpio to output zero duty, > this patch allows to roll it back from gpio to mux when the gpio > is freed. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Thanks for your patch! > --- a/drivers/pinctrl/sh-pfc/pinctrl.c > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > @@ -26,6 +26,7 @@ > #include "../pinconf.h" > > struct sh_pfc_pin_config { > + unsigned int mux_mark; Due to padding, adding this field will increase memory consumption by 6 bytes per pin. Probably sh_pfc_pin_group.{pins,mux} should be changed from unsigned int to u16, but that's out of scope for this patch. > bool mux_set; > bool gpio_enabled; > }; > @@ -353,6 +354,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > spin_lock_irqsave(&pfc->lock, flags); > > for (i = 0; i < grp->nr_pins; ++i) { > + int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]); > + struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > + > + /* > + * This doesn't assume the order which gpios are enabled > + * and then mux is set. I'm sorry, I don't understand what you mean? Can you please reword or elaborate? > + */ > + WARN_ON(cfg->gpio_enabled); Can this actually happen? Should this cause a failure instead? > + > ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION); > if (ret < 0) > goto done; > @@ -364,6 +374,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > > cfg->mux_set = true; > + cfg->mux_mark = grp->mux[i]; > } > > done: > @@ -417,6 +428,9 @@ static void sh_pfc_gpio_disable_free(struct pinctrl_dev *pctldev, > > spin_lock_irqsave(&pfc->lock, flags); > cfg->gpio_enabled = false; > + /* If mux is already set, this configure it here */ configures > + if (cfg->mux_set) > + sh_pfc_config_mux(pfc, cfg->mux_mark, PINMUX_TYPE_FUNCTION); Have you considered the case where more than one pin of a pinmux group was used as a GPIO? In that case sh_pfc_gpio_disable_free() will be called multiple times, possibly with the same mux_mark. I don't think this will cause issues, though. > spin_unlock_irqrestore(&pfc->lock, flags); > } Thanks! Gr{oetje,eeting}s, Geert
Hi Geert-san, Thank you for your review! > From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:03 PM > > Hi Shimoda-san, > > On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > R-Car PWM controller requires the gpio to output zero duty, > > this patch allows to roll it back from gpio to mux when the gpio > > is freed. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Thanks for your patch! > > > --- a/drivers/pinctrl/sh-pfc/pinctrl.c > > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > > @@ -26,6 +26,7 @@ > > #include "../pinconf.h" > > > > struct sh_pfc_pin_config { > > + unsigned int mux_mark; > > Due to padding, adding this field will increase memory consumption by > 6 bytes per pin. I see. > Probably sh_pfc_pin_group.{pins,mux} should be changed from unsigned int > to u16, but that's out of scope for this patch. I got it. > > bool mux_set; > > bool gpio_enabled; > > }; > > @@ -353,6 +354,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > > spin_lock_irqsave(&pfc->lock, flags); > > > > for (i = 0; i < grp->nr_pins; ++i) { > > + int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]); > > + struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > > + > > + /* > > + * This doesn't assume the order which gpios are enabled > > + * and then mux is set. > > I'm sorry, I don't understand what you mean? > Can you please reword or elaborate? I was also difficult to remember what I meant... Anyway, this meant, 1) if a device has the default pinctrl-0 property, the set_mux() ops is called before the device driver's probe() function is called by pinctrl_bind_pins() first, 2) so that any device drivers cannot call gpiod_get() before the 1). However, this comments don't cover an imbalance pinctrl/gpio handling. For example (as pseudo): - SCIF driver uses SCIF2 pinctrl, - but, IOMMU driver gets the SCIF2 pins before SCIF driver is probed. So, I'd like to revise the comments as following. What do you think? -- This driver cannot manage both gpio and mux when the gpio pin is already enabled. So, this function failed. -- > > + */ > > + WARN_ON(cfg->gpio_enabled); > > Can this actually happen? This cannot happen actually. > Should this cause a failure instead? I think so. > > + > > ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION); > > if (ret < 0) > > goto done; > > @@ -364,6 +374,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > > struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > > > > cfg->mux_set = true; > > + cfg->mux_mark = grp->mux[i]; > > } > > > > done: > > @@ -417,6 +428,9 @@ static void sh_pfc_gpio_disable_free(struct pinctrl_dev *pctldev, > > > > spin_lock_irqsave(&pfc->lock, flags); > > cfg->gpio_enabled = false; > > + /* If mux is already set, this configure it here */ > > configures Oops! I'll fix it. > > + if (cfg->mux_set) > > + sh_pfc_config_mux(pfc, cfg->mux_mark, PINMUX_TYPE_FUNCTION); > > Have you considered the case where more than one pin of a pinmux group > was used as a GPIO? In that case sh_pfc_gpio_disable_free() will be called > multiple times, possibly with the same mux_mark. I haven't considered the case. But, about the mux_mark, I checked the values and then they are not the same. For example (debug printk patch): diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c index bc29066..fdac71b 100644 --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -349,7 +349,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, unsigned int i; int ret = 0; - dev_dbg(pctldev->dev, "Configuring pin group %s\n", grp->name); + dev_info(pctldev->dev, "Configuring pin group %s\n", grp->name); spin_lock_irqsave(&pfc->lock, flags); @@ -375,6 +375,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, cfg->mux_set = true; cfg->mux_mark = grp->mux[i]; + dev_info(pctldev->dev, "%d: %x\n", i, cfg->mux_mark); } done:
Hi Shimoda-san, On Tue, Aug 6, 2019 at 1:38 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Tuesday, August 6, 2019 6:03 PM > > On Mon, Jul 8, 2019 at 11:08 AM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > R-Car PWM controller requires the gpio to output zero duty, > > > this patch allows to roll it back from gpio to mux when the gpio > > > is freed. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/pinctrl/sh-pfc/pinctrl.c > > > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > > > @@ -26,6 +26,7 @@ > > > #include "../pinconf.h" > > > > > > struct sh_pfc_pin_config { > > > + unsigned int mux_mark; > > > > Due to padding, adding this field will increase memory consumption by > > 6 bytes per pin. > > I see. > > > Probably sh_pfc_pin_group.{pins,mux} should be changed from unsigned int > > to u16, but that's out of scope for this patch. > > I got it. For now, please don't worry about it. I can make that change later, as it will affect all drivers. > > > @@ -353,6 +354,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > > > spin_lock_irqsave(&pfc->lock, flags); > > > > > > for (i = 0; i < grp->nr_pins; ++i) { > > > + int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]); > > > + struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > > > + > > > + /* > > > + * This doesn't assume the order which gpios are enabled > > > + * and then mux is set. > > > > I'm sorry, I don't understand what you mean? > > Can you please reword or elaborate? > > I was also difficult to remember what I meant... > Anyway, this meant, > 1) if a device has the default pinctrl-0 property, the set_mux() ops is called > before the device driver's probe() function is called by pinctrl_bind_pins() first, > 2) so that any device drivers cannot call gpiod_get() before the 1). > > However, this comments don't cover an imbalance pinctrl/gpio handling. > For example (as pseudo): > - SCIF driver uses SCIF2 pinctrl, > - but, IOMMU driver gets the SCIF2 pins before SCIF driver is probed. > > So, I'd like to revise the comments as following. What do you think? > > -- > This driver cannot manage both gpio and mux when the gpio pin > is already enabled. So, this function failed. > -- > > > > + */ > > > + WARN_ON(cfg->gpio_enabled); > > > > Can this actually happen? > > This cannot happen actually. > > > Should this cause a failure instead? > > I think so. OK. > > > + if (cfg->mux_set) > > > + sh_pfc_config_mux(pfc, cfg->mux_mark, PINMUX_TYPE_FUNCTION); > > > > Have you considered the case where more than one pin of a pinmux group > > was used as a GPIO? In that case sh_pfc_gpio_disable_free() will be called > > multiple times, possibly with the same mux_mark. > > I haven't considered the case. But, about the mux_mark, I checked the values and then > they are not the same. IC. At first I thought they were the internal enum for the whole pin group, but I was wrong. They are the mux *_MARK enu, which is unique for each pin/function combo. > For example (debug printk patch): > diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c > index bc29066..fdac71b 100644 > --- a/drivers/pinctrl/sh-pfc/pinctrl.c > +++ b/drivers/pinctrl/sh-pfc/pinctrl.c > @@ -349,7 +349,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > unsigned int i; > int ret = 0; > > - dev_dbg(pctldev->dev, "Configuring pin group %s\n", grp->name); > + dev_info(pctldev->dev, "Configuring pin group %s\n", grp->name); > > spin_lock_irqsave(&pfc->lock, flags); > > @@ -375,6 +375,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, > > cfg->mux_set = true; > cfg->mux_mark = grp->mux[i]; > + dev_info(pctldev->dev, "%d: %x\n", i, cfg->mux_mark); > } > > done: > -- > 2.7.4 > > For example (log): > [ 0.497647] sh-pfc e6060000.pin-controller: Configuring pin group scif2_data_a > [ 0.497711] sh-pfc e6060000.pin-controller: 0: 77b > [ 0.497715] sh-pfc e6060000.pin-controller: 1: 760 Thanks for checking! Gr{oetje,eeting}s, Geert
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c index 7e5aca2..bc29066 100644 --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -26,6 +26,7 @@ #include "../pinconf.h" struct sh_pfc_pin_config { + unsigned int mux_mark; bool mux_set; bool gpio_enabled; }; @@ -353,6 +354,15 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, spin_lock_irqsave(&pfc->lock, flags); for (i = 0; i < grp->nr_pins; ++i) { + int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]); + struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; + + /* + * This doesn't assume the order which gpios are enabled + * and then mux is set. + */ + WARN_ON(cfg->gpio_enabled); + ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION); if (ret < 0) goto done; @@ -364,6 +374,7 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector, struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; cfg->mux_set = true; + cfg->mux_mark = grp->mux[i]; } done: @@ -417,6 +428,9 @@ static void sh_pfc_gpio_disable_free(struct pinctrl_dev *pctldev, spin_lock_irqsave(&pfc->lock, flags); cfg->gpio_enabled = false; + /* If mux is already set, this configure it here */ + if (cfg->mux_set) + sh_pfc_config_mux(pfc, cfg->mux_mark, PINMUX_TYPE_FUNCTION); spin_unlock_irqrestore(&pfc->lock, flags); }
R-Car PWM controller requires the gpio to output zero duty, this patch allows to roll it back from gpio to mux when the gpio is freed. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pinctrl/sh-pfc/pinctrl.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)