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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Feb 05, 2025 at 11:53:53AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Havalige, Thippeswamy
> > Sent: Wednesday, February 5, 2025 5:08 PM
> > To: Bjorn Helgaas <helgaas@kernel.org>
> > 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 v8 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

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

Tangent: I don't know what "deassert" would mean here, since INTx is
an *incoming* interrupt and the Root Port is the receiver and can mask
or acknowledge the interrupt but not really deassert it.

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

> > This condition never met observing zero here.

> To satisfy this condition need to modify macros as following.
> #define AMD_MDB_PCIE_INTR_INTX_ASSERT(x)    BIT(x)
> #define AMD_MDB_PCIE_INTR_INTX_DEASSERT(x)    BIT(x+1)

Maybe I don't understand how the assert/deassert bits are laid out in
the register.

The original patch has this:

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

and if the odd bits are for deassert I thought that meant they would
look like this:

   #define AMD_MDB_PCIE_INTR_INTA_DEASSERT  17
   #define AMD_MDB_PCIE_INTR_INTB_DEASSERT  19
   #define AMD_MDB_PCIE_INTR_INTC_DEASSERT  21
   #define AMD_MDB_PCIE_INTR_INTD_DEASSERT  23

  +#define AMD_MDB_TLP_PCIE_INTX_MASK     GENMASK(23, 16)

If we extract AMD_MDB_TLP_PCIE_INTX_MASK with FIELD_GET(),
the field gets shifted right by 16, so we should end up with
something like this:

  INTA assert     0000 0001  ==  BIT(0)
  INTA deassert   0000 0010  ==  BIT(1)
  INTB assert     0000 0100  ==  BIT(2)
  INTB deassert   0000 1000  ==  BIT(3)
  INTC assert     0001 0000  ==  BIT(4)
  INTC deassert   0010 0000  ==  BIT(5)
  INTD assert     0100 0000  ==  BIT(6)
  INTD deassert   1000 0000  ==  BIT(7)

But maybe that's not how they're actually laid out?

I think the argument to AMD_MDB_PCIE_INTR_INTX_ASSERT() should
be the hwirq (0..3 for INTA..INTD), so if we use

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

as you propose, don't the assert/deassert bits collide?

  AMD_MDB_PCIE_INTR_INTX_ASSERT(0)   == BIT(0) for INTA assert
  AMD_MDB_PCIE_INTR_INTX_ASSERT(1)   == BIT(1) for INTB assert

  AMD_MDB_PCIE_INTR_INTX_DEASSERT(0) == BIT(1) for INTA deassert

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