Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <93487d3c-c324-4b9b-8b25-0b4ea52237b4@amd.com>
Date:   Wed, 22 Nov 2023 15:00:36 -0600
User-Agent: Mozilla Thunderbird Beta
Subject: Re: [PATCH v7 3/4] remoteproc: zynqmp: add pm domains support
Content-Language: en-US
To:     Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        michal.simek@amd.com, ben.levinsky@amd.com,
        linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20231117174238.1876655-1-tanmay.shah@amd.com>
 <20231117174238.1876655-4-tanmay.shah@amd.com> <ZV02P3bHEhPLQHBo@p14s>
From:   Tanmay Shah <tanmay.shah@amd.com>
In-Reply-To: <ZV02P3bHEhPLQHBo@p14s>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WDNXTlpHWWpsR3BEcHl6YjZxTGprakFNbWdwbStESk9SblRoQ1JycmFyUjhG?=
 =?utf-8?B?dDNsTEFMa0NYS3prdFpYRWdhaGQrZWl2VlV2b0dHdWRDYWVROGM0b1FKTHJZ?=
 =?utf-8?B?bUxMaExrekgzUTlQNXovQzZnMHNRQ2l0RlZnYUZTMWxSckN0cjZjTWl1WjF6?=
 =?utf-8?B?bXl3NlpSYUkzcnFQZmYweDVHZHV4UE1yMW84Wms5Z1FReWtkaXZlWm9xa09F?=
 =?utf-8?B?cUM3QU8zTFJMMWZJZHh5NWtyc3BKSHRxckdySGxPU0FBVndOQndDcXp1ODZ2?=
 =?utf-8?B?VmN3SkFuN0twL1R4NkFOY21TZ1pNVEFmcmdqQjlZK3Nod1Frb0Rxemg4dEU0?=
 =?utf-8?B?Z2tQb1B0UDdkZ1JObUVhZ09HbWJQNUNiSDdORnY2WEljZ2lxSkViemNtZWda?=
 =?utf-8?B?MnViTFp0eDR4YVFlMXc4bXJqVUpqV2tjS2w1Q1U5VlAzZERyYjNxYklaWVNU?=
 =?utf-8?B?azVrVFpBNGRWQUxrK3ZCWHFMZ001OUgrcUV4NDhtemlqNjNXWjJxY0dkY2NZ?=
 =?utf-8?B?aW9jWU5IL1lYZ2tvdGdFRDJlaC9DK3U4ckUxbWp4ZkJZRnIrUmRNeEo5VXl5?=
 =?utf-8?B?ZG9kQVhNekU2UzlqQmZ0VmJZWkM5SUVVRTdacGMwVTBISTFWTkhpekhUMGVi?=
 =?utf-8?B?clBxUG9Pd3RreU5qRXBWalVySUlQYmdFb3czcXhnWmdsSzdnMVdhVy9oeWdS?=
 =?utf-8?B?c3RkZW10ZU1DTVNaVzFtd0Rza014bm1MOHF1cWpINzVIdXNmaHE0MzZGUGY0?=
 =?utf-8?B?QTBoalVhejk1ejlUdWdGQW5mSFlSRXdSc1BXR0txM1NPVXY3ZndiMkFtNnRD?=
 =?utf-8?B?ZFNFQjJRTXFNOXB3bXo0eExrUmFDOUZaenRJY1Q4VVhYb2l2WlplenB6S3Ba?=
 =?utf-8?B?Zy9JL3BlVFZ6UUk5UFc3ZUFCMlpSK3VBd1ZoTC81NlFQU1pxK0lNY3lWaVZQ?=
 =?utf-8?B?TjVqN3htNXRGdzhCRHhmODlkS2plY2EwL2pZbkV1bmFjeGt3OG5BNDVZTFZM?=
 =?utf-8?B?bVgzb0diYkR4b3duRUgrNzFwcG9Qc2ZoNC8rcklIL1h0OGF4amxFdlNPTCtG?=
 =?utf-8?B?bTJwcHJMbnhBVEpFZ0F2amlMa0l2TUpBK2Q5SkJOekVqNGx6Mjg0U1dRbG5S?=
 =?utf-8?B?ZXUyV0RUazZhTkh4WUZFWk5YRjFkK3ZPZDR4czhnRUpFNC9Fc01pcWtxSitU?=
 =?utf-8?B?b1ZrTmlBWmszUmFRM0FPRHBKMEtzT2ZBb3IzaGs1THk3cmJoSmR1R1FlRW5W?=
 =?utf-8?B?ZCtPak84Z0VjOExhRWZpc0FYU2hmMklOR1VvVUpUdHRaVDVFWUlFbFdDZ2k0?=
 =?utf-8?B?QkkxSzRZdjN2VytzRmhBOXhXazkyRUUrbWFQTGRMZzRVbVlQYURTVGxXQURz?=
 =?utf-8?B?dTdNak1SdWJuTnF2UVFHcnFoK3YvZFJZNGJ3ZHMwZFUvYU9hRlBWTkRPdTk4?=
 =?utf-8?B?eW5NVU5taUFhZGxtZUhseDBYVi8vNkNPdm05SklYK1NIbUtHOUxnZ0I2MnRF?=
 =?utf-8?B?SmJEQlUwUWtxa1V4MC9wRVU5bnNVanFYejNRR2JRR0lDeGxqNTh1U2JFNWVP?=
 =?utf-8?B?NmZRYTNLazc2Z2FNeXpBTW5BM2IweTZ6ZkhaektjQWhEWlFKamViODhSazBm?=
 =?utf-8?B?OW1ydnBMVWlFRVlaODIvS1pPbFFMT0xTdXNJRW1OcGdVS0xUeEdjbmk3TWdY?=
 =?utf-8?B?ekNjckRZa1pXY2k1WjZYM3dISXlkaE1MMEluNXFOTUxCVS9Zcnp1bDM3SGV3?=
 =?utf-8?B?a1I1SzY2Zm8reWdJWmdEa3o3K3ZSWWxjZ1MrdmNXTGxUcVorQnc1eDlGajlt?=
 =?utf-8?B?akc0b3U0N0RKMkJPSVVRUGtXU2dtRnZxQXRjaWtQUE4vWnN5UEZnN2VnUG1Y?=
 =?utf-8?B?UTZvWnpSeHZMV0w3WmdKU3A3enBzUDVsT0lzQTlDbkRwNmNsckJ1UkxaZlBa?=
 =?utf-8?B?M29MTXQ4ZEdnTTlIN2Jjc3N1YmkwdktJOHo1ZDlibkp1dXIwWVNHbTRPWk1o?=
 =?utf-8?B?d0hEUHFuZVNuT2ZYYVVpd1dsMUFUWnY4dzBsSFB1enQ1encxaFNXcEdqT3gx?=
 =?utf-8?B?R2swVnMrUVZWbUJ1L28ycnoydC9IZHJaSDQ4WlBtWUVkdEJmd1RseXhlR0tW?=
 =?utf-8?Q?+7w+9LgMM7AVkjuM0OmPnX4Da?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64b5106a-b8b7-45b8-01b1-08dbeb9e14d5
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3683.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2023 21:00:39.1692
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SkBxTq66mSdLqTdxYOCbpCYi0D5dfy075RUlqtRiSHgjPJ6iKFPm2TvZ1lvwS3xR
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB9103
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Wed, 22 Nov 2023 13:01:06 -0800 (PST)

Hi Mathieu,

Please find my comments below.

On 11/21/23 4:59 PM, Mathieu Poirier wrote:
> Hi,
>
> On Fri, Nov 17, 2023 at 09:42:37AM -0800, 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 <tanmay.shah@amd.com>
> > ---
> > 
> > Changes in v7:
> >   - %s/pm_dev1/pm_dev_core0/r
> >   - %s/pm_dev_link1/pm_dev_core0_link/r
> >   - %s/pm_dev2/pm_dev_core1/r
> >   - %s/pm_dev_link2/pm_dev_core1_link/r
> >   - remove pm_domain_id check to move next patch
> >   - add comment about how 1st entry in pm domain list is used
> >   - fix loop when jump to fail_add_pm_domains loop
> > 
> >  drivers/remoteproc/xlnx_r5_remoteproc.c | 215 +++++++++++++++++++++++-
> >  1 file changed, 212 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > index 4395edea9a64..22bccc5075a0 100644
> > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of_reserved_mem.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/remoteproc.h>
> > +#include <linux/pm_domain.h>
> >  
> >  #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_dev_core0: pm domain virtual devices for power domain framework
> > + * @pm_dev_core0_link: pm domain device links after registration
> > + * @pm_dev_core1: used only in lockstep mode. second core's pm domain virtual devices
> > + * @pm_dev_core1_link: 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_dev_core0;
> > +	struct device_link **pm_dev_core0_link;
> > +	struct device **pm_dev_core1;
> > +	struct device_link **pm_dev_core1_link;
> >  };
> >  
> >  /**
> > @@ -651,7 +663,8 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
> >  					     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);
> > +			dev_err(dev, "failed to turn on TCM 0x%x",
> > +				pm_domain_id);
>
> Spurious change, you should have caught that.

Ack, need to observe changes more closely before sending them.

>
> >  			goto release_tcm_lockstep;
> >  		}
> >  
> > @@ -758,6 +771,189 @@ 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 = 1; i < r5_core->num_pm_dev; i++) {
> > +		device_link_del(r5_core->pm_dev_core0_link[i]);
> > +		dev_pm_domain_detach(r5_core->pm_dev_core0[i], false);
> > +	}
> > +
> > +	kfree(r5_core->pm_dev_core0);
> > +	r5_core->pm_dev_core0 = NULL;
> > +	kfree(r5_core->pm_dev_core0_link);
> > +	r5_core->pm_dev_core0_link = NULL;
> > +
> > +	if (cluster->mode == SPLIT_MODE) {
> > +		r5_core->num_pm_dev = 0;
> > +		return;
> > +	}
> > +
> > +	for (i = 1; i < r5_core->num_pm_dev; i++) {
> > +		device_link_del(r5_core->pm_dev_core1_link[i]);
> > +		dev_pm_domain_detach(r5_core->pm_dev_core1[i], false);
> > +	}
> > +
> > +	kfree(r5_core->pm_dev_core1);
> > +	r5_core->pm_dev_core1 = NULL;
> > +	kfree(r5_core->pm_dev_core1_link);
> > +	r5_core->pm_dev_core1_link = 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, j, num_pm_dev, ret;
> > +
> > +	cluster = dev_get_drvdata(dev->parent);
> > +
> > +	/* 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_dev_core0 = kcalloc(num_pm_dev,
> > +					sizeof(struct device *),
> > +					GFP_KERNEL);
> > +	if (!r5_core->pm_dev_core0)
> > +		ret = -ENOMEM;
> > +
> > +	r5_core->pm_dev_core0_link = kcalloc(num_pm_dev,
> > +					     sizeof(struct device_link *),
> > +					     GFP_KERNEL);
> > +	if (!r5_core->pm_dev_core0_link) {
> > +		kfree(r5_core->pm_dev_core0);
> > +		r5_core->pm_dev_core0 = NULL;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	r5_core->num_pm_dev = num_pm_dev;
> > +
> > +	/*
> > +	 * start from 2nd entry in power-domains property list as
> > +	 * for zynqmp we only add TCM power domains and not core's power domain.
> > +	 * 1st entry is used to configure r5 operation mode.
>
> You are still not saying _where_ ->pm_dev_core0[0] gets added.

So, pm_dev_core0[0] isn't really need to be added for zynqmp platform, as firmware starts it with call,

zynqmp_pm_request_wake during rproc_start callback. I will document this in next

revision. For new platforms pm_dev_core0[0] will be added in future.

I hope this meets expectations.


>
> > +	 */
> > +	for (i = 1; i < r5_core->num_pm_dev; i++) {
> > +		r5_core->pm_dev_core0[i] = dev_pm_domain_attach_by_id(dev, i);
> > +		if (IS_ERR_OR_NULL(r5_core->pm_dev_core0[i])) {
>
> Here IS_ERR_OR_NULL() is used while two if conditions for NULL and an error
> code are used in the loop for the lockstep mode.  Please pick one heuristic and
> stick with it.  I have no preference on which one.

Ok, I think IS_ERR_OR_NULL is more cleaner, I will address it in next revision.


>
> > +			dev_dbg(dev, "failed to attach pm domain %d, ret=%ld\n", i,
> > +				PTR_ERR(r5_core->pm_dev_core0[i]));
> > +			ret = -EINVAL;
> > +			goto fail_add_pm_domains;
> > +		}
> > +		r5_core->pm_dev_core0_link[i] = device_link_add(dev,
> > +								r5_core->pm_dev_core0[i],
> > +								DL_FLAG_STATELESS |
> > +								DL_FLAG_RPM_ACTIVE |
> > +								DL_FLAG_PM_RUNTIME);
> > +		if (!r5_core->pm_dev_core0_link[i]) {
> > +			dev_pm_domain_detach(r5_core->pm_dev_core0[i], true);
> > +			r5_core->pm_dev_core0[i] = NULL;
> > +			ret = -EINVAL;
> > +			goto fail_add_pm_domains;
> > +		}
> > +	}
> > +
> > +	if (cluster->mode == SPLIT_MODE)
> > +		return 0;
> > +
> > +	r5_core->pm_dev_core1 = kcalloc(num_pm_dev,
> > +					sizeof(struct device *),
> > +					GFP_KERNEL);
> > +	if (!r5_core->pm_dev_core1) {
> > +		ret = -ENOMEM;
> > +		goto fail_add_pm_domains;
> > +	}
> > +
> > +	r5_core->pm_dev_core1_link = kcalloc(num_pm_dev,
> > +					     sizeof(struct device_link *),
> > +					     GFP_KERNEL);
> > +	if (!r5_core->pm_dev_core1_link) {
> > +		kfree(r5_core->pm_dev_core1);
> > +		r5_core->pm_dev_core1 = NULL;
> > +		ret = -ENOMEM;
> > +		goto fail_add_pm_domains;
> > +	}
> > +
> > +	/* 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");
> > +		kfree(r5_core->pm_dev_core1);
> > +		kfree(r5_core->pm_dev_core1_link);
> > +		r5_core->pm_dev_core1 = NULL;
> > +		r5_core->pm_dev_core1_link = NULL;
> > +		ret = -EINVAL;
> > +		goto fail_add_pm_domains;
> > +	}
> > +
> > +	dev2 = &pdev->dev;
> > +
> > +	/* for zynqmp we only add TCM power domains and not core's power domain */
> > +	for (j = 1; j < r5_core->num_pm_dev; j++) {
> > +		r5_core->pm_dev_core1[j] = dev_pm_domain_attach_by_id(dev2, j);
> > +		if (!r5_core->pm_dev_core1[j]) {
> > +			dev_dbg(dev, "can't attach to pm domain %d\n", j);
> > +			ret = -EINVAL;
> > +			goto fail_add_pm_domains_lockstep;
> > +		} else if (IS_ERR(r5_core->pm_dev_core1[j])) {
> > +			dev_dbg(dev, "can't attach to pm domain %d\n", j);
> > +			ret = PTR_ERR(r5_core->pm_dev_core1[j]);
> > +			goto fail_add_pm_domains_lockstep;
> > +		}
> > +
> > +		r5_core->pm_dev_core1_link[j] = device_link_add(dev,
> > +								r5_core->pm_dev_core1[j],
> > +								DL_FLAG_STATELESS |
> > +								DL_FLAG_RPM_ACTIVE |
> > +								DL_FLAG_PM_RUNTIME);
> > +		if (!r5_core->pm_dev_core1_link[j]) {
> > +			dev_pm_domain_detach(r5_core->pm_dev_core1[j], true);
> > +			r5_core->pm_dev_core1[j] = NULL;
> > +			ret = -ENODEV;
> > +			goto fail_add_pm_domains_lockstep;
> > +		}
> > +	}
> > +
> > +fail_add_pm_domains_lockstep:
> > +	while (--j >= 0) {
> > +		device_link_del(r5_core->pm_dev_core1_link[j]);
> > +		dev_pm_domain_detach(r5_core->pm_dev_core1[j], true);
> > +	}
> > +	kfree(r5_core->pm_dev_core1);
> > +	r5_core->pm_dev_core1 = NULL;
> > +	kfree(r5_core->pm_dev_core1_link);
> > +	r5_core->pm_dev_core1_link = NULL;
> > +
> > +fail_add_pm_domains:
> > +	while (--i >= 0) {
> > +		device_link_del(r5_core->pm_dev_core0_link[i]);
> > +		dev_pm_domain_detach(r5_core->pm_dev_core0[i], true);
> > +	}
> > +	kfree(r5_core->pm_dev_core0);
> > +	r5_core->pm_dev_core0 = NULL;
> > +	kfree(r5_core->pm_dev_core0_link);
> > +	r5_core->pm_dev_core0_link = NULL;
> > +
>
> The error path is much cleaner and readable now.
>
> I will continue tomorrow.
>
> Mathieu
>
> > +	return ret;
> > +}
> > +
> >  /**
> >   * zynqmp_r5_rproc_prepare()
> >   * adds carveouts for TCM bank and reserved memory regions
> > @@ -770,19 +966,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");
> > +		return ret;
> > +	}
> > +
> >  	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,6 +1008,8 @@ 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))
> > -- 
> > 2.25.1
> >