On Wed, Feb 19, 2025 at 03:51:24PM +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_TLP_IR_DISABLE_MISC 0x4CC
> +
> +#define AMD_MDB_TLP_PCIE_INTX_MASK GENMASK(23, 16)
> +
> +#define AMD_MDB_PCIE_INTR_INTX_ASSERT(x) BIT((x) * 2)
> +#define AMD_MDB_PCIE_INTX_BIT(x) (1U << (2 * (x) + AMD_MDB_PCIE_INTR_INTX))
I don't think you need both AMD_MDB_PCIE_INTR_INTX_ASSERT() and
AMD_MDB_PCIE_INTX_BIT().
AMD_MDB_PCIE_INTX_BIT() is essentially the same as
FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, AMD_MDB_PCIE_INTR_INTX_ASSERT).
I would just do this:
#define AMD_MDB_PCIE_INTR_INTX(x) BIT((x) * 2)
and in amd_mdb_intx_irq_mask() and amd_mdb_intx_irq_unmask(), do this:
val = FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK,
AMD_MDB_PCIE_INTR_INTX(data->hwirq));
Then all the users do FIELD_PREP() or FIELD_GET() at the point of use.
It's a little confusing for dw_pcie_rp_intx_flow() to do the
FIELD_GET() there, but amd_mdb_intx_irq_mask() and
amd_mdb_intx_irq_unmask() have the equivalent of FIELD_PREP() hidden
inside AMD_MDB_PCIE_INTX_BIT().
> +static void amd_mdb_intx_irq_mask(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 val;
> +
> + raw_spin_lock_irqsave(&port->lock, flags);
> + val = AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> + pcie_write(pcie, val, AMD_MDB_TLP_IR_DISABLE_MISC);
I guess AMD_MDB_TLP_IR_DISABLE_MISC and AMD_MDB_TLP_IR_ENABLE_MISC are
set up so writing zero bits does nothing, and writing a one bit only
masks or unmasks that single interrupt? It's somewhat unusual, so a
comment to that effect might be useful.
> + raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void amd_mdb_intx_irq_unmask(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 val;
> +
> + raw_spin_lock_irqsave(&port->lock, flags);
> + val = AMD_MDB_PCIE_INTX_BIT(data->hwirq);
> + pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
> + raw_spin_unlock_irqrestore(&port->lock, flags);
> +}
> +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,
> + val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC));
The "val = pcie_read()" looks like a useless assignment to val. I
would do something like this:
val = pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);
intx_status = FIELD_GET(AMD_MDB_TLP_PCIE_INTX_MASK, val);
for (i = 0; i < PCI_NUM_INTX; i++) {
if (intx_status & AMD_MDB_PCIE_INTR_INTX(i))
generic_handle_domain_irq(pcie->intx_domain, i);
}
> + 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);
> + }
> +
> + return IRQ_HANDLED;
> +}
