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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Feb 04, 2025 at 09:37:51AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> ...
> > 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_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.
>
> ... the odd bits are meant for deasserting inta, intb intc & intd I
> ll include this in my next patch 

> > > +#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.

> Yes, there are 8 intx related bits I ll define them in my next
> patch. I was in confusion here regarding "PCI_NUM_INTX " since this
> macro indicates INTA INTB INTC INTD bits so I discarded deassert
> bits here.

It seems like what you have is a single 8-bit field that contains both
assert and deassert info, interspersed.  GENMASK()/FIELD_GET() isn't
enough to really separate them.  Maybe you can do something like this:

  #define AMD_MDB_TLP_PCIE_INTX_MASK          GENMASK(23, 16)

  #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(1 << x)

If you don't need the deassert bits, a comment would be useful, but
there's no point in adding a #define for them.  If you do need them,
maybe this:

  #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)  BIT((1 << x) + 1)

> > > +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)

> In next patch I will update value to 8 here.

And here you could do:

  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);

> > > +		generic_handle_domain_irq(pcie->intx_domain, i);

> > > +	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?
>
> At this stage, our objective is to notify the user of the occurrence
> of an event. While we intend to integrate these events with the AER
> subsystem in the future, for the time being, we will limit the
> functionality to notifying the user.

OK, just add a comment to that effect here.

Bjorn