From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A807DC3DA7F for ; Wed, 7 Aug 2024 16:49:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=sOY3ZUXzU7Co5/aIs2/fh9HLg4+/kCPnWROfIcUoUkc=; b=O4hFeWSfnTXpBzvNWj+cVi5l7f EsSnr4wNtLQQkOuGnK9CXNeMR8xTZNyK0b/04r9brCGvpMSfypbAM/gxbRsx1I5w0HoFbfVdFOsve Ey/LKjdXZzMG4TN9zhVkiVFhYT8LrA1f0UVbUjHoqt1zo8aTBoGxHR1rrL9tl8J4c6sM7jPgvnFeX YRWUIzfaMEJwIbqIgl4n1/n8cjp6TPK/3cBTJLder57fJvgOkhIlIk+CAjxOpiUOfXBRUvMNaUH3G LJqf/qLrR8602azbfuPoYtfLS+eeBc5+78StZbG+kbC7LHaocfIUD8fvDg/5Fyftik7elfKkBLaj8 AZVepNYA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sbjqe-00000005jrw-2e7R; Wed, 07 Aug 2024 16:49:16 +0000 Received: from mail-pg1-x52a.google.com ([2607:f8b0:4864:20::52a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sbjnQ-00000005jAE-09ET for linux-arm-kernel@lists.infradead.org; Wed, 07 Aug 2024 16:45:57 +0000 Received: by mail-pg1-x52a.google.com with SMTP id 41be03b00d2f7-7b93883666dso23233a12.1 for ; Wed, 07 Aug 2024 09:45:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1723049154; x=1723653954; darn=lists.infradead.org; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=sOY3ZUXzU7Co5/aIs2/fh9HLg4+/kCPnWROfIcUoUkc=; b=gPkNssW8fNjoam+QpLhwrGEukVTzYA9IYHzzvDzwvColVU2QcpdX8iuLllVmUXNbbH s2gRIKzdPOVv+7HVG+ir8sTofIYv4opAP7WnkDqS6zqfe77mk/96Yt4DnH9IUYSo3EfM baHZ0WpzE1bUh0k/eTqhBm1Bj6ZcCLJaQqPTrkpoCb8uz6rV7KWKLr3EJR5Ccz1LC4X9 MKjZm17UNVBeptLR3yRE38reDz28YQSL0MzFmIUxogljDI5mY8paYZ7YScGnceBQaFd8 7DbqjZSJVp/HLhTRPeAZH2ewgUoOOWOxNfVeudyPOkvdaURWlnTtj7Hmui3xMUpJVSKi sN8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723049154; x=1723653954; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=sOY3ZUXzU7Co5/aIs2/fh9HLg4+/kCPnWROfIcUoUkc=; b=imIwmaxAinse7iKZ07ozIJmPq+SihTRURd/lEcqKjO4qrJPhMdtaxKoKntEOKBZy3F zMuwBcttVFgkOeNhr16VbWv+HL6lA2PXJSGsW2gU3tJ/jp1/qlQrj9uO7jhnk2ldGcSo e2Gwj6v2AAV7/ULWQ2LlCNNTh4p2rc64PjIkl/cD+HC2m2mSC5LO4R3G7d24WzMjx8gR FmMnlX9IzH1VBF7KT5V/hZEdcAqxsm/91z4V0CQUJrMje4pgWLUa3+iX1Ea/esPwTH3B 1Yiy1r/FOb0OFt/fdVkh6nAQzAaM3yIztHb+GDiiikLDU2CSl/kWddNdNmnb/mbyOlmC JHMg== X-Forwarded-Encrypted: i=1; AJvYcCW70yaURGuucOvK4DI5f9xYy/jms61gy3oPJaHoNHmAi7lPpj178MQvCl9z6evI6PeQYUSy6Cyes3DZM/jCtjcsBTFqD61wEqTCkfEa+hGbzm45rSo= X-Gm-Message-State: AOJu0YygLfVXjWarbnVDTQbn1daRzGS7Tu4PdpvLgiN3mRZ0ZAni6mYt VXU2huZCkvmE/QkCcJHQxpDMCBDyHap8UlSE7YxhVDWyoqHxTuSyNc4SgsBTa8o= X-Google-Smtp-Source: AGHT+IENG/U2H5j6dHzOUizyHzNsqJWakSioMKdV7DC//hZnoLMtt6j0GfUx7Gr+UESNVoq9jSsQvg== X-Received: by 2002:a05:6a20:1015:b0:1c2:a29b:efb4 with SMTP id adf61e73a8af0-1c6ed7d6778mr3087168637.24.1723049154198; Wed, 07 Aug 2024 09:45:54 -0700 (PDT) Received: from localhost ([71.212.170.185]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-7106ecdfffasm8494736b3a.124.2024.08.07.09.45.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Aug 2024 09:45:53 -0700 (PDT) From: Kevin Hilman To: Markus Schneider-Pargmann Cc: Ulf Hansson , Nishanth Menon , Vibhore Vardhan , Dhruva Gole , Akashdeep Kaur , Sebin Francis , linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] pmdomain: ti_sci: add per-device latency constraint management In-Reply-To: References: <20240805-lpm-v6-10-constraints-pmdomain-v1-0-d186b68ded4c@baylibre.com> <20240805-lpm-v6-10-constraints-pmdomain-v1-1-d186b68ded4c@baylibre.com> <7hbk25w9du.fsf@baylibre.com> Date: Wed, 07 Aug 2024 09:45:52 -0700 Message-ID: <7hikwcl1rz.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240807_094556_114580_E410455F X-CRM114-Status: GOOD ( 38.04 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Markus Schneider-Pargmann writes: > On Tue, Aug 06, 2024 at 03:53:49PM GMT, Kevin Hilman wrote: >> Markus Schneider-Pargmann writes: >> >> > Hi Kevin, >> > >> > On Mon, Aug 05, 2024 at 04:38:39PM GMT, Kevin Hilman wrote: >> >> For each device in a TI SCI PM domain, check whether the device has >> >> any resume latency constraints set via per-device PM QoS. If >> >> constraints are set, send them to DM via the new SCI constraints API. >> >> >> >> Checking for constraints happen: >> >> >> >> 1) before SCI PM domain power off (->power_off() hook) >> >> 2) before system-wide suspend (via ->suspend() hook) >> >> >> >> For TI SCI devices that are runtime PM enabled, check (1) will be the >> >> primary method, and will happen when the TI SCI PM domain is powered >> >> off (e.g. when the runtime PM usecount of the last device in that >> >> domain goes to zero.) >> >> >> >> For devices that are either not runtime PM enabled, or are not yet >> >> runtime suspended (e.g. due to being used during the suspend path), >> >> the constraints check will happen by check(2). >> >> >> >> Since constraints can be sent by either (1) or (2), driver keeps track >> >> of whether a valid constraint has been sent already. >> >> >> >> An important detail here is that the PM domain driver inserts itself >> >> into the path of both the ->suspend() and ->resume() hook path >> >> of *all* devices in the PM domain. This allows generic PM domain code >> >> to handle the constraint management and communication with TI SCI. >> >> >> >> Further, this allows device drivers to use existing PM QoS APIs to >> >> add/update constraints. >> >> >> >> DM firmware clears constraints during its resume, so Linux has >> >> to check/update/send constraints each time system suspends. >> >> >> >> Co-developed-by: Vibhore Vardhan >> >> Signed-off-by: Vibhore Vardhan >> >> Signed-off-by: Kevin Hilman >> >> Signed-off-by: Dhruva Gole >> > >> > Reviewed-by: Markus Schneider-Pargmann >> > >> > In general this looks good, two small things below. >> > >> >> --- >> >> drivers/pmdomain/ti/ti_sci_pm_domains.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> >> 1 file changed, 91 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c >> >> index 1510d5ddae3d..4dc48a97f9b8 100644 >> >> --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c >> >> +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c >> >> @@ -13,6 +13,8 @@ >> >> #include >> >> #include >> >> #include >> >> +#include >> >> +#include >> >> #include >> >> #include >> >> >> >> @@ -47,10 +49,46 @@ struct ti_sci_pm_domain { >> >> struct generic_pm_domain pd; >> >> struct list_head node; >> >> struct ti_sci_genpd_provider *parent; >> >> + s32 lat_constraint; >> >> + bool constraint_sent; >> >> }; >> >> >> >> #define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd) >> >> >> >> +static inline bool ti_sci_pd_is_valid_constraint(s32 val) >> >> +{ >> >> + return val != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; >> >> +} >> >> + >> >> +static int ti_sci_pd_send_constraint(struct device *dev, s32 val) >> >> +{ >> >> + struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain); >> >> + struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd); >> >> + const struct ti_sci_handle *ti_sci = pd->parent->ti_sci; >> >> + int ret; >> >> + >> >> + ret = ti_sci->ops.pm_ops.set_latency_constraint(ti_sci, val, TISCI_MSG_CONSTRAINT_SET); >> >> + if (!ret) { >> >> + pd->constraint_sent = true; >> >> + dev_dbg(dev, "ti_sci_pd: ID:%d set latency constraint %d\n", >> >> + pd->idx, val); >> >> + } else { >> >> + dev_err(dev, "ti_sci_pd: set latency constraint failed: ret=%d\n", >> >> + ret); >> >> + } >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +static inline void ti_sci_pd_clear_constraints(struct device *dev) >> >> +{ >> >> + struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain); >> >> + struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd); >> >> + >> >> + pd->lat_constraint = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; >> >> + pd->constraint_sent = false; >> >> +} >> >> + >> >> /* >> >> * ti_sci_pd_power_off(): genpd power down hook >> >> * @domain: pointer to the powerdomain to power off >> >> @@ -59,6 +97,18 @@ static int ti_sci_pd_power_off(struct generic_pm_domain *domain) >> >> { >> >> struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(domain); >> >> const struct ti_sci_handle *ti_sci = pd->parent->ti_sci; >> >> + struct pm_domain_data *pdd; >> >> + >> >> + list_for_each_entry(pdd, &domain->dev_list, list_node) { >> >> + struct device *dev = pdd->dev; >> >> + s32 val; >> >> + >> >> + /* If device has any resume latency constraints, send 'em */ >> >> + val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY); >> >> + if (ti_sci_pd_is_valid_constraint(val) && !pd->constraint_sent) >> >> + ti_sci_pd_send_constraint(dev, val); >> >> + pd->lat_constraint = val; >> >> + } >> >> >> >> return ti_sci->ops.dev_ops.put_device(ti_sci, pd->idx); >> >> } >> >> @@ -79,6 +129,38 @@ static int ti_sci_pd_power_on(struct generic_pm_domain *domain) >> >> return ti_sci->ops.dev_ops.get_device(ti_sci, pd->idx); >> >> } >> >> >> >> +static int ti_sci_pd_resume(struct device *dev) >> >> +{ >> >> + ti_sci_pd_clear_constraints(dev); >> >> + return pm_generic_resume(dev); >> >> +} >> >> + >> >> +static int ti_sci_pd_suspend(struct device *dev) >> >> +{ >> >> + struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain); >> >> + struct ti_sci_pm_domain *pd = genpd_to_ti_sci_pd(genpd); >> >> + s32 val; >> >> + int ret; >> >> + >> >> + ret = pm_generic_suspend(dev); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + /* Check if device has any resume latency constraints */ >> >> + val = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY); >> >> + if (ti_sci_pd_is_valid_constraint(val) && !pd->constraint_sent) { >> >> + if (genpd && genpd->status == GENPD_STATE_OFF) >> >> + dev_warn(dev, "%s: %s: already off.\n", genpd->name, __func__); >> >> + else if (pm_runtime_suspended(dev)) >> >> + dev_warn(dev, "%s: %s: already RPM suspended.\n", genpd->name, __func__); >> >> + else >> >> + ti_sci_pd_send_constraint(dev, val); >> >> + } >> >> + pd->lat_constraint = val; >> > >> > Could you get rid of pd->constraint_sent? I don't really see a situation >> > where it would be necessary. >> >> It is necessary for runtime PM enabled devices that also use >> constraints. For a device like that, if a constraint is present it will >> be sent when the PM domain is powered off (right after the runtime PM >> usecount goes to zero.) Later, during system-wide suspend, the suspend >> hook for that same device might try to send the constraint again if we >> don't keep track of whether the constraint was already sent. > > But wouldn't in that case pd->lat_constraint also be valid as you set it > only after sending the constraint? So if you check for that instead of > pd->constraint_sent, could you remove constraint_sent? On resume it is > always reset as well. > > It just looked to me like if it is valid then it would have also been > sent, which is the same semantic as the bool. hmm, yeah. I think you're right that constraint valid and constraint sent are the same thing. In earlier versions of the firmware, there was slightly different behavior about how constraints were cleared, but with how it works now, I think it can be removed. I'll test with that for the next version. Thanks for the review! Kevin