[U-Boot] [PATCH] sysreset: Add support for gpio-restart

[Michal Simek]

On 16.7.2018 07:20, Simon Glass wrote:
> Hi Michal,
> 
> On 13 July 2018 at 03:15, Michal Simek <michal.simek at xilinx.com> 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 <michal.simek at xilinx.com>
>> ---
>>
>>  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 <sjg at chromium.org>
> 
> 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