Re: [PATCH v3 08/11] PCI: rockchip: Fix window mapping and address translation for endpoint

From: Damien Le Moal
Date: Wed Apr 05 2023 - 07:44:43 EST

Next message: David Laight: "RE: [PATCH] ext4: simplify 32bit calculation of lblk"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 00/10] Add IOMMU support to MT8365 SoC"
In reply to: Rick Wertenbroek: "[PATCH v3 08/11] PCI: rockchip: Fix window mapping and address translation for endpoint"
Next in thread: Rick Wertenbroek: "[PATCH v3 09/11] PCI: rockchip: Use u32 variable to access 32-bit registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/4/23 17:24, Rick Wertenbroek wrote:
> The RK3399 PCI endpoint core has 33 windows for PCIe space, now in the
> driver up to 32 fixed size (1M) windows are used and pages are allocated
> and mapped accordingly. The driver first used a single window and allocated
> space inside which caused translation issues (between CPU space and PCI
> space) because a window can only have a single translation at a given
> time, which if multiple pages are allocated inside will cause conflicts.
> Now each window is a single region of 1M which will always guarantee that
> the translation is not in conflict.
>
> Set the translation register addresses for physical function. As documented
> in the technical reference manual (TRM) section 17.5.5 "PCIe Address
> Translation" and section 17.6.8 "Address Translation Registers Description"
>
> Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Rick Wertenbroek <rick.wertenbroek@xxxxxxxxx>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 110 +++++++++-------------
> drivers/pci/controller/pcie-rockchip.h | 30 +++---
> 2 files changed, 64 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 7591a7be78e0..f366846ad77c 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -64,52 +64,30 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
> }
>
> static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
> - u32 r, u32 type, u64 cpu_addr,
> - u64 pci_addr, size_t size)
> + u32 r, u64 cpu_addr, u64 pci_addr,
> + size_t size)
> {
> u64 sz = 1ULL << fls64(size - 1);
> int num_pass_bits = ilog2(sz);
> - u32 addr0, addr1, desc0, desc1;
> - bool is_nor_msg = (type == AXI_WRAPPER_NOR_MSG);
> + u32 addr0, addr1, desc0;
>
> - /* The minimal region size is 1MB */
> if (num_pass_bits < 8)
> num_pass_bits = 8;
>
> - cpu_addr -= rockchip->mem_res->start;
> - addr0 = ((is_nor_msg ? 0x10 : (num_pass_bits - 1)) &
> - PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
> - (lower_32_bits(cpu_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> - addr1 = upper_32_bits(is_nor_msg ? cpu_addr : pci_addr);
> - desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | type;
> - desc1 = 0;
> -
> - if (is_nor_msg) {
> - rockchip_pcie_write(rockchip, 0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> - rockchip_pcie_write(rockchip, 0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> - rockchip_pcie_write(rockchip, desc0,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> - rockchip_pcie_write(rockchip, desc1,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> - } else {
> - /* PCI bus address region */
> - rockchip_pcie_write(rockchip, addr0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> - rockchip_pcie_write(rockchip, addr1,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> - rockchip_pcie_write(rockchip, desc0,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> - rockchip_pcie_write(rockchip, desc1,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> -
> - addr0 =
> - ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
> - (lower_32_bits(cpu_addr) &
> - PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> - addr1 = upper_32_bits(cpu_addr);
> - }
> + addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
> + (lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> + addr1 = upper_32_bits(pci_addr);
> + desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | AXI_WRAPPER_MEM_WRITE;
> +
> + /* PCI bus address region */
> + rockchip_pcie_write(rockchip, addr0,
> + ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> + rockchip_pcie_write(rockchip, addr1,
> + ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> + rockchip_pcie_write(rockchip, desc0,
> + ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> + rockchip_pcie_write(rockchip, 0,
> + ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> }
>
> static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
> @@ -248,6 +226,11 @@ static void rockchip_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar));
> }
>
> +static inline u32 rockchip_ob_region(phys_addr_t addr)
> +{
> + return (addr >> ilog2(SZ_1M)) & 0x1f;
> +}
> +
> static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
> phys_addr_t addr, u64 pci_addr,
> size_t size)
> @@ -256,18 +239,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
> struct rockchip_pcie *pcie = &ep->rockchip;
> u32 r;
>
> - r = find_first_zero_bit(&ep->ob_region_map, BITS_PER_LONG);
> - /*
> - * Region 0 is reserved for configuration space and shouldn't
> - * be used elsewhere per TRM, so leave it out.
> - */
> - if (r >= ep->max_regions - 1) {
> - dev_err(&epc->dev, "no free outbound region\n");
> - return -EINVAL;
> - }
> + r = rockchip_ob_region(addr);

Nit: you can move this together with the decalration:

u32 r = rockchip_ob_region(addr);

>
> - rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, AXI_WRAPPER_MEM_WRITE, addr,
> - pci_addr, size);
> + rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size);
>
> set_bit(r, &ep->ob_region_map);
> ep->ob_addr[r] = addr;
> @@ -282,15 +256,11 @@ static void rockchip_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
> struct rockchip_pcie *rockchip = &ep->rockchip;
> u32 r;
>
> - for (r = 0; r < ep->max_regions - 1; r++)
> + for (r = 0; r < ep->max_regions; r++)
> if (ep->ob_addr[r] == addr)
> break;
>
> - /*
> - * Region 0 is reserved for configuration space and shouldn't
> - * be used elsewhere per TRM, so leave it out.
> - */
> - if (r == ep->max_regions - 1)
> + if (r == ep->max_regions)
> return;
>
> rockchip_pcie_clear_ep_ob_atu(rockchip, r);
> @@ -388,6 +358,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> u16 flags, mme, data, data_mask;
> u8 msi_count;
> u64 pci_addr, pci_addr_mask = 0xff;

Nit: pci_addr_mask is constant and never changed, so we could get rid of this
variable and use a macro instead.

> + u32 r;
>
> /* Check MSI enable bit */
> flags = rockchip_pcie_read(&ep->rockchip,
> @@ -421,13 +392,12 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> PCI_MSI_ADDRESS_LO);
> - pci_addr &= GENMASK_ULL(63, 2);
>
> /* Set the outbound region if needed. */
> if (unlikely(ep->irq_pci_addr != (pci_addr & ~pci_addr_mask) ||
> ep->irq_pci_fn != fn)) {
> - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, ep->max_regions - 1,
> - AXI_WRAPPER_MEM_WRITE,
> + r = rockchip_ob_region(ep->irq_phys_addr);
> + rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> ep->irq_phys_addr,
> pci_addr & ~pci_addr_mask,
> pci_addr_mask + 1);
> @@ -516,6 +486,8 @@ static int rockchip_pcie_parse_ep_dt(struct rockchip_pcie *rockchip,
> if (err < 0 || ep->max_regions > MAX_REGION_LIMIT)
> ep->max_regions = MAX_REGION_LIMIT;
>
> + ep->ob_region_map = 0;
> +
> err = of_property_read_u8(dev->of_node, "max-functions",
> &ep->epc->max_functions);
> if (err < 0)
> @@ -536,7 +508,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> struct rockchip_pcie *rockchip;
> struct pci_epc *epc;
> size_t max_regions;
> - int err;
> + struct pci_epc_mem_window *windows = NULL;
> + int err, i;
>
> ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> if (!ep)
> @@ -583,15 +556,26 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> /* Only enable function 0 by default */
> rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
>
> - err = pci_epc_mem_init(epc, rockchip->mem_res->start,
> - resource_size(rockchip->mem_res), PAGE_SIZE);
> + windows = devm_kcalloc(dev, ep->max_regions, sizeof(struct pci_epc_mem_window), GFP_KERNEL);

Nit: long line. Please split it at sizeof(...).

> + if (!windows) {
> + err = -ENOMEM;
> + goto err_uninit_port;
> + }
> + for (i = 0; i < ep->max_regions; i++) {
> + windows[i].phys_base = rockchip->mem_res->start + (SZ_1M * i);
> + windows[i].size = SZ_1M;
> + windows[i].page_size = SZ_1M;
> + }
> + err = pci_epc_multi_mem_init(epc, windows, ep->max_regions);
> + devm_kfree(dev, windows);
> +
> if (err < 0) {
> dev_err(dev, "failed to initialize the memory space\n");
> goto err_uninit_port;
> }
>
> ep->irq_cpu_addr = pci_epc_mem_alloc_addr(epc, &ep->irq_phys_addr,
> - SZ_128K);
> + SZ_1M);
> if (!ep->irq_cpu_addr) {
> dev_err(dev, "failed to reserve memory space for MSI\n");
> err = -ENOMEM;
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index ffc68a3a5fee..5797ba73bb6b 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -139,6 +139,7 @@
>
> #define PCIE_RC_RP_ATS_BASE 0x400000
> #define PCIE_RC_CONFIG_NORMAL_BASE 0x800000
> +#define PCIE_EP_PF_CONFIG_REGS_BASE 0x800000
> #define PCIE_RC_CONFIG_BASE 0xa00000
> #define PCIE_EP_CONFIG_BASE 0xa00000
> #define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)
> @@ -232,13 +233,15 @@
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
> #define ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR 0x1
> -#define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
> +#define ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR 0x3
> +#define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) \
> + (PCIE_EP_PF_CONFIG_REGS_BASE + (((fn) << 12) & GENMASK(19, 12)))
> +#define ROCKCHIP_PCIE_EP_VIRT_FUNC_BASE(fn) \
> + (PCIE_EP_PF_CONFIG_REGS_BASE + 0x10000 + (((fn) << 12) & GENMASK(19, 12)))
> #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
> - (PCIE_RC_RP_ATS_BASE + 0x0840 + (fn) * 0x0040 + (bar) * 0x0008)
> + (PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
> #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \
> - (PCIE_RC_RP_ATS_BASE + 0x0844 + (fn) * 0x0040 + (bar) * 0x0008)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0000 + ((r) & 0x1f) * 0x0020)
> + (PCIE_CORE_AXI_CONF_BASE + 0x082c + (fn) * 0x0040 + (bar) * 0x0008)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN_MASK GENMASK(19, 12)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) \
> (((devfn) << 12) & \
> @@ -246,20 +249,21 @@
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK GENMASK(27, 20)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(bus) \
> (((bus) << 20) & ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK)
> +#define PCIE_RC_EP_ATR_OB_REGIONS_1_32 (PCIE_CORE_AXI_CONF_BASE + 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0000 + ((r) & 0x1f) * 0x0020)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0004 + ((r) & 0x1f) * 0x0020)
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0004 + ((r) & 0x1f) * 0x0020)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID BIT(23)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK GENMASK(31, 24)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(devfn) \
> (((devfn) << 24) & ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0008 + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x000c + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0018 + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x001c + ((r) & 0x1f) * 0x0020)
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0008 + ((r) & 0x1f) * 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x000c + ((r) & 0x1f) * 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_DESC2(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0010 + ((r) & 0x1f) * 0x0020)
>
> #define ROCKCHIP_PCIE_CORE_EP_FUNC_BAR_CFG0(fn) \
> (PCIE_CORE_CTRL_MGMT_BASE + 0x0240 + (fn) * 0x0008)

Next message: David Laight: "RE: [PATCH] ext4: simplify 32bit calculation of lblk"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 00/10] Add IOMMU support to MT8365 SoC"
In reply to: Rick Wertenbroek: "[PATCH v3 08/11] PCI: rockchip: Fix window mapping and address translation for endpoint"
Next in thread: Rick Wertenbroek: "[PATCH v3 09/11] PCI: rockchip: Use u32 variable to access 32-bit registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]