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@xxxxxxxxxx>
> > Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> > manivannan.sadhasivam@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> > conor+dt@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; jingoohan1@xxxxxxxxx; Simek, Michal
> > <michal.simek@xxxxxxx>; Gogada, Bharat Kumar
> > <bharat.kumar.gogada@xxxxxxx>
> > 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);
