Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform MSI as doorbell

From: Thomas Gleixner <tglx@linutronix.de>
To: Frank Li <Frank.Li@nxp.com>, lpieralisi@kernel.org
Cc: Frank.Li@nxp.com, aisheng.dong@nxp.com, bhelgaas@google.com,
	devicetree@vger.kernel.org, festevam@gmail.com,
	imx@lists.linux.dev, jdmason@kudzu.us, kernel@pengutronix.de,
	kishon@ti.com, krzysztof.kozlowski+dt@linaro.org, kw@linux.com,
	linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	lorenzo.pieralisi@arm.com, lznuaa@gmail.com,
	manivannan.sadhasivam@linaro.org, maz@kernel.org,
	ntb@lists.linux.dev, peng.fan@nxp.com, robh+dt@kernel.org,
	s.hauer@pengutronix.de, shawnguo@kernel.org
Subject: Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform MSI as doorbell
Date: Mon, 28 Nov 2022 20:14:44 +0100	[thread overview]
Message-ID: <87wn7evql7.ffs@tglx> (raw)
In-Reply-To: <20221124055036.1630573-3-Frank.Li@nxp.com>

On Thu, Nov 24 2022 at 00:50, Frank Li wrote:
> ┌────────────┐   ┌───────────────────────────────────┐   ┌────────────────┐
> │            │   │                                   │   │                │
> │            │   │ PCI Endpoint                      │   │ PCI Host       │
> │            │   │                                   │   │                │
> │            │◄──┤ 1.platform_msi_domain_alloc_irqs()│   │                │
> │            │   │                                   │   │                │
> │ MSI        ├──►│ 2.write_msi_msg()                 ├──►├─BAR<n>         │
> │ Controller │   │   update doorbell register address│   │                │
> │            │   │   for BAR                         │   │                │
> │            │   │                                   │   │ 3. Write BAR<n>│
> │            │◄──┼───────────────────────────────────┼───┤                │
> │            │   │                                   │   │                │
> │            ├──►│ 4.Irq Handle                      │   │                │
> │            │   │                                   │   │                │
> │            │   │                                   │   │                │
> └────────────┘   └───────────────────────────────────┘   └────────────────┘
>
> Using platform MSI interrupt controller as endpoint(EP)'s doorbell.

Can you please explain what the MSI controller is in this picture? MSI
controller is not a term which is common in the interrupt handling
landscape despite the fact that it's pretty wide spread in device tree
bindings presumably through intensive copy & pasta cargo cult.

> Basic working follow as
> 1. EP function driver call platform_msi_domain_alloc_irqs() alloc a
> MSI irq from MSI controller with call back function write_msi_msg();
> 2. write_msg_msg will config BAR and map to address defined in msi_msg;
> 3. Host side trigger an IRQ at Endpoint by write to BAR region.

You're explaining what the code does, but fail to explain the underlying
mechanisms.

Platform MSI is definitely the wrong mechanism here. Why?

This is about a PCIe endpoint, which is usually handled by a PCI/MSI
interrupt domain. Obviously this usage does not fit into the way how the
"global" PCI/MSI domains work.

There is upcoming work and at least the generic parts should show up in
6.2 which addresses exactly the problem you are trying to solve:

   https://lore.kernel.org/all/20221124225331.464480443@linutronix.de
   https://lore.kernel.org/all/20221124230505.073418677@linutronix.de

plus the prove that the platform MSI mess can be replaced by this:

   https://lore.kernel.org/all/20221121135653.208611233@linutronix.de

NTB in it's current form should never have happened, but that's water
down the bridge.

What you really want is:

  1) Convert your platform to the new MSI parent model

  2) Utilize PCI/IMS which is giving you exactly what you need with
     proper PCI semantics

Thanks,

        tglx