Received: by 2002:a05:7412:98c1:b0:fa:551:50a7 with SMTP id kc1csp1586411rdb; Mon, 8 Jan 2024 04:18:04 -0800 (PST) X-Google-Smtp-Source: AGHT+IEVIw3HVXgaCeluUzpcwrbI32OamiABHaaZfWDuQs/Ewc0q/8SXfnHSbZEmymRwe2AazXxz X-Received: by 2002:a05:622a:60b:b0:429:872a:d37b with SMTP id z11-20020a05622a060b00b00429872ad37bmr4878615qta.83.1704716284452; Mon, 08 Jan 2024 04:18:04 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704716284; cv=none; d=google.com; s=arc-20160816; b=JiC3UWA9taPCpSd3FEXuGoQOSxg7CDuDWYRMLUcYgkVH/dIVKmkVArD0uhIWvxkpiO ZKRQPUlIBdjYVC9gWNWdDUHmj89QcMXPVQKlEMXYGdNcT7XiruPBcUWKzulfabblSoGn YZH4CIh7WFSp42PjHHNnsJgVOYt6k07JHKnVElbd1eaH5Yo1XSnu1+022hlC8aOzqMqt NzD1xG0uh7qqC/bAZ6l8tvrceGkC51k2MU7WRCyRSv7DWY6aF3xQpqB4xm5Ao53UtR4o 3ZsrUcKdPrP5jC4TegnG4XvaT65UoVCK/TaBaMduc/HHMVq8Ef7hyd8F/r+09xXjhht9 wKag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:list-unsubscribe:list-subscribe:list-id:precedence :user-agent:content-transfer-encoding:references:in-reply-to:date:cc :to:from:subject:message-id; bh=wrtAs/zK9GCU909+Fk+xtDQonKLpjLbC4M2EGZH8NNs=; fh=FdO17+oZ4jq+PLfCTpA+5saROc3KCKWKhS9UrdrC7B0=; b=z7+ZnzGOwgqunnR2tBN/l3WOdnw+FLsN9SJwH7iuOMmxfczlffNWpA2fvJ4uIbMHOM 9EULuLvfcFsVeHsBZ42Ym0nHujP+iaFtxf2q1wo1nm/QKfjF2+p7/P0XyUy/wga5zy2n czuVeDns+ewUqg5eBJromYjS9VpEM/0EZSGId+hKLrqCcyNbi7QRzpksfAlVgf6M2CCb dtigPnkVfBWXwz+hO/9lb5yZ7rYBeA5iWYA9frO9Sxpd8Bg3VGUoxCogA8ZcY0foJZZc qa4+3sF6VO8kekaTuKjkWmlM1+ac2EfjeDWm04dErKTfRCeNf02WCj5PEIYrYCQzdHxY phXw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel+bounces-19485-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-19485-linux.lists.archive=gmail.com@vger.kernel.org" Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id i18-20020ac85e52000000b00427e96d51a7si7739890qtx.197.2024.01.08.04.18.04 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 04:18:04 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-19485-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel+bounces-19485-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-19485-linux.lists.archive=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 145C71C21461 for ; Mon, 8 Jan 2024 12:18:04 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 06FA23FE56; Mon, 8 Jan 2024 12:18:01 +0000 (UTC) X-Original-To: linux-kernel@vger.kernel.org Received: from metis.whiteo.stw.pengutronix.de (metis.whiteo.stw.pengutronix.de [185.203.201.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CE3303FE46 for ; Mon, 8 Jan 2024 12:17:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pengutronix.de Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.whiteo.stw.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rMoZR-000761-BP; Mon, 08 Jan 2024 13:17:33 +0100 Received: from [2a0a:edc0:0:900:1d::4e] (helo=lupine) by drehscheibe.grey.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rMoZP-001FLT-Rm; Mon, 08 Jan 2024 13:17:31 +0100 Received: from pza by lupine with local (Exim 4.96) (envelope-from ) id 1rMoZP-0008v2-2U; Mon, 08 Jan 2024 13:17:31 +0100 Message-ID: Subject: Re: [PATCH v2 2/4] reset: Instantiate reset GPIO controller for shared reset-gpios From: Philipp Zabel To: Krzysztof Kozlowski , Srinivas Kandagatla , Banajit Goswami , Bjorn Andersson , Konrad Dybcio , Liam Girdwood , Mark Brown , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jaroslav Kysela , Takashi Iwai , alsa-devel@alsa-project.org, linux-arm-msm@vger.kernel.org, linux-sound@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Bartosz Golaszewski , Sean Anderson Date: Mon, 08 Jan 2024 13:17:31 +0100 In-Reply-To: <20240105155918.279657-3-krzysztof.kozlowski@linaro.org> References: <20240105155918.279657-1-krzysztof.kozlowski@linaro.org> <20240105155918.279657-3-krzysztof.kozlowski@linaro.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.46.4-2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org 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. >=20 > 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= ]. >=20 > 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 > Cc: Sean Anderson > Signed-off-by: Krzysztof Kozlowski > --- > drivers/reset/core.c | 176 ++++++++++++++++++++++++++++--- > include/linux/reset-controller.h | 4 + > 2 files changed, 167 insertions(+), 13 deletions(-) >=20 > 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 > #include > #include > +#include > #include > #include > #include > @@ -23,6 +24,10 @@ static LIST_HEAD(reset_controller_list); > static DEFINE_MUTEX(reset_lookup_mutex); > static LIST_HEAD(reset_lookup_list); > =20 > +/* 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); > }; > =20 > +/** > + * 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 re= set_control *rstc) > kref_put(&rstc->refcnt, __reset_control_release); > } > =20 > +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 !=3D a2->args_count) > + return false; > + > + for (i =3D 0; i < a1->args_count; i++) > + if (a1->args[i] !=3D a2->args[i]) > + break; Just return false in the loop and simplify the following to return true. > + > + /* All args matched? */ > + if (i =3D=3D 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 G= PIO 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 =3D=3D rgpio_dev->of_args.np) { > + if (__reset_gpios_args_match(args, > + &rgpio_dev->of_args)) { > + ret =3D 0; > + goto out_unlock; > + } > + } > + } > + > + /* Not freed in normal path, persisent subsyst data */ > + rgpio_dev =3D 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 =3D -ENOMEM; > + goto out_unlock; > + } > + > + rgpio_dev->of_args =3D *args; > + pdev =3D platform_device_register_data(NULL, "reset-gpio", > + PLATFORM_DEVID_AUTO, &node, > + sizeof(node)); > + ret =3D 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 =3D NULL; > + list_for_each_entry(r, &reset_controller_list, list) { > + if (args->np =3D=3D 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] =3D 0; > + args->args_count =3D 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 =3D r; > + break; > + } > + } else { > + rcdev =3D r; > + break; > + } > + } > + } > + > + return rcdev; > +} > + > struct reset_control * > __of_reset_control_get(struct device_node *node, const char *id, int ind= ex, > bool shared, bool optional, bool acquired) > { > + struct of_phandle_args args =3D {0}; > + bool gpio_fallback =3D 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; > =20 > @@ -839,21 +960,50 @@ __of_reset_control_get(struct device_node *node, co= nst char *id, int index, > index, &args); > if (ret =3D=3D -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 =3D of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells", > + 0, &args); > + if (ret) > + return optional ? NULL : ERR_PTR(ret); > =20 > - mutex_lock(&reset_list_mutex); > - rcdev =3D NULL; > - list_for_each_entry(r, &reset_controller_list, list) { > - if (args.np =3D=3D r->of_node) { > - rcdev =3D r; > - break; > - } > + gpio_fallback =3D 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. > } > =20 > + mutex_lock(&reset_list_mutex); > + rcdev =3D __reset_find_rcdev(&args, gpio_fallback, NULL); This gets called with args as parsed. If there is a match, this will=20 overwrite args (in the gpio_fallback case) and return NULL. > + > if (!rcdev) { > - rstc =3D 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 =3D __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 =3D ERR_PTR(ret); > + goto out; > + } > + /* > + * Success: reset-gpio could probe immediately, so > + * re-check the lookup. > + */ > + rcdev =3D __reset_find_rcdev(&args, gpio_fallback, NULL); And this will again be called with args as parsed and overwrite args again. > + if (!rcdev) { > + rstc =3D ERR_PTR(-EPROBE_DEFER); > + goto out; > + } > + /* Success, rcdev is valid thus do not bail out */ > + } else { > + rstc =3D 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