| Message ID | 20180427125328.1172-1-mario.six@gdsys.cc |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,v2] reset: Add generic GPIO reset driver | expand |
Hi, On 27/04/2018 14:53, Mario Six wrote: > Some reset lines are implemented by toggling the line via a GPIO. > > Add a driver to properly drive such reset lines. You are defining a "gpio-reset" binding which has always been rejected under Linux, so I'm not sure it's a good idea to add it in U-Boot only... Neil > > Signed-off-by: Mario Six <mario.six@gdsys.cc> > --- > > v1 -> v2: > No changes > > --- > drivers/reset/Kconfig | 7 ++++ > drivers/reset/Makefile | 1 + > drivers/reset/gpio-reset.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 108 insertions(+) > create mode 100644 drivers/reset/gpio-reset.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 33c39b7fb6..b6e1da009c 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -98,4 +98,11 @@ config RESET_SOCFPGA > help > Support for reset controller on SoCFPGA platform. > > +config GPIO_RESET > + bool "Reset driver for GPIO-based reset lines" > + depends on DM_RESET > + help > + Support a generic reset controller that encapsulates a number of > + GPIOs of which each controls a single reset line. > + > endmenu > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index ad08be4c8c..4a9d4006a3 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -15,3 +15,4 @@ obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o > obj-$(CONFIG_RESET_ROCKCHIP) += reset-rockchip.o > obj-$(CONFIG_RESET_MESON) += reset-meson.o > obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o > +obj-$(CONFIG_GPIO_RESET) += gpio-reset.o > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c > new file mode 100644 > index 0000000000..52dd47df60 > --- /dev/null > +++ b/drivers/reset/gpio-reset.c > @@ -0,0 +1,100 @@ > +/* > + * (C) Copyright 2017 > + * Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <reset-uclass.h> > +#include <asm/gpio.h> > + > +struct gpio_reset_priv { > + struct gpio_desc gpios[16]; > + uint gpio_num; > +}; > + > +static int gpio_reset_request(struct reset_ctl *reset_ctl) > +{ > + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); > + > + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, > + reset_ctl->dev, reset_ctl->id); > + > + /* Reset ID = number of GPIO in list */ > + if (reset_ctl->id >= priv->gpio_num) > + return -EINVAL; > + > + return 0; > +} > + > +static int gpio_reset_free(struct reset_ctl *reset_ctl) > +{ > + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, > + reset_ctl->dev, reset_ctl->id); > + > + return 0; > +} > + > +static int gpio_reset_assert(struct reset_ctl *reset_ctl) > +{ > + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); > + > + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, > + reset_ctl->dev, reset_ctl->id); > + > + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 1); > + > + return 0; > +} > + > +static int gpio_reset_deassert(struct reset_ctl *reset_ctl) > +{ > + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); > + > + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, > + reset_ctl->dev, reset_ctl->id); > + > + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 0); > + > + return 0; > +} > + > +struct reset_ops gpio_reset_ops = { > + .request = gpio_reset_request, > + .free = gpio_reset_free, > + .rst_assert = gpio_reset_assert, > + .rst_deassert = gpio_reset_deassert, > +}; > + > +static int gpio_reset_probe(struct udevice *dev) > +{ > + struct gpio_reset_priv *priv = dev_get_priv(dev); > + int ret; > + > + debug("%s(dev=%p)\n", __func__, dev); > + > + ret = gpio_request_list_by_name(dev, "gpios", priv->gpios, > + ARRAY_SIZE(priv->gpios), GPIOD_IS_OUT); > + if (ret <= 0) > + return ret; > + > + priv->gpio_num = ret; > + > + return 0; > +} > + > +static const struct udevice_id gpio_reset[] = { > + { .compatible = "gpio-reset", }, > + { /* sentinel */ } > +}; > + > +U_BOOT_DRIVER(gpio_reset) = { > + .name = "gpio_reset", > + .id = UCLASS_RESET, > + .of_match = gpio_reset, > + .probe = gpio_reset_probe, > + .ops = &gpio_reset_ops, > + .priv_auto_alloc_size = sizeof(struct gpio_reset_priv), > +}; > -- > 2.16.1 >
Hi Neil, On 27 April 2018 at 07:01, Neil Armstrong <narmstrong@baylibre.com> wrote: > Hi, > > On 27/04/2018 14:53, Mario Six wrote: >> Some reset lines are implemented by toggling the line via a GPIO. >> >> Add a driver to properly drive such reset lines. > > You are defining a "gpio-reset" binding which has always been rejected > under Linux, so I'm not sure it's a good idea to add it in U-Boot only... What is the alternative? Regards, Simon
Hi Neil, On Fri, Apr 27, 2018 at 3:01 PM, Neil Armstrong <narmstrong@baylibre.com> wrote: > Hi, > > On 27/04/2018 14:53, Mario Six wrote: >> Some reset lines are implemented by toggling the line via a GPIO. >> >> Add a driver to properly drive such reset lines. > > You are defining a "gpio-reset" binding which has always been rejected > under Linux, so I'm not sure it's a good idea to add it in U-Boot only... > I see what you mean. I do think that the reasoning from the Linux debates (e.g. [1]) applies only partially to U-Boot, mostly because in U-Boot there is no good way to "have a separate device in the Linux driver model to abstract this". If it's not described in the DT, you have to manually create devices, which is probably not what we want here. This driver was inspired by the also generic reset driver in [2] (which was never applied, it seems?) which is equally as "abstract" as this one. At the end of the day, I'm fine with having a gpio-reset property in the device in question and then manually reset them, but I think we can do without the code duplication everywhere (plus, lots of boards already have U-Boot specific device tree include files, so adding the reset device is not a big deal). Is "u-boot,gpio-reset" good as an alternative compat string? [1] http://comments.gmane.org/gmane.linux.drivers.devicetree/41596 [2] https://lists.denx.de/pipermail/u-boot/2017-November/313135.html > Neil Best regards, Mario >> >> Signed-off-by: Mario Six <mario.six@gdsys.cc> >> --- >> >> v1 -> v2: >> No changes >> >> --- >> drivers/reset/Kconfig | 7 ++++ >> drivers/reset/Makefile | 1 + >> drivers/reset/gpio-reset.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 108 insertions(+) >> create mode 100644 drivers/reset/gpio-reset.c >> >> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig >> index 33c39b7fb6..b6e1da009c 100644 >> --- a/drivers/reset/Kconfig >> +++ b/drivers/reset/Kconfig >> @@ -98,4 +98,11 @@ config RESET_SOCFPGA >> help >> Support for reset controller on SoCFPGA platform. >> >> +config GPIO_RESET >> + bool "Reset driver for GPIO-based reset lines" >> + depends on DM_RESET >> + help >> + Support a generic reset controller that encapsulates a number of >> + GPIOs of which each controls a single reset line. >> + >> endmenu >> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile >> index ad08be4c8c..4a9d4006a3 100644 >> --- a/drivers/reset/Makefile >> +++ b/drivers/reset/Makefile >> @@ -15,3 +15,4 @@ obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o >> obj-$(CONFIG_RESET_ROCKCHIP) += reset-rockchip.o >> obj-$(CONFIG_RESET_MESON) += reset-meson.o >> obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o >> +obj-$(CONFIG_GPIO_RESET) += gpio-reset.o >> diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c >> new file mode 100644 >> index 0000000000..52dd47df60 >> --- /dev/null >> +++ b/drivers/reset/gpio-reset.c >> @@ -0,0 +1,100 @@ >> +/* >> + * (C) Copyright 2017 >> + * Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <reset-uclass.h> >> +#include <asm/gpio.h> >> + >> +struct gpio_reset_priv { >> + struct gpio_desc gpios[16]; >> + uint gpio_num; >> +}; >> + >> +static int gpio_reset_request(struct reset_ctl *reset_ctl) >> +{ >> + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); >> + >> + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, >> + reset_ctl->dev, reset_ctl->id); >> + >> + /* Reset ID = number of GPIO in list */ >> + if (reset_ctl->id >= priv->gpio_num) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static int gpio_reset_free(struct reset_ctl *reset_ctl) >> +{ >> + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, >> + reset_ctl->dev, reset_ctl->id); >> + >> + return 0; >> +} >> + >> +static int gpio_reset_assert(struct reset_ctl *reset_ctl) >> +{ >> + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); >> + >> + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, >> + reset_ctl->dev, reset_ctl->id); >> + >> + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 1); >> + >> + return 0; >> +} >> + >> +static int gpio_reset_deassert(struct reset_ctl *reset_ctl) >> +{ >> + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); >> + >> + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, >> + reset_ctl->dev, reset_ctl->id); >> + >> + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 0); >> + >> + return 0; >> +} >> + >> +struct reset_ops gpio_reset_ops = { >> + .request = gpio_reset_request, >> + .free = gpio_reset_free, >> + .rst_assert = gpio_reset_assert, >> + .rst_deassert = gpio_reset_deassert, >> +}; >> + >> +static int gpio_reset_probe(struct udevice *dev) >> +{ >> + struct gpio_reset_priv *priv = dev_get_priv(dev); >> + int ret; >> + >> + debug("%s(dev=%p)\n", __func__, dev); >> + >> + ret = gpio_request_list_by_name(dev, "gpios", priv->gpios, >> + ARRAY_SIZE(priv->gpios), GPIOD_IS_OUT); >> + if (ret <= 0) >> + return ret; >> + >> + priv->gpio_num = ret; >> + >> + return 0; >> +} >> + >> +static const struct udevice_id gpio_reset[] = { >> + { .compatible = "gpio-reset", }, >> + { /* sentinel */ } >> +}; >> + >> +U_BOOT_DRIVER(gpio_reset) = { >> + .name = "gpio_reset", >> + .id = UCLASS_RESET, >> + .of_match = gpio_reset, >> + .probe = gpio_reset_probe, >> + .ops = &gpio_reset_ops, >> + .priv_auto_alloc_size = sizeof(struct gpio_reset_priv), >> +}; >> -- >> 2.16.1 >> >
Hi Mario, On 4 May 2018 at 01:01, Mario Six <mario.six@gdsys.cc> wrote: > Hi Neil, > > On Fri, Apr 27, 2018 at 3:01 PM, Neil Armstrong <narmstrong@baylibre.com> wrote: >> Hi, >> >> On 27/04/2018 14:53, Mario Six wrote: >>> Some reset lines are implemented by toggling the line via a GPIO. >>> >>> Add a driver to properly drive such reset lines. >> >> You are defining a "gpio-reset" binding which has always been rejected >> under Linux, so I'm not sure it's a good idea to add it in U-Boot only... >> > > I see what you mean. I do think that the reasoning from the Linux debates (e.g. > [1]) applies only partially to U-Boot, mostly because in U-Boot there is no > good way to "have a separate device in the Linux driver model to abstract > this". If it's not described in the DT, you have to manually create devices, > which is probably not what we want here. > > This driver was inspired by the also generic reset driver in [2] (which was > never applied, it seems?) which is equally as "abstract" as this one. > > At the end of the day, I'm fine with having a gpio-reset property in the device > in question and then manually reset them, but I think we can do without the > code duplication everywhere (plus, lots of boards already have U-Boot specific > device tree include files, so adding the reset device is not a big deal). > > Is "u-boot,gpio-reset" good as an alternative compat string? I suppose so, but we should think about the implications. Presumably the controlling device would reference a particular reset line, and would control enabling/disabling reset, and thus the length of the reset pulse? How much code do we actually save by doing this? Would another option be to have some helper functions in the reset uclass? > > [1] http://comments.gmane.org/gmane.linux.drivers.devicetree/41596 > [2] https://lists.denx.de/pipermail/u-boot/2017-November/313135.html > >> Neil Regards, Simon
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 33c39b7fb6..b6e1da009c 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -98,4 +98,11 @@ config RESET_SOCFPGA help Support for reset controller on SoCFPGA platform. +config GPIO_RESET + bool "Reset driver for GPIO-based reset lines" + depends on DM_RESET + help + Support a generic reset controller that encapsulates a number of + GPIOs of which each controls a single reset line. + endmenu diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index ad08be4c8c..4a9d4006a3 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -15,3 +15,4 @@ obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o obj-$(CONFIG_RESET_ROCKCHIP) += reset-rockchip.o obj-$(CONFIG_RESET_MESON) += reset-meson.o obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o +obj-$(CONFIG_GPIO_RESET) += gpio-reset.o diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c new file mode 100644 index 0000000000..52dd47df60 --- /dev/null +++ b/drivers/reset/gpio-reset.c @@ -0,0 +1,100 @@ +/* + * (C) Copyright 2017 + * Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <reset-uclass.h> +#include <asm/gpio.h> + +struct gpio_reset_priv { + struct gpio_desc gpios[16]; + uint gpio_num; +}; + +static int gpio_reset_request(struct reset_ctl *reset_ctl) +{ + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); + + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, + reset_ctl->dev, reset_ctl->id); + + /* Reset ID = number of GPIO in list */ + if (reset_ctl->id >= priv->gpio_num) + return -EINVAL; + + return 0; +} + +static int gpio_reset_free(struct reset_ctl *reset_ctl) +{ + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, + reset_ctl->dev, reset_ctl->id); + + return 0; +} + +static int gpio_reset_assert(struct reset_ctl *reset_ctl) +{ + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); + + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, + reset_ctl->dev, reset_ctl->id); + + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 1); + + return 0; +} + +static int gpio_reset_deassert(struct reset_ctl *reset_ctl) +{ + struct gpio_reset_priv *priv = dev_get_priv(reset_ctl->dev); + + debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl, + reset_ctl->dev, reset_ctl->id); + + dm_gpio_set_value(&priv->gpios[reset_ctl->id], 0); + + return 0; +} + +struct reset_ops gpio_reset_ops = { + .request = gpio_reset_request, + .free = gpio_reset_free, + .rst_assert = gpio_reset_assert, + .rst_deassert = gpio_reset_deassert, +}; + +static int gpio_reset_probe(struct udevice *dev) +{ + struct gpio_reset_priv *priv = dev_get_priv(dev); + int ret; + + debug("%s(dev=%p)\n", __func__, dev); + + ret = gpio_request_list_by_name(dev, "gpios", priv->gpios, + ARRAY_SIZE(priv->gpios), GPIOD_IS_OUT); + if (ret <= 0) + return ret; + + priv->gpio_num = ret; + + return 0; +} + +static const struct udevice_id gpio_reset[] = { + { .compatible = "gpio-reset", }, + { /* sentinel */ } +}; + +U_BOOT_DRIVER(gpio_reset) = { + .name = "gpio_reset", + .id = UCLASS_RESET, + .of_match = gpio_reset, + .probe = gpio_reset_probe, + .ops = &gpio_reset_ops, + .priv_auto_alloc_size = sizeof(struct gpio_reset_priv), +};
Some reset lines are implemented by toggling the line via a GPIO. Add a driver to properly drive such reset lines. Signed-off-by: Mario Six <mario.six@gdsys.cc> --- v1 -> v2: No changes --- drivers/reset/Kconfig | 7 ++++ drivers/reset/Makefile | 1 + drivers/reset/gpio-reset.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 drivers/reset/gpio-reset.c -- 2.16.1