Re: [PATCH v18 3/4] PCI: microchip: Add host driver for Microchip PCIe controller

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Dec 03, 2020 at 12:10:17PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Add support for the Microchip PolarFire PCIe controller when
> configured in host (Root Complex) mode.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

> +static void mc_pcie_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct mc_port *port = irq_desc_get_handler_data(desc);
> +	struct device *dev = port->dev;
> +	struct mc_msi *msi = &port->msi;
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> +	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE1_CTRL_ADDR;
> +	u32 status;
> +	unsigned long intx_status;
> +	unsigned long msi_status;
> +	u32 bit;
> +	u32 virq;
> +
> +	/*
> +	 * The core provides a single interrupt for both INTx/MSI messages.
> +	 * So we'll read both INTx and MSI status.
> +	 */
> +	chained_irq_enter(chip, desc);
> +
> +	status = readl_relaxed(ctrl_base_addr + MC_SEC_ERROR_INT);

Other than a few in mc_setup_window(), it looks like all the accesses
in this driver are relaxed.

readl_relaxed() and writel_relaxed() are only used by a few of the
host bridge drivers.  I doubt this is because those devices behave
differently than all the rest, so I suspect there's a general rule
that they all should use.  I don't know what that rule is, but maybe
you do?

Per Documentation/memory-barriers.txt, the relaxed versions provide
weaker ordering guarantees, so the safest thing would be to use the
non-relaxed versions and include a little justification for when/why
it is safe to use the relaxed versions.

A lot of uses are in non-performance paths where there's really no
benefit to using the relaxed versions.

Not asking you to do anything here, but in case you've analyzed this
and come to the conclusion that the relaxed versions are safe here,
but not in mc_setup_window(), that rationale might be useful to others
if you included it in the commit log or a brief comment in the code.

> +static void mc_setup_window(void __iomem *bridge_base_addr, u32 index, phys_addr_t axi_addr,
> +			    phys_addr_t pci_addr, size_t size)
> +{
> +	u32 atr_sz = ilog2(size) - 1;
> +	u32 val;
> +
> +	if (index == 0)
> +		val = PCIE_CONFIG_INTERFACE;
> +	else
> +		val = PCIE_TX_RX_INTERFACE;
> +
> +	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + MC_ATR0_AXI4_SLV0_TRSL_PARAM);
> ...