Message-ID: <052bdaf1-37b2-4ee8-af6a-68912a152955@oss.qualcomm.com>
Date: Tue, 26 Aug 2025 10:43:01 -0700
From: Mayank Rana <mayank.rana@....qualcomm.com>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
        cros-qcom-dts-watchers@...omium.org,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Krzysztof Wilczyński <kwilczynski@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>
Cc: linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        quic_vbadigan@...cinc.com, quic_mrana@...cinc.com,
        quic_vpernami@...cinc.com, mmareddy@...cinc.com
Subject: Re: [PATCH v7 4/5] PCI: dwc: Add ECAM support with iATU configuration



On 8/26/2025 5:46 AM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/25/2025 11:35 PM, Mayank Rana wrote:
>> Hi Krishna
>>
>> On 8/22/2025 2:27 AM, Krishna Chaitanya Chundru wrote:
>>> The current implementation requires iATU for every configuration
>>> space access which increases latency & cpu utilization.
>>>
>>> Designware databook 5.20a, section 3.10.10.3 says about CFG Shift 
>>> Feature,
>>> which shifts/maps the BDF (bits [31:16] of the third header DWORD, which
>>> would be matched against the Base and Limit addresses) of the incoming
>>> CfgRd0/CfgWr0 down to bits[27:12]of the translated address.
>>>
>>> Configuring iATU in config shift feature enables ECAM feature to 
>>> access the
>>> config space, which avoids iATU configuration for every config access.
>>>
>>> Add "ctrl2" into struct dw_pcie_ob_atu_cfg  to enable config shift 
>>> feature.
>>>
>>> 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 achieve this.
>>>
>>> Enable the ECAM feature if the config space size is equal to size 
>>> required
>>> to represent number of buses in the bus range property.
>>
>> Also add 256 MB alignment requirement for using iATU config shift mode 
>> here.
>>
>>> Signed-off-by: Krishna Chaitanya Chundru 
>>> <krishna.chundru@....qualcomm.com>
>>> ---
>>>   drivers/pci/controller/dwc/Kconfig                |   1 +
>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 131 +++++++++++ 
>>> ++++++++---
>>>   drivers/pci/controller/dwc/pcie-designware.c      |   2 +-
>>>   drivers/pci/controller/dwc/pcie-designware.h      |   5 +
>>>   4 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/ 
>>> controller/dwc/Kconfig
>>> index 
>>> ff6b6d9e18ecfa44273e87931551f9e63fbe3cba..a0e7ad3fb5afec63b0f919732a50147229623186 100644
>>> --- a/drivers/pci/controller/dwc/Kconfig
>>> +++ b/drivers/pci/controller/dwc/Kconfig
>>> @@ -20,6 +20,7 @@ config PCIE_DW_HOST
>>>       bool
>>>       select PCIE_DW
>>>       select IRQ_MSI_LIB
>>> +    select PCI_HOST_COMMON
>>>   config PCIE_DW_EP
>>>       bool
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/ 
>>> drivers/pci/controller/dwc/pcie-designware-host.c
>>> index 
>>> 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..abb93265a19fd62d3fecc64f29f37baf67291b40 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -413,6 +413,81 @@ 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};
>>> +    resource_size_t bus_range_max;
>>> +    struct resource_entry *bus;
>>> +    int ret;
>>> +
>>> +    bus = resource_list_first_type(&pp->bridge->windows, 
>>> IORESOURCE_BUS);
>>> +
>>> +    /*
>>> +     * Root bus under the host bridge doesn't require any iATU 
>>> configuration
>>> +     * as DBI region will be used to access root bus config space.
>>> +     * Immediate bus under Root Bus, needs type 0 iATU configuration 
>>> and
>>> +     * remaining buses need type 1 iATU configuration.
>>> +     */
>>> +    atu.index = 0;
>>> +    atu.type = PCIE_ATU_TYPE_CFG0;
>>> +    atu.parent_bus_addr = pp->cfg0_base + SZ_1M;
>>> +    /* 1MiB is to cover 1 (bus) * 32 (devices) * 8 (functions) */
>>> +    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 = resource_size(bus->res);
>>> +
>>> +    if (bus_range_max < 2)
>>> +        return 0;
>>> +
>>> +    /* Configure remaining buses in type 1 iATU configuration */
>>> +    atu.index = 1;
>>> +    atu.type = PCIE_ATU_TYPE_CFG1;
>>> +    atu.parent_bus_addr = pp->cfg0_base + SZ_2M;
>>> +    atu.size = (SZ_1M * bus_range_max) - SZ_2M;
>>> +    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;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static bool dw_pcie_ecam_enabled(struct dw_pcie_rp *pp, struct 
>>> resource *config_res)
>>> +{
>>> +    struct resource *bus_range;
>>> +    u64 nr_buses;
>>
>> As change is using Synopsis IP based iATU config shift mode 
>> functionality, it is must that ECAM/DBI base address has to be 256 MB 
>> aligned. Hence add change to check against alignment.
>>
> Just to clarify this is not Synopsis IP requirement it is PCie
> requirement. PCIe spec 6, sec 7.2.2 says "base address of the range is
> aligned to a 2(n+20)-byte memory address boundary". n is 8 here.
Clarify what is n is suggesting here ?

Agree. ECAM is PCIe spec defined feature, and it doesn't suggest how
PCIe controller handle iATU configuration to support ECAM mode. The ECAM 
address alignment requirement is based on the size of the access being 
performed, and it needs to be naturally aligned i.e. 32-bit access must 
be 4-byte aligned.

As here you are explicitly configuring iATU once using config shift mode 
to support ECAM, this brings 256 MB requirement with Synopsis PCIe
controller. Even for bus range 1, We need 256 MB alignment requirement
with Synopsys PCIe controller when using iATU config shift mode. The 
iATU of controller uses bits [27:12] of the original address to form 
bits [31:16] (BDF location) of the outgoing CFG TLP when using iATU 
config shift mode.

We can have ECAM mode support without using iATU config shift mode in 
which all config space access will require to reconfigure iATU for each 
config space access, and doesn't need to hold this alignement 
requirement of 256 MB.

Regards,
Mayank>
> I will add this info as a comment.
>> #define IS_256MB_ALIGNED(x) IS_ALIGNED(x, SZ_256M)
>>
>> if (!IS_256MB_ALIGNED(config_res->start))
>>            return false;
>>
> Ack.
> 
> - Krishna Chaitanya.
>>> +
>>> +    bus_range = resource_list_first_type(&pp->bridge->windows, 
>>> IORESOURCE_BUS)->res;
>>> +    if (!bus_range)
>>> +        return false;
>>> +
>>> +    nr_buses = resource_size(config_res) >> PCIE_ECAM_BUS_SHIFT;
>>> +
>>> +    return !!(nr_buses >= resource_size(bus_range));
>>> +}
>>> +
>>>   static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>>>   {
>>>       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> @@ -422,10 +497,6 @@ static int dw_pcie_host_get_resources(struct 
>>> dw_pcie_rp *pp)
>>>       struct resource *res;
>>>       int ret;
>>> -    ret = dw_pcie_get_resources(pci);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>>       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
>>> "config");
>>>       if (!res) {
>>>           dev_err(dev, "Missing \"config\" reg space\n");
>>> @@ -435,9 +506,32 @@ static int dw_pcie_host_get_resources(struct 
>>> dw_pcie_rp *pp)
>>>       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);
>>> +    pp->ecam_enabled = dw_pcie_ecam_enabled(pp, res);
>>> +    if (pp->ecam_enabled) {
>>> +        ret = dw_pcie_create_ecam_window(pp, res);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        pp->bridge->ops = (struct pci_ops 
>>> *)&pci_generic_ecam_ops.pci_ops;
>>> +        pp->bridge->sysdata = pp->cfg;
>>> +        pp->cfg->priv = pp;
>>> +    } else {
>>> +        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 */
>>> +        pp->bridge->ops = &dw_pcie_ops;
>>> +        pp->bridge->child_ops = &dw_child_pcie_ops;
>>> +        pp->bridge->sysdata = pp;
>>> +    }
>>> +
>>> +    ret = dw_pcie_get_resources(pci);
>>> +    if (ret) {
>>> +        if (pp->cfg)
>>> +            pci_ecam_free(pp->cfg);
>>> +        return ret;
>>> +    }
>>>       /* Get the I/O range from DT */
>>>       win = resource_list_first_type(&pp->bridge->windows, 
>>> IORESOURCE_IO);
>>> @@ -476,14 +570,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>       if (ret)
>>>           return ret;
>>> -    /* Set default bus ops */
>>> -    bridge->ops = &dw_pcie_ops;
>>> -    bridge->child_ops = &dw_child_pcie_ops;
>>> -
>>>       if (pp->ops->init) {
>>>           ret = pp->ops->init(pp);
>>>           if (ret)
>>> -            return ret;
>>> +            goto err_free_ecam;
>>>       }
>>>       if (pci_msi_enabled()) {
>>> @@ -525,6 +615,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>       if (ret)
>>>           goto err_free_msi;
>>> +    if (pp->ecam_enabled) {
>>> +        ret = dw_pcie_config_ecam_iatu(pp);
>>> +        if (ret) {
>>> +            dev_err(dev, "Failed to configure iATU in ECAM mode\n");
>>> +            goto err_free_msi;
>>> +        }
>>> +    }
>>> +
>>>       /*
>>>        * Allocate the resource for MSG TLP before programming the iATU
>>>        * outbound window in dw_pcie_setup_rc(). Since the allocation 
>>> depends
>>> @@ -560,8 +658,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>           /* Ignore errors, the link may come up later */
>>>           dw_pcie_wait_for_link(pci);
>>> -    bridge->sysdata = pp;
>>> -
>>>       ret = pci_host_probe(bridge);
>>>       if (ret)
>>>           goto err_stop_link;
>>> @@ -587,6 +683,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>       if (pp->ops->deinit)
>>>           pp->ops->deinit(pp);
>>> +err_free_ecam:
>>> +    if (pp->cfg)
>>> +        pci_ecam_free(pp->cfg);
>>> +
>>>       return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(dw_pcie_host_init);
>>> @@ -609,6 +709,9 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
>>>       if (pp->ops->deinit)
>>>           pp->ops->deinit(pp);
>>> +
>>> +    if (pp->cfg)
>>> +        pci_ecam_free(pp->cfg);
>>>   }
>>>   EXPORT_SYMBOL_GPL(dw_pcie_host_deinit);
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/ 
>>> pci/controller/dwc/pcie-designware.c
>>> index 
>>> 4684c671a81bee468f686a83cc992433b38af59d..6826ddb9478d41227fa011018cffa8d2242336a9 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>> @@ -576,7 +576,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>>>           val = dw_pcie_enable_ecrc(val);
>>>       dw_pcie_writel_atu_ob(pci, atu->index, PCIE_ATU_REGION_CTRL1, 
>>> val);
>>> -    val = PCIE_ATU_ENABLE;
>>> +    val = PCIE_ATU_ENABLE | atu->ctrl2;
>>>       if (atu->type == PCIE_ATU_TYPE_MSG) {
>>>           /* The data-less messages only for now */
>>>           val |= PCIE_ATU_INHIBIT_PAYLOAD | atu->code;
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/ 
>>> pci/controller/dwc/pcie-designware.h
>>> index 
>>> ceb022506c3191cd8fe580411526e20cc3758fed..f770e160ce7c538e0835e7cf80bae9ed099f906c 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -20,6 +20,7 @@
>>>   #include <linux/irq.h>
>>>   #include <linux/msi.h>
>>>   #include <linux/pci.h>
>>> +#include <linux/pci-ecam.h>
>>>   #include <linux/reset.h>
>>>   #include <linux/pci-epc.h>
>>> @@ -169,6 +170,7 @@
>>>   #define PCIE_ATU_REGION_CTRL2        0x004
>>>   #define PCIE_ATU_ENABLE            BIT(31)
>>>   #define PCIE_ATU_BAR_MODE_ENABLE    BIT(30)
>>> +#define PCIE_ATU_CFG_SHIFT_MODE_ENABLE    BIT(28)
>>>   #define PCIE_ATU_INHIBIT_PAYLOAD    BIT(22)
>>>   #define PCIE_ATU_FUNC_NUM_MATCH_EN      BIT(19)
>>>   #define PCIE_ATU_LOWER_BASE        0x008
>>> @@ -387,6 +389,7 @@ struct dw_pcie_ob_atu_cfg {
>>>       u8 func_no;
>>>       u8 code;
>>>       u8 routing;
>>> +    u32 ctrl2;
>>>       u64 parent_bus_addr;
>>>       u64 pci_addr;
>>>       u64 size;
>>> @@ -425,6 +428,8 @@ struct dw_pcie_rp {
>>>       struct resource        *msg_res;
>>>       bool            use_linkup_irq;
>>>       struct pci_eq_presets    presets;
>>> +    bool            ecam_enabled;
>>> +    struct pci_config_window *cfg;
>>>   };
>>>   struct dw_pcie_ep_ops {
>>>
>> Regards,
>> Mayank

Powered by blists - more mailing lists