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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Monday, February 3, 2025 11:58 PM
> 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 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_TLP_IR_STATUS_MISC		0x4C0
> > +#define AMD_MDB_TLP_IR_MASK_MISC		0x4C4
> > +#define AMD_MDB_TLP_IR_ENABLE_MISC		0x4C8
> > +
> > +#define AMD_MDB_PCIE_IDRN_SHIFT			16
> 
> Remove this _SHIFT #define and use something like this instead:
> 
>   #define AMD_MDB_PCIE_INTX_BIT(x)
> FIELD_PREP(AMD_MDB_TLP_PCIE_INTX_MASK, BIT(x))
Thanks, will update this in next patch.
> 
> I don't know what exactly the right name for that is; it looks like maybe these
> bits apply to all the above registers (AMD_MDB_TLP_IR_STATUS_MISC,
> AMD_MDB_TLP_IR_MASK_MISC,
> AMD_MDB_TLP_IR_ENABLE_MISC)
> 
> > +#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.
- Thanks for review comments, 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.
Thanks for review comments. 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.
> 
> > +static void amd_mdb_mask_intx_irq(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 mask, val;
> > +
> > +	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
> > +
> > +	raw_spin_lock_irqsave(&port->lock, flags);
> > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> 
>   val &= ~AMD_MDB_PCIE_INTX_BIT(data->hwirq);
>   pcie_write(pcie, val, AMD_MDB_TLP_IR_ENABLE_MISC);
- Thanks for review comments, Will update this in our next patch.
> 
> > +	pcie_write(pcie, (val & (~mask)), AMD_MDB_TLP_IR_ENABLE_MISC);
> > +	raw_spin_unlock_irqrestore(&port->lock, flags); }
> > +
> > +static void amd_mdb_unmask_intx_irq(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 mask;
> > +	u32 val;
> > +
> > +	mask = BIT(data->hwirq + AMD_MDB_PCIE_IDRN_SHIFT);
> > +
> > +	raw_spin_lock_irqsave(&port->lock, flags);
> > +	val = pcie_read(pcie, AMD_MDB_TLP_IR_MASK_MISC);
> 
>   val |= AMD_MDB_PCIE_INTX_BIT(data->hwirq);
- Thanks for review comments, Will update this in our next patch.
> 
> > +	pcie_write(pcie, (val | mask), AMD_MDB_TLP_IR_ENABLE_MISC);
> > +	raw_spin_unlock_irqrestore(&port->lock, flags); }
> > +
> > +static struct irq_chip amd_mdb_intx_irq_chip = {
> > +	.name		= "AMD MDB INTx",
> > +	.irq_mask	= amd_mdb_mask_intx_irq,
> > +	.irq_unmask	= amd_mdb_unmask_intx_irq,
> 
> Prefer
> 
>   .irq_mask       = amd_mdb_intx_irq_mask,
>   .irq_unmask     = amd_mdb_intx_irq_unmask,
> 
> so the function names match the grep pattern of the function pointers
> (".*_irq_mask").
- Thanks for review comments, Will update this in our next patch.
> 
> > +static struct irq_chip amd_mdb_event_irq_chip = {
> > +	.name		= "AMD MDB RC-Event",
> > +	.irq_mask	= amd_mdb_mask_event_irq,
> > +	.irq_unmask	= amd_mdb_unmask_event_irq,
> 
> Same function name comment.
- Thanks for review comments, Will update this in our next patch.
> 
> > +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)
- Thanks for review comments, In next patch I will update value to 8 here.
> 
> > +		generic_handle_domain_irq(pcie->intx_domain, i);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > +
> > +static irqreturn_t amd_mdb_pcie_intr_handler(int irq, void *args) {
> > +	struct amd_mdb_pcie *pcie = args;
> > +	struct device *dev;
> > +	struct irq_data *d;
> > +
> > +	dev = pcie->pci.dev;
> > +
> > +	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?
- Thank you for your valuable review comments. 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.
> 
> > +	return IRQ_HANDLED;
> > +}
> 
> > +static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
> > +				 struct platform_device *pdev)
> > +{
> > +	struct dw_pcie *pci = &pcie->pci;
> > +	struct dw_pcie_rp *pp = &pci->pp;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	pcie->slcr = devm_platform_ioremap_resource_byname(pdev, "slcr");
> > +	if (IS_ERR(pcie->slcr))
> > +		return PTR_ERR(pcie->slcr);
> > +
> > +	ret = amd_mdb_pcie_init_irq_domains(pcie, pdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	amd_mdb_pcie_init_port(pcie);
> 
> amd_mdb_pcie_init_port() doesn't initialize anything other than
> disabling/clearing/enabling interrupts.  Seems like it could be squashed into
> amd_mdb_setup_irq() or called from there so it's obvious that it's interrupt-
> related.
Thanks for review comment, I will update this in next patch.
> 
> > +	ret = amd_mdb_setup_irq(pcie, pdev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to set up interrupts\n");
> > +		goto out;
> > +	}
> > +
> > +	pp->ops = &amd_mdb_pcie_host_ops;
> > +
> > +	ret = dw_pcie_host_init(pp);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize host\n");
> > +		goto out;
> > +	}
> > +
> > +	return 0;
> > +
> > +out:
> > +	amd_mdb_pcie_free_irq_domains(pcie);
> > +	return ret;
> > +}

Regards,
Thippeswamy H