Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, February 5, 2025 3:42 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>
> 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
>
> On Tue, Feb 04, 2025 at 09:37:51AM +0000, Havalige, Thippeswamy wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > ...
> > > 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))
- Thanks for reviewing, This condition never met observing zero here.
> 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.
- Thanks for reviewing, will add this in next patch.
>
> Bjorn
