RE: [PATCH v10 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

["Havalige, Thippeswamy" <thippeswamy.havalige@amd.com>]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, February 12, 2025 12:04 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-pci@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; jingoohan1@gmail.com; Simek, Michal
> <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>
> 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