Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Saturday, November 30, 2024 1:52 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; jingoohan1@xxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@xxxxxxx>
> Subject: Re: [PATCH 2/2] PCI: amd-mdb: Add AMD MDB Root Port driver
>
> On Wed, Nov 27, 2024 at 05:28:04PM +0530, Thippeswamy Havalige wrote:
> > Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.
> >
> > The Versal2 devices include MDB Module. The integrated block for MDB
> > along with the integrated bridge can function as PCIe Root Port
> > controller at
> > Gen5 speed.
>
> What speed is Gen5? Please include the numeric speed so we don't have to
> Google it.
Thanks for review, ll update in next patch
>
> > Bridge error and legacy interrupts in Versal2 MDB are handled using
> > Versal2 MDB specific interrupt line.
> >
> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 10 +
> > drivers/pci/controller/dwc/Makefile | 1 +
> > drivers/pci/controller/dwc/pcie-amd-mdb.c | 455
> > ++++++++++++++++++++++
> > 3 files changed, 466 insertions(+)
> > create mode 100644 drivers/pci/controller/dwc/pcie-amd-mdb.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index b6d6778b0698..e7ddab8da2c4 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -14,6 +14,16 @@ config PCIE_DW_EP
> > bool
> > select PCIE_DW
> >
> > +config PCIE_AMD_MDB
> > + bool "AMD PCIe controller (host mode)"
> > + depends on OF || COMPILE_TEST
> > + depends on PCI && PCI_MSI
> > + select PCIE_DW_HOST
> > + help
> > + Say Y here to enable PCIe controller support on AMD SoCs. The
> > + PCIe controller is based on DesignWare Hardware and uses AMD
> > + hardware wrappers.
>
> Alphabetize by vendor name. I suppose "Advanced" *would* sort before "Amazon",
> but since "AMD" isn't spelled out, I think we have to sort by the initialism and put
> "Ama" before "AMD".
Thanks for review, ll update in next patch
>
> > config PCIE_AL
> > bool "Amazon Annapurna Labs PCIe controller"
> > depends on OF && (ARM64 || COMPILE_TEST)
>
> > +static void amd_mdb_mask_leg_irq(struct irq_data *data)
>
> s/_leg_/_intx_/
Thanks for review, ll update in next patch
>
> > +{
> > + struct dw_pcie_rp *port = irq_data_get_irq_chip_data(data);
> > + struct amd_mdb_pcie *pcie;
> > + unsigned long flags;
> > + u32 mask, val;
> > +
> > + pcie = get_mdb_pcie(port);
>
> Here and elsewhere, this could be done in the automatic variable list above since
> this is non-interesting setup.
>
> > +static void amd_mdb_unmask_leg_irq(struct irq_data *data)
>
> Ditto.
Thanks for review, ll update in next patch
>
> > +static struct irq_chip amd_mdb_leg_irq_chip = {
>
> Ditto.
Thanks for review, ll update in next patch
>
> > +static int amd_mdb_pcie_rp_intx_map(struct irq_domain *domain,
> > + unsigned int irq, irq_hw_number_t hwirq)
>
> "_rp_" in name unnecessary.
>
Thanks for review, ll update in next patch
> > +static irqreturn_t amd_mdb_pcie_rp_intr_handler(int irq, void
> > +*dev_id) {
> > + struct dw_pcie_rp *port = dev_id;
> > + struct amd_mdb_pcie *pcie;
> > + struct device *dev;
> > + struct irq_data *d;
> > +
> > + pcie = get_mdb_pcie(port);
> > + dev = pcie->pci.dev;
> > +
> > + d = irq_domain_get_irq_data(pcie->mdb_domain, irq);
> > + if (intr_cause[d->hwirq].str)
> > + dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
> > + else
> > + dev_warn(dev, "Unknown IRQ %ld\n", d->hwirq);
> > +
> > + return IRQ_HANDLED;
>
> I see that some of these messages are "Correctable/Non-Fatal/Fatal error
> message"; I assume this Root Port doesn't have an AER Capability, and this
> interrupt is the "System Error" controlled by the Root Control Error Enable bits in the
> PCIe Capability? (See PCIe r6.0, sec 6.2.6)
>
> Is there any way to hook this into the AER handling so we can do something about
> it, since the devices *below* the Root Port may support AER and may have useful
> information logged?
>
> Since this is DWC-based, I suppose these are general questions that apply to all
> the similar drivers.
Thanks for review, We have this in our plan to hook platform specific error interrupts
to AER in future will add this support.
>
> > +static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
> > + struct platform_device *pdev)
> > +{
> > + struct dw_pcie *pci = &pcie->pci;
> > + struct dw_pcie_rp *pp = &pci->pp;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + pp->ops = &amd_mdb_pcie_host_ops;
>
> This is dw-related initialization; move it down just before the first use at
> dw_pcie_host_init().
Thanks for review, ll update in next patch
>
> > + pcie->mdb_base = devm_platform_ioremap_resource_byname(pdev,
> "mdb_pcie_slcr");
> > + if (IS_ERR(pcie->mdb_base))
> > + return PTR_ERR(pcie->mdb_base);
> > +
> > + ret = amd_mdb_pcie_rp_init_irq_domain(pcie, pdev);
>
> Other drivers use "*_pcie_init_irq_domain" (without "rp"). It's helpful to use similar
> names so it's easier to compare implementations.
>
> Since amd_mdb_pcie_free_irq_domains() cleans this up, I think both should end
> with "domains" (with an "s") so they match
Thanks for review, ll update in next patch
>
> > + if (ret)
> > + return ret;
> > +
> > + amd_mdb_pcie_rp_init_port(pcie, pdev);
>
> Other drivers use "*_pcie_init_port" (without "rp").
Thanks for review, ll update in next patch
>
> > + ret = amd_mdb_setup_irq(pcie, pdev);
> > + if (ret) {
> > + dev_err(dev, "Failed to set up interrupts\n");
> > + goto out;
> > + }
> > +
> > + ret = dw_pcie_host_init(pp);
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize host\n");
> > + goto out;
> > + }
> > +
> > + return 0;
> > +
> > +out:
> > + amd_mdb_pcie_free_irq_domains(pcie);
> > + return ret;
> > +}
>
> Bjorn
