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

[Manivannan Sadhasivam]

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.

> > 
> > > 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? 

Thanks,
Mani

-- 
மணிவண்ணன் சதாசிவம்