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

[Gupta, Nipun]

> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Tuesday, May 9, 2023 1:32 PM
> To: Gupta, Nipun <Nipun.Gupta@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> maz@xxxxxxxxxx; jgg@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; Anand, Harpreet
> <harpreet.anand@xxxxxxx>; Jansen Van Vuuren, Pieter <pieter.jansen-
> van-vuuren@xxxxxxx>; Agarwal, Nikhil <nikhil.agarwal@xxxxxxx>; Simek,
> Michal <michal.simek@xxxxxxx>; Gangurde, Abhijit
> <abhijit.gangurde@xxxxxxx>; Cascon, Pablo <pablo.cascon@xxxxxxx>;
> Gupta, Nipun <Nipun.Gupta@xxxxxxx>
> Subject: Re: [PATCH] cdx: add MSI support for CDX bus
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, May 08 2023 at 19:39, Nipun Gupta wrote:
> > Add CDX-MSI domain with gic-its domain as 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.
> 
> This lacks any information why this needs to have a separate irq domain
> and what this is about. Changelogs need to be self explanatory and
> providing a link to some RFC series which might have more information
> does not cut it.
> 
> Just for the record. I complained about the useless changelog in that
> RFC series already.
> 
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> 
> This Signed-off-by chain is broken. If this is intended to denote
> co-authorship, then please follow:
> 
>   https://www.kernel.org/doc/html/latest/process/submitting-
> patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
> > +static struct irq_chip cdx_msi_irq_chip = {
> > +     .name                   = "CDX-MSI",
> > +     .irq_mask               = irq_chip_mask_parent,
> > +     .irq_unmask             = irq_chip_unmask_parent,
> > +     .irq_eoi                = irq_chip_eoi_parent,
> > +     .irq_set_affinity       = msi_domain_set_affinity,
> > +     .irq_write_msi_msg      = cdx_msi_write_msg
> > +};
> 
> The only real CDX specific functionality here is a CDX specific
> irq_write_msi_msg() callback, right?
> 
> And I gave you a pointer how this should be handled, but instead of
> helping this effort along you go off and implement it differently just
> because. Sigh!

Hi Thomas,

As you rightly mentioned the irq_chip has only irq_write_msi_msg() as
callback, but there is also cdx_msi_prepare() in msi_domain_ops which
needs to fetch device ID from CDX device, due to which we are currently
using separate CDX domain.

IIUC, as per your suggestion we should have CDX bus token added into 
its_init_dev_msi_info() of
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git/tree/drivers/irqchip/irq-gic-v3-its-msi-parent.c?h=devmsi-arm,
and register CDX specific 'msi_prepare' here; so that we can use
msi_create_device_irq_domain() to create a per device domain?

Thanks,
Nipun

> 
> Thanks,
> 
>         tglx