| Message ID | 20240129115216.96479-5-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | c721f189e89c0d4db119d7bb2b46768d0fb5f6b1 |
| Headers | show |
| Series | reset: gpio: ASoC: shared GPIO resets | expand |
Hi Krzysztof, something is odd with the addresses on this patch, because neither GPIO maintainer is on CC nor linux-gpio@vger, and it's such a GPIO-related patch. We only saw it through side effects making <linux/gpio/driver.h> optional, as required by this patch. Please also CC Geert Uytterhoeven, the author of the GPIO aggregator. i.e. this: > 2. !GPIOLIB stub: > https://lore.kernel.org/all/20240125081601.118051-3-krzysztof.kozlowski@linaro.org/ On Mon, Jan 29, 2024 at 12:53 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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, while "resets" > Devicetree property is missing but there is a "reset-gpios" one, > 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]. > > To avoid creating multiple "reset-gpio" platform devices, store the > Devicetree "reset-gpios" GPIO specifiers used for new devices on a > linked list. Later such Devicetree GPIO specifier (phandle to GPIO > controller, GPIO number and GPIO flags) is used to check if reset > controller for given GPIO was already registered. > > If two devices have conflicting "reset-gpios" property, e.g. with > different ACTIVE_xxx flags, this would allow to spawn two separate > "reset-gpio" devices, where the second would fail probing on busy GPIO > request. > > Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: Sean Anderson <sean.anderson@seco.com> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> (...) In my naive view, this implements the following: reset -> virtual "gpio" -> many physical gpios[0..n] So if there was already a way in the kernel to map one GPIO to many GPIOs, the framework could just use that with a simple single GPIO? See the bindings in: Documentation/devicetree/bindings/gpio/gpio-delay.yaml This is handled by drivers/gpio/gpio-aggregator.c. This supports a 1-to-1 map: one GPIO in, one GPIO out, same offset. So if that is extended to support 1-to-many, this problem is solved. Proposed solution: add a single boolean property such as aggregate-all-gpios; to the gpio-delay node, making it provide one single gpio at offset 0 on the consumer side, and refuse any more consumers. This will also solve the problem with induced delays on some GPIO lines as I can see was discussed in the bindings, the gpio aggregator already supports that, but it would work fine with a delay being zero as well. This avoids all the hackery with driver stubs etc as well. Yours, Linus Walleij
On Wed, Jan 31, 2024 at 9:57 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Krzysztof, > > something is odd with the addresses on this patch, because neither GPIO > maintainer is on CC nor linux-gpio@vger, and it's such a GPIO-related > patch. We only saw it through side effects making <linux/gpio/driver.h> > optional, as required by this patch. > > Please also CC Geert Uytterhoeven, the author of the GPIO aggregator. > > i.e. this: > > 2. !GPIOLIB stub: > > https://lore.kernel.org/all/20240125081601.118051-3-krzysztof.kozlowski@linaro.org/ > > On Mon, Jan 29, 2024 at 12:53 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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, while "resets" > > Devicetree property is missing but there is a "reset-gpios" one, > > 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]. > > > > To avoid creating multiple "reset-gpio" platform devices, store the > > Devicetree "reset-gpios" GPIO specifiers used for new devices on a > > linked list. Later such Devicetree GPIO specifier (phandle to GPIO > > controller, GPIO number and GPIO flags) is used to check if reset > > controller for given GPIO was already registered. > > > > If two devices have conflicting "reset-gpios" property, e.g. with > > different ACTIVE_xxx flags, this would allow to spawn two separate > > "reset-gpio" devices, where the second would fail probing on busy GPIO > > request. > > > > Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] > > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Cc: Sean Anderson <sean.anderson@seco.com> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > (...) > > In my naive view, this implements the following: > > reset -> virtual "gpio" -> many physical gpios[0..n] This is a different problem: it supports many users enabling the same GPIO (in Krzysztof's patch it's one but could be more if needed) but - unlike the broken NONEXCLUSIVE GPIOs in GPIOLIB - it counts the number of users and doesn't disable the GPIO for as long as there's at least one. Bart > > So if there was already a way in the kernel to map one GPIO to > many GPIOs, the framework could just use that with a simple > single GPIO? > > See the bindings in: > Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > This is handled by drivers/gpio/gpio-aggregator.c. > > This supports a 1-to-1 map: one GPIO in, one GPIO out, same offset. > So if that is extended to support 1-to-many, this problem is solved. > > Proposed solution: add a single boolean property such as > aggregate-all-gpios; to the gpio-delay node, making it provide > one single gpio at offset 0 on the consumer side, and refuse any > more consumers. > > This will also solve the problem with induced delays on > some GPIO lines as I can see was discussed in the bindings, > the gpio aggregator already supports that, but it would work > fine with a delay being zero as well. > > This avoids all the hackery with driver stubs etc as well. > > Yours, > Linus Walleij
On 31/01/2024 09:57, Linus Walleij wrote: > Hi Krzysztof, > > something is odd with the addresses on this patch, because neither GPIO Nothing is odd - I use get_maintainers.pl which just don't print your names. I can add your addresses manually, no problem, but don't blame the contributor that get_maintainers.pl has a missing content-regex. If you want to be Cced on usage of GPIOs, you need to be sure that MAINTAINERS file has appropriate pattern. > maintainer is on CC nor linux-gpio@vger, and it's such a GPIO-related > patch. We only saw it through side effects making <linux/gpio/driver.h> > optional, as required by this patch. > > Please also CC Geert Uytterhoeven, the author of the GPIO aggregator. > > i.e. this: >> 2. !GPIOLIB stub: >> https://lore.kernel.org/all/20240125081601.118051-3-krzysztof.kozlowski@linaro.org/ > > On Mon, Jan 29, 2024 at 12:53 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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, while "resets" >> Devicetree property is missing but there is a "reset-gpios" one, >> 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]. >> >> To avoid creating multiple "reset-gpio" platform devices, store the >> Devicetree "reset-gpios" GPIO specifiers used for new devices on a >> linked list. Later such Devicetree GPIO specifier (phandle to GPIO >> controller, GPIO number and GPIO flags) is used to check if reset >> controller for given GPIO was already registered. >> >> If two devices have conflicting "reset-gpios" property, e.g. with >> different ACTIVE_xxx flags, this would allow to spawn two separate >> "reset-gpio" devices, where the second would fail probing on busy GPIO >> request. >> >> Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] >> Cc: Bartosz Golaszewski <brgl@bgdev.pl> >> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >> Cc: Sean Anderson <sean.anderson@seco.com> >> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > (...) > > In my naive view, this implements the following: > > reset -> virtual "gpio" -> many physical gpios[0..n] It does not, there is no single GPIO here. There is a single reset controller, though, but still multiple GPIOs in DTS. > > So if there was already a way in the kernel to map one GPIO to > many GPIOs, the framework could just use that with a simple > single GPIO? > > See the bindings in: > Documentation/devicetree/bindings/gpio/gpio-delay.yaml > > This is handled by drivers/gpio/gpio-aggregator.c. > > This supports a 1-to-1 map: one GPIO in, one GPIO out, same offset. > So if that is extended to support 1-to-many, this problem is solved. It does not match the hardware thus I don't know how to implement it in DTS while keeping the requirement that we are describing hardware, not OS abstractions. > > Proposed solution: add a single boolean property such as > aggregate-all-gpios; to the gpio-delay node, making it provide > one single gpio at offset 0 on the consumer side, and refuse any > more consumers. And how do you solve the reset requirements? The problem is not just to share GPIO. The problem is to share in a way that devices operate properly when they assert/deassert reset. > > This will also solve the problem with induced delays on > some GPIO lines as I can see was discussed in the bindings, > the gpio aggregator already supports that, but it would work > fine with a delay being zero as well. > > This avoids all the hackery with driver stubs etc as well. So none of these ideas were posted in previous threads, just because you were not CCed (except one thread)? https://lore.kernel.org/lkml/20191030120440.3699-1-peter.ujfalusi@ti.com/ https://lore.kernel.org/all/9eebec9b-e6fd-4a22-89ea-b434f446e061@linaro.org/ https://lore.kernel.org/all/20231018100055.140847-1-krzysztof.kozlowski@linaro.org/ https://social.treehouse.systems/@marcan/111268780311634160 Please implement some custom lei filter, so you will get such notifications earlier. We keep discussing this for many months on various attempts and this specific attempt already reached v6. Best regards, Krzysztof
On Wed, Jan 31, 2024 at 10:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > [Me] > > reset -> virtual "gpio" -> many physical gpios[0..n] > > This is a different problem: it supports many users enabling the same > GPIO (in Krzysztof's patch it's one but could be more if needed) but - > unlike the broken NONEXCLUSIVE GPIOs in GPIOLIB - it counts the number > of users and doesn't disable the GPIO for as long as there's at least > one. I don't know if the NONEXCLUSIVE stuff is broken, if you mean reference counting isn't working on them, then that is by design because they were invented for regulators and such use cases that do their own reference counting. It's also used for hacks where people need to look up a desc in a second spot, (perhaps we can fix those better). As I say in commit b0ce7b29bfcd090ddba476f45a75ec0a797b048a "This solution with a special flag is not entirely elegant and should ideally be replaced by something more careful as this makes it possible for several consumers to enable/disable the same GPIO line to the left and right without any consistency." I think for regulators (which is the vast majority using it) it isn't broken because the regulator reference counting is working. So if we solve that problem for reset, we probably should put it in drivers/gpio/* somewhere so we can reuse the same solution for regulators and get rid of NONEXCLUSIVE altogether I think? The NONEXCLUSIVE stuff was prompted by converting regulators to gpio descriptors, so it was for the greater good one can say. Or the lesser evil :( my judgement can be questioned here. Yours, Linus Walleij
On 31/01/2024 14:17, Linus Walleij wrote: > On Wed, Jan 31, 2024 at 10:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> [Me] >>> reset -> virtual "gpio" -> many physical gpios[0..n] >> >> This is a different problem: it supports many users enabling the same >> GPIO (in Krzysztof's patch it's one but could be more if needed) but - >> unlike the broken NONEXCLUSIVE GPIOs in GPIOLIB - it counts the number >> of users and doesn't disable the GPIO for as long as there's at least >> one. > > I don't know if the NONEXCLUSIVE stuff is broken, if you mean reference > counting isn't working on them, then that is by design because they were > invented for regulators and such use cases that do their own reference > counting. It's also used for hacks where people need to look up a desc in > a second spot, (perhaps we can fix those better). > > As I say in commit b0ce7b29bfcd090ddba476f45a75ec0a797b048a > "This solution with a special flag is not entirely elegant and should ideally > be replaced by something more careful as this makes it possible for > several consumers to enable/disable the same GPIO line to the left > and right without any consistency." > > I think for regulators (which is the vast majority using it) it isn't broken > because the regulator reference counting is working. > > So if we solve that problem for reset, we probably should put it in > drivers/gpio/* somewhere so we can reuse the same solution for > regulators and get rid of NONEXCLUSIVE altogether I think? > > The NONEXCLUSIVE stuff was prompted by converting regulators to > gpio descriptors, so it was for the greater good one can say. Or the > lesser evil :( my judgement can be questioned here. I discussed the non-exclusive GPIOs with Bartosz quite a lot, who was Cced since beginning of this patchset, because that was my first approach, which was rejected: https://lore.kernel.org/all/b7aeda24-d638-45b7-8e30-80d287f498f8@sirena.org.uk/ The non-exclusive GPIO was made explicitly for regulators, so it is working fine there, but it is broken everywhere else, where the drivers do not handle it in sane way as regulator core does. To make it working, either GPIO should be enable-count-aware, to which Bartosz was opposing with talks with me, or the subsystem should mimic regulators approach. In some way, my patchset is the second way here - reset framework subsystem being aware of shared GPIO and handles the enable-count, even though it is not using non-exclusive flag. Best regards, Krzysztof
On Wed, Jan 31, 2024 at 02:42:17PM +0100, Linus Walleij wrote: > I guess it may be an issue that regulators are not using Device Tree > exclusively, but also has to deal with a slew of platform_devices:s :/ > IIRC that was one of the reasons why it looks as it does. Also ACPI, and this is a long standing binding so we can't change the ABI for DT. We could potentially use a refcounting mechanism provided by the GPIO core but we'd need to know when the refcount changes from 0 to 1 and back, we need to take other actions (inserting delays and generating notifications) when it does so I'm not sure how exciting it is to factor out the refcount. I think part of the decision making with the current design was that there was likely going to need to be some higher level stuff like that in the users so it wasn't clear that trying to abstract the reference count away in gpiolib was buying us much.
On 29/01/2024 12:52, 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, while "resets" > Devicetree property is missing but there is a "reset-gpios" one, > 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]. > > To avoid creating multiple "reset-gpio" platform devices, store the > Devicetree "reset-gpios" GPIO specifiers used for new devices on a > linked list. Later such Devicetree GPIO specifier (phandle to GPIO > controller, GPIO number and GPIO flags) is used to check if reset > controller for given GPIO was already registered. > > If two devices have conflicting "reset-gpios" property, e.g. with > different ACTIVE_xxx flags, this would allow to spawn two separate > "reset-gpio" devices, where the second would fail probing on busy GPIO > request. > > Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: Sean Anderson <sean.anderson@seco.com> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Are any reviewers comments unresolved or unsatisfied with the discussion? I have impression that discussion bikeschedded a bit towards NONEXCLUSIVE, which was later clarified that NONEXCLUSIVE is not the solution for this problem, but maybe we miss some final Ack? Anyone is here unhappy with this solution? To remind: this is a generic solution solving at least two people's problems, probably more. It does not solve all people's problem, but I doubt anyone has enough of time to satisfy all people... Best regards, Krzysztof
On Mon, Jan 29, 2024 at 12:53 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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, while "resets" > Devicetree property is missing but there is a "reset-gpios" one, > 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]. > > To avoid creating multiple "reset-gpio" platform devices, store the > Devicetree "reset-gpios" GPIO specifiers used for new devices on a > linked list. Later such Devicetree GPIO specifier (phandle to GPIO > controller, GPIO number and GPIO flags) is used to check if reset > controller for given GPIO was already registered. > > If two devices have conflicting "reset-gpios" property, e.g. with > different ACTIVE_xxx flags, this would allow to spawn two separate > "reset-gpio" devices, where the second would fail probing on busy GPIO > request. > > Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: Sean Anderson <sean.anderson@seco.com> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> I can't think of anything better, that is reasonable to ask for. I feel slightly icky about the way the code reaches into gpiolib, and I think regulators should be able to reuse the code, but unfortunately only the day they have no board files left :/ I do feel the core code handling "reset-gpios" could as well have been used to handle "enable-gpios" in regulators, just that the regulator code has more requirements, and would be really hard to rewrite, and deals with descriptors passed in from drivers instead of centralizing it. Like regulators, reset grows core support for handling GPIO for resets which is *long due*, given how common it must be. We really need something like this, and this is certainly elegant enough to do the job. Yours, Linus Walleij
On Mon, Feb 12, 2024 at 9:48 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Jan 29, 2024 at 12:53 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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, while "resets" > > Devicetree property is missing but there is a "reset-gpios" one, > > 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]. > > > > To avoid creating multiple "reset-gpio" platform devices, store the > > Devicetree "reset-gpios" GPIO specifiers used for new devices on a > > linked list. Later such Devicetree GPIO specifier (phandle to GPIO > > controller, GPIO number and GPIO flags) is used to check if reset > > controller for given GPIO was already registered. > > > > If two devices have conflicting "reset-gpios" property, e.g. with > > different ACTIVE_xxx flags, this would allow to spawn two separate > > "reset-gpio" devices, where the second would fail probing on busy GPIO > > request. > > > > Link: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ [1] > > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Cc: Sean Anderson <sean.anderson@seco.com> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > I can't think of anything better, that is reasonable to ask for. > > I feel slightly icky about the way the code reaches into gpiolib, and I think As long as it doesn't include gpiolib.h, I'm fine with it. > regulators should be able to reuse the code, but unfortunately only the day > they have no board files left :/ > > I do feel the core code handling "reset-gpios" could as well have been > used to handle "enable-gpios" in regulators, just that the regulator code > has more requirements, and would be really hard to rewrite, and deals > with descriptors passed in from drivers instead of centralizing it. > > Like regulators, reset grows core support for handling GPIO for resets > which is *long due*, given how common it must be. We really need > something like this, and this is certainly elegant enough to do the job. > > Yours, > Linus Walleij Agreed. Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> I will pick up the stub patches tomorrow and send a tag for Philipp to pull. Bartosz
diff --git a/drivers/reset/core.c b/drivers/reset/core.c index 4d5a78d3c085..dba74e857be6 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -5,14 +5,19 @@ * Copyright 2013 Philipp Zabel, Pengutronix */ #include <linux/atomic.h> +#include <linux/cleanup.h> #include <linux/device.h> #include <linux/err.h> #include <linux/export.h> #include <linux/kernel.h> #include <linux/kref.h> +#include <linux/gpio/driver.h> +#include <linux/gpio/machine.h> +#include <linux/idr.h> #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 +28,11 @@ static LIST_HEAD(reset_controller_list); static DEFINE_MUTEX(reset_lookup_mutex); static LIST_HEAD(reset_lookup_list); +/* Protects reset_gpio_lookup_list */ +static DEFINE_MUTEX(reset_gpio_lookup_mutex); +static LIST_HEAD(reset_gpio_lookup_list); +static DEFINE_IDA(reset_gpio_ida); + /** * struct reset_control - a reset control * @rcdev: a pointer to the reset controller device @@ -63,6 +73,16 @@ struct reset_control_array { struct reset_control *rstc[] __counted_by(num_rstcs); }; +/** + * struct reset_gpio_lookup - lookup key for ad-hoc created reset-gpio devices + * @of_args: phandle to the reset controller with all the args like GPIO number + * @list: list entry for the reset_gpio_lookup_list + */ +struct reset_gpio_lookup { + struct of_phandle_args of_args; + struct list_head list; +}; + static const char *rcdev_name(struct reset_controller_dev *rcdev) { if (rcdev->dev) @@ -71,6 +91,9 @@ static const char *rcdev_name(struct reset_controller_dev *rcdev) if (rcdev->of_node) return rcdev->of_node->full_name; + if (rcdev->of_args) + return rcdev->of_args->np->full_name; + return NULL; } @@ -99,6 +122,9 @@ static int of_reset_simple_xlate(struct reset_controller_dev *rcdev, */ int reset_controller_register(struct reset_controller_dev *rcdev) { + if (rcdev->of_node && rcdev->of_args) + return -EINVAL; + if (!rcdev->of_xlate) { rcdev->of_reset_n_cells = 1; rcdev->of_xlate = of_reset_simple_xlate; @@ -813,12 +839,171 @@ static void __reset_control_put_internal(struct reset_control *rstc) kref_put(&rstc->refcnt, __reset_control_release); } +static int __reset_add_reset_gpio_lookup(int id, struct device_node *np, + unsigned int gpio, + unsigned int of_flags) +{ + const struct fwnode_handle *fwnode = of_fwnode_handle(np); + unsigned int lookup_flags; + const char *label_tmp; + + /* + * Later we map GPIO flags between OF and Linux, however not all + * constants from include/dt-bindings/gpio/gpio.h and + * include/linux/gpio/machine.h match each other. + */ + if (of_flags > GPIO_ACTIVE_LOW) { + pr_err("reset-gpio code does not support GPIO flags %u for GPIO %u\n", + of_flags, gpio); + return -EINVAL; + } + + struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode); + if (!gdev) + return -EPROBE_DEFER; + + label_tmp = gpio_device_get_label(gdev); + if (!label_tmp) + return -EINVAL; + + char *label __free(kfree) = kstrdup(label_tmp, GFP_KERNEL); + if (!label) + return -ENOMEM; + + /* Size: one lookup entry plus sentinel */ + struct gpiod_lookup_table *lookup __free(kfree) = kzalloc(struct_size(lookup, table, 2), + GFP_KERNEL); + if (!lookup) + return -ENOMEM; + + lookup->dev_id = kasprintf(GFP_KERNEL, "reset-gpio.%d", id); + if (!lookup->dev_id) + return -ENOMEM; + + lookup_flags = GPIO_PERSISTENT; + lookup_flags |= of_flags & GPIO_ACTIVE_LOW; + lookup->table[0] = GPIO_LOOKUP(no_free_ptr(label), gpio, "reset", + lookup_flags); + + /* Not freed on success, because it is persisent subsystem data. */ + gpiod_add_lookup_table(no_free_ptr(lookup)); + + return 0; +} + +/* + * @args: phandle to the GPIO provider with all the args like GPIO number + */ +static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) +{ + struct reset_gpio_lookup *rgpio_dev; + struct platform_device *pdev; + int id, ret; + + /* + * Currently only #gpio-cells=2 is supported with the meaning of: + * args[0]: GPIO number + * args[1]: GPIO flags + * TODO: Handle other cases. + */ + if (args->args_count != 2) + return -ENOENT; + + /* + * Registering reset-gpio device might cause immediate + * bind, resulting in its probe() registering new reset controller thus + * taking reset_list_mutex lock via reset_controller_register(). + */ + lockdep_assert_not_held(&reset_list_mutex); + + mutex_lock(&reset_gpio_lookup_mutex); + + list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) { + if (args->np == rgpio_dev->of_args.np) { + if (of_phandle_args_equal(args, &rgpio_dev->of_args)) + goto out; /* Already on the list, done */ + } + } + + id = ida_alloc(&reset_gpio_ida, GFP_KERNEL); + if (id < 0) { + ret = id; + goto err_unlock; + } + + /* Not freed on success, because it is persisent subsystem data. */ + rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL); + if (!rgpio_dev) { + ret = -ENOMEM; + goto err_ida_free; + } + + ret = __reset_add_reset_gpio_lookup(id, args->np, args->args[0], + args->args[1]); + if (ret < 0) + goto err_kfree; + + rgpio_dev->of_args = *args; + /* + * We keep the device_node reference, but of_args.np is put at the end + * of __of_reset_control_get(), so get it one more time. + * Hold reference as long as rgpio_dev memory is valid. + */ + of_node_get(rgpio_dev->of_args.np); + pdev = platform_device_register_data(NULL, "reset-gpio", id, + &rgpio_dev->of_args, + sizeof(rgpio_dev->of_args)); + ret = PTR_ERR_OR_ZERO(pdev); + if (ret) + goto err_put; + + list_add(&rgpio_dev->list, &reset_gpio_lookup_list); + +out: + mutex_unlock(&reset_gpio_lookup_mutex); + + return 0; + +err_put: + of_node_put(rgpio_dev->of_args.np); +err_kfree: + kfree(rgpio_dev); +err_ida_free: + ida_free(&reset_gpio_ida, id); +err_unlock: + mutex_unlock(&reset_gpio_lookup_mutex); + + return ret; +} + +static struct reset_controller_dev *__reset_find_rcdev(const struct of_phandle_args *args, + bool gpio_fallback) +{ + struct reset_controller_dev *rcdev; + + lockdep_assert_held(&reset_list_mutex); + + list_for_each_entry(rcdev, &reset_controller_list, list) { + if (gpio_fallback) { + if (rcdev->of_args && of_phandle_args_equal(args, + rcdev->of_args)) + return rcdev; + } else { + if (args->np == rcdev->of_node) + return rcdev; + } + } + + return NULL; +} + struct reset_control * __of_reset_control_get(struct device_node *node, const char *id, int index, bool shared, bool optional, bool acquired) { + bool gpio_fallback = false; struct reset_control *rstc; - struct reset_controller_dev *r, *rcdev; + struct reset_controller_dev *rcdev; struct of_phandle_args args; int rstc_id; int ret; @@ -839,39 +1024,52 @@ __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) { + if (!IS_ENABLED(CONFIG_RESET_GPIO)) + 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; + /* + * There can be only one reset-gpio for regular devices, so + * don't bother with the "reset-gpios" phandle index. + */ + ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells", + 0, &args); + if (ret) + return optional ? NULL : ERR_PTR(ret); + + gpio_fallback = true; + + ret = __reset_add_reset_gpio_device(&args); + if (ret) { + rstc = ERR_PTR(ret); + goto out_put; } } + mutex_lock(&reset_list_mutex); + rcdev = __reset_find_rcdev(&args, gpio_fallback); if (!rcdev) { rstc = ERR_PTR(-EPROBE_DEFER); - goto out; + goto out_unlock; } if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) { rstc = ERR_PTR(-EINVAL); - goto out; + goto out_unlock; } rstc_id = rcdev->of_xlate(rcdev, &args); if (rstc_id < 0) { rstc = ERR_PTR(rstc_id); - goto out; + goto out_unlock; } /* reset_list_mutex also protects the rcdev's reset_control list */ rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired); -out: +out_unlock: mutex_unlock(&reset_list_mutex); +out_put: of_node_put(args.np); return rstc; diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h index 0fa4f60e1186..357df16ede32 100644 --- a/include/linux/reset-controller.h +++ b/include/linux/reset-controller.h @@ -60,6 +60,9 @@ struct reset_control_lookup { * @reset_control_head: head of internal list of requested reset controls * @dev: corresponding driver model device struct * @of_node: corresponding device tree node as phandle target + * @of_args: for reset-gpios controllers: corresponding phandle args with + * of_node and GPIO number complementing of_node; either this or + * of_node should be present * @of_reset_n_cells: number of cells in reset line specifiers * @of_xlate: translation function to translate from specifier as found in the * device tree to id as given to the reset control ops, defaults @@ -73,6 +76,7 @@ struct reset_controller_dev { struct list_head reset_control_head; struct device *dev; struct device_node *of_node; + const struct of_phandle_args *of_args; int of_reset_n_cells; int (*of_xlate)(struct reset_controller_dev *rcdev, const struct of_phandle_args *reset_spec);