Re: [PATCH v2] PCI: mediatek-gen3: Fix translation window

["Jianjun Wang (王建军)" <Jianjun.Wang@mediatek.com>]

Hi Maintainers,

Gentle ping for this patch, if there is anything I need to do, please
let me know.

Thanks.

On Mon, 2023-10-23 at 16:14 +0800, Jianjun Wang wrote:
> The size of translation table should be a power of 2, using fls()
> cannot get the proper value when the size is not a power of 2. For
> example, fls(0x3e00000) - 1 = 25, hence the PCIe translation window
> size will be set to 0x2000000 instead of the expected size 0x3e00000.
> 
> Fix translation window by splitting the MMIO space to multiple tables
> if
> its size is not a power of 2.
> 
> Fixes: d3bf75b579b9 ("PCI: mediatek-gen3: Add MediaTek Gen3 driver
> for MT8192")
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> 
> ---
> Changes in v2:
> Only print warning message when reach the maximum translation table
> number, the MEM space still works but will be smaller than expected.
> 
> Bootup logs on MT8195 Platform:
> 
> > Before this patch:
> 
> mtk-pcie-gen3 112f0000.pcie: Parsing ranges property...
> mtk-pcie-gen3 112f0000.pcie:       IO 0x0020000000..0x00201fffff ->
> 0x0020000000
> mtk-pcie-gen3 112f0000.pcie:      MEM 0x0020200000..0x0023ffffff ->
> 0x0020200000
> mtk-pcie-gen3 112f0000.pcie: set IO trans window[0]: cpu_addr =
> 0x20000000, pci_addr = 0x20000000, size = 0x200000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[1]: cpu_addr =
> 0x20200000, pci_addr = 0x20200000, size = 0x3e00000
> 
> > We expect the MEM trans window size to be 0x3e00000, but its actual
> > available size is 0x2000000.
> > After applying this patch:
> 
> mtk-pcie-gen3 112f0000.pcie: Parsing ranges property...
> mtk-pcie-gen3 112f0000.pcie:       IO 0x0020000000..0x00201fffff ->
> 0x0020000000
> mtk-pcie-gen3 112f0000.pcie:      MEM 0x0020200000..0x0023ffffff ->
> 0x0020200000
> mtk-pcie-gen3 112f0000.pcie: set IO trans window[0]: cpu_addr =
> 0x20000000, pci_addr = 0x20000000, size = 0x200000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[1]: cpu_addr =
> 0x20200000, pci_addr = 0x20200000, size = 0x200000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[2]: cpu_addr =
> 0x20400000, pci_addr = 0x20400000, size = 0x400000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[3]: cpu_addr =
> 0x20800000, pci_addr = 0x20800000, size = 0x800000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[4]: cpu_addr =
> 0x21000000, pci_addr = 0x21000000, size = 0x1000000
> mtk-pcie-gen3 112f0000.pcie: set MEM trans window[5]: cpu_addr =
> 0x22000000, pci_addr = 0x22000000, size = 0x2000000
> 
> > Total available size for MEM trans window is 0x3e00000.
> 
> ---
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 85 ++++++++++++-------
> --
>  1 file changed, 50 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> index e0e27645fdf4..975b3024fb08 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -245,35 +245,60 @@ static int mtk_pcie_set_trans_table(struct
> mtk_gen3_pcie *pcie,
>  				    resource_size_t cpu_addr,
>  				    resource_size_t pci_addr,
>  				    resource_size_t size,
> -				    unsigned long type, int num)
> +				    unsigned long type, int *num)
>  {
> +	resource_size_t remaining = size;
> +	resource_size_t table_size;
> +	resource_size_t addr_align;
> +	const char *range_type;
>  	void __iomem *table;
>  	u32 val;
>  
> -	if (num >= PCIE_MAX_TRANS_TABLES) {
> -		dev_err(pcie->dev, "not enough translate table for
> addr: %#llx, limited to [%d]\n",
> -			(unsigned long long)cpu_addr,
> PCIE_MAX_TRANS_TABLES);
> -		return -ENODEV;
> -	}
> +	while (remaining && (*num < PCIE_MAX_TRANS_TABLES)) {
> +		/* Table size needs to be a power of 2 */
> +		table_size = BIT(fls(remaining) - 1);
> +
> +		if (cpu_addr > 0) {
> +			addr_align = BIT(ffs(cpu_addr) - 1);
> +			table_size = min(table_size, addr_align);
> +		}
> +
> +		/* Minimum size of translate table is 4KiB */
> +		if (table_size < 0x1000) {
> +			dev_err(pcie->dev, "illegal table size
> %#llx\n",
> +				(unsigned long long)table_size);
> +			return -EINVAL;
> +		}
>  
> -	table = pcie->base + PCIE_TRANS_TABLE_BASE_REG +
> -		num * PCIE_ATR_TLB_SET_OFFSET;
> +		table = pcie->base + PCIE_TRANS_TABLE_BASE_REG + *num *
> PCIE_ATR_TLB_SET_OFFSET;
> +		writel_relaxed(lower_32_bits(cpu_addr) |
> PCIE_ATR_SIZE(fls(table_size) - 1), table);
> +		writel_relaxed(upper_32_bits(cpu_addr), table +
> PCIE_ATR_SRC_ADDR_MSB_OFFSET);
> +		writel_relaxed(lower_32_bits(pci_addr), table +
> PCIE_ATR_TRSL_ADDR_LSB_OFFSET);
> +		writel_relaxed(upper_32_bits(pci_addr), table +
> PCIE_ATR_TRSL_ADDR_MSB_OFFSET);
>  
> -	writel_relaxed(lower_32_bits(cpu_addr) |
> PCIE_ATR_SIZE(fls(size) - 1),
> -		       table);
> -	writel_relaxed(upper_32_bits(cpu_addr),
> -		       table + PCIE_ATR_SRC_ADDR_MSB_OFFSET);
> -	writel_relaxed(lower_32_bits(pci_addr),
> -		       table + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);
> -	writel_relaxed(upper_32_bits(pci_addr),
> -		       table + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);
> +		if (type == IORESOURCE_IO) {
> +			val = PCIE_ATR_TYPE_IO | PCIE_ATR_TLP_TYPE_IO;
> +			range_type = "IO";
> +		} else {
> +			val = PCIE_ATR_TYPE_MEM |
> PCIE_ATR_TLP_TYPE_MEM;
> +			range_type = "MEM";
> +		}
>  
> -	if (type == IORESOURCE_IO)
> -		val = PCIE_ATR_TYPE_IO | PCIE_ATR_TLP_TYPE_IO;
> -	else
> -		val = PCIE_ATR_TYPE_MEM | PCIE_ATR_TLP_TYPE_MEM;
> +		writel_relaxed(val, table +
> PCIE_ATR_TRSL_PARAM_OFFSET);
>  
> -	writel_relaxed(val, table + PCIE_ATR_TRSL_PARAM_OFFSET);
> +		dev_dbg(pcie->dev, "set %s trans window[%d]: cpu_addr =
> %#llx, pci_addr = %#llx, size = %#llx\n",
> +			range_type, *num, (unsigned long long)cpu_addr,
> +			(unsigned long long)pci_addr, (unsigned long
> long)table_size);
> +
> +		cpu_addr += table_size;
> +		pci_addr += table_size;
> +		remaining -= table_size;
> +		(*num)++;
> +	}
> +
> +	if (remaining)
> +		dev_warn(pcie->dev, "not enough translate table for
> addr: %#llx, limited to [%d]\n",
> +			 (unsigned long long)cpu_addr,
> PCIE_MAX_TRANS_TABLES);
>  
>  	return 0;
>  }
> @@ -380,30 +405,20 @@ static int mtk_pcie_startup_port(struct
> mtk_gen3_pcie *pcie)
>  		resource_size_t cpu_addr;
>  		resource_size_t pci_addr;
>  		resource_size_t size;
> -		const char *range_type;
>  
> -		if (type == IORESOURCE_IO) {
> +		if (type == IORESOURCE_IO)
>  			cpu_addr = pci_pio_to_address(res->start);
> -			range_type = "IO";
> -		} else if (type == IORESOURCE_MEM) {
> +		else if (type == IORESOURCE_MEM)
>  			cpu_addr = res->start;
> -			range_type = "MEM";
> -		} else {
> +		else
>  			continue;
> -		}
>  
>  		pci_addr = res->start - entry->offset;
>  		size = resource_size(res);
>  		err = mtk_pcie_set_trans_table(pcie, cpu_addr,
> pci_addr, size,
> -					       type, table_index);
> +					       type, &table_index);
>  		if (err)
>  			return err;
> -
> -		dev_dbg(pcie->dev, "set %s trans window[%d]: cpu_addr =
> %#llx, pci_addr = %#llx, size = %#llx\n",
> -			range_type, table_index, (unsigned long
> long)cpu_addr,
> -			(unsigned long long)pci_addr, (unsigned long
> long)size);
> -
> -		table_index++;
>  	}
>  
>  	return 0;