Message-ID: <4303204a-12a0-e81b-395e-14b3dc7f64ee@seco.com>
Date:   Mon, 25 Oct 2021 14:17:49 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] reset: Add GPIO-based reset controller



On 10/18/21 7:49 PM, Sean Anderson wrote:
> This adds a driver to control GPIO-based resets using the reset
> controller API. This allows using a common interface for devices which
> may have both GPIO resets and reset controllers, depending on the
> configuration. It also allows for easier sharing of reset GPIOs in the
> case where one GPIO is a reset for multiple devices.
> 
> There are several properties for specifying pre/post-(de)assert delays.
> This device can also use a "reset done" GPIO, for cases when such a GPIO
> is provided by the device being reset. This can be useful when the
> datasheet does not otherwise specify reset timings, or specifies a much
> longer maximum reset delay than the typical delay.
> 
> There is one queue for waiters on done GPIOs. I don't anticipate there
> being a penalty from this, since there will likely only be concurrent
> waiters during startup. I believe that wait_event_idle_timeout is the
> correct function to use here, but there are a lot of variants of this
> function, so I am not completely sure it is the best.
> 
> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
> ---
> 
>   MAINTAINERS                |   5 +
>   drivers/reset/Kconfig      |  11 ++
>   drivers/reset/Makefile     |   1 +
>   drivers/reset/reset-gpio.c | 223 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 240 insertions(+)
>   create mode 100644 drivers/reset/reset-gpio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d118d7957d2..0a54c4dd83d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7813,6 +7813,11 @@ F:	Documentation/i2c/muxes/i2c-mux-gpio.rst
>   F:	drivers/i2c/muxes/i2c-mux-gpio.c
>   F:	include/linux/platform_data/i2c-mux-gpio.h
>   
> +GENERIC GPIO RESET DRIVER
> +M:	Sean Anderson <seanga2@...il.com>
> +S:	Supported
> +F:	drivers/reset/reset-gpio.c
> +
>   GENERIC HDLC (WAN) DRIVERS
>   M:	Krzysztof Halasa <khc@...waw.pl>
>   S:	Maintained
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index be799a5abf8a..24888005baf8 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -66,6 +66,17 @@ config RESET_BRCMSTB_RESCAL
>   	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
>   	  BCM7216.
>   
> +config RESET_GPIO
> +	tristate "GPIO reset controller"
> +	depends on OF
> +	help
> +	  This enables a generic controller for resets attached via GPIOs. It
> +	  may be used to add GPIO resets to drivers which expect a reset
> +	  controller. It supports adding delays and waiting for a "done" GPIO
> +	  to be asserted.
> +
> +	  If compiled as module, it will be called reset-gpio.
> +
>   config RESET_HSDK
>   	bool "Synopsys HSDK Reset Driver"
>   	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 21d46d8869ff..f577ec16fd93 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>   obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>   obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>   obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> +obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
>   obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>   obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>   obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
> new file mode 100644
> index 000000000000..93d3dbb150e0
> --- /dev/null
> +++ b/drivers/reset/reset-gpio.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021 Sean Anderson <sean.anderson@...o.com>
> + *
> + * This driver controls GPIOs used to reset device(s). It may be used for when
> + * there is a need for more complex behavior than a simple reset-gpios
> + * property. It may also be used to unify code paths between device-based and
> + * gpio-based resets.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +
> +/**
> + * struct reset_gpio_priv - Private data for GPIO reset driver
> + * @rc: Reset controller for this driver
> + * @done_queue: Queue to wait for changes on done GPIOs. Events occur whenever
> + *              the value of any done GPIO changes. Valid only when @done is
> + *              non-%NULL.
> + * @reset: Array of gpios to use when (de)asserting resets
> + * @done: Array of gpios to determine whether a reset has finished; may be
> + *        %NULL
> + * @done_timeout_jiffies: Timeout when waiting for a done GPIO to be asserted, in jiffies
> + * @post_assert_delay: Time to wait after asserting a reset, in us
> + * @post_deassert_delay: Time to wait after deasserting a reset, in us
> + */
> +struct reset_gpio_priv {
> +	struct reset_controller_dev rc;
> +	struct wait_queue_head done_queue;
> +	struct gpio_descs *reset;
> +	struct gpio_descs *done;
> +	unsigned long done_timeout_jiffies;
> +	u32 pre_assert_delay;
> +	u32 post_assert_delay;
> +	u32 pre_deassert_delay;
> +	u32 post_deassert_delay;
> +};
> +
> +static inline struct reset_gpio_priv
> +*rc_to_reset_gpio(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct reset_gpio_priv, rc);
> +}
> +
> +static int reset_gpio_assert(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	if (priv->pre_assert_delay)
> +		fsleep(priv->pre_assert_delay);
> +	gpiod_set_value_cansleep(priv->reset->desc[id], 1);
> +	if (priv->post_assert_delay)
> +		fsleep(priv->post_assert_delay);
> +	return 0;
> +}
> +
> +static int reset_gpio_deassert(struct reset_controller_dev *rc,
> +			       unsigned long id)
> +{
> +	int ret = 0;
> +	unsigned int remaining;
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	if (priv->pre_deassert_delay)
> +		fsleep(priv->pre_deassert_delay);
> +	gpiod_set_value_cansleep(priv->reset->desc[id], 0);
> +	if (priv->post_deassert_delay)
> +		fsleep(priv->post_deassert_delay);
> +
> +	if (!priv->done)
> +		return 0;
> +
> +	remaining = wait_event_idle_timeout(
> +		priv->done_queue,
> +		(ret = gpiod_get_value_cansleep(priv->done->desc[id])),
> +		priv->done_timeout_jiffies);
> +	dev_dbg(rc->dev, "%s: remaining=%u\n", __func__, remaining);
> +	if (ret < 0)
> +		return ret;
> +	if (ret)
> +		return 0;
> +	return -ETIMEDOUT;
> +}
> +
> +static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	int ret = reset_gpio_assert(rc, id);
> +
> +	if (!ret)
> +		return ret;
> +
> +	return reset_gpio_deassert(rc, id);
> +}
> +
> +static int reset_gpio_status(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	return gpiod_get_value_cansleep(priv->reset->desc[id]);
> +}
> +
> +static const struct reset_control_ops reset_gpio_ops = {
> +	.reset = reset_gpio_reset,
> +	.assert = reset_gpio_assert,
> +	.deassert = reset_gpio_deassert,
> +	.status = reset_gpio_status,
> +};
> +
> +static irqreturn_t reset_gpio_irq(int irq, void *data)
> +{
> +	struct reset_gpio_priv *priv = data;
> +
> +	wake_up(&priv->done_queue);
> +	return IRQ_HANDLED;
> +}
> +
> +static int reset_gpio_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct reset_gpio_priv *priv;
> +	u32 done_timeout_us;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* A short macro to reduce repetitive error handling */
> +#define read_delay(propname, val) do { \
> +	ret = of_property_read_u32(dev->of_node, (propname), &(val)); \
> +	if (ret == -EINVAL) \
> +		(val) = 0; \
> +	else if (ret) \
> +		return dev_err_probe(dev, ret, \
> +				     "Could not read %s\n", propname); \
> +} while (0)
> +
> +	read_delay("pre-assert-us", priv->pre_assert_delay);
> +	read_delay("post-assert-us", priv->post_assert_delay);
> +	read_delay("pre-deassert-us", priv->pre_deassert_delay);
> +	read_delay("post-deassert-us", priv->post_deassert_delay);
> +
> +	ret = of_property_read_u32(np, "done-timeout-us", &done_timeout_us);
> +	if (ret == -EINVAL) {
> +		if (priv->post_deassert_delay)
> +			done_timeout_us = 10 * priv->post_deassert_delay;
> +		else
> +			done_timeout_us = 1000;
> +	} else if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Could not read done timeout\n");
> +	priv->done_timeout_jiffies = usecs_to_jiffies(done_timeout_us);
> +
> +	priv->reset = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Could not get reset gpios\n");
> +
> +	priv->done = devm_gpiod_get_array_optional(dev, "done",
> +						   GPIOD_IN);
> +	if (IS_ERR(priv->done))
> +		return dev_err_probe(dev, PTR_ERR(priv->done),
> +				     "Could not get done gpios\n");
> +	if (priv->done) {
> +		int i;
> +
> +		if (priv->reset->ndescs != priv->done->ndescs)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Number of reset and done gpios does not match\n");
> +		init_waitqueue_head(&priv->done_queue);
> +		for (i = 0; i < priv->done->ndescs; i++) {
> +			ret = gpiod_to_irq(priv->done->desc[i]);
> +			if (ret < 0)
> +				return dev_err_probe(dev, ret,
> +						     "Could not convert GPIO to IRQ\n");
> +
> +			ret = devm_request_irq(dev, ret, reset_gpio_irq,
> +					       IRQF_SHARED, dev_name(dev),
> +					       priv);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Could not request IRQ\n");
> +		}
> +	}
> +
> +	priv->rc.ops = &reset_gpio_ops;
> +	priv->rc.owner = THIS_MODULE;
> +	priv->rc.dev = dev;
> +	priv->rc.of_node = np;
> +	priv->rc.nr_resets = priv->reset->ndescs;
> +	ret = devm_reset_controller_register(dev, &priv->rc);
> +	if (!ret)
> +		dev_info(dev, "probed with %u resets\n", priv->reset->ndescs);
> +	return ret;
> +}
> +
> +static const struct of_device_id reset_gpio_of_match[] = {
> +	{ .compatible = "gpio-reset", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, reset_gpio_of_match);
> +
> +static struct platform_driver reset_gpio_driver = {
> +	.probe = reset_gpio_probe,
> +	.driver = {
> +		.name = "gpio-reset",
> +		.of_match_table = of_match_ptr(reset_gpio_of_match),
> +	},
> +};
> +module_platform_driver(reset_gpio_driver);
> +
> +MODULE_ALIAS("platform:gpio-reset");
> +MODULE_DESCRIPTION("Generic GPIO reset driver");
> +MODULE_LICENSE("GPL v2");
> 

ping?

Philipp, should I be CCing anyone else? MAINTAINERS only lists you and vger...

--Sean

Powered by blists - more mailing lists