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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Feb 19, 2025 at 03:51:24PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB (Multimedia DMA Bridge) IP core as Root Port.

> +#define AMD_MDB_TLP_IR_STATUS_MISC		0x4C0
> +#define AMD_MDB_TLP_IR_MASK_MISC		0x4C4
> +#define AMD_MDB_TLP_IR_ENABLE_MISC		0x4C8
> +#define AMD_MDB_TLP_IR_DISABLE_MISC		0x4CC
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)
> +
> +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)	BIT((x) * 2)

> +#define AMD_MDB_PCIE_INTX_BIT(x) (1U << (2 * (x) + AMD_MDB_PCIE_INTR_INTX))

I don't think you need both AMD_MDB_PCIE_INTR_INTX_ASSERT() and
AMD_MDB_PCIE_INTX_BIT().

AMD_MDB_PCIE_INTX_BIT() is essentially the same as
FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, AMD_MDB_PCIE_INTR_INTX_ASSERT).

I would just do this:

  #define AMD_MDB_PCIE_INTR_INTX(x)     BIT((x) * 2)

and in amd_mdb_intx_irq_mask() and amd_mdb_intx_irq_unmask(), do this:

  val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK,
                   AMD_MDB_PCIE_INTR_INTX(data->hwirq));

Then all the users do FIELD_PREP() or FIELD_GET() at the point of use.

It's a little confusing for dw_pcie_rp_intx_flow() to do the
FIELD_GET() there, but amd_mdb_intx_irq_mask() and
amd_mdb_intx_irq_unmask() have the equivalent of FIELD_PREP() hidden
inside AMD_MDB_PCIE_INTX_BIT().

> +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 = AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> +	pcie_write(pcie, val, AMD_MDB_TLP_IR_DISABLE_MISC);

I guess AMD_MDB_TLP_IR_DISABLE_MISC and AMD_MDB_TLP_IR_ENABLE_MISC are
set up so writing zero bits does nothing, and writing a one bit only
masks or unmasks that single interrupt?  It's somewhat unusual, so a
comment to that effect might be useful.

> +	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 = 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)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK,
> +			val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));

The "val = pcie_read()" looks like a useless assignment to val.  I
would do something like this:

  val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
  intx_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val);

  for (i = 0; i < PCI_NUM_INTX; i++) {
    if (intx_status & AMD_MDB_PCIE_INTR_INTX(i))
      generic_handle_domain_irq(pcie->intx_domain, i);
  }

> +	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);
> +	}
> +
> +	return IRQ_HANDLED;
> +}