Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, February 12, 2025 12:04 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 v10 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver
>
> On Tue, Feb 11, 2025 at 12:08:51PM +0530, Thippeswamy Havalige wrote:
> > Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.
>
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -27,6 +27,17 @@ config PCIE_AL
> > required only for DT-based platforms. ACPI platforms with the
> > Annapurna Labs PCIe controller don't need to enable this.
> >
> > +config PCIE_AMD_MDB
> > + bool "AMD MDB Versal2 PCIe Host controller"
> > + depends on OF || COMPILE_TEST
> > + depends on PCI && PCI_MSI
> > + select PCIE_DW_HOST
> > + help
> > + Say Y here if you want to enable PCIe controller support on AMD
> > + Versal2 SoCs. The AMD MDB Versal2 PCIe controller is based on
> DesignWare
> > + IP and therefore the driver re-uses the Designware core functions to
> > + implement the driver.
>
> Wrap to fit in 75-78 columns like the rest of the file. This gets
> chopped off in an 80 column menuconfig window.
Thank you for reviewing, I will update it in next patch.
>
> > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c
>
> > +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
> > +
> > +#define AMD_MDB_PCIE_INTX_BIT(x)
> FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))
> > +
> > +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2)
> > +#define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x) BIT(((x) * 2) + 1)
>
> > +static void amd_mdb_intx_irq_mask(struct irq_data *data)
> > +{
> > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
> > + struct dw_pcie *pci = &pcie->pci;
> > + struct dw_pcie_rp *port = &pci->pp;
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&port->lock, flags);
> > + val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> > + val &= ~AMD_MDB_PCIE_INTX_BIT(data->hwirq);
>
> This doesn't look right to me. hwirq should be 0, 1, 2, or 3 (INTA,
> INTB, INTC, or INTD):
Thank you for reviewing,
Will update macro to following
#define AMD_MDB_PCIE_INTX_BIT(x) (1U << (2 * (x) + AMD_MDB_PCIE_INTR_INTX)).
>
> AMD_MDB_PCIE_INTX_BIT(0) == 0001 0000 (INTA assert)
> AMD_MDB_PCIE_INTX_BIT(1) == 0002 0000 (INTA deassert)
> AMD_MDB_PCIE_INTX_BIT(2) == 0004 0000 (INTB assert)
> AMD_MDB_PCIE_INTX_BIT(3) == 0008 0000 (INTB deassert)
>
> Maybe the AMD_MDB_TLP_IR_ENABLE_MISC register is laid out differently
> than AMD_MDB_TLP_IR_STATUS_MISC? If so, and you're updating a
> four-bit field, it needs a different GENMASK.
>
> > + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
>
> This *looks* like it's supposed to be a read/modify/write of
> AMD_MDB_TLP_IR_MASK_MISC, but you read
> AMD_MDB_TLP_IR_MASK_MISC and
> then write AMD_MDB_TLP_IR_ENABLE_MISC. Same below in
> amd_mdb_intx_irq_unmask().
- Thank you for reviewing, Here *_STATUS/ENABLE/MASK_MISC register are
Laid in the same way.
>
> > + raw_spin_unlock_irqrestore(&port->lock, flags);
> > +}
> > +
> > +static void amd_mdb_intx_irq_unmask(struct irq_data *data)
> > +{
> > + struct amd_mdb_pcie *pcie = irq_data_get_irq_chip_data(data);
> > + struct dw_pcie *pci = &pcie->pci;
> > + struct dw_pcie_rp *port = &pci->pp;
> > + unsigned long flags;
> > + u32 val;
> > +
> > + raw_spin_lock_irqsave(&port->lock, flags);
> > + val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> > + val |= AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> > + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
> > + raw_spin_unlock_irqrestore(&port->lock, flags);
> > +}
>
> > +static irqreturn_t dw_pcie_rp_intx_flow(int irq, void *args)
>
> It'd be nice if this were close in the source file to the INTx
> mask/unmask above.
Thank you for reviewing, Will move this in next patch.
>
> > +{
> > + struct amd_mdb_pcie *pcie = args;
> > + unsigned long val;
> > + int i;
> > +
> > + val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> > + pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> > +
> > + for (i = 0; i < PCI_NUM_INTX; i++) {
> > + if (val & AMD_MDB_PCIE_INTR_INTX_ASSERT(i))
> > + generic_handle_domain_irq(pcie->intx_domain, i);
> > + if (val & AMD_MDB_PCIE_INTR_INTX_DEASSERT(i)
> > + generic_handle_domain_irq(pcie->intx_domain, (i *
> 2) + 1);
>
> Why call generic_handle_domain_irq() for deassert? No other drivers
> do that AFAIK. If you do need it, "(i * 2) + 1" looks completely
> wrong; it should be the hwirq value.
Thank you for reviewing, will remove this deassert generic handler in next patch.
>
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
>
> > +static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args)
> > +{
> > + struct amd_mdb_pcie *pcie = args;
> > + struct device *dev;
> > + struct irq_data *d;
> > +
> > + dev = pcie->pci.dev;
> > +
> > + /**
>
> /* (not /**)
- Thank you for reviewing, Will update this in next patch.
>
> > + * In future, error reporting will be hooked to the AER subsystem.
> > + * Currently, the driver prints a warning message to the user.
> > + */
> > + 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_once(dev, "Unknown IRQ %ld\n", d->hwirq);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> > +static int amd_mdb_setup_irq(struct amd_mdb_pcie *pcie,
> > + struct platform_device *pdev)
> > +{
> > ...
>
> > +
> > + /* Plug the main event handler */
> > + err = devm_request_irq(dev, pp->irq, amd_mdb_pcie_event_flow,
> > + IRQF_SHARED | IRQF_NO_THREAD, "amd_mdb
> pcie_irq", pcie);
>
> Wrap to fit in 80 columns like the rest of the file.
-Thank you for reviewing, will update this in next patch.
>
> Bjorn
