Re: [RFC PATCH] gpio: Add a defer reset object to send board specific reset
From: Alexandre Courbot
Date: Sun Jun 08 2014 - 09:02:54 EST
On Sun, Jun 8, 2014 at 10:09 AM, Houcheng Lin <houcheng@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> + *
> + * 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/