Received-SPF: pass (google.com: domain of linux-kernel+bounces-74710-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <929bf3bd-fb5a-4d1d-aa46-a5ce43ce574b@amd.com>
Date: Wed, 21 Feb 2024 18:10:39 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7] cdx: add MSI support for CDX bus
To: Thomas Gleixner <tglx@linutronix.de>, gregkh@linuxfoundation.org,
 maz@kernel.org, jgg@ziepe.ca, linux-kernel@vger.kernel.org
Cc: git@amd.com, harpreet.anand@amd.com, pieter.jansen-van-vuuren@amd.com,
 nikhil.agarwal@amd.com, michal.simek@amd.com, abhijit.gangurde@amd.com,
 srivatsa@csail.mit.edu
References: <20240202113811.2546311-1-nipun.gupta@amd.com>
 <877cj0a4fj.ffs@tglx>
Content-Language: en-US
From: "Gupta, Nipun" <nipun.gupta@amd.com>
In-Reply-To: <877cj0a4fj.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RUxlU1ZEU2grTTRPYUVZZkIvd08vbFJ1cnNraU1JY0IxaW5QV1ZXV2RYenQ1?=
 =?utf-8?B?SEFudWVCVFVrUTBWaTQ5WnlKZEsyTTNiMzNTYWdZaWFXbE9oc3R0UjN5V2xh?=
 =?utf-8?B?WWlaK2t3YzVZNkQ5cUtyYStxVGZTaG5wV1NNdzFyOTBJUGViVmo3M0xjV1VP?=
 =?utf-8?B?Z2R2cWk2MFkzYmlhNUswanc3NURyc2FzcEJWcFQ0MzQxTXVkallJSUlNajlZ?=
 =?utf-8?B?TnFXQmdqM01NdENsUUo1NGR5RlRUTnR4ZG84cS9hSTJxVzRZcjlCYWpxMXlt?=
 =?utf-8?B?WUZBSXI4emViRzhmalQ3elFCOFRWMzVhbGZDR1NNbWZzcHBNb2E5bVU0SU5L?=
 =?utf-8?B?bXZGTVpQdUt1VDIrQ1RKckp3UmZRam55SWZBRmEyQWZKVFphZ0ZBZzFGdTBV?=
 =?utf-8?B?SzVvUUQ4dFIxMmRseXdXcHpYRnRiQng1d0VGTzVnSndaT2c1QURMYTdtZFBY?=
 =?utf-8?B?V3hZU0VVcnllakdYcnYzQ0lCcUlBNTduZ0lEb25iY2RocE96SHpqTDd3YWE4?=
 =?utf-8?B?dmI1Wkw3VDc2SHpqb1dGWVh0dUVObWowV3BkQmg5a2JTWE9iQXFoM21qV29y?=
 =?utf-8?B?ZGFQaTR0S3NLTUJtNU5hWFNGQWFiWDlubGpQTzFoQnZ2a3pPSnVJbDQ1WjRM?=
 =?utf-8?B?WnBJT3pwUkJHeC9MTS9ndldmOVBOczQ0d095KzF2eFBJYk41d01ob2h5WGI0?=
 =?utf-8?B?bmF4TlFMaDZoa3dLN2hCV3cyZjR0bVlYVXk0YjRvNzlnRjFQVkp4bFBXSWpW?=
 =?utf-8?B?RnNUMm1ZUGNHU2dnUHQ5TVh6QWRqMk1NNVF3OE9tWjRqbGpCcm5xeVNDSjcr?=
 =?utf-8?B?Um4yS3RJRmp0R2Vuc05sNitlNHZBNEhrQTZVQTNvWjI0ZzA4OWFFQnFWMEJY?=
 =?utf-8?B?VVhmVGtkQ0swb3E3V2xJSmlVNWpGWDhpdkVwcG5kMU42MkJhWU0zNGlSM20y?=
 =?utf-8?B?ZlNJRTVDT1pxdXd1M3VhczlySXFDV2FoSWFKVURISWE3dGVyWnRRakxYTlk1?=
 =?utf-8?B?U0UxQWFONHprdmU4RzJNUWRjQWxxRjQrYkJYdXVmaEsrSUZEcnJET0UvblYw?=
 =?utf-8?B?aWtGTWZBU2hPdFJDNURiUldNUEptQWk0cFhlQVU4dUsvL3VxaFFleC9wR0to?=
 =?utf-8?B?ZWgyZ2E3Y3dYN01zZmNyZnQyT00rYzZ3WGdrS1FBdnFBL0JDTURpbHRzMmRI?=
 =?utf-8?B?c2hqLzJ4eWJYNXRqbzdZUzJtaGsrZDhVYzBVYlhETWpUNlZoRFdEWGpwM0o0?=
 =?utf-8?B?U21kcFVJV3FLNWY0cko0aWk3dmp3NTdtZHBqMGZUQWN3ZkZFbUZ5UUlJNVI4?=
 =?utf-8?B?dldTYm1WZTRvMVhRLzFUeElBRUZQb3AxclpnaEhOcTI3Wkw0UFFVWDdES0d0?=
 =?utf-8?B?V3lZSmF0NmI4L0FtZEZrYWdKYTEycU9NcW5mS05seFIyMHJjTVpaUENFZTE5?=
 =?utf-8?B?M3FpRGJRM2RoZU1Pb2pBZlNDU1FTM0hLNkZ3NHFmUzBxZGtvQTlFMzF3d2RN?=
 =?utf-8?B?dEZLYTY1bDdFMVVnT1pyOEZYZmVDNkp0UHo1alhyQis3b2hmY1NndUhaYnVP?=
 =?utf-8?B?aDVhUFhPV3lMbDNIL3lrTHMza1RrQzlkSHpOdkkwSmhpNmdpQWF1V1RRT3la?=
 =?utf-8?B?d3lEWWpzNW8zZHFranZ2TWNNanZVbHB4MWE0Z0lQOXdwU0UwamVKNkN5Titw?=
 =?utf-8?B?SEJNRksyb0Q2YUMzTTR1d2t5Um5WVlpDVUZKamZpN1RwSk1YZk5UUFVhN1pw?=
 =?utf-8?B?WWlaU084dGRweklQVEF5ZGhqT0xCU2NIVDV0OTVTR09PWERkcDcwMTRzUC9M?=
 =?utf-8?B?L1JaNkJDYytWdWFnRndZWUpZeUwvcmZzQ2NvSUgrdGxDU1BMQlFXRWNNbW9H?=
 =?utf-8?B?bW9sTTBBMllLNWxKM2FDRlkzSkNyQTk5VDZyT1FHSTlrNVlVUlQ4dWpmdGZW?=
 =?utf-8?B?ZkdjTnF0bTZ1blZ6NEl0S2dEVnFWeWlGdElHQVlqZlZaY2R0Z1lxQThMRlA2?=
 =?utf-8?B?emt4MHg2QkdteDluMTJBK0FVR24rdzFjdFc2S3krN2hMRFNkN29jSHhBWDR1?=
 =?utf-8?B?UTNKRm1TOHc5YXR3Sks3SkJaTE0wRGtvdjB4eXFLS1pqcG02ZnlmU2NOWU51?=
 =?utf-8?Q?6PqtLcr1hwqKIwuRcXSshYJ7i?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d0567f77-9a2d-4734-54ab-08dc32da5547
X-MS-Exchange-CrossTenant-AuthSource: CH3PR12MB9193.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2024 12:40:49.6034
 (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: DeoPgbk936mOSpugmq2WzZIjBJyrdzqNNX7zEnjVeKnK0NIye5Chs3cvAXqymaf/
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB8018


On 2/19/2024 8:26 PM, Thomas Gleixner wrote:
> On Fri, Feb 02 2024 at 17:08, Nipun Gupta wrote:
>> Add CDX-MSI domain per CDX controller with gic-its domain as
>> a parent, to support MSI for CDX devices. CDX devices allocate
>> MSIs from the CDX domain. Also, introduce APIs to alloc and free
>> IRQs for CDX domain.
>>
>> In CDX subsystem firmware is a controller for all devices and
>> their configuration. CDX bus controller sends all the write_msi_msg
>> commands to firmware running on RPU and the firmware interfaces with
>> actual devices to pass this information to devices
>>
>> Since, CDX controller is the only way to communicate with the Firmware
>> for MSI write info, CDX domain per controller required in contrast to
>> having a CDX domain per device.
>>
>> Changes v6->v7:
>> - Rebased on Linux 6.8-rc2
>>   ...
>> Changes v1->v2:
>> - fixed scenario where msi write was called asynchronously in
>>    an atomic context, by using irq_chip_(un)lock, and using sync
>>    MCDI API for write MSI message.
>> - fixed broken Signed-off-by chain.
> 
> Please put the Changes documentation after the --- separator. That's not
> part of the change log and just creates work for the maintainer to remove.

Thanks for the review. Sure, will fix in the next spin.

> 
>> +#ifdef CONFIG_GENERIC_MSI_IRQ
> 
> Why do you need this #ifdef? AFAICT it's completely pointless

Agree, will remove.

> 
>> +/**
>> + * cdx_msi_domain_init - Init the CDX bus MSI domain.
>> + * @dev: Device of the CDX bus controller
>> + *
>> + * Return: CDX MSI domain, NULL on failure
>> + */
>> +struct irq_domain *cdx_msi_domain_init(struct device *dev);
>> +#endif
>>   #endif /* _CDX_H_ */
> 
>> +	/*
>> +	 * dev_configure is a controller callback which can interact with
> 
> s/dev_configure/dev_configure()/ which makes it clear that this is about
> a function

makes sense. will update this.

> 
>> +	 * Firmware or other entities, and can sleep, so invoke this function
>> +	 * outside of the mutex lock.
> 
> s/lock/held region/

Will update this.

> 
>> +	 */
>> +	dev_config.type = CDX_DEV_MSI_CONF;
>> +	if (cdx->ops->dev_configure)
>> +		cdx->ops->dev_configure(cdx, cdx_dev->bus_num, cdx_dev->dev_num,
>> +					&dev_config);
> 
> Please use either a single line, which is within the 100 character limit
> or place brackets around the condition:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules
> 
> All over the place.

Sure will update.

> 
>> +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
>> +{
>> +	int ret;
>> +
>> +	ret = msi_setup_device_data(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = msi_domain_alloc_irqs_range(dev, MSI_DEFAULT_DOMAIN,
>> +					  0, irq_count - 1);
>> +	if (ret)
>> +		dev_err(dev, "Failed to allocate IRQs: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cdx_msi_domain_alloc_irqs);
> 
> Why does this need a special allocation function instead of setting the
> irq domain of the device and using the generic allocation function
> directly?

Do you suggest we call msi_setup_device_data() during the 
cdx_device_add() in the CDX bus where we also assign the IRQ domain to 
the device; so CDX device driver can directly call 
msi_domain_alloc_irqs_range() API to allocate the IRQs?

> 
>> +static int cdx_msi_prepare(struct irq_domain *msi_domain,
>> +			   struct device *dev,
>> +			   int nvec, msi_alloc_info_t *info)
>> +{
>> +	struct cdx_device *cdx_dev = to_cdx_device(dev);
>> +	struct device *parent = cdx_dev->cdx->dev;
>> +	struct msi_domain_info *msi_info;
>> +	u32 dev_id;
>> +	int ret;
>> +
>> +	/* Retrieve device ID from requestor ID using parent device */
>> +	ret = of_map_id(parent->of_node, cdx_dev->msi_dev_id, "msi-map",
>> +			"msi-map-mask", NULL, &dev_id);
>> +	if (ret) {
>> +		dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +#ifdef GENERIC_MSI_DOMAIN_OPS
>> +	/* Set the device Id to be passed to the GIC-ITS */
>> +	info->scratchpad[0].ul = dev_id;
>> +#endif
> 
> Is this ever used on a platform which does not have
> GENERIC_MSI_DOMAIN_OPS ?

This was added to enable the COMPILE_TEST.

> 
>> @@ -120,9 +135,13 @@ struct cdx_controller {
>>    * @req_id: Requestor ID associated with CDX device
>>    * @is_bus: Is this bus device
>>    * @enabled: is this bus enabled
>> + * @msi_dev_id: MSI Device ID associated with CDX device
>> + * @num_msi: Number of MSI's supported by the device
>>    * @driver_override: driver name to force a match; do not set directly,
>>    *                   because core frees it; use driver_set_override() to
>>    *                   set or clear it.
>> + * @irqchip_lock: lock to synchronize irq/msi configuration
>> + * @msi_write_pending: MSI write pending for this device
>>    */
>>   struct cdx_device {
>>   	struct device dev;
>> @@ -144,7 +163,11 @@ struct cdx_device {
>>   	u32 req_id;
>>   	bool is_bus;
>>   	bool enabled;
>> +	u32 msi_dev_id;
>> +	u32 num_msi;
>>   	const char *driver_override;
>> +	struct mutex irqchip_lock; /* Serialize write msi configuration */
> 
> This tail comment is pointless. It's already documented above, no?

Yes, will remove.

Thanks,
Nipun

> 
> Other than those nitpicks this looks reasonable.
> 
> Thanks,
> 
>          tglx