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@robh.at.kernel.org/ [1] Cc: Bartosz Golaszewski brgl@bgdev.pl Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
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