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

[Gupta, Nipun]

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