linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: daire.mcnamara@microchip.com
Cc: lorenzo.pieralisi@arm.com, bhelgaas@google.com, robh@kernel.org,
	linux-pci@vger.kernel.org, robh+dt@kernel.org,
	devicetree@vger.kernel.org, david.abdurachmanov@gmail.com,
	cyril.jean@microchip.com, ben.dooks@codethink.co.uk
Subject: Re: [PATCH v18 3/4] PCI: microchip: Add host driver for Microchip PCIe controller
Date: Thu, 3 Dec 2020 15:07:48 -0600	[thread overview]
Message-ID: <20201203210748.GA1594918@bjorn-Precision-5520> (raw)
In-Reply-To: <20201203121018.16432-4-daire.mcnamara@microchip.com>

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);
> ...

  reply	other threads:[~2020-12-03 21:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-03 12:10 [PATCH v18 0/4] PCI: microchip: Add host driver for Microchip PCIe controller daire.mcnamara
2020-12-03 12:10 ` [PATCH v18 1/4] PCI: Call platform_set_drvdata earlier in devm_pci_alloc_host_bridge daire.mcnamara
2020-12-03 12:10 ` [PATCH v18 2/4] dt-bindings: PCI: microchip: Add Microchip PolarFire host binding daire.mcnamara
2020-12-03 12:10 ` [PATCH v18 3/4] PCI: microchip: Add host driver for Microchip PCIe controller daire.mcnamara
2020-12-03 21:07   ` Bjorn Helgaas [this message]
2020-12-03 23:44     ` Rob Herring
2020-12-07 17:59   ` Lorenzo Pieralisi
2020-12-03 12:10 ` [PATCH v18 4/4] Add Daire McNamara as maintainer for the Microchip PCIe driver daire.mcnamara
2020-12-07 18:01   ` Lorenzo Pieralisi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201203210748.GA1594918@bjorn-Precision-5520 \
    --to=helgaas@kernel.org \
    --cc=ben.dooks@codethink.co.uk \
    --cc=bhelgaas@google.com \
    --cc=cyril.jean@microchip.com \
    --cc=daire.mcnamara@microchip.com \
    --cc=david.abdurachmanov@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).