RE: [EXT] Re: [PATCH v6 4/4] pcie: endpoint: pci-epf-vntb: add endpoint MSI support

[Frank Li]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: Thursday, September 1, 2022 8:39 PM
> To: Frank Li <frank.li@xxxxxxx>
> Cc: maz@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> jdmason@xxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-
> imx <linux-imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> ntb@xxxxxxxxxxxxxxx; lznuaa@xxxxxxxxx
> Subject: Re: [EXT] Re: [PATCH v6 4/4] pcie: endpoint: pci-epf-vntb: add
> endpoint MSI support
> 
> Caution: EXT Email
> 
> On Wed, Aug 31, 2022 at 04:19:17PM +0000, Frank Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > Sent: Wednesday, August 31, 2022 5:42 AM
> > > To: Frank Li <frank.li@xxxxxxx>
> > > Cc: maz@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> > > krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> > > s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> > > kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> > > <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> > > jdmason@xxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-
> linux-
> > > imx <linux-imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> > > ntb@xxxxxxxxxxxxxxx; lznuaa@xxxxxxxxx
> > > Subject: [EXT] Re: [PATCH v6 4/4] pcie: endpoint: pci-epf-vntb: add
> endpoint
> > > MSI support
> > >
> > > Caution: EXT Email
> > >
> > > On Thu, Aug 18, 2022 at 10:11:27AM -0500, Frank Li wrote:
> > > >                         ┌───────┐          ┌──────────┐
> > > >                         │       │          │          │
> > > >       ┌─────────────┐   │       │          │ PCI Host │
> > > >       │ MSI         │◄┐ │       │          │          │
> > > >       │ Controller  │ │ │       │          │          │
> > > >       └─────────────┘ └─┼───────┼──
> ─
> > > ───────┼─BAR0     │
> > > >                         │ PCI   │          │ BAR1     │
> > > >                         │ Func  │          │ BAR2     │
> > > >                         │       │          │ BAR3     │
> > > >                         │       │          │ BAR4     │
> > > >                         │       ├─────────►│          │
> > > >                         └───────┘          └──────────┘
> > > >
> > >
> > > This diagram doesn't say which side is host and which one is endpoint.
> > > And not conveying any useful information.
> >
> > [Frank Li] At V2 version, this diagram is in cover letter.  Bjorn suggest move
> to here.
> > I think you have good background knowledge.  But it should be helpful for
> new
> > People,  who just touch this area.
> >
> 
> Having the block diagram always helps but my point is that this diagram
> doesn't
> convey the immediate knowledge that it is supposed to do so. Like there is no
> partition between host and endpoint and you did not add any explanation
> about
> it in the below text. So in v2, please incorporate those.
> 
> > I already mark "PCI Func" and "PCI Host".
> >
> 
> Sorry, that's not helpful and you need to improve it.
> 
> > >
> > > > Linux supports endpoint functions. PCI Host write BAR<n> space like
> write
> > > > to memory. The EP side can't know memory changed by the host driver.
> > > >
> > >
> > > I think you just say, that there is no defined way of raising IRQs by host
> > > to the endpoint.
> > >
> > > > PCI Spec has not defined a standard method to do that. Only define
> MSI(x)
> > > > to let EP notified RC status change.
> > > >
> > >
> > > MSI is from EP, right? Throughout the driver you should call it as
> "doorbell"
> > > and not MSI.
> >
> > [Frank Li] What's I want said is that PCI standard define MSI(x) to let EP
> notify RC.
> > But there are not standard way for reverse direction.  MSI should be
> correct here.
> >
> 
> Right. But also use "MSI/MSI-X" instead of "MSI(x)"
> 
> > >
> > > > The basic idea is to trigger an IRQ when PCI RC writes to a memory
> > > > address. That's what MSI controller provided. EP drivers just need to
> > > > request a platform MSI interrupt, struct msi_msg *msg will pass down a
> > > > memory address and data. EP driver will map such memory address to
> one
> > > of
> > > > PCI BAR<n>.  Host just writes such an address to trigger EP side irq.
> > > >
> > >
> > > IIUC (by looking at other patches in the series), the memory assigned for
> BAR
> > > region by the PCI host is mapped to the platform interrupt controller in
> > > PCI Endpoint. Such that, whenever the PCI host writes to the BAR region,
> it
> > > will trigger an IRQ in the Endpoint.
> > >
> > > This kind of setup is available in other platforms like Qualcomm where
> the
> > > mapping of a register region available in BAR0 and interrupt controller is
> > > done in the hardware itself. So whenever the PCI host writes to that
> register
> > > in BAR0, an IRQ will be delivered to the endpoint.
> >
> > [Frank Li] Yes,  not all platform have it. And EP driver have not provide a API
> > to get register region.  I think platform msi API is pretty good API.
> > Many system have GIC ITS,  so EP function driver can use it.  Our test
> platform
> > have not ITS yet,  so we added a simple MU-MSI driver to do it. I think
> qualcomm
> > platform can use similar method.  So all EP function driver can use common
> method
> > to get notification from PCI host.
> >
> 
> What is the common method here? If you want to make this doorbell feature
> common across all EPF drivers, then you need to provide EPF APIs.

[Frank Li] Existed MSI API have matched requirement.  EPF just reused it.
This patch provided demo method to show how to use platform MSI API to make this
Doorbell. 

> 
> > >
> > > > Add MSI support for pci-epf-vntb. pci-epf-vntb driver query if system
> > > > have MSI controller. Setup doorbell address according to struct
> msi_msg.
> > > >
> > > > So PCIe host can write this doorbell address to triger EP side's irq.
> > > >
> > > > If no MSI controller exist, fall back to software polling.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 134 +++++++++++++++-
> --
> > > >  1 file changed, 112 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> 
> [...]
> 
> > > > +static void epf_ntb_epc_msi_init(struct epf_ntb *ntb)
> > > > +{
> > > > +     struct device *dev = &ntb->epf->dev;
> > > > +     struct irq_domain *domain;
> > > > +     int virq;
> > > > +     int ret;
> > > > +     int i;
> > > > +
> > > > +     domain = dev_get_msi_domain(ntb->epf->epc->dev.parent);
> > > > +     if (!domain)
> > > > +             return;
> > > > +
> > > > +     dev_set_msi_domain(dev, domain);
> > > > +
> > > > +     if (platform_msi_domain_alloc_irqs(&ntb->epf->dev,
> > > > +             ntb->db_count,
> > > > +             epf_ntb_write_msi_msg)) {
> > > > +             dev_info(dev, "Can't allocate MSI, fall back to poll mode\n");
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     dev_info(dev, "vntb use MSI as doorbell\n");
> > > > +
> > >
> > > Why are you using the interrupt controller as the MSI controller? Why not
> > > just
> > > a plain interrupt controller?
> >
> > [Frank Li] what's your means?   I think only MSI controller support write
> memory to trigger irq.
> >
> 
> From EPF driver perspective, only the IRQs need to be requested, right? So
> why
> cannot you expose MU as a generic irqchip driver, instead of a MSI controller?	

[Frank Li] No.  EPF need two information. 
1. IRQ number. 
2. A physical address, which map such physical address to PCIe Bar, so PCI host can 
Write it and trigger EP side IRQ.

> 
> Thanks,
> Mani
> 
> --
> மணிவண்ணன் சதாசிவம்