Re: [PATCH v1 8/9] PCI: microchip: Partition inbound address translation

From: Conor Dooley <conor@kernel.org>
To: daire.mcnamara@microchip.com
Cc: conor.dooley@microchip.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, paul.walmsley@sifive.com,
	palmer@dabbelt.com, aou@eecs.berkeley.edu, lpieralisi@kernel.org,
	kw@linux.com, bhelgaas@google.com,
	linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v1 8/9] PCI: microchip: Partition inbound address translation
Date: Wed, 23 Nov 2022 23:05:34 +0000	[thread overview]
Message-ID: <Y36nPubPl08F/nag@spud> (raw)
In-Reply-To: <20221116135504.258687-9-daire.mcnamara@microchip.com>

On Wed, Nov 16, 2022 at 01:55:03PM +0000, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> On Microchip PolarFire SoC the PCIe rootport is behind a set of fabric
> inter connect (fic) busses that encapsulate busses like ABP/AHP, AXI-S
> and AXI-M. Depending on which fic(s) the rootport is wired through to
> cpu space, the rootport driver needs to take account of the address
> translation done by a parent (e.g. fabric) node before setting up its
> own inbound address translation tables from attached devices.
> 
> Parse the dma-range properties to determine how much address translation
> to perform in the root port and how much is being provided by the
> fabric.
> 
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 184 ++++++++++++++++++-
>  1 file changed, 178 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 62f8c5edfd0e..a90a0a675f14 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c

> @@ -940,6 +954,46 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port)
>  	return mc_allocate_msi_domains(port);
>  }
>  
> +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port)
> +{
> +	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> +	phys_addr_t pcie_addr;
> +	phys_addr_t axi_addr;
> +	u32 atr_size;
> +	u32 val;
> +	int i;
> +
> +	for (i = 0; i < port->num_inbound_windows; i++) {
> +		atr_size = ilog2(port->inbound_windows[i].size) - 1;
> +		atr_size &= GENMASK(5, 0);
> +
> +		pcie_addr = port->inbound_windows[i].pci_addr;
> +
> +		val = lower_32_bits(pcie_addr) & GENMASK(31, 12);
> +		val |= (atr_size << ATR_SIZE_SHIFT);
> +		val |= ATR_IMPL_ENABLE;
> +		writel(val, bridge_base_addr +
> +		       ATR0_PCIE_WIN0_SRCADDR_PARAM + (i * ATR_WINDOW_DESC_SIZE));
> +		writel(upper_32_bits(pcie_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_SRC_ADDR + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		axi_addr = port->inbound_windows[i].axi_addr;
> +
> +		writel(lower_32_bits(axi_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_ADDR_LSB + (i * ATR_WINDOW_DESC_SIZE));
> +		writel(upper_32_bits(axi_addr), bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_ADDR_UDW + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr +
> +		       ATR0_PCIE_WIN0_TRSL_PARAM + (i * ATR_WINDOW_DESC_SIZE));
> +
> +		dev_dbg(&pdev->dev, "0x%010llx..0x%010llx -> 0x%010llx\n",
> +			pcie_addr, pcie_addr + port->inbound_windows[i].size - 1, axi_addr);

If you're going to leave the dbg print in, can you make it more verbose
so that the log message stands on its own?

Other than that, I made most of my comments before submission so LGTM..
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.