[U-Boot] [PATCH] sysreset: Add support for gpio-restart
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@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
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. + 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 */ + 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, +};
On 13 Jul 2018, at 11:15, Michal Simek michal.simek@xilinx.com wrote:
The Linux kernel has binding for gpio-restart node. This patch is adding basic support without supporting any optional properties.
Nice. This may also give us an opportunity to streamline some of the reset logic (and the FDT-interface to the ATF) on our RK3399-Q7 board.
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@xilinx.com
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Hi Michal,
On 13 July 2018 at 03:15, Michal Simek michal.simek@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@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@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 GPIOhelpRestart 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.
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.
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
Regards, Simon
On 16.7.2018 07:20, Simon Glass wrote:
Hi Michal,
On 13 July 2018 at 03:15, Michal Simek michal.simek@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@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@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 GPIOhelpRestart 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-R...
"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
Hi Michal,
On 15 July 2018 at 23:31, Michal Simek michal.simek@xilinx.com wrote:
On 16.7.2018 07:20, Simon Glass wrote:
Hi Michal,
On 13 July 2018 at 03:15, Michal Simek michal.simek@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@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@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 GPIOhelpRestart 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-R...
"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."
Perhaps this corresponds to warm and cold reset? I'm not sure. But I don't know of a board which supports resetting without changing anything. At the least the CPU is put back into a start where it can start at its reset vector.
This really doesn't make any sense to me. I think we should stick with 'reset' to avoid confusion. By all means add some notes to the uclass header file about what it should mean. We already have enum sysreset_t, so you could define 'restart' there, perhaps?
Regards, Simon
On 17.7.2018 05:45, Simon Glass wrote:
Hi Michal,
On 15 July 2018 at 23:31, Michal Simek michal.simek@xilinx.com wrote:
On 16.7.2018 07:20, Simon Glass wrote:
Hi Michal,
On 13 July 2018 at 03:15, Michal Simek michal.simek@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@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@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 GPIOhelpRestart 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-R...
"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."
Perhaps this corresponds to warm and cold reset? I'm not sure. But I don't know of a board which supports resetting without changing anything. At the least the CPU is put back into a start where it can start at its reset vector.
This really doesn't make any sense to me. I think we should stick with 'reset' to avoid confusion. By all means add some notes to the uclass header file about what it should mean. We already have enum sysreset_t, so you could define 'restart' there, perhaps?
I have sent v2 with reset. TBH adding restart to enum will just cause another confusion when we don't know what's the exact difference.
Thanks, Michal
participants (3)
-
Dr. Philipp Tomsich -
Michal Simek -
Simon Glass