[RFC PATCH] gpio: Add a defer reset object to send board specific reset

[RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Houcheng Lin]

The Problem
-----------
The reset signal on a hardware board is send either:
    - during machine initialization
    - during bus master's initialization

In some hardware design, devices on bus need a non-standard and extra reset
signal after bus is initialied. Most reason is to wake up device from hanging
state.

The board spefic reset code can not be put into machine init code, as it is
too early. This code can not also be put onto chip's driver, as it is board
specific and not suitable for a common chip driver.

Defer Reset Object
------------------
The defer reset object is to resolve this issue, developer can put a defer-
reset device on the board's dts file and enable DEFER RESET OBJECT CONFIG.
During driver init-calls, a defer-reset object is created and issue reset signal
after the enclosing device is initialized.

This eliminate the need to rewrite a driver module with only one purpose: sending
a board specific reset. This also allow mainstream kernel to support many boards
that modify the common drivers to send board specific reset. Configuring defer-reset
device in dts leave the board specific reset rules on board level and simple to
maintain.

Example dts File
----------------
    usb-ehci-chip@1211000{
        usb-phy = <&usb2_phy>;
        defer_reset_vbus {
            compatible = "defer-reset";
            reset-gpios = <&gpx3 5 1>;
            reset-init = <0>;
            reset-end = <1>;
            delay = <5>;
        };
    };

Block Diagram of dts File
-------------------------
    +-------------------------------------+
    | usb-ehci-chip@1211000               |
    |   +-------------------------+       |
    |   | defer-reset(gpx3)       |       |
    |   +-------------------------+       |
    +-------------------------------------+

Signed-off-by: Houcheng Lin <houcheng@gmail.com>
---
 drivers/gpio/Kconfig            |   8 ++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-defer-reset.c | 179 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 drivers/gpio/gpio-defer-reset.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a86c49a..99aa0d6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -851,6 +851,14 @@ config GPIO_BCM_KONA
 	help
 	  Turn on GPIO support for Broadcom "Kona" chips.
 
+config GPIO_DEFER_RESET
+	bool "Defer reset driver via gpio"
+	depends on OF_GPIO
+	help
+	  Enable defer reset drvier
+	  The reset signal would be issued after a device on USB or PCI bus is initialied.
+	  The dependency of reset signal and device can be specified in board's dts file.
+
 comment "USB GPIO expanders:"
 
 config GPIO_VIPERBOARD
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 6309aff..0754758 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,3 +101,4 @@ obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
 obj-$(CONFIG_GPIO_XTENSA)	+= gpio-xtensa.o
 obj-$(CONFIG_GPIO_ZEVIO)	+= gpio-zevio.o
+obj-$(CONFIG_GPIO_DEFER_RESET)	+= gpio-defer-reset.o
diff --git a/drivers/gpio/gpio-defer-reset.c b/drivers/gpio/gpio-defer-reset.c
new file mode 100644
index 0000000..c6decab
--- /dev/null
+++ b/drivers/gpio/gpio-defer-reset.c
@@ -0,0 +1,179 @@
+/*
+ * GPIO Defer Reset Driver
+ *
+ * Copyright (C) 2014 Houcheng Lin
+ * Author: Houcheng Lin <houcheng@gmail.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+#include <linux/usb/samsung_usb_phy.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <linux/usb/otg.h>
+
+
+#define DRIVER_DESC "GPIO Defer Reset Driver"
+#define GDR_MAX_DEV 32
+
+
+static DEFINE_MUTEX(deferred_reset_mutex);
+static LIST_HEAD(deferred_reset_list);
+
+
+struct defer_reset_private {
+	struct list_head next;
+	struct device_node *node;  /* defer_reset device */
+	int    issued;
+};
+
+static void gdr_issue_reset(
+	struct platform_device *pdev, struct device_node *dnode)
+{
+	int gpio;
+	int i;
+	int count = of_gpio_named_count(dnode, "reset-gpios");
+	u32 reset_init[GDR_MAX_DEV];
+	u32 reset_end[GDR_MAX_DEV];
+	u32 delay;
+
+	if (count != of_property_count_u32_elems(dnode, "reset-init")) {
+		dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
+			of_property_count_u32_elems(dnode, "reset-init"));
+		return;
+	}
+	if (count != of_property_count_u32_elems(dnode, "reset-end")) {
+		dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
+			of_property_count_u32_elems(dnode, "reset-end"));
+		return;
+	}
+	if (count > GDR_MAX_DEV) {
+		dev_err(&pdev->dev, "too large reset array!\n");
+		return;
+	}
+
+	/* setup parameters */
+	of_property_read_u32_array(dnode, "reset-init", reset_init, count);
+	of_property_read_u32_array(dnode, "reset-end", reset_end, count);
+	of_property_read_u32(dnode, "delay", &delay);
+
+	/* reset init */
+	for (i = 0; i < count; i++) {
+		gpio = of_get_named_gpio(dnode, "reset-gpios", i);
+		dev_info(&pdev->dev,
+			"issue reset to gpio %d for device [%s]\n",
+			gpio, dnode->parent->name);
+		if (!gpio_is_valid(gpio))
+			continue;
+		__gpio_set_value(gpio, reset_init[i]);
+	}
+	if (delay == 0)
+		delay = 5;
+	mdelay(delay);
+	/* reset end */
+	for (i = 0; i < count; i++) {
+		gpio = of_get_named_gpio(dnode, "reset-gpios", i);
+		if (!gpio_is_valid(gpio))
+			continue;
+		__gpio_set_value(gpio, reset_end[i]);
+	}
+}
+
+/**
+  * the pdev parameter is null as it is provided by register routine, platform_device_register_simple
+  **/
+static int gdr_probe(struct platform_device *pdev)
+{
+	struct list_head *pos;
+
+	pr_debug("gpio defer reset probe\n");
+	list_for_each(pos, &deferred_reset_list) {
+		struct platform_device *pd;
+		struct defer_reset_private *prv = list_entry(
+			pos, struct defer_reset_private, next);
+		if (prv->issued)
+			continue;
+		pd = of_find_device_by_node(
+			prv->node->parent);
+		if (pd->dev.driver != 0) {
+			gdr_issue_reset(pdev, prv->node);
+			prv->issued = 1;
+		}
+	}
+	list_for_each(pos, &deferred_reset_list) {
+		struct defer_reset_private *prv = list_entry(pos,
+			struct defer_reset_private, next);
+		if (!prv->issued)
+			return -EPROBE_DEFER;
+	}
+	return 0;
+}
+
+static int gdr_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id gdr_match[] = {
+	{ .compatible = "gpio-defer-reset" },
+	{ .compatible = "defer-reset" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gdr_match);
+#endif
+
+
+static struct platform_driver gdr_driver = {
+	.probe      = gdr_probe,
+	.remove     = gdr_remove,
+	.driver = {
+		.name   = "defer-reset",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_match_ptr(gdr_match),
+	}
+};
+
+static int __init gdr_init(void)
+{
+	struct device_node *node;
+	pr_info("defer-reset-object initialied.\n");
+	platform_device_register_simple("defer-reset", -1, NULL, 0);
+	mutex_lock(&deferred_reset_mutex);
+	for_each_compatible_node(node, NULL, "defer-reset") {
+		struct defer_reset_private *prv = kmalloc(
+			sizeof(struct defer_reset_private), GFP_KERNEL);
+		prv->node = node;
+		prv->issued = 0;
+		list_add_tail(&prv->next, &deferred_reset_list);
+	}
+	mutex_unlock(&deferred_reset_mutex);
+	return platform_driver_register(&gdr_driver);
+}
+module_init(gdr_init);
+
+static void __exit gdr_cleanup(void)
+{
+	platform_driver_unregister(&gdr_driver);
+}
+module_exit(gdr_cleanup);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_ALIAS("platform:defer-reset");
+MODULE_AUTHOR("Houcheng Lin");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Alexandre Courbot]

On Sun, Jun 8, 2014 at 10:09 AM, Houcheng Lin <houcheng@gmail.com> wrote:
> The Problem
> -----------
> The reset signal on a hardware board is send either:
>     - during machine initialization
>     - during bus master's initialization
>
> In some hardware design, devices on bus need a non-standard and extra reset
> signal after bus is initialied. Most reason is to wake up device from hanging
> state.
>
> The board spefic reset code can not be put into machine init code, as it is
> too early. This code can not also be put onto chip's driver, as it is board
> specific and not suitable for a common chip driver.
>
> Defer Reset Object
> ------------------
> The defer reset object is to resolve this issue, developer can put a defer-
> reset device on the board's dts file and enable DEFER RESET OBJECT CONFIG.
> During driver init-calls, a defer-reset object is created and issue reset signal
> after the enclosing device is initialized.
>
> This eliminate the need to rewrite a driver module with only one purpose: sending
> a board specific reset. This also allow mainstream kernel to support many boards
> that modify the common drivers to send board specific reset. Configuring defer-reset
> device in dts leave the board specific reset rules on board level and simple to
> maintain.

Interesting approach to a long-standing problem. I had my own
embarrassing attempt at it (power sequences), and more recently Olof
(CC'd) sent something related for the MMC bus
(http://www.spinics.net/lists/devicetree/msg18900.html - I'm not sure
what has become of this patch?). And there are certainly other
attempts that I don't know of.

So, let's say that this time we do it for real. There are some points
I like in your approach, like the fact that it is completely
bus-agnostic. But although it will certainly not end up being as
controversial as the power sequences have been, I am not sure
everybody will agree to use the DT this way...

>
> Example dts File
> ----------------
>     usb-ehci-chip@1211000{
>         usb-phy = <&usb2_phy>;
>         defer_reset_vbus {
>             compatible = "defer-reset";
>             reset-gpios = <&gpx3 5 1>;
>             reset-init = <0>;
>             reset-end = <1>;
>             delay = <5>;
>         };
>     };

Here I am not convinced that everybody will like the fact that this
appears as a device of its own, with its own compatible property.
Let's see what the DT people think of it.

>
> Block Diagram of dts File
> -------------------------
>     +-------------------------------------+
>     | usb-ehci-chip@1211000               |
>     |   +-------------------------+       |
>     |   | defer-reset(gpx3)       |       |
>     |   +-------------------------+       |
>     +-------------------------------------+
>
> Signed-off-by: Houcheng Lin <houcheng@gmail.com>
> ---
>  drivers/gpio/Kconfig            |   8 ++
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-defer-reset.c | 179 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/gpio/gpio-defer-reset.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..99aa0d6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -851,6 +851,14 @@ config GPIO_BCM_KONA
>         help
>           Turn on GPIO support for Broadcom "Kona" chips.
>
> +config GPIO_DEFER_RESET
> +       bool "Defer reset driver via gpio"
> +       depends on OF_GPIO
> +       help
> +         Enable defer reset drvier

s/drvier/driver

> +         The reset signal would be issued after a device on USB or PCI bus is initialied.

s/initialied/initialized

> +         The dependency of reset signal and device can be specified in board's dts file.
> +
>  comment "USB GPIO expanders:"
>
>  config GPIO_VIPERBOARD
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..0754758 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -101,3 +101,4 @@ obj-$(CONFIG_GPIO_WM8994)   += gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)      += gpio-xilinx.o
>  obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
>  obj-$(CONFIG_GPIO_ZEVIO)       += gpio-zevio.o
> +obj-$(CONFIG_GPIO_DEFER_RESET) += gpio-defer-reset.o
> diff --git a/drivers/gpio/gpio-defer-reset.c b/drivers/gpio/gpio-defer-reset.c
> new file mode 100644
> index 0000000..c6decab
> --- /dev/null
> +++ b/drivers/gpio/gpio-defer-reset.c
> @@ -0,0 +1,179 @@
> +/*
> + * GPIO Defer Reset Driver
> + *
> + * Copyright (C) 2014 Houcheng Lin
> + * Author: Houcheng Lin <houcheng@gmail.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/phy.h>
> +#include <linux/usb/samsung_usb_phy.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/otg.h>
> +
> +
> +#define DRIVER_DESC "GPIO Defer Reset Driver"
> +#define GDR_MAX_DEV 32
> +
> +
> +static DEFINE_MUTEX(deferred_reset_mutex);
> +static LIST_HEAD(deferred_reset_list);
> +
> +
> +struct defer_reset_private {
> +       struct list_head next;
> +       struct device_node *node;  /* defer_reset device */
> +       int    issued;
> +};
> +
> +static void gdr_issue_reset(
> +       struct platform_device *pdev, struct device_node *dnode)
> +{
> +       int gpio;
> +       int i;
> +       int count = of_gpio_named_count(dnode, "reset-gpios");
> +       u32 reset_init[GDR_MAX_DEV];
> +       u32 reset_end[GDR_MAX_DEV];
> +       u32 delay;
> +
> +       if (count != of_property_count_u32_elems(dnode, "reset-init")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-init"));
> +               return;
> +       }
> +       if (count != of_property_count_u32_elems(dnode, "reset-end")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-end"));
> +               return;
> +       }
> +       if (count > GDR_MAX_DEV) {
> +               dev_err(&pdev->dev, "too large reset array!\n");
> +               return;
> +       }
> +
> +       /* setup parameters */
> +       of_property_read_u32_array(dnode, "reset-init", reset_init, count);
> +       of_property_read_u32_array(dnode, "reset-end", reset_end, count);
> +       of_property_read_u32(dnode, "delay", &delay);
> +
> +       /* reset init */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);

Quick note: please make use of the gpiod interface in your code
(include/linux/gpio/consumer.h and Documentation/gpio/consumer.txt).
That will simplify it a great deal and will force you to actually
request the GPIOs, which you omitted here.

> +               dev_info(&pdev->dev,
> +                       "issue reset to gpio %d for device [%s]\n",
> +                       gpio, dnode->parent->name);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_init[i]);
> +       }
> +       if (delay == 0)
> +               delay = 5;
> +       mdelay(delay);
> +       /* reset end */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_end[i]);
> +       }
> +}
> +
> +/**
> +  * the pdev parameter is null as it is provided by register routine, platform_device_register_simple
> +  **/
> +static int gdr_probe(struct platform_device *pdev)
> +{
> +       struct list_head *pos;
> +
> +       pr_debug("gpio defer reset probe\n");
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct platform_device *pd;
> +               struct defer_reset_private *prv = list_entry(
> +                       pos, struct defer_reset_private, next);
> +               if (prv->issued)
> +                       continue;
> +               pd = of_find_device_by_node(
> +                       prv->node->parent);
> +               if (pd->dev.driver != 0) {
> +                       gdr_issue_reset(pdev, prv->node);
> +                       prv->issued = 1;

Is there anything that prevents you from removing (and freeing) the
items from the list once the reset is issued?

> +               }
> +       }
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct defer_reset_private *prv = list_entry(pos,
> +                       struct defer_reset_private, next);
> +               if (!prv->issued)
> +                       return -EPROBE_DEFER;
> +       }
> +       return 0;

If you can remove your defer_reset_private instances as they are
processed you can simple return 0 if the list is empty, or
-EPROBE_DEFER otherwise. And probably get rid of your "issued" member
too.

Also, once everything is processed, it would be nice to de-register your device.

Before doing a more thorough review, I'd like to discuss the basic
idea. All the previous attempts at implementing an out-of-bus reset
mechanism are strong evidence that something like this is needed.
Having a generic mechanism would be a great plus. I am not convinced
that using a dummy device instance is the right thing to do though.
Also depending on the bus or device you might desire a different
timing for issuing the reset: e.g. I know of a modem on a discoverable
bus (MMC) that will only let itself be probed after the reset is
applied.

Maybe something ought to be implemented at a deeper level, like the
bus (as in, all of them) of even the device layer?

Alex.

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Houcheng Lin]

# RESEND and please ignore previous mail #

On 2014/06/08 20:58, Alexandre Courbot wrote:
> Interesting approach to a long-standing problem. I had my own
> embarrassing attempt at it (power sequences), and more recently Olof
> (CC'd) sent something related for the MMC bus
> (http://www.spinics.net/lists/devicetree/msg18900.html - I'm not sure
> what has become of this patch?). And there are certainly other
> attempts that I don't know of. So, let's say that this time we do it
> for real. There are some points I like in your approach, like the fact
> that it is completely bus-agnostic. But although it will certainly not
> end up being as controversial as the power sequences have been, I am
> not sure everybody will agree to use the DT this way...

Thanks a lot for your reply. I not sure if DT people like this approach to use
DT. As most poeple use DT as a script to describe a hardware board: cpu,
buses, memory, address mappings and devices. But I think the DT file can also
be a interface between hardware and software engineers. From the software
engineer's view to a board, he may not get clear informed that this board's
some chip need a special reset signal when bus is on. From the hardware
engineer's view, every devices may works well on bootloader but not in Linux.
The DT file can serve as note between the two engineers, and a defered reset
object specify in DT file level can a useful note in this situation.

>> Example dts File
>> ----------------
>>     usb-ehci-chip@1211000{
>>         usb-phy = <&usb2_phy>;
>>         defer_reset_vbus {
>>             compatible = "defer-reset";
>>             reset-gpios = <&gpx3 5 1>;
>>             reset-init = <0>;
>>             reset-end = <1>;
>>             delay = <5>;
>>         };
>>     };
>
> Here I am not convinced that everybody will like the fact that this
> appears as a device of its own, with its own compatible property.
> Let's see what the DT people think of it.
Yes, this patch need DT people agree to use DT in this way.
>
> +       /* setup parameters */
> +       of_property_read_u32_array(dnode, "reset-init", reset_init, count);
> +       of_property_read_u32_array(dnode, "reset-end", reset_end, count);
> +       of_property_read_u32(dnode, "delay", &delay);
> +
> +       /* reset init */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);
>
> Quick note: please make use of the gpiod interface in your code.
> (include/linux/gpio/consumer.h and Documentation/gpio/consumer.txt).
> That will simplify it a great deal and will force you to actually
> request the GPIOs, which you omitted here.
I ommitted this and will add code to use consumer.h in next version.
> +static int gdr_probe(struct platform_device *pdev)
> +{
> +       struct list_head *pos;
> +
> +       pr_debug("gpio defer reset probe\n");
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct platform_device *pd;
> +               struct defer_reset_private *prv = list_entry(
> +                       pos, struct defer_reset_private, next);
> +               if (prv->issued)
> +                       continue;
> +               pd = of_find_device_by_node(
> +                       prv->node->parent);
> +               if (pd->dev.driver != 0) {
> +                       gdr_issue_reset(pdev, prv->node);
> +                       prv->issued = 1;
>
> Is there anything that prevents you from removing (and freeing) the
> items from the list once the reset is issued?

Nothing prevents the removing and freeing. The resources claimed by this
driver are no depend to others and should be release immediately after usage.
The free of resources will be added in next version.

>
>
>> +               }
>> +       }
>> +       list_for_each(pos, &deferred_reset_list) {
>> +               struct defer_reset_private *prv = list_entry(pos,
>> +                       struct defer_reset_private, next);
>> +               if (!prv->issued)
>> +                       return -EPROBE_DEFER;
>> +       }
>> +       return 0;
>
> If you can remove your defer_reset_private instances as they are
> processed you can simple return 0 if the list is empty, or
> -EPROBE_DEFER otherwise. And probably get rid of your "issued" member
> too.
>
> Also, once everything is processed, it would be nice to de-register your device.

Yes, the driver only need a list to keep all pending deferred reset. Once
a reset is issued, the corresponding deferred-reset object in list can be
removed and that the member "issued" can no longer needed. When list empty,
de-register this device from kernel.

>
> Before doing a more thorough review, I'd like to discuss the basic
> idea. All the previous attempts at implementing an out-of-bus reset
> mechanism are strong evidence that something like this is needed.
> Having a generic mechanism would be a great plus. I am not convinced
> that using a dummy device instance is the right thing to do though.
> Also depending on the bus or device you might desire a different
> timing for issuing the reset: e.g. I know of a modem on a discoverable
> bus (MMC) that will only let itself be probed after the reset is
> applied.

On some hardware board's resetting signal's timing sequence depends on
initialization of a device. Pull the reseting logic onto the device level fits
the original hardware and no need to modify the chip driver's source code.

However, this mechanism is encasulated as a dummy, pseudo device in kerne. By
nature, it should be a system type/errata, bus feature or kernel feature. I am
not sure if it is okay that a kernel feature code can registers a driver, run
probe code and de-register driver. Is there any suggestion ?

>
> Maybe something ought to be implemented at a deeper level, like the
> bus (as in, all of them) of even the device layer?
>
> Alex.

Thanks a lot for spending time and giving comments.
Best regards,
Houcheng Lin

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Linus Walleij]

On Sun, Jun 8, 2014 at 3:09 AM, Houcheng Lin <houcheng@gmail.com> wrote:

> In some hardware design, devices on bus need a non-standard and extra reset
> signal after bus is initialied. Most reason is to wake up device from hanging
> state.
>
> The board spefic reset code can not be put into machine init code, as it is
> too early. This code can not also be put onto chip's driver, as it is board
> specific and not suitable for a common chip driver.
>
> Defer Reset Object
> ------------------
> The defer reset object is to resolve this issue, developer can put a defer-
> reset device on the board's dts file and enable DEFER RESET OBJECT CONFIG.
> During driver init-calls, a defer-reset object is created and issue reset signal
> after the enclosing device is initialized.
>
> This eliminate the need to rewrite a driver module with only one purpose: sending
> a board specific reset. This also allow mainstream kernel to support many boards
> that modify the common drivers to send board specific reset. Configuring defer-reset
> device in dts leave the board specific reset rules on board level and simple to
> maintain.
>
> Example dts File
> ----------------
>     usb-ehci-chip@1211000{
>         usb-phy = <&usb2_phy>;
>         defer_reset_vbus {
>             compatible = "defer-reset";
>             reset-gpios = <&gpx3 5 1>;
>             reset-init = <0>;
>             reset-end = <1>;

This needs some serious DT binding documentation. What are you trying to
achive here? Any kind of sequence encoding in DT is out of the question.

>             delay = <5>;

nanoseconds? milliseconds? seconds? years? picoseconds?

>         };
>     };

OK this is interesting stuff..

But this driver *MUST* live under drivers/power/reset/*, as it
has absolutely *nothing* to do with GPIO except for being a consumer
of it. I won't merge it.

And you should discuss it with Dmitry and David W.

But I will review it nevertheless :-D

> +#include <linux/clk.h>

What? This driver is not using any clocks.

> +#include <linux/dma-mapping.h>

What?

> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>

No way. Include
#include <linux/gpio/consumer.h>

Read up on how to use that in
Documentation/gpio/consumer.txt
Thanks.

> +#include <linux/platform_device.h>
> +#include <linux/usb/phy.h>
> +#include <linux/usb/samsung_usb_phy.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/otg.h>

Please go over this list and see what is actually needed.

> +
> +
> +#define DRIVER_DESC "GPIO Defer Reset Driver"

#define DRIVER_NAME would be more to the point.

> +#define GDR_MAX_DEV 32
> +
> +static DEFINE_MUTEX(deferred_reset_mutex);
> +static LIST_HEAD(deferred_reset_list);
> +
> +struct defer_reset_private {
> +       struct list_head next;
> +       struct device_node *node;  /* defer_reset device */
> +       int    issued;

Use a bool.

> +};
> +
> +static void gdr_issue_reset(
> +       struct platform_device *pdev, struct device_node *dnode)
> +{
> +       int gpio;
> +       int i;
> +       int count = of_gpio_named_count(dnode, "reset-gpios");

Hm seems reasonable...

> +       u32 reset_init[GDR_MAX_DEV];
> +       u32 reset_end[GDR_MAX_DEV];
> +       u32 delay;
> +
> +       if (count != of_property_count_u32_elems(dnode, "reset-init")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-init"));
> +               return;
> +       }
> +       if (count != of_property_count_u32_elems(dnode, "reset-end")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-end"));
> +               return;
> +       }
> +       if (count > GDR_MAX_DEV) {
> +               dev_err(&pdev->dev, "too large reset array!\n");
> +               return;
> +       }
> +
> +       /* setup parameters */
> +       of_property_read_u32_array(dnode, "reset-init", reset_init, count);
> +       of_property_read_u32_array(dnode, "reset-end", reset_end, count);
> +       of_property_read_u32(dnode, "delay", &delay);

OK so you stack up these u32's...

> +       /* reset init */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);

Use gpiod interface to obtain a GPIO descriptor.

> +               dev_info(&pdev->dev,
> +                       "issue reset to gpio %d for device [%s]\n",
> +                       gpio, dnode->parent->name);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_init[i]);

__gpio_set_value??

Are you using the platform-specific implementation? No way.

gpiod_set_value() please.

> +       }
> +       if (delay == 0)
> +               delay = 5;

So what on earth says that this is a sensible default?
Why not, say 10 seconds?

I think probe should simply fail if delay is not specified.

> +       mdelay(delay);

Aha so it is milliseconds. OK.

> +       /* reset end */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_end[i]);
> +       }

OK so reset init and reset end is a way to specify when reset
is asserted (as one or zero).

DON'T do this. The GPIO bindings already know a way to define
if GPIOs are active high or low, and the gpiod interface will invert
the signal for you if this is specified as active low.

This way you get rid of both strange properties.

> +/**
> +  * the pdev parameter is null as it is provided by register routine, platform_device_register_simple
> +  **/
> +static int gdr_probe(struct platform_device *pdev)
> +{
> +       struct list_head *pos;
> +
> +       pr_debug("gpio defer reset probe\n");
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct platform_device *pd;
> +               struct defer_reset_private *prv = list_entry(
> +                       pos, struct defer_reset_private, next);
> +               if (prv->issued)
> +                       continue;
> +               pd = of_find_device_by_node(
> +                       prv->node->parent);
> +               if (pd->dev.driver != 0) {
> +                       gdr_issue_reset(pdev, prv->node);
> +                       prv->issued = 1;
> +               }
> +       }
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct defer_reset_private *prv = list_entry(pos,
> +                       struct defer_reset_private, next);
> +               if (!prv->issued)
> +                       return -EPROBE_DEFER;
> +       }
> +       return 0;
> +}
> +
> +static int gdr_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}

Isn't this callback optional then?

> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gdr_match[] = {
> +       { .compatible = "gpio-defer-reset" },
> +       { .compatible = "defer-reset" },

Why two compatible strings? If we're inventing this let's stick to
one.

> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, gdr_match);
> +#endif
> +
> +
> +static struct platform_driver gdr_driver = {
> +       .probe      = gdr_probe,
> +       .remove     = gdr_remove,
> +       .driver = {
> +               .name   = "defer-reset",

So why are you not using this #define DRIVER_DESC for the name
you did up there?

> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_match_ptr(gdr_match),
> +       }
> +};
> +
> +static int __init gdr_init(void)
> +{
> +       struct device_node *node;
> +       pr_info("defer-reset-object initialied.\n");
> +       platform_device_register_simple("defer-reset", -1, NULL, 0);
> +       mutex_lock(&deferred_reset_mutex);
> +       for_each_compatible_node(node, NULL, "defer-reset") {
> +               struct defer_reset_private *prv = kmalloc(
> +                       sizeof(struct defer_reset_private), GFP_KERNEL);
> +               prv->node = node;
> +               prv->issued = 0;
> +               list_add_tail(&prv->next, &deferred_reset_list);
> +       }
> +       mutex_unlock(&deferred_reset_mutex);
> +       return platform_driver_register(&gdr_driver);
> +}
> +module_init(gdr_init);

Hm hm. I don't know if this is a good approach. Grant will know maybe.

> +
> +static void __exit gdr_cleanup(void)
> +{
> +       platform_driver_unregister(&gdr_driver);
> +}
> +module_exit(gdr_cleanup);

You forgot to free the deferred_reset_list here. Fix that.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Houcheng Lin]

Hi Linus, please see my replies below,

2014-06-12 20:35 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:
>
> OK this is interesting stuff..
>
> But this driver *MUST* live under drivers/power/reset/*, as it
> has absolutely *nothing* to do with GPIO except for being a consumer
> of it. I won't merge it.
>
> And you should discuss it with Dmitry and David W.
>
> But I will review it nevertheless :-D

Thanks for this information, I changed source to under drivers/
power/reset.

>
> OK so reset init and reset end is a way to specify when reset
> is asserted (as one or zero).
>
> DON'T do this. The GPIO bindings already know a way to define
> if GPIOs are active high or low, and the gpiod interface will invert
> the signal for you if this is specified as active low.
>
> This way you get rid of both strange properties.
>

May I reserve the "reset-init" and ""reset-end" properties? As
some boards use power line off to reset device. The power line
is active high when as power but active low when as a reset signal.

>
> You forgot to free the deferred_reset_list here. Fix that.
>

Thank a lot for your reviews :) Here are fixed of version 2:
    - change location to drivers/power/reset
    - remove unused #include
    - change to use GPIO descriptor interface and use customer.h
    - free resources after issue reset. However, if a enclosing
      chip is not probled, the correpsonding reset will not send
      and memeory is still not free in this case.
    - other minor fixs

Best regards,
Houcheng Lin

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Ulf Hansson]

On 8 June 2014 03:09, Houcheng Lin <houcheng@gmail.com> wrote:
> The Problem
> -----------
> The reset signal on a hardware board is send either:
>     - during machine initialization
>     - during bus master's initialization
>
> In some hardware design, devices on bus need a non-standard and extra reset
> signal after bus is initialied. Most reason is to wake up device from hanging
> state.
>
> The board spefic reset code can not be put into machine init code, as it is
> too early. This code can not also be put onto chip's driver, as it is board
> specific and not suitable for a common chip driver.
>

If I understand correct, you need a per device reset functionality?
And obviously the reset implementation is board/SoC specific.

We have the "Reset Controller framework", drivers/reset. Could this help you?

Kind regards
Ulf Hansson

> Defer Reset Object
> ------------------
> The defer reset object is to resolve this issue, developer can put a defer-
> reset device on the board's dts file and enable DEFER RESET OBJECT CONFIG.
> During driver init-calls, a defer-reset object is created and issue reset signal
> after the enclosing device is initialized.
>
> This eliminate the need to rewrite a driver module with only one purpose: sending
> a board specific reset. This also allow mainstream kernel to support many boards
> that modify the common drivers to send board specific reset. Configuring defer-reset
> device in dts leave the board specific reset rules on board level and simple to
> maintain.
>
> Example dts File
> ----------------
>     usb-ehci-chip@1211000{
>         usb-phy = <&usb2_phy>;
>         defer_reset_vbus {
>             compatible = "defer-reset";
>             reset-gpios = <&gpx3 5 1>;
>             reset-init = <0>;
>             reset-end = <1>;
>             delay = <5>;
>         };
>     };
>
> Block Diagram of dts File
> -------------------------
>     +-------------------------------------+
>     | usb-ehci-chip@1211000               |
>     |   +-------------------------+       |
>     |   | defer-reset(gpx3)       |       |
>     |   +-------------------------+       |
>     +-------------------------------------+
>
> Signed-off-by: Houcheng Lin <houcheng@gmail.com>
> ---
>  drivers/gpio/Kconfig            |   8 ++
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-defer-reset.c | 179 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 drivers/gpio/gpio-defer-reset.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..99aa0d6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -851,6 +851,14 @@ config GPIO_BCM_KONA
>         help
>           Turn on GPIO support for Broadcom "Kona" chips.
>
> +config GPIO_DEFER_RESET
> +       bool "Defer reset driver via gpio"
> +       depends on OF_GPIO
> +       help
> +         Enable defer reset drvier
> +         The reset signal would be issued after a device on USB or PCI bus is initialied.
> +         The dependency of reset signal and device can be specified in board's dts file.
> +
>  comment "USB GPIO expanders:"
>
>  config GPIO_VIPERBOARD
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..0754758 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -101,3 +101,4 @@ obj-$(CONFIG_GPIO_WM8994)   += gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)      += gpio-xilinx.o
>  obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
>  obj-$(CONFIG_GPIO_ZEVIO)       += gpio-zevio.o
> +obj-$(CONFIG_GPIO_DEFER_RESET) += gpio-defer-reset.o
> diff --git a/drivers/gpio/gpio-defer-reset.c b/drivers/gpio/gpio-defer-reset.c
> new file mode 100644
> index 0000000..c6decab
> --- /dev/null
> +++ b/drivers/gpio/gpio-defer-reset.c
> @@ -0,0 +1,179 @@
> +/*
> + * GPIO Defer Reset Driver
> + *
> + * Copyright (C) 2014 Houcheng Lin
> + * Author: Houcheng Lin <houcheng@gmail.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/phy.h>
> +#include <linux/usb/samsung_usb_phy.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/otg.h>
> +
> +
> +#define DRIVER_DESC "GPIO Defer Reset Driver"
> +#define GDR_MAX_DEV 32
> +
> +
> +static DEFINE_MUTEX(deferred_reset_mutex);
> +static LIST_HEAD(deferred_reset_list);
> +
> +
> +struct defer_reset_private {
> +       struct list_head next;
> +       struct device_node *node;  /* defer_reset device */
> +       int    issued;
> +};
> +
> +static void gdr_issue_reset(
> +       struct platform_device *pdev, struct device_node *dnode)
> +{
> +       int gpio;
> +       int i;
> +       int count = of_gpio_named_count(dnode, "reset-gpios");
> +       u32 reset_init[GDR_MAX_DEV];
> +       u32 reset_end[GDR_MAX_DEV];
> +       u32 delay;
> +
> +       if (count != of_property_count_u32_elems(dnode, "reset-init")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-init"));
> +               return;
> +       }
> +       if (count != of_property_count_u32_elems(dnode, "reset-end")) {
> +               dev_err(&pdev->dev, "should call std error here, number is wrong:%d\n",
> +                       of_property_count_u32_elems(dnode, "reset-end"));
> +               return;
> +       }
> +       if (count > GDR_MAX_DEV) {
> +               dev_err(&pdev->dev, "too large reset array!\n");
> +               return;
> +       }
> +
> +       /* setup parameters */
> +       of_property_read_u32_array(dnode, "reset-init", reset_init, count);
> +       of_property_read_u32_array(dnode, "reset-end", reset_end, count);
> +       of_property_read_u32(dnode, "delay", &delay);
> +
> +       /* reset init */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);
> +               dev_info(&pdev->dev,
> +                       "issue reset to gpio %d for device [%s]\n",
> +                       gpio, dnode->parent->name);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_init[i]);
> +       }
> +       if (delay == 0)
> +               delay = 5;
> +       mdelay(delay);
> +       /* reset end */
> +       for (i = 0; i < count; i++) {
> +               gpio = of_get_named_gpio(dnode, "reset-gpios", i);
> +               if (!gpio_is_valid(gpio))
> +                       continue;
> +               __gpio_set_value(gpio, reset_end[i]);
> +       }
> +}
> +
> +/**
> +  * the pdev parameter is null as it is provided by register routine, platform_device_register_simple
> +  **/
> +static int gdr_probe(struct platform_device *pdev)
> +{
> +       struct list_head *pos;
> +
> +       pr_debug("gpio defer reset probe\n");
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct platform_device *pd;
> +               struct defer_reset_private *prv = list_entry(
> +                       pos, struct defer_reset_private, next);
> +               if (prv->issued)
> +                       continue;
> +               pd = of_find_device_by_node(
> +                       prv->node->parent);
> +               if (pd->dev.driver != 0) {
> +                       gdr_issue_reset(pdev, prv->node);
> +                       prv->issued = 1;
> +               }
> +       }
> +       list_for_each(pos, &deferred_reset_list) {
> +               struct defer_reset_private *prv = list_entry(pos,
> +                       struct defer_reset_private, next);
> +               if (!prv->issued)
> +                       return -EPROBE_DEFER;
> +       }
> +       return 0;
> +}
> +
> +static int gdr_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gdr_match[] = {
> +       { .compatible = "gpio-defer-reset" },
> +       { .compatible = "defer-reset" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, gdr_match);
> +#endif
> +
> +
> +static struct platform_driver gdr_driver = {
> +       .probe      = gdr_probe,
> +       .remove     = gdr_remove,
> +       .driver = {
> +               .name   = "defer-reset",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_match_ptr(gdr_match),
> +       }
> +};
> +
> +static int __init gdr_init(void)
> +{
> +       struct device_node *node;
> +       pr_info("defer-reset-object initialied.\n");
> +       platform_device_register_simple("defer-reset", -1, NULL, 0);
> +       mutex_lock(&deferred_reset_mutex);
> +       for_each_compatible_node(node, NULL, "defer-reset") {
> +               struct defer_reset_private *prv = kmalloc(
> +                       sizeof(struct defer_reset_private), GFP_KERNEL);
> +               prv->node = node;
> +               prv->issued = 0;
> +               list_add_tail(&prv->next, &deferred_reset_list);
> +       }
> +       mutex_unlock(&deferred_reset_mutex);
> +       return platform_driver_register(&gdr_driver);
> +}
> +module_init(gdr_init);
> +
> +static void __exit gdr_cleanup(void)
> +{
> +       platform_driver_unregister(&gdr_driver);
> +}
> +module_exit(gdr_cleanup);
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_ALIAS("platform:defer-reset");
> +MODULE_AUTHOR("Houcheng Lin");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset

[Houcheng Lin]

Hi,

2014-06-16 21:18 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>:
>
> If I understand correct, you need a per device reset functionality?
> And obviously the reset implementation is board/SoC specific.
>
> We have the "Reset Controller framework", drivers/reset. Could this help you?
>
> Kind regards
> Ulf Hansson
>

Thanks for your information. The defer reset object is to send
board specific reset signal and this signal has to be sent after
some device is intialized. For example, a USB device may need extra
reset signal after root hub is initialized.

As machine init code is run before driver init-call, can not add reset
sending here if it depends on a specific device's initialization.

Also it provides a DT node that helps both HW/SW developer to use this
feature.

Best regards,
Houcheng Lin