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

[Bjorn Helgaas]

On Wed, Jan 29, 2025 at 05:00:29PM +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_PCIE_IDRN_SHIFT			16

Remove this _SHIFT #define and use something like this instead:

  #define AMD_MDB_PCIE_INTX_BIT(x) FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))

I don't know what exactly the right name for that is; it looks like
maybe these bits apply to all the above registers
(AMD_MDB_TLP_IR_STATUS_MISC, AMD_MDB_TLP_IR_MASK_MISC,
AMD_MDB_TLP_IR_ENABLE_MISC)

> +#define AMD_MDB_PCIE_INTR_INTA_ASSERT		16
> +#define AMD_MDB_PCIE_INTR_INTB_ASSERT		18
> +#define AMD_MDB_PCIE_INTR_INTC_ASSERT		20
> +#define AMD_MDB_PCIE_INTR_INTD_ASSERT		22

It's kind of weird that these skip the odd-numbered bits, since
dw_pcie_rp_intx_flow(), amd_mdb_mask_intx_irq(),
amd_mdb_unmask_intx_irq() only use bits 19:16.  Something seems wrong
and needs either a fix or a comment about why this is the way it is.

> +#define IMR(x) BIT(AMD_MDB_PCIE_INTR_ ##x)
> +#define AMD_MDB_PCIE_IMR_ALL_MASK			\
> +	(						\
> +		IMR(CMPL_TIMEOUT)	|		\
> +		IMR(INTA_ASSERT)	|		\
> +		IMR(INTB_ASSERT)	|		\
> +		IMR(INTC_ASSERT)	|		\
> +		IMR(INTD_ASSERT)	|		\
> +		IMR(PM_PME_RCVD)	|		\
> +		IMR(PME_TO_ACK_RCVD)	|		\
> +		IMR(MISC_CORRECTABLE)	|		\
> +		IMR(NONFATAL)		|		\
> +		IMR(FATAL)				\
> +	)
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK	GENMASK(23, 16)

I would drop AMD_MDB_PCIE_INTR_INTA_ASSERT, etc, and just use
AMD_MDB_TLP_PCIE_INTX_MASK in the AMD_MDB_PCIE_IMR_ALL_MASK
definition.

If there are really eight bits of INTx-related things here for the
four INTx interrupts, I think you should make two #defines to separate
them out.

> +static void amd_mdb_mask_intx_irq(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 mask, val;
> +
> +	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
> +
> +	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);

> +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void amd_mdb_unmask_intx_irq(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 mask;
> +	u32 val;
> +
> +	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
> +
> +	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 | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> +	raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static struct irq_chip amd_mdb_intx_irq_chip = {
> +	.name		= "AMD MDB INTx",
> +	.irq_mask	= amd_mdb_mask_intx_irq,
> +	.irq_unmask	= amd_mdb_unmask_intx_irq,

Prefer

  .irq_mask       = amd_mdb_intx_irq_mask,
  .irq_unmask     = amd_mdb_intx_irq_unmask,

so the function names match the grep pattern of the function pointers
(".*_irq_mask").

> +static struct irq_chip amd_mdb_event_irq_chip = {
> +	.name		= "AMD MDB RC-Event",
> +	.irq_mask	= amd_mdb_mask_event_irq,
> +	.irq_unmask	= amd_mdb_unmask_event_irq,

Same function name comment.

> +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,
> +			pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
> +
> +	for_each_set_bit(i, &val, 4)

  for_each_set_bit(..., PCI_NUM_INTX)

> +		generic_handle_domain_irq(pcie->intx_domain, i);
> +
> +	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;
> +
> +	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);

What's the point of an interrupt handler that only logs it?

> +	return IRQ_HANDLED;
> +}

> +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;
> +
> +	pcie->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> +	if (IS_ERR(pcie->slcr))
> +		return PTR_ERR(pcie->slcr);
> +
> +	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
> +	if (ret)
> +		return ret;
> +
> +	amd_mdb_pcie_init_port(pcie);

amd_mdb_pcie_init_port() doesn't initialize anything other than
disabling/clearing/enabling interrupts.  Seems like it could be
squashed into amd_mdb_setup_irq() or called from there so it's
obvious that it's interrupt-related.

> +	ret = amd_mdb_setup_irq(pcie, pdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set up interrupts\n");
> +		goto out;
> +	}
> +
> +	pp->ops = &amd_mdb_pcie_host_ops;
> +
> +	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;
> +}