From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 C24A122D4DC; Fri, 25 Apr 2025 07:09:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745564958; cv=none; b=k1blH5TxLtWiLeXfcD0GWRIILT+puVCiPmq96pMyp+RmoBA0AL2FYx9Ea4LBBP0/B70eqkjgDZDileNr+dDbuxVEp+fSc7DQ25tq5VQiCdVv84/r5ocstFlwmFW4lA1lZk/edXckAZTUSLl68BomY1KJTsCfVyIfC/eV4CNnvZU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1745564958; c=relaxed/simple; bh=XRJaobu20wNxHoup9s4R3YDQJkYv5i9HIMk9O25Dr6k=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=gUPVdwd65aq/AdAe4L598+Ip8yRxYfTerbHqwp5usmyCDCYE+pYZvndFzClr2g6br8OQEaDHEsli9OMuraX/6cPA51P1dqyjCBXeZyCyR2N/9cyfpgrv0JFbHKlwdZ/MQ3rZIUmYhaqkt0UojvERFX9LE8TQ5kTuOoQqsBVTvr8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=WfgEML46; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="WfgEML46" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B205AC4CEE4; Fri, 25 Apr 2025 07:09:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1745564958; bh=XRJaobu20wNxHoup9s4R3YDQJkYv5i9HIMk9O25Dr6k=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WfgEML463b0+bWtPIloy53Rqep5I1ojunbyxFP+bzc9GS2sAG8LRFar6fOcW3siuO IgP7K5EsZ6MZphcoA07kXQzRZG8ouQRTz9x1UlLLGL9Dy8KrEWrGdfsV01I8QjXWlm FErYl8AxCDHubZU1yT+tw3NHF9OveIHBZkpmYiIKVxsQkAMDqoU0BPZW/fLePr3Dpb JH+SnSZAIbQ3bSNGeGpOwudvdgH6b8FmITLdvJKt7L88ybptMF2UYs3N/bRXnEZ2n8 e8pBVM32tAJLS1iedQXy1EGxWOOJtqFV/4Zhla839fffv+QnMDOjaIDiRjAtkYN1uG +KO9/gzMUiDZA== Date: Fri, 25 Apr 2025 09:09:15 +0200 From: Maxime Ripard To: Ulf Hansson Cc: Michal Wilczynski , Stephen Boyd , "Rafael J. Wysocki" , Danilo Krummrich , Pavel Machek , Drew Fustini , Guo Ren , Fu Wei , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Philipp Zabel , Frank Binns , Matt Coster , Maarten Lankhorst , Thomas Zimmermann , David Airlie , Simona Vetter , m.szyprowski@samsung.com, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 1/4] PM: device: Introduce platform_resources_managed flag Message-ID: <20250425-lumpy-marmot-of-popularity-cdbbcd@houat> References: <20250414-apr_14_for_sending-v2-0-70c5af2af96c@samsung.com> <20250414-apr_14_for_sending-v2-1-70c5af2af96c@samsung.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha384; protocol="application/pgp-signature"; boundary="vw35xu6fptfqmd4m" Content-Disposition: inline In-Reply-To: --vw35xu6fptfqmd4m Content-Type: text/plain; protected-headers=v1; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH v2 1/4] PM: device: Introduce platform_resources_managed flag MIME-Version: 1.0 Hi, On Thu, Apr 24, 2025 at 06:51:00PM +0200, Ulf Hansson wrote: > On Thu, 17 Apr 2025 at 18:19, Michal Wilczynski > wrote: > > On 4/16/25 16:48, Rafael J. Wysocki wrote: > > > On Wed, Apr 16, 2025 at 3:32=E2=80=AFPM Michal Wilczynski > > > wrote: > > >> > > >> On 4/15/25 18:42, Rafael J. Wysocki wrote: > > >>> On Mon, Apr 14, 2025 at 8:53=E2=80=AFPM Michal Wilczynski > > >>> wrote: > > >>>> > > >>>> Introduce a new dev_pm_info flag - platform_resources_managed, to > > >>>> indicate whether platform PM resources such as clocks or resets are > > >>>> managed externally (e.g. by a generic power domain driver) instead= of > > >>>> directly by the consumer device driver. > > >>> > > >>> I think that this is genpd-specific and so I don't think it belongs= in > > >>> struct dev_pm_info. > > >>> > > >>> There is dev->power.subsys_data->domain_data, why not use it for th= is? > > >> > > >> Hi Rafael, > > >> > > >> Thanks for the feedback. > > >> > > >> You're right =E2=80=94 this behavior is specific to genpd, so embedd= ing the flag > > >> directly in struct dev_pm_info may not be the best choice. Using > > >> dev->power.subsys_data->domain_data makes more sense and avoids bloa= ting > > >> the core PM structure. > > >> > > >>> > > >>> Also, it should be documented way more comprehensively IMV. > > >>> > > >>> Who is supposed to set it and when? What does it mean when it is s= et? > > >> > > >> To clarify the intended usage, I would propose adding the following > > >> explanation to the commit message: > > >> > > >> "This flag is intended to be set by a generic PM domain driver (e.g., > > >> from within its attach_dev callback) to indicate that it will manage > > >> platform specific runtime power management resources =E2=80=94 such = as clocks > > >> and resets =E2=80=94 on behalf of the consumer device. This implies = a delegation > > >> of runtime PM control to the PM domain, typically implemented through > > >> its start and stop callbacks. > > >> > > >> When this flag is set, the consumer driver (e.g., drm/imagination) c= an > > >> check it and skip managing such resources in its runtime PM callbacks > > >> (runtime_suspend, runtime_resume), avoiding conflicts or redundant > > >> operations." > > > > > > This sounds good and I would also put it into a code comment somewher= e. > > > > > > I guess you'll need helpers for setting and testing this flag, so > > > their kerneldoc comments can be used for that. > > > > > >> This could also be included as a code comment near the flag definiti= on > > >> if you think that=E2=80=99s appropriate. > > >> > > >> Also, as discussed earlier with Maxime and Matt [1], this is not abo= ut > > >> full "resource ownership," but more about delegating runtime control= of > > >> PM resources like clocks/resets to the genpd. That nuance may be wor= th > > >> reflecting in the flag name as well, I would rename it to let's say > > >> 'runtime_pm_platform_res_delegated', or more concise > > >> 'runtime_pm_delegated'. > > > > > > Or just "rpm_delegated" I suppose. > > > > > > But if the genpd driver is going to set that flag, it will rather mean > > > that this driver will now control the resources in question, so the > > > driver should not attempt to manipulate them directly. Is my > > > understanding correct? > > > > Yes, your understanding is correct =E2=80=94 with one minor clarificati= on. > > > > When the genpd driver sets the flag, it indicates that it will take over > > control of the relevant PM resources in the context of runtime PM, i.e., > > via its start() and stop() callbacks. As a result, the device driver > > should not manipulate those resources from within its RUNTIME_PM_OPS > > (e.g., runtime_suspend, runtime_resume) to avoid conflicts. > > > > However, outside of the runtime PM callbacks, the consumer device driver > > may still access or use those resources if needed e.g for devfreq. > > > > > > > > Assuming that it is correct, how is the device driver going to know > > > which resources in particular are now controlled by the genpd driver? > > > > Good question =E2=80=94 to allow finer-grained control, we could replac= e the > > current single boolean flag with a u32 bitmask field. Each bit would > > correspond to a specific category of platform managed resources. For > > example: > > > > #define RPM_TAKEOVER_CLK BIT(0) > > #define RPM_TAKEOVER_RESET BIT(1) > > > > This would allow a PM domain driver to selectively declare which > > resources it is taking over and let the consumer driver query only the > > relevant parts. >=20 > Assuming we are targeting device specific resources for runtime PM; > why would we want the driver to be responsible for some resources and > the genpd provider for some others? I would assume we want to handle > all these RPM-resources from the genpd provider, if/when possible, > right? >=20 > The tricky part though (maybe Stephen had some ideas in his talk [a] > at OSS), is to teach the genpd provider about what resources it should > handle. In principle the genpd provider will need some kind of device > specific knowledge, perhaps based on the device's compatible-string > and description in DT. >=20 > My point is, using a bitmask doesn't scale as it would end up having > one bit for each clock (a device may have multiple clocks), regulator, > pinctrl, phy, etc. In principle, reflecting the description in DT. My understanding is that it's to address a situation where a "generic" driver interacts with some platform specific code. I think it's tied to the discussion with the imagination GPU driver handling his clocks, and the platform genpd clocks overlapping a bit. But then, my question is: does it matter? clocks are refcounted, and resets are as well iirc, so why do we need a transition at all? Can't we just let the platform genpd code take a reference on the clock, the GPU driver take one as well, and it's all good, right? Maxime --vw35xu6fptfqmd4m Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iJUEABMJAB0WIQTkHFbLp4ejekA/qfgnX84Zoj2+dgUCaAs1GwAKCRAnX84Zoj2+ do8sAX9FbcAEu04m80R/QWM9SUbClB57KbU/Zbb/SrxS41bJSYzLDFQbballhRWa UWhmg2YBfRcskqou4btHfDm4ChKCQ7giQfUQ0IOm9WRVcNZTCKQJvrz64wvKsA7C NKJ4T4yLqA== =loki -----END PGP SIGNATURE----- --vw35xu6fptfqmd4m--