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 BACDBE784AB for ; Mon, 2 Oct 2023 17:12:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=UZmFDP4JHqV2QZ0LiKEGesuAjWkxWfDPE6kK3eRDjvc=; b=HN5Ph2qTyUlJP3 KvNlHtBJ1epBZUtup2FfWSOrarBLcXpz4bNLFg/CBhEDNVQmXOhxAFB55kvPl/OTe45ynFrqYki8J DKeaBgT0QgG121RamuuM57JHGiKc5jHib8Rv6VLn2fv7LAQ3X7zuWBxO/cJbcoR6AkRwn6khHY4Fc 97CMPyyeE1ISFDSNmAlboAzs4HJi76IGeCUezTsE9FzmsNHAfKVZV7AvdDrv4jwCKKWlAWH6emy4K fgkgN4/RGuuDXOXrpValKB3XEgTKMhFgiJ5QqoiL5zGVcjES0pon9XJra8f8onkMFYxK1lErWnemw 9A95Jo8i3sdbeU7RVCXQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qnMSJ-00D7lg-33; Mon, 02 Oct 2023 17:11:39 +0000 Received: from mail-pf1-x42b.google.com ([2607:f8b0:4864:20::42b]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qnMSG-00D7l6-2s for linux-arm-kernel@lists.infradead.org; Mon, 02 Oct 2023 17:11:38 +0000 Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-690bc3f82a7so14942b3a.0 for ; Mon, 02 Oct 2023 10:11:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1696266695; x=1696871495; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=RO8LmnAQAaGitSXUySpyKWLVPTjigR/N/h30MsqWtSs=; b=E66/WPcPCfNK6iBxX9HMxFpBxqX26P2nvlZ987v3QdRygxFmZhOE40xPpveDDwv+9S nkKiXi60E4kjiJg3p9Y5f+R0U8ij1zeS6/TAY+g/X7akHWaOEuV8uExXZYYMGpEpu3N3 XTl/+6SsZBJFdg1HPa1In5VjgFaVHE3HMA6JLgIyO2NC0l6CEIrIeLfHtghaxx4S1sMW szSRRbuns4qGK2YM8tyBp0eXIVXHyc7ugFrGGaalu+b2YC0jmi90z4Y8uT8h8UAm+0Ee nT0wRpiLIG3aCYlyXEJVI30LCz12zCO6IVCNiLhaXc6zbaUJvrVXTYZ2LDxlXPxHythg 7/LA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696266695; x=1696871495; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=RO8LmnAQAaGitSXUySpyKWLVPTjigR/N/h30MsqWtSs=; b=PU10ZRZcbrDm2ApGYUfOX9RIGAevpZsZPMUZpFO+nHRr6WK5cZZGIZe8zRFPT8sE+T WZjT6qLP8CWmaGleZUTM6bZOgqm8Cs1T4bVZFixpRd5ggIYbl3/P3CJ4L9zXrAn//fyH /hvGzh/qHoSld7HQlpeBthuuAjs1c5+6ksS4yITW7YPM4QvoTZkdVFfzs4QxxbGwV9u5 7tDpBthSLiZoreLNsPB8CzQz8D6eqiv8Aizd4uPt3ikuK+uIjV9efMRXaaULbI6jV/IX lMIig+O/CMfmlM2fq8hFEQ7WQzAizFfwo3ms98WHkLnaUXAEaQg21ftL0ttN85DQoq88 4KIg== X-Gm-Message-State: AOJu0Yz1pYUY1C/BRDssSKF36aF83dmJu0IXYeQZctTe9/w6YFNnGFQG IPFaN54y35qM2ugL8lSPJMr/wg== X-Google-Smtp-Source: AGHT+IF1FZLmlAaibekVfNPfOP8jIXo5x7QhPxzko11+4JiGyUuhCz3pCTLhKVh8fga2WKh0RYfltw== X-Received: by 2002:a05:6a20:8410:b0:154:6480:83b4 with SMTP id c16-20020a056a20841000b00154648083b4mr12437393pzd.14.1696266695325; Mon, 02 Oct 2023 10:11:35 -0700 (PDT) Received: from p14s ([2604:3d09:148c:c800:fa62:edba:ab23:c762]) by smtp.gmail.com with ESMTPSA id f24-20020aa782d8000000b0069309cbc220sm11386769pfn.121.2023.10.02.10.11.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 02 Oct 2023 10:11:34 -0700 (PDT) Date: Mon, 2 Oct 2023 11:11:32 -0600 From: Mathieu Poirier To: Tanmay Shah Cc: andersson@kernel.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, michal.simek@amd.com, radhey.shyam.pandey@amd.com, ben.levinsky@amd.com, linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 3/4] remoteproc: zynqmp: add pm domains support Message-ID: References: <20230928155900.3987103-1-tanmay.shah@amd.com> <20230928155900.3987103-4-tanmay.shah@amd.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230928155900.3987103-4-tanmay.shah@amd.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231002_101137_001638_BC9C29CE X-CRM114-Status: GOOD ( 35.68 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 28, 2023 at 08:58:59AM -0700, Tanmay Shah wrote: > Use TCM pm domains extracted from device-tree > to power on/off TCM using general pm domain framework > > Signed-off-by: Tanmay Shah > --- > drivers/remoteproc/xlnx_r5_remoteproc.c | 224 ++++++++++++++++++++++-- > 1 file changed, 205 insertions(+), 19 deletions(-) > > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c > index 4395edea9a64..27ed2c070ebb 100644 > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > > #include "remoteproc_internal.h" > > @@ -102,6 +103,12 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = { > * @rproc: rproc handle > * @pm_domain_id: RPU CPU power domain id > * @ipi: pointer to mailbox information > + * @num_pm_dev: number of tcm pm domain devices for this core > + * @pm_dev1: pm domain virtual devices for power domain framework > + * @pm_dev_link1: pm domain device links after registration > + * @pm_dev2: used only in lockstep mode. second core's pm domain virtual devices > + * @pm_dev_link2: used only in lockstep mode. second core's pm device links after > + * registration > */ > struct zynqmp_r5_core { > struct device *dev; > @@ -111,6 +118,11 @@ struct zynqmp_r5_core { > struct rproc *rproc; > u32 pm_domain_id; > struct mbox_info *ipi; > + int num_pm_dev; > + struct device **pm_dev1; > + struct device_link **pm_dev_link1; > + struct device **pm_dev2; > + struct device_link **pm_dev_link2; > }; > > /** > @@ -575,12 +587,21 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc) > bank_size = r5_core->tcm_banks[i]->size; > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id; > > - ret = zynqmp_pm_request_node(pm_domain_id, > - ZYNQMP_PM_CAPABILITY_ACCESS, 0, > - ZYNQMP_PM_REQUEST_ACK_BLOCKING); > - if (ret < 0) { > - dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id); > - goto release_tcm_split; > + /* > + * If TCM information is available in device-tree then > + * in that case, pm domain framework will power on/off TCM. > + * In that case pm_domain_id is set to 0. If hardcode > + * bindings from driver is used, then only this driver will > + * use pm_domain_id. > + */ > + if (pm_domain_id) { > + ret = zynqmp_pm_request_node(pm_domain_id, > + ZYNQMP_PM_CAPABILITY_ACCESS, 0, > + ZYNQMP_PM_REQUEST_ACK_BLOCKING); > + if (ret < 0) { > + dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id); > + goto release_tcm_split; > + } > } > > dev_dbg(dev, "TCM carveout split mode %s addr=%llx, da=0x%x, size=0x%lx", > @@ -646,13 +667,16 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc) > for (i = 0; i < num_banks; i++) { > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id; > > - /* Turn on each TCM bank individually */ > - ret = zynqmp_pm_request_node(pm_domain_id, > - ZYNQMP_PM_CAPABILITY_ACCESS, 0, > - ZYNQMP_PM_REQUEST_ACK_BLOCKING); > - if (ret < 0) { > - dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id); > - goto release_tcm_lockstep; > + if (pm_domain_id) { > + /* Turn on each TCM bank individually */ > + ret = zynqmp_pm_request_node(pm_domain_id, > + ZYNQMP_PM_CAPABILITY_ACCESS, 0, > + ZYNQMP_PM_REQUEST_ACK_BLOCKING); > + if (ret < 0) { > + dev_err(dev, "failed to turn on TCM 0x%x", > + pm_domain_id); > + goto release_tcm_lockstep; > + } > } > > bank_size = r5_core->tcm_banks[i]->size; > @@ -687,8 +711,10 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc) > /* If failed, Turn off all TCM banks turned on before */ > for (i--; i >= 0; i--) { > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id; > - zynqmp_pm_release_node(pm_domain_id); > + if (pm_domain_id) > + zynqmp_pm_release_node(pm_domain_id); > } > + Spurious change > return ret; > } > > @@ -758,6 +784,153 @@ static int zynqmp_r5_parse_fw(struct rproc *rproc, const struct firmware *fw) > return ret; > } > > +static void zynqmp_r5_remove_pm_domains(struct rproc *rproc) > +{ > + struct zynqmp_r5_core *r5_core = rproc->priv; > + struct device *dev = r5_core->dev; > + struct zynqmp_r5_cluster *cluster; > + int i; > + > + cluster = platform_get_drvdata(to_platform_device(dev->parent)); > + > + for (i = 0; i < r5_core->num_pm_dev; i++) { > + if (r5_core->pm_dev_link1 && r5_core->pm_dev_link1[i]) > + device_link_del(r5_core->pm_dev_link1[i]); > + if (r5_core->pm_dev1 && !IS_ERR_OR_NULL(r5_core->pm_dev1[i])) > + dev_pm_domain_detach(r5_core->pm_dev1[i], false); > + } > + A global function such as this one should not have to deal with error conditions. Those should be dealt with in the allocation function where cleanup is done on error conditions. See my comment below in zynqmp_r5_add_pm_domains(). > + kfree(r5_core->pm_dev1); > + r5_core->pm_dev1 = NULL; > + kfree(r5_core->pm_dev_link1); > + r5_core->pm_dev_link1 = NULL; > + > + if (cluster->mode == SPLIT_MODE) { > + r5_core->num_pm_dev = 0; > + return; > + } > + > + for (i = 0; i < r5_core->num_pm_dev; i++) { > + if (r5_core->pm_dev_link2 && r5_core->pm_dev_link2[i]) > + device_link_del(r5_core->pm_dev_link2[i]); > + if (r5_core->pm_dev2 && !IS_ERR_OR_NULL(r5_core->pm_dev2[i])) > + dev_pm_domain_detach(r5_core->pm_dev2[i], false); > + } > + > + kfree(r5_core->pm_dev2); > + r5_core->pm_dev2 = NULL; > + kfree(r5_core->pm_dev_link2); > + r5_core->pm_dev_link2 = NULL; > + r5_core->num_pm_dev = 0; > +} > + > +static int zynqmp_r5_add_pm_domains(struct rproc *rproc) > +{ > + struct zynqmp_r5_core *r5_core = rproc->priv; > + struct device *dev = r5_core->dev, *dev2; > + struct zynqmp_r5_cluster *cluster; > + struct platform_device *pdev; > + struct device_node *np; > + int i, num_pm_dev, ret; I'm not sure 'ret' is needed - see below. > + > + cluster = platform_get_drvdata(to_platform_device(dev->parent)); Why not use dev_get_drvdata() as it is done elsewhere in this driver? > + > + /* get number of power-domains */ > + num_pm_dev = of_count_phandle_with_args(r5_core->np, "power-domains", > + "#power-domain-cells"); > + > + if (num_pm_dev <= 0) > + return -EINVAL; > + > + r5_core->pm_dev1 = kcalloc(num_pm_dev, > + sizeof(struct device *), > + GFP_KERNEL); > + if (!r5_core->pm_dev1) > + ret = -ENOMEM; What's the goal of the assignment? Did you mean to return an error instead? > + > + r5_core->pm_dev_link1 = kcalloc(num_pm_dev, > + sizeof(struct device_link *), > + GFP_KERNEL); > + if (!r5_core->pm_dev_link1) > + return -ENOMEM; In case of error, always cleanup the work done in the current function. That way cleanup functions such as zynqmp_r5_remove_pm_domains() are simple and easy to read. > + > + r5_core->num_pm_dev = num_pm_dev; > + > + /* for zynqmp we only add TCM power domains and not core's power domain */ > + for (i = 1; i < r5_core->num_pm_dev; i++) { > + r5_core->pm_dev1[i] = dev_pm_domain_attach_by_id(dev, i); > + if (IS_ERR(r5_core->pm_dev1[i])) { > + dev_dbg(dev, "failed to attach pm domain %d\n", i); > + return PTR_ERR(r5_core->pm_dev1[i]); > + } > + if (!r5_core->pm_dev1[i]) { > + dev_dbg(dev, "can't attach to pm domain %d\n", i); > + return -EINVAL; > + } > + > + r5_core->pm_dev_link1[i] = device_link_add(dev, r5_core->pm_dev1[i], > + DL_FLAG_STATELESS | > + DL_FLAG_RPM_ACTIVE | > + DL_FLAG_PM_RUNTIME); > + if (!r5_core->pm_dev_link1[i]) { > + dev_pm_domain_detach(r5_core->pm_dev1[i], true); > + r5_core->pm_dev1[i] = NULL; > + return -EINVAL; > + } > + } > + > + if (cluster->mode == SPLIT_MODE) > + return 0; > + > + r5_core->pm_dev2 = kcalloc(num_pm_dev, > + sizeof(struct device *), > + GFP_KERNEL); > + if (!r5_core->pm_dev2) > + return -ENOMEM; > + > + r5_core->pm_dev_link2 = kcalloc(num_pm_dev, > + sizeof(struct device_link *), > + GFP_KERNEL); > + if (!r5_core->pm_dev_link2) > + return -ENOMEM; > + > + /* get second core's device to detach its power-domains */ > + np = of_get_next_child(cluster->dev->of_node, of_node_get(dev->of_node)); > + > + pdev = of_find_device_by_node(np); > + if (!pdev) { > + dev_err(cluster->dev, "core1 platform device not available\n"); > + return -EINVAL; > + } > + > + dev2 = &pdev->dev; > + > + /* for zynqmp we only add TCM power domains and not core's power domain */ > + for (i = 1; i < r5_core->num_pm_dev; i++) { > + r5_core->pm_dev2[i] = dev_pm_domain_attach_by_id(dev2, i); > + if (IS_ERR(r5_core->pm_dev2[i])) { > + dev_dbg(dev, "can't attach to pm domain %d\n", i); > + return PTR_ERR(r5_core->pm_dev2[i]); > + } > + if (!r5_core->pm_dev2[i]) { > + dev_dbg(dev, "can't attach to pm domain %d\n", i); > + return -EINVAL; > + } > + > + r5_core->pm_dev_link2[i] = device_link_add(dev, r5_core->pm_dev2[i], > + DL_FLAG_STATELESS | > + DL_FLAG_RPM_ACTIVE | > + DL_FLAG_PM_RUNTIME); > + if (!r5_core->pm_dev_link2[i]) { > + dev_pm_domain_detach(r5_core->pm_dev2[i], true); > + r5_core->pm_dev2[i] = NULL; > + return -ENODEV; > + } > + } > + > + return 0; > +} > + > /** > * zynqmp_r5_rproc_prepare() > * adds carveouts for TCM bank and reserved memory regions > @@ -770,19 +943,30 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc) > { > int ret; > > + ret = zynqmp_r5_add_pm_domains(rproc); > + if (ret) { > + dev_err(&rproc->dev, "failed to add pm domains\n"); > + goto fail_prepare; > + } > + > ret = add_tcm_banks(rproc); > if (ret) { > dev_err(&rproc->dev, "failed to get TCM banks, err %d\n", ret); > - return ret; > + goto fail_prepare; > } > > ret = add_mem_regions_carveout(rproc); > if (ret) { > dev_err(&rproc->dev, "failed to get reserve mem regions %d\n", ret); > - return ret; > + goto fail_prepare; > } > > return 0; > + > +fail_prepare: > + zynqmp_r5_remove_pm_domains(rproc); > + > + return ret; > } > > /** > @@ -801,11 +985,13 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc) > > r5_core = rproc->priv; > > + zynqmp_r5_remove_pm_domains(rproc); > + > for (i = 0; i < r5_core->tcm_bank_count; i++) { > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id; > - if (zynqmp_pm_release_node(pm_domain_id)) > - dev_warn(r5_core->dev, > - "can't turn off TCM bank 0x%x", pm_domain_id); > + if (pm_domain_id && zynqmp_pm_release_node(pm_domain_id)) > + dev_dbg(r5_core->dev, > + "can't turn off TCM bank 0x%x", pm_domain_id); > } > > return 0; > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel