+ Marc (for the IRQCHIP implementation review)
On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote:
>
> On 2025/1/19 20:23, Manivannan Sadhasivam wrote:
> > On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote:
> > > From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> > >
> > > Add support for PCIe controller in SG2042 SoC. The controller
> > > uses the Cadence PCIe core programmed by pcie-cadence*.c. The
> > > PCIe controller will work in host mode only.
> > >
> > Please add more info about the IP. Like IP revision, PCIe Gen, lane count,
> > etc...
> ok.
> > > Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> > > ---
> > > drivers/pci/controller/cadence/Kconfig | 13 +
> > > drivers/pci/controller/cadence/Makefile | 1 +
> > > drivers/pci/controller/cadence/pcie-sg2042.c | 528 +++++++++++++++++++
> > > 3 files changed, 542 insertions(+)
> > > create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c
> > >
> > > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> > > index 8a0044bb3989..292eb2b20e9c 100644
> > > --- a/drivers/pci/controller/cadence/Kconfig
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP
> > > endpoint mode. This PCIe controller may be embedded into many
> > > different vendors SoCs.
> > > +config PCIE_SG2042
> > > + bool "Sophgo SG2042 PCIe controller (host mode)"
> > > + depends on ARCH_SOPHGO || COMPILE_TEST
> > > + depends on OF
> > > + select IRQ_MSI_LIB
> > > + select PCI_MSI
> > > + select PCIE_CADENCE_HOST
> > > + help
> > > + Say Y here if you want to support the Sophgo SG2042 PCIe platform
> > > + controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
> > > + PCIe core.
> > > +
> > > config PCI_J721E
> > > bool
> > > @@ -67,4 +79,5 @@ config PCI_J721E_EP
> > > Say Y here if you want to support the TI J721E PCIe platform
> > > controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
> > > core.
> > > +
> > Spurious newline.
>
> oops, I will fix this.
>
> [......]
>
> > > +/*
> > > + * SG2042 PCIe controller supports two ways to report MSI:
> > > + *
> > > + * - Method A, the PCIe controller implements an MSI interrupt controller
> > > + * inside, and connect to PLIC upward through one interrupt line.
> > > + * Provides memory-mapped MSI address, and by programming the upper 32
> > > + * bits of the address to zero, it can be compatible with old PCIe devices
> > > + * that only support 32-bit MSI address.
> > > + *
> > > + * - Method B, the PCIe controller connects to PLIC upward through an
> > > + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
> > > + * controller provides multiple(up to 32) interrupt sources to PLIC.
> > > + * Compared with the first method, the advantage is that the interrupt
> > > + * source is expanded, but because for SG2042, the MSI address provided by
> > > + * the MSI controller is fixed and only supports 64-bit address(> 2^32),
> > > + * it is not compatible with old PCIe devices that only support 32-bit MSI
> > > + * address.
> > > + *
> > > + * Method A & B can be configured in DTS, default is Method B.
> > How to configure them? I can only see "sophgo,sg2042-msi" in the binding.
>
>
> The value of the msi-parent attribute is used in dts to distinguish them,
> for example:
>
> ```dts
>
> msi: msi-controller@7030010300 {
> ......
> };
>
> pcie_rc0: pcie@7060000000 {
> msi-parent = <&msi>;
> };
>
> pcie_rc1: pcie@7062000000 {
> ......
> msi-parent = <&msi_pcie>;
> msi_pcie: interrupt-controller {
> ......
> };
> };
>
> ```
>
> Which means:
>
> pcie_rc0 uses Method B
>
> pcie_rc1 uses Method A.
>
Ok. you mentioned 'default method' which is not accurate since the choice
obviously depends on DT. Maybe you should say, 'commonly used method'? But both
the binding and dts patches make use of in-built MSI controller only (method A).
In general, for MSI implementations inside the PCIe IP, we don't usually add a
dedicated devicetree node since the IP is the same. But in your reply to the my
question on the bindings patch, you said it is a separate IP. I'm confused now.
> [......]
>
> > > +struct sg2042_pcie {
> > > + struct cdns_pcie *cdns_pcie;
> > > +
> > > + struct regmap *syscon;
> > > +
> > > + u32 link_id;
> > > +
> > > + struct irq_domain *msi_domain;
> > > +
> > > + int msi_irq;
> > > +
> > > + dma_addr_t msi_phys;
> > > + void *msi_virt;
> > > +
> > > + u32 num_applied_vecs; /* used to speed up ISR */
> > > +
> > Get rid of the newline between struct members, please.
> ok
> > > + raw_spinlock_t msi_lock;
> > > + DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> > > +};
> > > +
> > [...]
> >
> > > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,
> > > + struct device_node *msi_node)
> > > +{
> > > + struct device *dev = pcie->cdns_pcie->dev;
> > > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
> > > + struct irq_domain *parent_domain;
> > > + int ret = 0;
> > > +
> > > + if (!of_property_read_bool(msi_node, "msi-controller"))
> > > + return -ENODEV;
> > > +
> > > + ret = of_irq_get_byname(msi_node, "msi");
> > > + if (ret <= 0) {
> > > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node);
> > > + return ret;
> > > + }
> > > + pcie->msi_irq = ret;
> > > +
> > > + irq_set_chained_handler_and_data(pcie->msi_irq,
> > > + sg2042_pcie_msi_chained_isr, pcie);
> > > +
> > > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
> > > + &sg2042_pcie_msi_domain_ops, pcie);
> > > + if (!parent_domain) {
> > > + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode);
> > > + return -ENOMEM;
> > > + }
> > > + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
> > > +
> > The MSI controller is wired to PLIC isn't it? If so, why can't you use
> > hierarchial MSI domain implementation as like other controller drivers?
>
> The method used here is somewhat similar to dw_pcie_allocate_domains() in
> drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is
> about Method A, the PCIe controller implements an MSI interrupt controller
> inside, and connect to PLIC upward through only ONE interrupt line. Because
> MSI to PLIC is multiple to one, I use linear mode here and use chained ISR
> to handle the interrupts.
>
Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP
implementation part.
- Mani
--
மணிவண்ணன் சதாசிவம்
