On Fr, 2024-01-05 at 16:59 +0100, Krzysztof Kozlowski wrote:
> Devices sharing a reset GPIO could use the reset framework for
> coordinated handling of that shared GPIO line. We have several cases of
> such needs, at least for Devicetree-based platforms.
>
> If Devicetree-based device requests a reset line, which is missing but
^^^^^^^^^^^^^^^^
Nitpick: the "resets" property is missing, not the reset line.
"If Devicetree-based device requests a reset line, but there only is a
reset-gpios property instead of a "resets" property, ..." maybe?
> there is a reset-gpios property, instantiate a new "reset-gpio" platform
> device which will handle such reset line. This allows seamless handling
> of such shared reset-gpios without need of changing Devicetree binding [1].
>
> All newly registered "reset-gpio" platform devices will be stored on
> their own list to avoid any duplicated devices.
That's not strictly true. The reset_gpio_device_list only contains the
of_phandle_args for lookup.
> The key to find each of
> such platform device is the entire Devicetree GPIO specifier: phandle to
> GPIO controller, GPIO number and GPIO flags. If two devices have
> conflicting "reset-gpios" property, e.g. with different ACTIVE_xxx
> flags, this would spawn two separate "reset-gpio" devices, where the
> second would fail probing on busy GPIO reques
request.
Is that true? The code below looks like overwrites of_phandle_args so
that only one reset-gpio device is spawned for each gpio node.
> Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@xxxxxxxxxxxxxxxxxx/ [1]
> Cc: Bartosz Golaszewski <brgl@xxxxxxxx>
> Cc: Sean Anderson <sean.anderson@xxxxxxxx>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> ---
> drivers/reset/core.c | 176 ++++++++++++++++++++++++++++---
> include/linux/reset-controller.h | 4 +
> 2 files changed, 167 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 4d5a78d3c085..ec9b3ff419cf 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -13,6 +13,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/acpi.h>
> +#include <linux/platform_device.h>
> #include <linux/reset.h>
> #include <linux/reset-controller.h>
> #include <linux/slab.h>
> @@ -23,6 +24,10 @@ static LIST_HEAD(reset_controller_list);
> static DEFINE_MUTEX(reset_lookup_mutex);
> static LIST_HEAD(reset_lookup_list);
>
> +/* Protects reset_gpio_device_list */
> +static DEFINE_MUTEX(reset_gpio_device_mutex);
> +static LIST_HEAD(reset_gpio_device_list);
I would call this reset_gpio_lookup_list or
reset_gpio_phandle_args_list.
> +
> /**
> * struct reset_control - a reset control
> * @rcdev: a pointer to the reset controller device
> @@ -63,6 +68,16 @@ struct reset_control_array {
> struct reset_control *rstc[] __counted_by(num_rstcs);
> };
>
> +/**
> + * struct reset_gpio_device - ad-hoc created reset-gpio device
> + * @of_args: phandle to the reset controller with all the args like GPIO number
> + * @list: list entry for the reset_lookup_list
> + */
> +struct reset_gpio_device {
Similarly, I would call this reset_gpio_lookup or
reset_gpio_phandle_args.
> + struct of_phandle_args of_args;
> + struct list_head list;
> +};
> +
> static const char *rcdev_name(struct reset_controller_dev *rcdev)
> {
> if (rcdev->dev)
> @@ -813,13 +828,119 @@ static void __reset_control_put_internal(struct reset_control *rstc)
> kref_put(&rstc->refcnt, __reset_control_release);
> }
>
> +static bool __reset_gpios_args_match(const struct of_phandle_args *a1,
> + const struct of_phandle_args *a2)
> +{
> + unsigned int i;
> +
> + if (!a2)
> + return false;
> +
> + if (a1->args_count != a2->args_count)
> + return false;
> +
> + for (i = 0; i < a1->args_count; i++)
> + if (a1->args[i] != a2->args[i])
> + break;
Just return false in the loop and simplify the following to return
true.
> +
> + /* All args matched? */
> + if (i == a1->args_count)
> + return true;
> +
> + return false;
> +}
> +
> +/*
> + * @node: node of the device requesting reset
> + * @reset_args: phandle to the reset controller with all the args like GPIO number
> + */
> +static int __reset_add_reset_gpio_device(struct device_node *node,
> + struct of_phandle_args *args)
> +{
> + struct reset_gpio_device *rgpio_dev;
> + struct platform_device *pdev;
> + int ret;
> +
> + lockdep_assert_not_held(&reset_list_mutex);
> +
> + mutex_lock(&reset_gpio_device_mutex);
> +
> + list_for_each_entry(rgpio_dev, &reset_gpio_device_list, list) {
> + if (args->np == rgpio_dev->of_args.np) {
> + if (__reset_gpios_args_match(args,
> + &rgpio_dev->of_args)) {
> + ret = 0;
> + goto out_unlock;
> + }
> + }
> + }
> +
> + /* Not freed in normal path, persisent subsyst data */
> + rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL);
Since this is persistent, instead of letting the reset-gpio driver call
of_parse_phandle_with_args() again, this could be passed in via
platform data. Is there a reason not to do that instead?
> + if (!rgpio_dev) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + rgpio_dev->of_args = *args;
> + pdev = platform_device_register_data(NULL, "reset-gpio",
> + PLATFORM_DEVID_AUTO, &node,
> + sizeof(node));
> + ret = PTR_ERR_OR_ZERO(pdev);
> + if (!ret)
> + list_add(&rgpio_dev->list, &reset_gpio_device_list);
> + else
> + kfree(rgpio_dev);
> +
> +out_unlock:
> + mutex_unlock(&reset_gpio_device_mutex);
> +
> + return ret;
> +}
> +
> +static struct reset_controller_dev *__reset_find_rcdev(struct of_phandle_args *args,
> + bool gpio_fallback,
> + const void *cookie)
Unused cookie.
> +{
> + struct reset_controller_dev *r, *rcdev;
> +
> + lockdep_assert_held(&reset_list_mutex);
> +
> + rcdev = NULL;
> + list_for_each_entry(r, &reset_controller_list, list) {
> + if (args->np == r->of_node) {
> + if (gpio_fallback) {
> + if (__reset_gpios_args_match(args, r->of_args)) {
> + /*
> + * Fake args (take first reset) and
> + * args_count (to matcg reset-gpio
match
> + * of_reset_n_cells) because reset-gpio
> + * has only one reset and does not care
> + * about reset of GPIO specifier.
> + */
> + args->args[0] = 0;
> + args->args_count = 1;
I'd expect args to be an input-only argument, but here its contents are
overwritten after a match. Why?
This has an effect in __of_reset_control_get(), that I find hard to
follow. See below.
> + rcdev = r;
> + break;
> + }
> + } else {
> + rcdev = r;
> + break;
> + }
> + }
> + }
> +
> + return rcdev;
> +}
> +
> struct reset_control *
> __of_reset_control_get(struct device_node *node, const char *id, int index,
> bool shared, bool optional, bool acquired)
> {
> + struct of_phandle_args args = {0};
> + bool gpio_fallback = false;
> struct reset_control *rstc;
> - struct reset_controller_dev *r, *rcdev;
> - struct of_phandle_args args;
> + struct reset_controller_dev *rcdev;
> int rstc_id;
> int ret;
>
> @@ -839,21 +960,50 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
> index, &args);
> if (ret == -EINVAL)
> return ERR_PTR(ret);
> - if (ret)
> - return optional ? NULL : ERR_PTR(ret);
> + if (ret) {
> + /*
> + * There can be only one reset-gpio for regular devices, so
> + * don't bother with GPIO index.
> + */
I don't understand this comment. The GPIO index should be checked as
part of __reset_gpios_args_match(), or should it not?
> + ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells",
> + 0, &args);
> + if (ret)
> + return optional ? NULL : ERR_PTR(ret);
>
> - mutex_lock(&reset_list_mutex);
> - rcdev = NULL;
> - list_for_each_entry(r, &reset_controller_list, list) {
> - if (args.np == r->of_node) {
> - rcdev = r;
> - break;
> - }
> + gpio_fallback = true;
Is there a reason not just call __reset_add_reset_gpio_device() here?
With that, there should be no need to call __reset_find_rcdev() twice.
> }
>
> + mutex_lock(&reset_list_mutex);
> + rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);
This gets called with args as parsed. If there is a match, this will
overwrite args (in the gpio_fallback case) and return NULL.
> +
> if (!rcdev) {
> - rstc = ERR_PTR(-EPROBE_DEFER);
> - goto out;
> + if (gpio_fallback) {
> + /*
> + * Registering reset-gpio device might cause immediate
> + * bind, thus taking reset_list_mutex lock via
> + * reset_controller_register().
> + */
> + mutex_unlock(&reset_list_mutex);
> + ret = __reset_add_reset_gpio_device(node, &args);
So this will also be called with args as parsed.
> + mutex_lock(&reset_list_mutex);
> + if (ret) {
> + rstc = ERR_PTR(ret);
> + goto out;
> + }
> + /*
> + * Success: reset-gpio could probe immediately, so
> + * re-check the lookup.
> + */
> + rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);
And this will again be called with args as parsed and overwrite args
again.
> + if (!rcdev) {
> + rstc = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> + }
> + /* Success, rcdev is valid thus do not bail out */
> + } else {
> + rstc = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> + }
> }
So at this point args is overwritten in the gpio_fallback case. I would
find it much clearer to just overwrite args here and make the first
parameter to __reset_find_rcdev() const.
regards
Philipp
