| Message ID | 20240124074527.48869-1-krzysztof.kozlowski@linaro.org |
|---|---|
| Headers | show |
| Series | reset: gpio: ASoC: shared GPIO resets | expand |
On 24/01/2024 08:45, 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Depends on previous of change. > --- > drivers/reset/core.c | 215 +++++++++++++++++++++++++++++-- > include/linux/reset-controller.h | 4 + > 2 files changed, 206 insertions(+), 13 deletions(-) > LKP reported issue when building !GPIOLIB: https://lore.kernel.org/oe-kbuild-all/202401250958.YksQmnWj-lkp@intel.com/ but I intend to solve it providing the stubs. Therefore this patch will not change. Best regards, Krzysztof
On Thu, Jan 25, 2024 at 9:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 24/01/2024 08:45, 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> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > --- > > > > Depends on previous of change. > > --- > > drivers/reset/core.c | 215 +++++++++++++++++++++++++++++-- > > include/linux/reset-controller.h | 4 + > > 2 files changed, 206 insertions(+), 13 deletions(-) > > > > LKP reported issue when building !GPIOLIB: > https://lore.kernel.org/oe-kbuild-all/202401250958.YksQmnWj-lkp@intel.com/ > > but I intend to solve it providing the stubs. Therefore this patch will > not change. > > Best regards, > Krzysztof > Ah, so this is why you sent the patches. I don't like stubs in gpio/driver.h but I get why they're needed here. Maybe we should consider adding gpio/misc.h for that kind of stuff. Bart
On Mi, 2024-01-24 at 16:07 -0600, Rob Herring wrote: > On Wed, Jan 24, 2024 at 08:45:22AM +0100, Krzysztof Kozlowski wrote: > > Add a helper comparing two "struct of_phandle_args" to avoid > > reinventing the wheel. > > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > > > Dependency of cpufreq and reset change. > > --- > > include/linux/of.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > Acked-by: Rob Herring <robh@kernel.org> > > I've wanted to write this series for some time. Great work. How would you prefer this to be merged? Stable tag from the devicetree tree? Go through the reset tree as part of this series? regards Philipp
On Mi, 2024-01-24 at 08:45 +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, 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> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> regards Philipp
On 25/01/2024 09:27, neil.armstrong@linaro.org wrote: > On 24/01/2024 08:45, 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> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Depends on previous of change. >> --- >> drivers/reset/core.c | 215 +++++++++++++++++++++++++++++-- >> include/linux/reset-controller.h | 4 + >> 2 files changed, 206 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index 4d5a78d3c085..60a8a33c4419 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c > > <snip> > >> + } >> + >> + ret = __reset_add_reset_gpio_lookup(id, args->np, args->args[0], >> + args->args[1]); > > What would happen with gpio controllers using #gpio-cells = <3> (or more) like allwinner,sun4i-a10-pinctrl.yaml ? > > On this example the flags are args->args[2] so this would probably fail. > > This would also fails badly with #gpio-cells = <1>, args->args[1] value would be undefined. > > You should probably limit to args->args_count == 2 for now. Hm, good point. Both cells are actually possible, so I need to check it. Thanks. Best regards, Krzysztof
Hi, Patch #2 (cpufreq: do not open-code of_phandle_args_equal()) and patch #4 (reset: Instantiate reset GPIO controller for shared reset-gpios) depend on OF change (patch #1). Changes in v5 ============= 1. Minor comments from Philipp: missing cleanup.h, correct indentation of pr_err(), shorten gpio_device_find_by_fwnode() line. 2. Acks/Rbs. Changes in v4 ============= 1. New patches: of: add of_phandle_args_equal() helper cpufreq: do not open-code of_phandle_args_equal() 2. reset-gpio.c: - Drop unneeded comment (Bartosz), add Rb tag. - Do not assign of_node. 3. reset/core.c: - Implement most of Bartosz feedback (I responded to one which I did not implement) and comments from Philipp. - Expect either rcdev->of_args or rcdev->of_node. - Drop __reset_gpios_args_match() and use common helper (Philipp). - Move declarations of automatic-cleanup variables in __reset_add_reset_gpio_lookup() to place of use (Bartosz). - Separate gpio_device_get_label() and kstrdup() (Philipp). - Correct doc for __reset_add_reset_gpio_device(), rewrite few comments. - Drop unneeded "r" variable in __reset_find_rcdev() (Philipp). - Drop of_phandle_args initialization in __of_reset_control_get (Philipp). - Check if CONFIG_RESET_GPIO is enabled before trying to look up reset-gpios. 4. Drop Chris' patch: "i2c: muxes: pca954x: Allow sharing reset GPIO", because discussion is on going. Changes in v3 ============= 1. reset-gpio.c: - Add reset_gpio_of_xlate (Philipp). - reset_gpio_of_args_put->reset_gpio_of_node_put (Philipp). - Expect via platdata of_phandle_args. - Do not call device_set_node() to attach itself to reset consumer (the final device). This was questionable idea in the first place. Bartosz suggested to use GPIO_LOOKUP to solve this. 2. reset/core.c, implement Philipp's feedback. That was a lot: - Commit msg fixes. - Add new platform_device earlier, when reset core found "reset-gpios" but not "resets". - Do not overwrite of_phandle_args. - Expect matching .of_reset_n_cells. - Pass of_phandle_args as platdata to reset-gpio. - Rename reset_gpio_device->reset_gpio_lookup and others. Fix few comments and code cleanup pointed on review. - From Bartosz: Use GPIO_LOOKUP and a lot of cleanup.h in __reset_add_reset_gpio_lookup(). 3. Include here Chris' patch: "i2c: muxes: pca954x: Allow sharing reset GPIO". Changes in v2 ============= 1. wsa884x.c: add missing return in wsa884x_get_reset(), correct comment. 2. qcom,wsa8840.yaml: fix oneOf syntax. 3. reset-gpio.c: - Fix smatch warning on platdata evaluation. - Parse GPIO args and store them in rc.of_args. 4. reset/core.c: - Revise approach based on Bartosz comments: parse the reset-gpios phandle with arguments, do not use deprecated API and do not rely on gpio_desc pointer. - Create a list of instantiated platform devices to avoid any duplicates. - After creating reset-gpio platform device, try to get new reset controller or return EPROBE_DEFER. - Drop the "cookie" member and add new "of_args" to "struct reset_controller_dev". Description =========== We have at least few cases where hardware engineers decided to use one powerdown/shutdown/reset GPIO line for multiple devices: 1. WSA884x (this and previous patch): https://lore.kernel.org/all/b7aeda24-d638-45b7-8e30-80d287f498f8@sirena.org.uk/ 2. https://lore.kernel.org/all/20231027033104.1348921-1-chris.packham@alliedtelesis.co.nz/ 3. https://lore.kernel.org/lkml/20191030120440.3699-1-peter.ujfalusi@ti.com/ 4. https://lore.kernel.org/all/20211018234923.1769028-1-sean.anderson@seco.com/ 5. https://social.treehouse.systems/@marcan/111268780311634160 I try to solve my case, hopefuly Chris' (2), partially Sean's (4) and maybe Hectors (5), using Rob's suggestion: https://lore.kernel.org/all/YXi5CUCEi7YmNxXM@robh.at.kernel.org/ Best regards, Krzysztof Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> Cc: Bartosz Golaszewski <brgl@bgdev.pl> Cc: Sean Anderson <sean.anderson@seco.com> Krzysztof Kozlowski (6): of: Add of_phandle_args_equal() helper cpufreq: do not open-code of_phandle_args_equal() reset: gpio: Add GPIO-based reset controller reset: Instantiate reset GPIO controller for shared reset-gpios ASoC: dt-bindings: qcom,wsa8840: Add reset-gpios for shared line ASoC: codecs: wsa884x: Allow sharing reset GPIO .../bindings/sound/qcom,wsa8840.yaml | 11 +- MAINTAINERS | 5 + drivers/reset/Kconfig | 9 + drivers/reset/Makefile | 1 + drivers/reset/core.c | 215 ++++++++++++++++-- drivers/reset/reset-gpio.c | 119 ++++++++++ include/linux/cpufreq.h | 3 +- include/linux/of.h | 16 ++ include/linux/reset-controller.h | 4 + sound/soc/codecs/wsa884x.c | 53 ++++- 10 files changed, 410 insertions(+), 26 deletions(-) create mode 100644 drivers/reset/reset-gpio.c