Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 10/18/2025 2:55 PM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 10/18/2025 8:39 AM, Manivannan Sadhasivam wrote:
>> On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
>>> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru 
>>> wrote:
>>>> When the vendor configuration space is 256MB aligned, the DesignWare
>>>> PCIe host driver enables ECAM access and sets the DBI base to the 
>>>> dw_pcie_ecam_conf_map_busstart
>>>> of the config space. This causes vendor drivers to incorrectly program
>>>> iATU regions, as they rely on the DBI address for internal accesses.
>>>>
>>>> To fix this, avoid overwriting the DBI base when ECAM is enabled.
>>>> Instead, introduce a custom ECAM PCI ops implementation that accesses
>>>> the DBI region directly for bus 0 and uses ECAM for other buses.
>>>>
>>>> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM 
>>>> mechanism using iATU 'CFG Shift Feature'")
>>>> Reported-by: Ron Economos <re@w6rz.net>
>>>> Closes: 
>>>> https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
>>>> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
>>>> Signed-off-by: Krishna Chaitanya Chundru 
>>>> <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 
>>>> +++++++++++++++++++----
>>>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c 
>>>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> index 
>>>> 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> @@ -23,6 +23,7 @@
>>>>   #include "pcie-designware.h"
>>>>   static struct pci_ops dw_pcie_ops;
>>>> +static struct pci_ops dw_pcie_ecam_ops;
>>>>   static struct pci_ops dw_child_pcie_ops;
>>>>   #define DW_PCIE_MSI_FLAGS_REQUIRED 
>>>> (MSI_FLAG_USE_DEF_DOM_OPS        | \
>>>> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct 
>>>> dw_pcie_rp *pp, struct resource *re
>>>>       if (IS_ERR(pp->cfg))
>>>>           return PTR_ERR(pp->cfg);
>>>> -    pci->dbi_base = pp->cfg->win;
>>>> -    pci->dbi_phys_addr = res->start;
>>>> -
>>>>       return 0;
>>>>   }
>>>> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct 
>>>> dw_pcie_rp *pp)
>>>>           if (ret)
>>>>               return ret;
>>>> -        pp->bridge->ops = (struct pci_ops 
>>>> *)&pci_generic_ecam_ops.pci_ops;
>>>> +        pp->bridge->ops = &dw_pcie_ecam_ops;
>>>>           pp->bridge->sysdata = pp->cfg;
>>>>           pp->cfg->priv = pp;
>>>>       } else {
>>>> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct 
>>>> pci_bus *bus, unsigned int devfn,
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>>> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, 
>>>> unsigned int devfn, int where)
>>>> +{
>>>> +    struct pci_config_window *cfg = bus->sysdata;
>>>> +    struct dw_pcie_rp *pp = cfg->priv;
>>>> +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> +    unsigned int busn = bus->number;
>>>> +
>>>> +    if (busn > 0)
>>>> +        return pci_ecam_map_bus(bus, devfn, where);
>>>
>>> Is there a way to avoid the "root bus is bus 00" assumption here?  It
>>> looks like something like this might work (it inverts the condition
>>> to take care of the root bus special case first):
>>>
>>>    if (bus == pp->bridge->bus) {
>>>      if (PCI_SLOT(devfn) > 0)
>>>        return NULL;
>>>
>>>      return pci->dbi_base + where;
>>>    }
>>>
>>>    return pci_ecam_map_bus(bus, devfn, where);
>>>
>>
>> I guess it will work.
>>
>>>> +    if (PCI_SLOT(devfn) > 0)
>>>> +        return NULL;
>>>
>>> This essentially says only one function (00.0) can be on the root bus.
>>> I assume that someday that will be relaxed and there may be multiple
>>> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
>>> didn't have to have this check.
>>>
>>> What happens without it?  Does the IP return the ~0 data that the PCI
>>> core would interpret as "there's no device here"?
>>>
>>
>> I hope the read returns ~0, but the idea is to catch the invalid 
>> access before
>> trying to read/write. In case of multi Root Port design, I don't think 
>> we have a
>> way to identify it. So maybe it is safe to remove this check.
>>
> For multi root port we may need to revisit this, currently along with
> dbi there are some other registers iATU, elbi etc. So there might be
> chances to read these registers like iATU as part of enumeration and
> these can return non ~0 values which will have adverse effects.
> So we should have this check for now.
> 
One more issue is some controllers may pass only 4k memory as dbi memory
so we might get unmapped address access issues also.
- Krishna Chaitanya.
> - Krishna Chaitanya.
> 
>> - Mani
>>