Re: [PATCH v7] cdx: add MSI support for CDX bus

["Gupta, Nipun" <nipun.gupta@amd.com>]

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