Message ID | 20240108081834.408403-1-treapking@chromium.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:37c1:b0:101:2151:f287 with SMTP id y1csp890062dyq; Mon, 8 Jan 2024 00:19:09 -0800 (PST) X-Google-Smtp-Source: AGHT+IFXLvH7wrOnKWPOfubf7RjXhrDoL0lS9En7Tr3yAMBKhPnpi1q58HvoZOAEAyJcuEVMTWtL X-Received: by 2002:a05:6e02:3101:b0:35f:aba9:1031 with SMTP id bg1-20020a056e02310100b0035faba91031mr6303768ilb.126.1704701948922; Mon, 08 Jan 2024 00:19:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704701948; cv=none; d=google.com; s=arc-20160816; b=iInLk4KCnRCZFIrscRcAL3J7Lnyi82t4W+mkCj++umJ82YU1e2eEpZjSPinVBENmAo Zjm/glbg6fIL0Nv+NppOK0lP9IvvG1QbpYSXOhlFhWjIPQ6cLKHu3dCZo/ldiYMZmH2f 7NdM1e6kG5k2hHPpYXOA34HF+4sztKFDnc+iZKvgWaIL8COFUuCG+8YxmG6eDbyBNasw U3Esm4zOaJVDCWEaRJ9FAydbGv0q5CTNmi3P/Aj8YXTmJyVP4hGvnXm2tkTp5Aj9fyrD +Rrn5BRRNpUBaPjw4ox9wEw++mOab5exs0GwEIoH5S0IkAJQF94PALvc207eBoa3b5/h 2ijQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=xNB8eseBc6is+G1DowRroY47UdkWdAr6xj971vFYoJE=; fh=fPdquzm+9LuuJ1J/Hwd5PA6zjMSXShuxeZpvBr14aW4=; b=rPcnHMt1NxRr/X4wqyV+CW6wtEAQiFwGnQfIHKA1elMT2M+d6UdawfShvJZZd+CFCs 2rR3UB7Fqs0NAJxTUj7Hl494lpeJWolq+EwxLQmpuaRQszqJNZ2/a8hKsb6OW2DUVo3X zNwV0n0AXfL2uZ4e2hp4RIUyTbDPpHOtasMKV99m3TrbM540k6JR0i1W0XPXaVDOH43j RRJlSAaRXQxl8dvxIT3uf1ipi0nqOqU89Z7UB/el4klBWKwoQOpSSKp4SRPn0Qhczo66 Usorsvv/RuQEcpzwq7cTJ+zb5ZNE9uMz8qTkC7syWPyiHE0rKOQuELZZo6OCyrThfIHv vdyg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=FndB4o7Z; spf=pass (google.com: domain of linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [147.75.48.161]) by mx.google.com with ESMTPS id b30-20020a63715e000000b005ce02d8ef08si5688324pgn.884.2024.01.08.00.19.08 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 00:19:08 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=FndB4o7Z; spf=pass (google.com: domain of linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) smtp.mailfrom="linux-kernel+bounces-19176-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.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 sy.mirrors.kernel.org (Postfix) with ESMTPS id E6460B213B7 for <ouuuleilei@gmail.com>; Mon, 8 Jan 2024 08:19:01 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 31E68BE4C; Mon, 8 Jan 2024 08:18:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="FndB4o7Z" X-Original-To: linux-kernel@vger.kernel.org Received: from mail-pl1-f180.google.com (mail-pl1-f180.google.com [209.85.214.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5ED7BB661 for <linux-kernel@vger.kernel.org>; Mon, 8 Jan 2024 08:18:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Received: by mail-pl1-f180.google.com with SMTP id d9443c01a7336-1d4df66529bso2674505ad.1 for <linux-kernel@vger.kernel.org>; Mon, 08 Jan 2024 00:18:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1704701920; x=1705306720; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=xNB8eseBc6is+G1DowRroY47UdkWdAr6xj971vFYoJE=; b=FndB4o7ZuxG/C0uqLE3o6CFfYSHJd3myJpCrSCU1PxzFB0CLR7UauvbOsjZ1fKusEw 4lbZXn5nGRjNT5x0/P2sOfOlERYYg7yik6PHV+3W51QUS4nUNOUiX5LiH8rud1noDK61 I4iZZHhcW5UkB7wONwtHUwF+5HJ6hfWTFyMy8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704701920; x=1705306720; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=xNB8eseBc6is+G1DowRroY47UdkWdAr6xj971vFYoJE=; b=WFuwKsTqDenafzvI19Za+iPR4tg3wncUZWCYWyyHAfHmt3mnCu8U+zvbA+yyL/VMdb UCNnzJUv/+ZsRRl9AA3fVjNdc24ubwRHmekO3F7vqS/Y3DAq0Af8NCy/CuD5+46r0FK1 SVOVmVwP/68UqDJKZ+9XwYUyZoq/Qv8DVYyzfP4XwCwIDu3Yp3wV3lTWPhohYoWeNzoV xT6byQsquqIqT/ANTgCKcwYP0rYhcGfZg+nTruZjJNBqFrLOZqqTImz03OS7r7Ap61vo 7dGLNoQnHKO9QNn2xO2EiZfoqnV6Lw1U+tvkfK2gtpkjNw6ywi4qbCU+9XdVU2KDnXwu KGdA== X-Gm-Message-State: AOJu0YwejYiNkb9RvElB8CfsjV8rc30WxyebBTmsod2dlIv6KunFKMYr cbecwHUl0T263W5ri18ITOWxUvesVE22 X-Received: by 2002:a17:902:8a88:b0:1d4:be64:263f with SMTP id p8-20020a1709028a8800b001d4be64263fmr1076115plo.120.1704701920600; Mon, 08 Jan 2024 00:18:40 -0800 (PST) Received: from treapking.tpe.corp.google.com ([2401:fa00:1:10:6859:f8da:3370:7a74]) by smtp.gmail.com with ESMTPSA id jj4-20020a170903048400b001d078445059sm5672513plb.143.2024.01.08.00.18.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 00:18:40 -0800 (PST) From: Pin-yen Lin <treapking@chromium.org> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Matthias Brugger <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cc: Pin-yen Lin <treapking@chromium.org>, Chen-Yu Tsai <wenst@chromium.org>, Weiyi Lu <weiyi.lu@mediatek.com>, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/2] clk: mediatek: Introduce need_pm_runtime to mtk_clk_desc Date: Mon, 8 Jan 2024 16:18:15 +0800 Message-ID: <20240108081834.408403-1-treapking@chromium.org> X-Mailer: git-send-email 2.43.0.472.g3155946c3a-goog Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787509550588218646 X-GMAIL-MSGID: 1787509550588218646 |
Series |
[v3,1/2] clk: mediatek: Introduce need_pm_runtime to mtk_clk_desc
|
|
Commit Message
Pin-yen Lin
Jan. 8, 2024, 8:18 a.m. UTC
Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
this clock controller needs runtime PM for its operations.
Also do a runtime PM get on the clock controller during the
probing stage to workaround a possible deadlock.
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---
Changes in v3:
- Update the commit message and the comments before runtime PM call
Changes in v2:
- Fix the order of error handling
- Update the commit message and add a comment before the runtime PM call
drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
drivers/clk/mediatek/clk-mtk.h | 2 ++
2 files changed, 21 insertions(+)
Comments
On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote: > > Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate > this clock controller needs runtime PM for its operations. > Also do a runtime PM get on the clock controller during the > probing stage to workaround a possible deadlock. > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> The patch itself looks fine. Besides the MT8183 MFG clock issues, we do actually need this for the MT8192 ADSP clock. Its power domain is not enabled by default. > --- > > Changes in v3: > - Update the commit message and the comments before runtime PM call > > Changes in v2: > - Fix the order of error handling > - Update the commit message and add a comment before the runtime PM call > > drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++ > drivers/clk/mediatek/clk-mtk.h | 2 ++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c > index 2e55368dc4d8..ba1d1c495bc2 100644 > --- a/drivers/clk/mediatek/clk-mtk.c > +++ b/drivers/clk/mediatek/clk-mtk.c > @@ -13,6 +13,7 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/slab.h> > > #include "clk-mtk.h" > @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM; > } > > + > + if (mcd->need_runtime_pm) { > + devm_pm_runtime_enable(&pdev->dev); > + /* > + * Do a pm_runtime_resume_and_get() to workaround a possible > + * deadlock between clk_register() and the genpd framework. > + */ > + r = pm_runtime_resume_and_get(&pdev->dev); > + if (r) > + return r; > + } > + > /* Calculate how many clk_hw_onecell_data entries to allocate */ > num_clks = mcd->num_clks + mcd->num_composite_clks; > num_clks += mcd->num_fixed_clks + mcd->num_factor_clks; > @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > goto unregister_clks; > } > > + if (mcd->need_runtime_pm) > + pm_runtime_put(&pdev->dev); > + > return r; > > unregister_clks: > @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > free_base: > if (mcd->shared_io && base) > iounmap(base); > + > + if (mcd->need_runtime_pm) > + pm_runtime_put(&pdev->dev); > return r; > } > > diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h > index 22096501a60a..c17fe1c2d732 100644 > --- a/drivers/clk/mediatek/clk-mtk.h > +++ b/drivers/clk/mediatek/clk-mtk.h > @@ -237,6 +237,8 @@ struct mtk_clk_desc { > > int (*clk_notifier_func)(struct device *dev, struct clk *clk); > unsigned int mfg_clk_idx; > + > + bool need_runtime_pm; > }; > > int mtk_clk_pdev_probe(struct platform_device *pdev); > -- > 2.43.0.472.g3155946c3a-goog >
On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > > On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote: > >> > >> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate > >> this clock controller needs runtime PM for its operations. > >> Also do a runtime PM get on the clock controller during the > >> probing stage to workaround a possible deadlock. > >> > >> Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > > > The patch itself looks fine. > > > > Besides the MT8183 MFG clock issues, we do actually need this for the > > MT8192 ADSP clock. Its power domain is not enabled by default. > > > > ...but on MT8195 the ADSP clock works - because the ADSP node exists. That's an indirect dependency that should not be relied on. Say the clock driver probed but the ADSP hasn't, and you try to read out the current status. What would happen? - Read out works fine, because the power domain is default on, and hasn't been turned off by late cleanup - Read out is bogus (but you can't tell) - Read out hangs. The third is what happens on MT8192. There's still some issues on that front, as even after I applied the ADSP power domain patches from MediaTek, the readout was still hanging. > This poses a question: should we make clock controllers depend on power domains, > or should we keep everything powered off (hence clocks down - no power consumption) > *unless* the user exists? That's a policy discussion separate from actual hardware dependencies. *If* the clock controller needs the power domain to be active for the registers to be accessed, the clock controller *must* have a direct dependency on the power domain. > For the second one, this means that the *device* gets the power domain (adsp), and > not the clock controller (which clocks are effectively useless if there's no user). No. See my previous paragraph. ChenYu > Angelo > > >> --- > >> > >> Changes in v3: > >> - Update the commit message and the comments before runtime PM call > >> > >> Changes in v2: > >> - Fix the order of error handling > >> - Update the commit message and add a comment before the runtime PM call > >> > >> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++ > >> drivers/clk/mediatek/clk-mtk.h | 2 ++ > >> 2 files changed, 21 insertions(+) > >> > >> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c > >> index 2e55368dc4d8..ba1d1c495bc2 100644 > >> --- a/drivers/clk/mediatek/clk-mtk.c > >> +++ b/drivers/clk/mediatek/clk-mtk.c > >> @@ -13,6 +13,7 @@ > >> #include <linux/of.h> > >> #include <linux/of_address.h> > >> #include <linux/platform_device.h> > >> +#include <linux/pm_runtime.h> > >> #include <linux/slab.h> > >> > >> #include "clk-mtk.h" > >> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM; > >> } > >> > >> + > >> + if (mcd->need_runtime_pm) { > >> + devm_pm_runtime_enable(&pdev->dev); > >> + /* > >> + * Do a pm_runtime_resume_and_get() to workaround a possible > >> + * deadlock between clk_register() and the genpd framework. > >> + */ > >> + r = pm_runtime_resume_and_get(&pdev->dev); > >> + if (r) > >> + return r; > >> + } > >> + > >> /* Calculate how many clk_hw_onecell_data entries to allocate */ > >> num_clks = mcd->num_clks + mcd->num_composite_clks; > >> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks; > >> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >> goto unregister_clks; > >> } > >> > >> + if (mcd->need_runtime_pm) > >> + pm_runtime_put(&pdev->dev); > >> + > >> return r; > >> > >> unregister_clks: > >> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >> free_base: > >> if (mcd->shared_io && base) > >> iounmap(base); > >> + > >> + if (mcd->need_runtime_pm) > >> + pm_runtime_put(&pdev->dev); > >> return r; > >> } > >> > >> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h > >> index 22096501a60a..c17fe1c2d732 100644 > >> --- a/drivers/clk/mediatek/clk-mtk.h > >> +++ b/drivers/clk/mediatek/clk-mtk.h > >> @@ -237,6 +237,8 @@ struct mtk_clk_desc { > >> > >> int (*clk_notifier_func)(struct device *dev, struct clk *clk); > >> unsigned int mfg_clk_idx; > >> + > >> + bool need_runtime_pm; > >> }; > >> > >> int mtk_clk_pdev_probe(struct platform_device *pdev); > >> -- > >> 2.43.0.472.g3155946c3a-goog > >> > > >
On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: > > On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > >>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote: > >>>> > >>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate > >>>> this clock controller needs runtime PM for its operations. > >>>> Also do a runtime PM get on the clock controller during the > >>>> probing stage to workaround a possible deadlock. > >>>> > >>>> Signed-off-by: Pin-yen Lin <treapking@chromium.org> > >>> > >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > >>> > >>> The patch itself looks fine. > >>> > >>> Besides the MT8183 MFG clock issues, we do actually need this for the > >>> MT8192 ADSP clock. Its power domain is not enabled by default. > >>> > >> > >> ...but on MT8195 the ADSP clock works - because the ADSP node exists. > > > > That's an indirect dependency that should not be relied on. Say the clock > > driver probed but the ADSP hasn't, and you try to read out the current > > status. What would happen? > > > > - Read out works fine, because the power domain is default on, and hasn't > > been turned off by late cleanup > > - Read out is bogus (but you can't tell) > > - Read out hangs. > > > > The third is what happens on MT8192. There's still some issues on that > > front, as even after I applied the ADSP power domain patches from MediaTek, > > the readout was still hanging. > > > > That MT8192 lockup story is getting crazy in my head... anyway, besides that, > I get the point - I was somehow ignoring the fact that kernel modules do exist. > > Eh, sorry about that :-) > > >> This poses a question: should we make clock controllers depend on power domains, > >> or should we keep everything powered off (hence clocks down - no power consumption) > >> *unless* the user exists? > > > > That's a policy discussion separate from actual hardware dependencies. > > *If* the clock controller needs the power domain to be active for the > > registers to be accessed, the clock controller *must* have a direct > > dependency on the power domain. > > > > I admit I should've worded that better. > > "should we make clock controllers depend on power domains" was actually implying > "IF those need one" :-) > > I really wonder if - at this point - it's simply a better idea to not restrict > the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`. > > Do we really need to exclude that on other clock controllers that don't have > any power domain dependency? Any side effect? > > Saying this because if we can avoid yet another per-SoC flag I'm really happy, > as readability is also impacted and besides - if we ever find out that one of > those need a power domain in the future, we'll need just one commit and just > only in the devicetree, instead of enabling a flag in driver X as well as that, > avoiding some (potentially unnecessary) noise... I guess. > > P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call > is not being checked! > > ....... > > In short.... > > Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it > wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning > that we perform pm_runtime calls for all clock drivers unconditionally)? > > If this is about longer boot time, I don't think that it's going to be more than > a millisecond or two, so that should be completely ignorable. I think it's just more of a "don't enable features you don't need" thing. We already ran into a weird deadlock, which is why the devm_pm_runtime_enable() call has that comment. I don't think anyone has actually looked at it. As you said it shouldn't be much, at least during boot time. It's one call per clock controller. > Can you please do a test for that, or should I? The earliest I can work on it would be some time next week. Does that work for you? ChenYu > Cheers > Angelo > > >> For the second one, this means that the *device* gets the power domain (adsp), and > >> not the clock controller (which clocks are effectively useless if there's no user). > > > > No. See my previous paragraph. > > > > ChenYu > > > >> Angelo > >> > >>>> --- > >>>> > >>>> Changes in v3: > >>>> - Update the commit message and the comments before runtime PM call > >>>> > >>>> Changes in v2: > >>>> - Fix the order of error handling > >>>> - Update the commit message and add a comment before the runtime PM call > >>>> > >>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++ > >>>> drivers/clk/mediatek/clk-mtk.h | 2 ++ > >>>> 2 files changed, 21 insertions(+) > >>>> > >>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c > >>>> index 2e55368dc4d8..ba1d1c495bc2 100644 > >>>> --- a/drivers/clk/mediatek/clk-mtk.c > >>>> +++ b/drivers/clk/mediatek/clk-mtk.c > >>>> @@ -13,6 +13,7 @@ > >>>> #include <linux/of.h> > >>>> #include <linux/of_address.h> > >>>> #include <linux/platform_device.h> > >>>> +#include <linux/pm_runtime.h> > >>>> #include <linux/slab.h> > >>>> > >>>> #include "clk-mtk.h" > >>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM; > >>>> } > >>>> > >>>> + > >>>> + if (mcd->need_runtime_pm) { > >>>> + devm_pm_runtime_enable(&pdev->dev); > >>>> + /* > >>>> + * Do a pm_runtime_resume_and_get() to workaround a possible > >>>> + * deadlock between clk_register() and the genpd framework. > >>>> + */ > >>>> + r = pm_runtime_resume_and_get(&pdev->dev); > >>>> + if (r) > >>>> + return r; > >>>> + } > >>>> + > >>>> /* Calculate how many clk_hw_onecell_data entries to allocate */ > >>>> num_clks = mcd->num_clks + mcd->num_composite_clks; > >>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks; > >>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >>>> goto unregister_clks; > >>>> } > >>>> > >>>> + if (mcd->need_runtime_pm) > >>>> + pm_runtime_put(&pdev->dev); > >>>> + > >>>> return r; > >>>> > >>>> unregister_clks: > >>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, > >>>> free_base: > >>>> if (mcd->shared_io && base) > >>>> iounmap(base); > >>>> + > >>>> + if (mcd->need_runtime_pm) > >>>> + pm_runtime_put(&pdev->dev); > >>>> return r; > >>>> } > >>>> > >>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h > >>>> index 22096501a60a..c17fe1c2d732 100644 > >>>> --- a/drivers/clk/mediatek/clk-mtk.h > >>>> +++ b/drivers/clk/mediatek/clk-mtk.h > >>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc { > >>>> > >>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk); > >>>> unsigned int mfg_clk_idx; > >>>> + > >>>> + bool need_runtime_pm; > >>>> }; > >>>> > >>>> int mtk_clk_pdev_probe(struct platform_device *pdev); > >>>> -- > >>>> 2.43.0.472.g3155946c3a-goog > >>>> > >> > >> > >> > > >
Il 29/02/24 11:34, Chen-Yu Tsai ha scritto: > On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: >>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: >>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> wrote: >>>>>> >>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate >>>>>> this clock controller needs runtime PM for its operations. >>>>>> Also do a runtime PM get on the clock controller during the >>>>>> probing stage to workaround a possible deadlock. >>>>>> >>>>>> Signed-off-by: Pin-yen Lin <treapking@chromium.org> >>>>> >>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>>>> >>>>> The patch itself looks fine. >>>>> >>>>> Besides the MT8183 MFG clock issues, we do actually need this for the >>>>> MT8192 ADSP clock. Its power domain is not enabled by default. >>>>> >>>> >>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists. >>> >>> That's an indirect dependency that should not be relied on. Say the clock >>> driver probed but the ADSP hasn't, and you try to read out the current >>> status. What would happen? >>> >>> - Read out works fine, because the power domain is default on, and hasn't >>> been turned off by late cleanup >>> - Read out is bogus (but you can't tell) >>> - Read out hangs. >>> >>> The third is what happens on MT8192. There's still some issues on that >>> front, as even after I applied the ADSP power domain patches from MediaTek, >>> the readout was still hanging. >>> >> >> That MT8192 lockup story is getting crazy in my head... anyway, besides that, >> I get the point - I was somehow ignoring the fact that kernel modules do exist. >> >> Eh, sorry about that :-) >> >>>> This poses a question: should we make clock controllers depend on power domains, >>>> or should we keep everything powered off (hence clocks down - no power consumption) >>>> *unless* the user exists? >>> >>> That's a policy discussion separate from actual hardware dependencies. >>> *If* the clock controller needs the power domain to be active for the >>> registers to be accessed, the clock controller *must* have a direct >>> dependency on the power domain. >>> >> >> I admit I should've worded that better. >> >> "should we make clock controllers depend on power domains" was actually implying >> "IF those need one" :-) >> >> I really wonder if - at this point - it's simply a better idea to not restrict >> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`. >> >> Do we really need to exclude that on other clock controllers that don't have >> any power domain dependency? Any side effect? >> >> Saying this because if we can avoid yet another per-SoC flag I'm really happy, >> as readability is also impacted and besides - if we ever find out that one of >> those need a power domain in the future, we'll need just one commit and just >> only in the devicetree, instead of enabling a flag in driver X as well as that, >> avoiding some (potentially unnecessary) noise... I guess. >> >> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call >> is not being checked! >> >> ....... >> >> In short.... >> >> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it >> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning >> that we perform pm_runtime calls for all clock drivers unconditionally)? >> >> If this is about longer boot time, I don't think that it's going to be more than >> a millisecond or two, so that should be completely ignorable. > > I think it's just more of a "don't enable features you don't need" thing. > We already ran into a weird deadlock, which is why the devm_pm_runtime_enable() > call has that comment. > > I don't think anyone has actually looked at it. As you said it shouldn't be > much, at least during boot time. It's one call per clock controller. > >> Can you please do a test for that, or should I? > > The earliest I can work on it would be some time next week. Does that work > for you? > The earliest I'd be able to work on this myself would be at the end of next week if not later.. so yes, please take your time, no worries. Thank you! > ChenYu > >> Cheers >> Angelo >> >>>> For the second one, this means that the *device* gets the power domain (adsp), and >>>> not the clock controller (which clocks are effectively useless if there's no user). >>> >>> No. See my previous paragraph. >>> >>> ChenYu >>> >>>> Angelo >>>> >>>>>> --- >>>>>> >>>>>> Changes in v3: >>>>>> - Update the commit message and the comments before runtime PM call >>>>>> >>>>>> Changes in v2: >>>>>> - Fix the order of error handling >>>>>> - Update the commit message and add a comment before the runtime PM call >>>>>> >>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++ >>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++ >>>>>> 2 files changed, 21 insertions(+) >>>>>> >>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c >>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644 >>>>>> --- a/drivers/clk/mediatek/clk-mtk.c >>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> #include <linux/of.h> >>>>>> #include <linux/of_address.h> >>>>>> #include <linux/platform_device.h> >>>>>> +#include <linux/pm_runtime.h> >>>>>> #include <linux/slab.h> >>>>>> >>>>>> #include "clk-mtk.h" >>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, >>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM; >>>>>> } >>>>>> >>>>>> + >>>>>> + if (mcd->need_runtime_pm) { >>>>>> + devm_pm_runtime_enable(&pdev->dev); >>>>>> + /* >>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible >>>>>> + * deadlock between clk_register() and the genpd framework. >>>>>> + */ >>>>>> + r = pm_runtime_resume_and_get(&pdev->dev); >>>>>> + if (r) >>>>>> + return r; >>>>>> + } >>>>>> + >>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */ >>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks; >>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks; >>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, >>>>>> goto unregister_clks; >>>>>> } >>>>>> >>>>>> + if (mcd->need_runtime_pm) >>>>>> + pm_runtime_put(&pdev->dev); >>>>>> + >>>>>> return r; >>>>>> >>>>>> unregister_clks: >>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, >>>>>> free_base: >>>>>> if (mcd->shared_io && base) >>>>>> iounmap(base); >>>>>> + >>>>>> + if (mcd->need_runtime_pm) >>>>>> + pm_runtime_put(&pdev->dev); >>>>>> return r; >>>>>> } >>>>>> >>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h >>>>>> index 22096501a60a..c17fe1c2d732 100644 >>>>>> --- a/drivers/clk/mediatek/clk-mtk.h >>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h >>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc { >>>>>> >>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk); >>>>>> unsigned int mfg_clk_idx; >>>>>> + >>>>>> + bool need_runtime_pm; >>>>>> }; >>>>>> >>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev); >>>>>> -- >>>>>> 2.43.0.472.g3155946c3a-goog >>>>>> >>>> >>>> >>>> >> >> >>
diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c index 2e55368dc4d8..ba1d1c495bc2 100644 --- a/drivers/clk/mediatek/clk-mtk.c +++ b/drivers/clk/mediatek/clk-mtk.c @@ -13,6 +13,7 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include "clk-mtk.h" @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM; } + + if (mcd->need_runtime_pm) { + devm_pm_runtime_enable(&pdev->dev); + /* + * Do a pm_runtime_resume_and_get() to workaround a possible + * deadlock between clk_register() and the genpd framework. + */ + r = pm_runtime_resume_and_get(&pdev->dev); + if (r) + return r; + } + /* Calculate how many clk_hw_onecell_data entries to allocate */ num_clks = mcd->num_clks + mcd->num_composite_clks; num_clks += mcd->num_fixed_clks + mcd->num_factor_clks; @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, goto unregister_clks; } + if (mcd->need_runtime_pm) + pm_runtime_put(&pdev->dev); + return r; unregister_clks: @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev, free_base: if (mcd->shared_io && base) iounmap(base); + + if (mcd->need_runtime_pm) + pm_runtime_put(&pdev->dev); return r; } diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h index 22096501a60a..c17fe1c2d732 100644 --- a/drivers/clk/mediatek/clk-mtk.h +++ b/drivers/clk/mediatek/clk-mtk.h @@ -237,6 +237,8 @@ struct mtk_clk_desc { int (*clk_notifier_func)(struct device *dev, struct clk *clk); unsigned int mfg_clk_idx; + + bool need_runtime_pm; }; int mtk_clk_pdev_probe(struct platform_device *pdev);