On 16.7.2018 07:20, Simon Glass wrote: > Hi Michal, > > On 13 July 2018 at 03:15, Michal Simek <[email protected]> wrote: >> The Linux kernel has binding for gpio-restart node. >> This patch is adding basic support without supporting any optional >> properties. >> This driver was tested on Microblaze system where gpio is connected to >> SoC reset logic. >> Output value is handled via gpios cells values. >> >> In gpio_reboot_request() set_value is writing 1 because >> dm_gpio_set_value() is capable to changing it when it is ACTIVE_LOW. >> ... >> if (desc->flags & GPIOD_ACTIVE_LOW) >> value = !value; >> ... >> >> Signed-off-by: Michal Simek <[email protected]> >> --- >> >> drivers/sysreset/Kconfig | 6 ++++ >> drivers/sysreset/Makefile | 1 + >> drivers/sysreset/sysreset_gpio.c | 59 >> ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 66 insertions(+) >> create mode 100644 drivers/sysreset/sysreset_gpio.c > > Reviewed-by: Simon Glass <[email protected]> > > But please see below. > >> >> diff --git a/drivers/sysreset/Kconfig b/drivers/sysreset/Kconfig >> index a6d48e8a662c..1e228b97443a 100644 >> --- a/drivers/sysreset/Kconfig >> +++ b/drivers/sysreset/Kconfig >> @@ -15,6 +15,12 @@ config SYSRESET >> >> if SYSRESET >> >> +config SYSRESET_GPIO >> + bool "Enable support for GPIO restart driver" >> + select GPIO >> + help >> + Restart support via GPIO pin connected reset logic. > > What does this mean? Please expand this to explain what it means. > > Also, what is the difference between restart and reset? If there is no > difference please use the word 'reset'. If there is a difference, > please explain it here.
I have taken restart name because this is what it is written Linux kernel. Based on this explanation: https://kb.netgear.com/1001/Defining-Terms-Power-Cycle-Boot-Reboot-Restart-Reset-and-Hard-Reset "Unlike a reset which changes something, a restart means to turn something on, possibly without changing settings. When upgrading firmware or software you are often asked to restart. A restart would be probably be used if there were a major change to functionality, while a reset often just changes settings of existing functionality." >> + >> config SYSRESET_PSCI >> bool "Enable support for PSCI System Reset" >> depends on ARM_PSCI_FW >> diff --git a/drivers/sysreset/Makefile b/drivers/sysreset/Makefile >> index 0da58a1cf6ad..ca533cfefaad 100644 >> --- a/drivers/sysreset/Makefile >> +++ b/drivers/sysreset/Makefile >> @@ -3,6 +3,7 @@ >> # (C) Copyright 2016 Cadence Design Systems Inc. >> >> obj-$(CONFIG_SYSRESET) += sysreset-uclass.o >> +obj-$(CONFIG_SYSRESET_GPIO) += sysreset_gpio.o >> obj-$(CONFIG_SYSRESET_PSCI) += sysreset_psci.o >> obj-$(CONFIG_SYSRESET_SYSCON) += sysreset_syscon.o >> obj-$(CONFIG_SYSRESET_WATCHDOG) += sysreset_watchdog.o >> diff --git a/drivers/sysreset/sysreset_gpio.c >> b/drivers/sysreset/sysreset_gpio.c >> new file mode 100644 >> index 000000000000..4c1c1510f285 >> --- /dev/null >> +++ b/drivers/sysreset/sysreset_gpio.c >> @@ -0,0 +1,59 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2018 Xilinx, Inc. - Michal Simek >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <errno.h> >> +#include <sysreset.h> >> +#include <asm/gpio.h> >> + >> +struct gpio_reboot_priv { >> + struct gpio_desc gpio; >> +}; >> + >> +static int gpio_reboot_request(struct udevice *dev, enum sysreset_t type) >> +{ >> + struct gpio_reboot_priv *priv = dev_get_priv(dev); >> + >> + /* >> + * When debug log is enabled please make sure that chars won't end up >> + * in output fifo. Or you can append udelay(); to get enough time >> + * to HW to emit output fifo. >> + */ >> + debug("GPIO restart\n"); >> + >> + /* 1 is just setting value - based on gpio->flags 0 or 1 is written >> */ > > Do you mean that it respects polarity (active high/low)? If so, it > might be less confusing to state that. ok - will update. > >> + return dm_gpio_set_value(&priv->gpio, 1); >> +} >> + >> +static struct sysreset_ops gpio_reboot_ops = { >> + .request = gpio_reboot_request, >> +}; >> + >> +int gpio_reboot_probe(struct udevice *dev) >> +{ >> + struct gpio_reboot_priv *priv = dev_get_priv(dev); >> + >> + /* >> + * Linux kernel DT binding contain others optional properties >> + * which are not supported now >> + */ >> + >> + return gpio_request_by_name(dev, "gpios", 0, &priv->gpio, >> GPIOD_IS_OUT); >> +} >> + >> +static const struct udevice_id led_gpio_ids[] = { >> + { .compatible = "gpio-restart" }, >> + { } >> +}; >> + >> +U_BOOT_DRIVER(gpio_reboot) = { >> + .id = UCLASS_SYSRESET, >> + .name = "gpio_restart", >> + .of_match = led_gpio_ids, >> + .ops = &gpio_reboot_ops, >> + .priv_auto_alloc_size = sizeof(struct gpio_reboot_priv), >> + .probe = gpio_reboot_probe, >> +}; >> -- >> 1.9.1 >> > Thanks, Michal _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
