Re: [PATCH v10 5/7] PCI: mediatek-gen3: Add MSI support

[Marc Zyngier]

On Tue, 20 Apr 2021 10:44:02 +0100,
Pali Rohár <pali@xxxxxxxxxx> wrote:
> 
> Hello!
> 
> On Tuesday 20 April 2021 14:17:21 Jianjun Wang wrote:
> > +static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > +{
> > +	int i;
> > +	u32 val;
> > +
> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {
> > +		struct mtk_msi_set *msi_set = &port->msi_sets[i];
> > +
> > +		msi_set->base = port->base + PCIE_MSI_SET_BASE_REG +
> > +				i * PCIE_MSI_SET_OFFSET;
> > +		msi_set->msg_addr = port->reg_base + PCIE_MSI_SET_BASE_REG +
> > +				    i * PCIE_MSI_SET_OFFSET;
> > +
> > +		/* Configure the MSI capture address */
> > +		writel_relaxed(lower_32_bits(msi_set->msg_addr), msi_set->base);
> > +		writel_relaxed(upper_32_bits(msi_set->msg_addr),
> > +			       port->base + PCIE_MSI_SET_ADDR_HI_BASE +
> > +			       i * PCIE_MSI_SET_ADDR_HI_OFFSET);
> 
> This looks like as setting MSI doorbell address to MSI doorbell address.
> 
> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> > +{
> > +	struct mtk_msi_set *msi_set = irq_data_get_irq_chip_data(data);
> > +	struct mtk_pcie_port *port = data->domain->host_data;
> > +	unsigned long hwirq;
> > +
> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;
> > +
> > +	msg->address_hi = upper_32_bits(msi_set->msg_addr);
> > +	msg->address_lo = lower_32_bits(msi_set->msg_addr);
> > +	msg->data = hwirq;
> > +	dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",
> > +		hwirq, msg->address_hi, msg->address_lo, msg->data);
> 
> ... which is later used in compose_msi_msg().
> 
> Marc in some other patches for other pci controller drivers changed this
> address to just main "port" structure. It simplified implementations and
> also avoided need to declare additional member "msg_addr".
> 
> Marc, would it be possible to simplify it also for this driver and just
> set msg_addr to virt_to_phys(port)?

Maybe. It really depends on what range the HW accepts, and the sole
requirement is to use an address that the endpoint cannot DMA
to. Here, the driver seems to be using something based on the port
base address, which is good enough as far as I am concerned (the thing
I usually object to is the allocation of memory just for the sake of
getting a capture address).

If you want to further simplify it, you could simply use port.reg_base
as the MSI address for all sets, as I don't think they have to be
distinct. But someone with access to the TRM for this should go and
check it.

I don't believe this should gate the merging od this driver though.

	M.

-- 
Without deviation from the norm, progress is not possible.