Re: [PATCH 2/3] PCI: dwc: Add ECAM support with iATU configuration

Manivannan Sadhasivam
2024-12-02T16:42:17Z

On Sun, Nov 17, 2024 at 03:30:19AM +0530, Krishna chaitanya chundru wrote:
> The current implementation requires iATU for every configuration
> space access which increases latency & cpu utilization.
>
> Configuring iATU in config shift mode enables ECAM feature to access the

Can you please elaborate 'config shift mode'? Quote relevant section in DWC
databook for reference.

> config space, which avoids iATU configuration for every config access.
>
> Add "ctrl2" into struct dw_pcie_ob_atu_cfg to enable config shift mode.
>
> As DBI comes under config space, this avoids remapping of DBI space
> separately. Instead, it uses the mapped config space address returned from
> ECAM initialization. Change the order of dw_pcie_get_resources() execution
> to acheive this.
>
> Introduce new ecam_init() function op for the clients to configure after

We use 'DWC glue drivers' to refer the 'clients' of this driver.

> ecam window creation has been done.
>

Use 'ECAM' everywhere.

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 114 ++++++++++++++++++----
> drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> drivers/pci/controller/dwc/pcie-designware.h | 6 ++
> 3 files changed, 102 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 3e41865c7290..e98cc841a2a9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -418,6 +418,62 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
> }
> }
>
> +static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct dw_pcie_ob_atu_cfg atu = {0};
> + struct resource_entry *bus;
> + int ret, bus_range_max;
> +
> + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS);
> +
> + /*
> + * Bus 1 config space needs type 0 atu configuration
> + * Remaining buses need type 1 atu configuration
> + */
> + atu.index = 0;
> + atu.type = PCIE_ATU_TYPE_CFG0;
> + atu.cpu_addr = pp->cfg0_base + SZ_1M;

You didn't mention what occupies the first 1MB.

> + atu.size = SZ_1M;
> + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE;
> + ret = dw_pcie_prog_outbound_atu(pci, &atu);
> + if (ret)
> + return ret;
> +
> + bus_range_max = bus->res->end - bus->res->start + 1;

resource_size(bus->res)

> +
> + /* Configure for bus 2 - bus_range_max in type 1 */
> + atu.index = 1;
> + atu.type = PCIE_ATU_TYPE_CFG1;
> + atu.cpu_addr = pp->cfg0_base + SZ_2M;
> + atu.size = (SZ_1M * (bus_range_max - 2));
> + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE;
> + return dw_pcie_prog_outbound_atu(pci, &atu);
> +}
> +
> +static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *res)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct device *dev = pci->dev;
> + struct resource_entry *bus;
> +
> + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS);
> + if (!bus)
> + return -ENODEV;
> +
> + pp->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops);
> + if (IS_ERR(pp->cfg))
> + return PTR_ERR(pp->cfg);
> +
> + pci->dbi_base = pp->cfg->win;
> + pci->dbi_phys_addr = res->start;
> +
> + if (pp->ops->ecam_init)
> + pp->ops->ecam_init(pci, pp->cfg);
> +
> + return 0;
> +}
> +
> int dw_pcie_host_init(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -431,19 +487,8 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>
> raw_spin_lock_init(&pp->lock);
>
> - ret = dw_pcie_get_resources(pci);
> - if (ret)
> - return ret;
> -
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
> - if (res) {
> - pp->cfg0_size = resource_size(res);
> - pp->cfg0_base = res->start;
> -
> - pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
> - if (IS_ERR(pp->va_cfg0_base))
> - return PTR_ERR(pp->va_cfg0_base);
> - } else {
> + if (!res) {
> dev_err(dev, "Missing *config* reg space\n");
> return -ENODEV;
> }
> @@ -454,6 +499,30 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>
> pp->bridge = bridge;
>
> + pp->cfg0_size = resource_size(res);
> + pp->cfg0_base = res->start;
> +
> + if (!pp->enable_ecam) {

Why can't you just use the ECAM mode when there is enough memory defined in DT?
Using this flag slightly defeats the purpose of the ECAM mode.

> + pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
> + if (IS_ERR(pp->va_cfg0_base))
> + return PTR_ERR(pp->va_cfg0_base);
> +
> + /* Set default bus ops */
> + bridge->ops = &dw_pcie_ops;
> + bridge->child_ops = &dw_child_pcie_ops;
> + bridge->sysdata = pp;
> + } else {
> + ret = dw_pcie_create_ecam_window(pp, res);
> + if (ret)
> + return ret;
> + bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> + pp->bridge->sysdata = pp->cfg;

'bridge->sysdata = pp->cfg'?

- Mani

--
மணிவண்ணன் சதாசிவம்