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

From: Nipun Gupta
Date: Mon May 15 2023 - 09:11:05 EST

On 5/12/2023 11:45 PM, Thomas Gleixner wrote:


On Fri, May 12 2023 at 19:50, Nipun Gupta wrote:
On 5/11/2023 3:59 AM, Thomas Gleixner wrote:
CDX is not any different than PCI. The actual "interrupt chip" is not
part of the bus, it's part of the device and pretending that it is a bus
specific thing is just running in to the same cul-de-sac sooner than

I understand your viewpoint, but would state that CDX bus is somewhat
different than PCI in the sense that firmware is a controller for
all the devices and their configuration. CDX bus controller sends all
the write_msi_msg commands to firmware running on RPU over the RPmsg and
it is the firmware which interfaces with actual devices to pass this
information to devices in a way agreed between firmware and device. The
only way to pass MSI information to device is via firmware and CDX bus
controller is only entity which can communicate with the firmware for

Fair enough, but we wouldn't had this dicussion if the above information
would have been part of the changelog. See?

Yes, you are correct, we will make sure to update and add more info in the changelog in next spin.

IIRC, there is a gap vs. interrupt affinity setting from user space,
which is irrelevant for I2C, SPI etc. configured interrupt chips as they
raise interrupt via an SoC interrupt pin and that's the entity which
does the affinity management w/o requiring I2C/SPI. IIRC I posted a
patch snippet to that effect in one of those lengthy PCI/MSI/IMS threads
because that is also required for MSI storage which happens to be in
queue memory and needs to be synchronized via some command channel. But
I can't be bothered to search for it as it's a no-brainer to fix that

Thanks for this analysis and pointing the hidden crucial issues with the
implementation. These needs to be fixed.

As per your suggestion, we can add Firmware interaction code in the
irq_bus_sync_xx APIs. Another option is to change the
cdx_mcdi_rpc_async() API to atomic synchronous API.

I'm not a great fan of that. Depending on how long this update takes the
CPU will busy wait for it to complete with interrupts disabled and locks

Agree. we are also inclined towards using irq_bus_sync_xx APIs. This would definitely solve the issue (#1 and #2) for the setup_irq which you mentioned.

For MSI affinity, since GIC-ITS always return IRQ_SET_MASK_OK_DONE, the irq_chip_write_msi_msg does not get called.

ret = parent->chip->irq_set_affinity(...);
// For GIC ITS it always return IRQ_SET_MASK_OK_DONE
if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
Since CDX bus is specific to ARM and uses GIC ITS, it seems we do not need to do anything here. Can you please share your opinion on this?


We are evaluating both the solutions and will update the
implementation accordingly.