Message-ID: <cda08910-52f1-b0d2-3e06-6c5b930f898a@amd.com>
Date:   Wed, 22 Feb 2023 14:49:34 -0600
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:110.0)
 Gecko/20100101 Thunderbird/110.0
Subject: Re: [PATCH v3 3/3] remoteproc: xilinx: add mailbox channels for rpmsg
Content-Language: en-US
To:     Mathieu Poirier <mathieu.poirier@linaro.org>,
        Tanmay Shah <tanmay.shah@amd.com>
Cc:     michal.simek@amd.com, andersson@kernel.org,
        jaswinder.singh@linaro.org, ben.levinsky@amd.com,
        shubhrajyoti.datta@amd.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-remoteproc@vger.kernel.org
References: <20230213211825.3507034-1-tanmay.shah@amd.com>
 <20230213211825.3507034-4-tanmay.shah@amd.com> <20230222190623.GC909075@p14s>
From:   Tanmay Shah <tanmays@amd.com>
In-Reply-To: <20230222190623.GC909075@p14s>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U3ZwemEyQnF6bk1PNUNHdldIT1QzYUpYMVlBTjQ5YXFmWXl1bjNWRXB3U3BO?=
 =?utf-8?B?MHhaVmRkSko1d1Z4bnFjRmlldnVSTWFVeXorN2hmVVEvTEhmRTB1UUtTdnRN?=
 =?utf-8?B?cGdwc0dJN1J5L3JWYTNrWFlLWUh3SHcza1pZdXlINmUybC8xdWdsTk45Yzds?=
 =?utf-8?B?OXV1Z3Bub0JiWlRVditHdnAzWFBvNDQvMWxreVk3RVRMeHQ4RkM5VllzbENp?=
 =?utf-8?B?Vi90YzRGcUM2YlpEUngvdEFOcUhRclZSQXAvSmtjUnY2dTFuNTltc3J1WnRr?=
 =?utf-8?B?eStvdzZjK1RIcnhVSFo0OVZpRk5CSHdkWlMweE9FbnZEVEV6R0pPT0pTMnR2?=
 =?utf-8?B?UjJvMmRVUmdWeEk0VjlPd0R4Ty9WZFBRK1pPOW56NFQwZkJvMHZUOEZvRlJu?=
 =?utf-8?B?ZzJvRVN5ZEZsLzRJNVZYVm5EbGNsNWV6YjhXVlhpMmgrclRPVGVPQTR5RStZ?=
 =?utf-8?B?ZWpPb1NYZ3EwTmNIOUZKY2V1VUtRRUJHY0tmRWd6eFhianMyWWtTZStFZUpu?=
 =?utf-8?B?emFYbDk0ampOZ3UzWWk1eWx0ckRRck5ZdlhTV0VSYVpqWVBtT2FQWDBOQzYz?=
 =?utf-8?B?dzFoTmJGeTZ3ZVdPdjF6WTNBc0VWK1lqc2hVT3FXZ2tCVlh6Z3BDK0JkeWRz?=
 =?utf-8?B?TEQ3VXJVeFVzNGswZ0VGNkZTZThkM1ZaM2wzbGZwSXN0RUFJWTU5aHY1YVNk?=
 =?utf-8?B?emcvRlViM09nSDJFVHFqV29aWXljYm9UZU0xQzZWb1JpUWQySENtbUROcC9z?=
 =?utf-8?B?blp2TG1Wam9Qa2o4MVg1VktMWENIN1BNQmRZbkIrb3l0dEl5RWNKUjVROFRn?=
 =?utf-8?B?ZmV5bUJlZkdEdElnUVdndWlnaS9aaDQ2dG05UDF5Z3JxczBDOXc1R24zTm5I?=
 =?utf-8?B?S1RUTlQ4ZG5IOHFHNFYyaGtjSmhELzJodk1jcGw2VHVmSlo4MXpRQzN4UzMy?=
 =?utf-8?B?bDczczlNTFo4d1R0Y2h6dloybEwxM003TWpPaHNGYTZCNGpUMy93VklzY2FU?=
 =?utf-8?B?WnBWMUxQa2xFZ1Nhbzc3YzVOU2RNZnN2QmJ4bS9QNFVZdW0zTGF6aDI4ZEZL?=
 =?utf-8?B?d1ZscTVScHhEY0hRd0dqUFNGdHhOUmRneTRDajBvU1JhMzhNRVpHei9EN3hz?=
 =?utf-8?B?UTZ0RWJEMzlYZHMrekkzMGFhMU5LcjZ5NGQ3ek1LYWo2anNoUVBqWUo2ZVB5?=
 =?utf-8?B?a1NWSS9hN0U1V1JTbXpGOVlveWJSRmgycTBLd1J1anRVajFWNDBIVURWQlF6?=
 =?utf-8?B?d0ZvZm1uUDVxVHhNYWVHaEJMZVg4ejRZdVZlMk1XemJralNZWkg3ZDNZVUZN?=
 =?utf-8?B?ZTBTR1pybjNmTVovcE16QlVKeVAxVkJIUjdIWjR6TW9UT1RxWWk4QVU4R0w3?=
 =?utf-8?B?a0w2SytvaG1ybkhhdEVJS1c1RFVMczB6T3B6VFUvaStia1h0OHVQUE02dW5L?=
 =?utf-8?B?bXg3VlRWK0ZXODZ1SVFvbXplSXltbWtxaDM4NVRLbW50MkJ4NFZoQlE5NUFu?=
 =?utf-8?B?b2VlTlRUenhLTkljVlQvanN4Y0paRDFxTEhLT0ZWc1RLY1hvTkdDT0N5K2tJ?=
 =?utf-8?B?ZVJkVjhQQzUrdVRPUWhuMXFDNUVhNjZ3d1gvMEZ4NHhKRG1tRk9Zb2VQZGJx?=
 =?utf-8?B?NzdnWmlHSHIzcGRUN1d0dWF0OEdVTlZnZG14RmUyWWk5ZGQ0NjdNUFhHMWZl?=
 =?utf-8?B?NWxzM0VkUCtIUDlKWVh5YWtlejZGMXVjVlpzU3lIRjIrSlAzNTVvWG1KQmVE?=
 =?utf-8?B?QUZ5eWJTY2pRSjdDajZoZWgxajFodS9GcEYxZTAzcGxYclpMcjVRQTRaZ2o0?=
 =?utf-8?B?c3BsOFdHS1RidUhzVGtCc0toSGVKay85RW5xdU9TWWtEMUxVekhQV2Njdkc5?=
 =?utf-8?B?RThraDk1UlpTN1o4Um84MDBoZFVCU0lrU0tydkZVZW00N2VtRzRVOGJmWkE0?=
 =?utf-8?B?eXkrTFVxMkFXbTRSWGxrOXZGbWRVQjJuU1gwYTBoTDBHR0RidnpUVnh1N0hG?=
 =?utf-8?B?Qzg5NjhVZDdGdXlEWWFUOU5WSHlKR0xZZ2hQZnBtYmJ2czk1Z0pJY1VuT2xu?=
 =?utf-8?B?NXhFbEhoR29xVTFtUlF5Rk8xWVRFMm9iY0FvNkJ6S25mVzluN1U3blFVelo2?=
 =?utf-8?B?c0JSZC90bnR3K1E2ekV1MGJXYk9sZklWMGNQS2g0NkZ5SFJ2dnE5cnl5eVdx?=
 =?utf-8?Q?Yr3UMsNLUXa33Fjp+dEGniKaXxglnwSxnadYQvpAfV69?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 194e56c0-2b84-4342-4d8b-08db15164fc1
X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB3688.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2023 20:49:37.6464
 (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: H1l/P8qYX9NIQdc5GZmGE52AI1mL/fyhxKeZavI3ugE/JzRZQYQc9V7kmL5/AOMW
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR12MB8352
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/22/23 11:06 AM, Mathieu Poirier wrote:
> On Mon, Feb 13, 2023 at 01:18:26PM -0800, Tanmay Shah wrote:
>> [ ... ]
>> +
>>   /*
>>    * zynqmp_r5_set_mode()
>>    *
>> @@ -617,7 +819,7 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
>>   	return 0;
>>   }
>>   
>> -static const struct rproc_ops zynqmp_r5_rproc_ops = {
>> +static struct rproc_ops zynqmp_r5_rproc_ops = {
>>   	.prepare	= zynqmp_r5_rproc_prepare,
>>   	.unprepare	= zynqmp_r5_rproc_unprepare,
>>   	.start		= zynqmp_r5_rproc_start,
>> @@ -642,6 +844,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>   {
>>   	struct zynqmp_r5_core *r5_core;
>>   	struct rproc *r5_rproc;
>> +	struct mbox_info *ipi;
>>   	int ret;
>>   
>>   	/* Set up DMA mask */
>> @@ -649,12 +852,23 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>   	if (ret)
>>   		return ERR_PTR(ret);
>>   
>> +	/*
>> +	 * If mailbox nodes are disabled using "status" property then setting up
>> +	 * mailbox channels will be failed. In that case we don't really need
>> +	 * kick() operation. Include .kick() only if mbox channels are acquired
>> +	 * successfully.
>> +	 */
>> +	ipi = zynqmp_r5_setup_mbox(cdev);
>> +	if (ipi)
>> +		zynqmp_r5_rproc_ops.kick = zynqmp_r5_rproc_kick;
>> +
>>   	/* Allocate remoteproc instance */
>>   	r5_rproc = rproc_alloc(cdev, dev_name(cdev),
>>   			       &zynqmp_r5_rproc_ops,
>>   			       NULL, sizeof(struct zynqmp_r5_core));
>>   	if (!r5_rproc) {
>>   		dev_err(cdev, "failed to allocate memory for rproc instance\n");
>> +		zynqmp_r5_free_mbox(ipi);
>>   		return ERR_PTR(-ENOMEM);
>>   	}
>>   
>> @@ -665,6 +879,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>   	if (!r5_core->np) {
>>   		dev_err(cdev, "can't get device node for r5 core\n");
>>   		ret = -EINVAL;
>> +		zynqmp_r5_free_mbox(ipi);
>>   		goto free_rproc;
>>   	}
>>   
>> @@ -672,10 +887,17 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>   	ret = rproc_add(r5_rproc);
>>   	if (ret) {
>>   		dev_err(cdev, "failed to add r5 remoteproc\n");
>> +		zynqmp_r5_free_mbox(ipi);
>>   		goto free_rproc;
>>   	}
>>   
>> +	if (ipi) {
>> +		r5_core->ipi = ipi;
>> +		ipi->r5_core = r5_core;
>> +	}
>> +
>>   	r5_core->rproc = r5_rproc;
>> +
>>   	return r5_core;
>>   
>>   free_rproc:
>> @@ -918,6 +1140,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>>   	while (i >= 0) {
>>   		put_device(child_devs[i]);
>>   		if (r5_cores[i]) {
>> +			zynqmp_r5_free_mbox(r5_cores[i]->ipi);
> The mailboxes are initialized in zynqmp_r5_add_rproc_core() but free'd here in
> case of trouble, which introduces coupling between the two functions.  I suggest
> moving zynqmp_r5_setup_mbox() in zynqmp_r5_cluster_init() and initialize both
> mailboxes in it.
>
> I am done reviewing this set.


Ack. Yes that makes sense.

Thanks,

Tanmay


>
> Thanks,
> Mathieu
>
> Thanks,
> Mathieu
>
>>   			of_reserved_mem_device_release(r5_cores[i]->dev);
>>   			rproc_del(r5_cores[i]->rproc);
>>   			rproc_free(r5_cores[i]->rproc);
>> @@ -942,6 +1165,7 @@ static void zynqmp_r5_cluster_exit(void *data)
>>   
>>   	for (i = 0; i < cluster->core_count; i++) {
>>   		r5_core = cluster->r5_cores[i];
>> +		zynqmp_r5_free_mbox(r5_core->ipi);
>>   		of_reserved_mem_device_release(r5_core->dev);
>>   		put_device(r5_core->dev);
>>   		rproc_del(r5_core->rproc);
>> -- 
>> 2.25.1
>>