| Message ID | 1540295058-26090-5-git-send-email-aisheng.dong@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote: > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > + clk_port = devm_clk_get(&pdev->dev, "port"); > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { if (clk_gpio == ERR_PTR(-EPROBE_DEFER) || clk_port == ERR_PTR(-EPROBE_DEFER)) {
Hi Russell, > -----Original Message----- > From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk] > Sent: Tuesday, October 23, 2018 8:04 PM [...] > On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote: > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > > + clk_port = devm_clk_get(&pdev->dev, "port"); > > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > > if (clk_gpio == ERR_PTR(-EPROBE_DEFER) || > clk_port == ERR_PTR(-EPROBE_DEFER)) { > Thanks for the suggestion. I will update it in next series. Before that, let's wait a moment to see if any more review comments. BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare -EPROBE_DEFER, I'm not quite get the point why the new approach you suggested is better, is it less error-prone? Or something else? would you please help clarify a bit more? Regards Dong Aisheng > -- > RMK's Patch system: > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > armlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Caishen > g.dong%40nxp.com%7C34fcfce2f5d042efd0b808d638dfaaed%7C686ea1d3bc > 2b4c6fa92cd99c5c301635%7C0%7C0%7C636758930627945785&sdata= > PME05RkmX0mxRmhO%2Bj%2FofV3VN2VMx3FWU9bbqFr3XAg%3D&res > erved=0 > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps > up According to speedtest.net: 11.9Mbps down 500kbps up
On Tue, Oct 23, 2018 at 12:23:12PM +0000, A.s. Dong wrote: > Hi Russell, > > > -----Original Message----- > > From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk] > > Sent: Tuesday, October 23, 2018 8:04 PM > [...] > > On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote: > > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > > > + clk_port = devm_clk_get(&pdev->dev, "port"); > > > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > > > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > > > > if (clk_gpio == ERR_PTR(-EPROBE_DEFER) || > > clk_port == ERR_PTR(-EPROBE_DEFER)) { > > > > Thanks for the suggestion. I will update it in next series. > Before that, let's wait a moment to see if any more review comments. > > BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare > -EPROBE_DEFER, I'm not quite get the point why the new approach > you suggested is better, is it less error-prone? Or something else? > would you please help clarify a bit more? See the discussion in https://lore.kernel.org/patchwork/patch/999602/. Best regards Uwe
> -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: Tuesday, October 23, 2018 8:42 PM > To: A.s. Dong <aisheng.dong@nxp.com> > Cc: Russell King - ARM Linux <linux@armlinux.org.uk>; dongas86@gmail.com; > Linus Walleij <linus.walleij@linaro.org>; Stefan Agner <stefan@agner.ch>; > linux-gpio@vger.kernel.org; robh+dt@kernel.org; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; shawnguo@kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH V2 4/8] gpio: vf610: add optional clock support > > On Tue, Oct 23, 2018 at 12:23:12PM +0000, A.s. Dong wrote: > > Hi Russell, > > > > > -----Original Message----- > > > From: Russell King - ARM Linux [mailto:linux@armlinux.org.uk] > > > Sent: Tuesday, October 23, 2018 8:04 PM > > [...] > > > On Tue, Oct 23, 2018 at 11:49:17AM +0000, A.s. Dong wrote: > > > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > > > > + clk_port = devm_clk_get(&pdev->dev, "port"); > > > > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > > > > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > > > > > > if (clk_gpio == ERR_PTR(-EPROBE_DEFER) || > > > clk_port == ERR_PTR(-EPROBE_DEFER)) { > > > > > > > Thanks for the suggestion. I will update it in next series. > > Before that, let's wait a moment to see if any more review comments. > > > > BTW, as I see kernel currently is widely using PTR_ERR(ptr) to compare > > -EPROBE_DEFER, I'm not quite get the point why the new approach you > > suggested is better, is it less error-prone? Or something else? > > would you please help clarify a bit more? > > See the discussion in > https://lore.kernel.org/patchwork/patch/999602/ > Thanks for sharing the info. It does help. Regards Dong Aisheng > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C06 > 5f26466899474bf61a08d638e4e308%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C636758953039817883&sdata=hSgiadfyiFoK4AVTF2BR > guOU5H8C77%2BY2bdjViC4Sfw%3D&reserved=0 |
On 23.10.2018 13:49, A.s. Dong wrote: > Some SoCs need the gpio clock to be enabled before accessing > HW registers. This patch add the optional clock handling. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: linux-gpio@vger.kernel.org > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > v1->v2: > * new patch > --- > drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c > index d4ad6d0..cbc4f44 100644 > --- a/drivers/gpio/gpio-vf610.c > +++ b/drivers/gpio/gpio-vf610.c > @@ -16,6 +16,7 @@ > */ > > #include <linux/bitops.h> > +#include <linux/clk.h> > #include <linux/err.h> > #include <linux/gpio.h> > #include <linux/init.h> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > + struct clk *clk_gpio, *clk_port; > struct vf610_gpio_port *port; > struct resource *iores; > struct gpio_chip *gc; > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device *pdev) > if (port->irq < 0) > return port->irq; > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > + clk_port = devm_clk_get(&pdev->dev, "port"); Are you sure that those are two independent clocks? On i.MX 7 usually there was a single clock gate controlling multiple clocks at once (which should be modeled as a single clock gate in the clock tree). -- Stefan > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > + return -EPROBE_DEFER; > + } else if (!IS_ERR_OR_NULL(clk_gpio) && > + !IS_ERR_OR_NULL(clk_port)) { > + ret = clk_prepare_enable(clk_gpio); > + if (ret) > + return ret; > + > + ret = clk_prepare_enable(clk_port); > + if (ret) { > + clk_disable_unprepare(clk_gpio); > + return ret; > + } > + } > + > gc = &port->gc; > gc->of_node = np; > gc->parent = dev;
> -----Original Message----- > From: Stefan Agner [mailto:stefan@agner.ch] > Sent: Friday, October 26, 2018 1:59 AM [...] > > On 23.10.2018 13:49, A.s. Dong wrote: > > Some SoCs need the gpio clock to be enabled before accessing HW > > registers. This patch add the optional clock handling. > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Stefan Agner <stefan@agner.ch> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: linux-gpio@vger.kernel.org > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > v1->v2: > > * new patch > > --- > > drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c > > index d4ad6d0..cbc4f44 100644 > > --- a/drivers/gpio/gpio-vf610.c > > +++ b/drivers/gpio/gpio-vf610.c > > @@ -16,6 +16,7 @@ > > */ > > > > #include <linux/bitops.h> > > +#include <linux/clk.h> > > #include <linux/err.h> > > #include <linux/gpio.h> > > #include <linux/init.h> > > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device > > *pdev) { > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > + struct clk *clk_gpio, *clk_port; > > struct vf610_gpio_port *port; > > struct resource *iores; > > struct gpio_chip *gc; > > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device > *pdev) > > if (port->irq < 0) > > return port->irq; > > > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > > + clk_port = devm_clk_get(&pdev->dev, "port"); > > Are you sure that those are two independent clocks? On i.MX 7 usually there > was a single clock gate controlling multiple clocks at once (which should be > modeled as a single clock gate in the clock tree). > Yes, they're two separate clocks in HW for gpio and port controller respectively. One is for GPIO general purpose input/output function which another for port Interrupt. Just like we have separate register ranges for gpio and port. Regards Dong Aisheng > -- > Stefan > > > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > > + return -EPROBE_DEFER; > > + } else if (!IS_ERR_OR_NULL(clk_gpio) && > > + !IS_ERR_OR_NULL(clk_port)) { > > + ret = clk_prepare_enable(clk_gpio); > > + if (ret) > > + return ret; > > + > > + ret = clk_prepare_enable(clk_port); > > + if (ret) { > > + clk_disable_unprepare(clk_gpio); > > + return ret; > > + } > > + } > > + > > gc = &port->gc; > > gc->of_node = np; > > gc->parent = dev;
On 26.10.2018 05:49, A.s. Dong wrote: >> -----Original Message----- >> From: Stefan Agner [mailto:stefan@agner.ch] >> Sent: Friday, October 26, 2018 1:59 AM > [...] >> >> On 23.10.2018 13:49, A.s. Dong wrote: >> > Some SoCs need the gpio clock to be enabled before accessing HW >> > registers. This patch add the optional clock handling. >> > >> > Cc: Linus Walleij <linus.walleij@linaro.org> >> > Cc: Stefan Agner <stefan@agner.ch> >> > Cc: Shawn Guo <shawnguo@kernel.org> >> > Cc: linux-gpio@vger.kernel.org >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> >> > --- >> > v1->v2: >> > * new patch >> > --- >> > drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++ >> > 1 file changed, 20 insertions(+) >> > >> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c >> > index d4ad6d0..cbc4f44 100644 >> > --- a/drivers/gpio/gpio-vf610.c >> > +++ b/drivers/gpio/gpio-vf610.c >> > @@ -16,6 +16,7 @@ >> > */ >> > >> > #include <linux/bitops.h> >> > +#include <linux/clk.h> >> > #include <linux/err.h> >> > #include <linux/gpio.h> >> > #include <linux/init.h> >> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device >> > *pdev) { >> > struct device *dev = &pdev->dev; >> > struct device_node *np = dev->of_node; >> > + struct clk *clk_gpio, *clk_port; >> > struct vf610_gpio_port *port; >> > struct resource *iores; >> > struct gpio_chip *gc; >> > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device >> *pdev) >> > if (port->irq < 0) >> > return port->irq; >> > >> > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); >> > + clk_port = devm_clk_get(&pdev->dev, "port"); >> >> Are you sure that those are two independent clocks? On i.MX 7 usually there >> was a single clock gate controlling multiple clocks at once (which should be >> modeled as a single clock gate in the clock tree). >> > > Yes, they're two separate clocks in HW for gpio and port controller > respectively. > One is for GPIO general purpose input/output function which another > for port Interrupt. > Just like we have separate register ranges for gpio and port. Oh I see, that is the same with Vybrid actually. However, in Vybrid, for some reason, there is only a clock for port (Port X multiplexing control). Can we make port clock independently optional? E.g. clk_port = devm_clk_get(&pdev->dev, "port"); if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) return -EPROBE_DEFER; clk_gpio = devm_clk_get(&pdev->dev, "gpio"); if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) return -EPROBE_DEFER; if (!IS_ERR_OR_NULL(clk_port)) { ret = clk_prepare_enable(clk_port); if (ret) return ret; } if (!IS_ERR_OR_NULL(clk_gpio)) ret = clk_prepare_enable(clk_gpio); if (ret) { clk_disable_unprepare(clk_port); return ret; } } Also there is some error handling a bit further down which needs proper disabling the clocks. -- Stefan > > Regards > Dong Aisheng > >> -- >> Stefan >> >> > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || >> > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { >> > + return -EPROBE_DEFER; >> > + } else if (!IS_ERR_OR_NULL(clk_gpio) && >> > + !IS_ERR_OR_NULL(clk_port)) { >> > + ret = clk_prepare_enable(clk_gpio); >> > + if (ret) >> > + return ret; >> > + >> > + ret = clk_prepare_enable(clk_port); >> > + if (ret) { >> > + clk_disable_unprepare(clk_gpio); >> > + return ret; >> > + } >> > + } >> > + >> > gc = &port->gc; >> > gc->of_node = np; >> > gc->parent = dev;
Hi Stefan, [...] > > On 26.10.2018 05:49, A.s. Dong wrote: > >> -----Original Message----- > >> From: Stefan Agner [mailto:stefan@agner.ch] > >> Sent: Friday, October 26, 2018 1:59 AM > > [...] > >> > >> On 23.10.2018 13:49, A.s. Dong wrote: > >> > Some SoCs need the gpio clock to be enabled before accessing HW > >> > registers. This patch add the optional clock handling. > >> > > >> > Cc: Linus Walleij <linus.walleij@linaro.org> > >> > Cc: Stefan Agner <stefan@agner.ch> > >> > Cc: Shawn Guo <shawnguo@kernel.org> > >> > Cc: linux-gpio@vger.kernel.org > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > >> > --- > >> > v1->v2: > >> > * new patch > >> > --- > >> > drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++ > >> > 1 file changed, 20 insertions(+) > >> > > >> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c > >> > index d4ad6d0..cbc4f44 100644 > >> > --- a/drivers/gpio/gpio-vf610.c > >> > +++ b/drivers/gpio/gpio-vf610.c > >> > @@ -16,6 +16,7 @@ > >> > */ > >> > > >> > #include <linux/bitops.h> > >> > +#include <linux/clk.h> > >> > #include <linux/err.h> > >> > #include <linux/gpio.h> > >> > #include <linux/init.h> > >> > @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct > >> > platform_device > >> > *pdev) { > >> > struct device *dev = &pdev->dev; > >> > struct device_node *np = dev->of_node; > >> > + struct clk *clk_gpio, *clk_port; > >> > struct vf610_gpio_port *port; > >> > struct resource *iores; > >> > struct gpio_chip *gc; > >> > @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct > >> > platform_device > >> *pdev) > >> > if (port->irq < 0) > >> > return port->irq; > >> > > >> > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > >> > + clk_port = devm_clk_get(&pdev->dev, "port"); > >> > >> Are you sure that those are two independent clocks? On i.MX 7 usually > >> there was a single clock gate controlling multiple clocks at once > >> (which should be modeled as a single clock gate in the clock tree). > >> > > > > Yes, they're two separate clocks in HW for gpio and port controller > > respectively. > > One is for GPIO general purpose input/output function which another > > for port Interrupt. > > Just like we have separate register ranges for gpio and port. > > Oh I see, that is the same with Vybrid actually. However, in Vybrid, for some > reason, there is only a clock for port (Port X multiplexing control). > > Can we make port clock independently optional? > > E.g. > > clk_port = devm_clk_get(&pdev->dev, "port"); > if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) > return -EPROBE_DEFER; > > clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) > return -EPROBE_DEFER; > > if (!IS_ERR_OR_NULL(clk_port)) { > ret = clk_prepare_enable(clk_port); > if (ret) > return ret; > } > > if (!IS_ERR_OR_NULL(clk_gpio)) > ret = clk_prepare_enable(clk_gpio); > if (ret) { > clk_disable_unprepare(clk_port); > return ret; > } > } > > Also there is some error handling a bit further down which needs proper > disabling the clocks. > Got it, thanks for the suggestion. Will change in next version. Regards Dong Aisheng > -- > Stefan > > > > > > Regards > > Dong Aisheng > > > >> -- > >> Stefan > >> > >> > + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || > >> > + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { > >> > + return -EPROBE_DEFER; > >> > + } else if (!IS_ERR_OR_NULL(clk_gpio) && > >> > + !IS_ERR_OR_NULL(clk_port)) { > >> > + ret = clk_prepare_enable(clk_gpio); > >> > + if (ret) > >> > + return ret; > >> > + > >> > + ret = clk_prepare_enable(clk_port); > >> > + if (ret) { > >> > + clk_disable_unprepare(clk_gpio); > >> > + return ret; > >> > + } > >> > + } > >> > + > >> > gc = &port->gc; > >> > gc->of_node = np; > >> > gc->parent = dev;
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index d4ad6d0..cbc4f44 100644 --- a/drivers/gpio/gpio-vf610.c +++ b/drivers/gpio/gpio-vf610.c @@ -16,6 +16,7 @@ */ #include <linux/bitops.h> +#include <linux/clk.h> #include <linux/err.h> #include <linux/gpio.h> #include <linux/init.h> @@ -256,6 +257,7 @@ static int vf610_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; + struct clk *clk_gpio, *clk_port; struct vf610_gpio_port *port; struct resource *iores; struct gpio_chip *gc; @@ -280,6 +282,24 @@ static int vf610_gpio_probe(struct platform_device *pdev) if (port->irq < 0) return port->irq; + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); + clk_port = devm_clk_get(&pdev->dev, "port"); + if ((PTR_ERR(clk_gpio) == -EPROBE_DEFER) || + (PTR_ERR(clk_port) == -EPROBE_DEFER)) { + return -EPROBE_DEFER; + } else if (!IS_ERR_OR_NULL(clk_gpio) && + !IS_ERR_OR_NULL(clk_port)) { + ret = clk_prepare_enable(clk_gpio); + if (ret) + return ret; + + ret = clk_prepare_enable(clk_port); + if (ret) { + clk_disable_unprepare(clk_gpio); + return ret; + } + } + gc = &port->gc; gc->of_node = np; gc->parent = dev;
Some SoCs need the gpio clock to be enabled before accessing HW registers. This patch add the optional clock handling. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Stefan Agner <stefan@agner.ch> Cc: Shawn Guo <shawnguo@kernel.org> Cc: linux-gpio@vger.kernel.org Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- v1->v2: * new patch --- drivers/gpio/gpio-vf610.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)