Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 03:11:46 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Elad Nachman <enachman@marvell.com>
Cc:     thomas.petazzoni@bootlin.com, bhelgaas@google.com,
        lpieralisi@kernel.org, robh@kernel.org, kw@linux.com,
        krzysztof.kozlowski+dt@linaro.org, linux-pci@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 8/8] PCI: dwc: Introduce region limit from DT
Message-ID: <20230323001146.xbqbav2fu2qelsub@mobilestation>
References: <20230313124016.17102-1-enachman@marvell.com>
 <20230313124016.17102-9-enachman@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230313124016.17102-9-enachman@marvell.com>
Precedence: bulk

On Mon, Mar 13, 2023 at 02:40:16PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Allow dts override of region limit for SOCs with older Synopsis
> Designware PCIe IP but with greater than 32-bit address range support,
> such as the Armada 7020/7040/8040 family of SOCs by Marvell,
> when the DT file places the PCIe window above the 4GB region.
> The Synopsis Designware PCIe IP in these SOCs is too old to specify the
> highest memory location supported by the PCIe, but practically supports
> such locations. Allow these locations to be specified in the DT file.
> DT property is called num-regionmask , and can range between 33 and 64.

The implemented algorithm doesn't prevents you from specifying the
outbound MW base above 4GB. It prevents you from overflowing the limit
address which is of the 32-bits width only for the chips older v4.60a
and if the INCREASE_REGION_SIZE IP-core synthesize parameter is set to
zero.

In other words you must make sure that dma-ranges/ranges entries size
when combined with the source address doesn't cause the limit address
overflow (4GB boundary cross in your case). For instance, you want to
map 0x1F0000000 CPU-address region of 512MB size to 0x0 PCIe address. In
that case you'd specify the ranges property like this:
< ranges = <0x82000000 0 0 0x1 0xf0000000 0 0x20000000>;
The CPU-base address is ok since iATU always supports 64-bit base
addresses. But after you add 0x20000000 to 0x1f0000000 you'll get the
4GB boundary overflow (0x210000000) which can't be described with the
32-bit limit address CSR. In this particular case the maximum range
size you can specify is 0x10000000 (256MB).

Anyway judging by the patch content you do nothing but hacking the
ranges entries sanity check procedure which I don't think is a good
thing to do.

-Serge(y)

> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
> v4:
>    1) Fix blank lines removal / addition
> 
>    2) Remove usage of variable with same name as dt binding property
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 53a16b8b6ac2..9773c110c733 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -735,8 +735,10 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  {
>  	int max_region, ob, ib;
> -	u32 val, min, dir;
> +	u32 val, min, dir, ret;
>  	u64 max;
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
>  
>  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>  	if (val == 0xFFFFFFFF) {
> @@ -781,7 +783,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  		dw_pcie_writel_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT, 0xFFFFFFFF);
>  		max = dw_pcie_readl_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT);
>  	} else {
> -		max = 0;
> +		/* Allow dts override of region limit for older IP with above 32-bit support: */
> +		ret = of_property_read_u32(np, "num-regionmask", &val);
> +		if (!ret && val > 32) {
> +			max = GENMASK(val - 33, 0);
> +			dev_info(pci->dev, "Overriding region limit to %u bits\n", val);
> +		} else
> +			max = 0;
>  	}
>  
>  	pci->num_ob_windows = ob;
> -- 
> 2.17.1
> 
>