Hi Niklas,

On Friday 10 March 2017 06:01 PM, Niklas Cassel wrote:

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:

quoted

Hi,

On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:

quoted

On 03/09/2017 03:48 PM, Niklas Cassel wrote:

quoted

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:

quoted

dwc has 2 dbi address space labeled dbics and dbics2. The existing
helper to access dbi address space can access only dbics. However
dbics2 has to be accessed for programming the BAR registers in the
case of EP mode. This is in preparation for adding EP mode support
to dwc driver.

Hello Kishon

I don't really like the idea of adding an extra argument to every existing read/write.
Will not a read/write using dbi2 be quite uncommon compared to a read/write
using dbi?

How about something like this:

void __dw_pcie_writel(struct dw_pcie *pci, void __iomem *base, u32 reg, u32 val)
{
    if (pci->ops->writel_dbi)
        pci->ops->writel_dbi(pci, base, reg, val);
    else
        writel(val, base + reg);
}

#define dw_pcie_writel_dbi(pci, reg, val) __dw_pcie_writel(pci, pci->dbi_base, reg, val)
#define dw_pcie_writel_dbi2(pci, reg, val) __dw_pcie_writel(pci, pci->dbi_base2, reg, val)

Perhaps make dw_pcie_writel_dbi2 a function rather than a define,
so we can return an error if pci->dbi_base2 == NULL.

Should we return an error? We don't return error for dbi_base either. I think
it should be sufficient to return errors while populating dbi_base or
dbi_base2. Otherwise it's a bug and should result in abort. Joao?

Sorry for previous empty email.


What I meant to write:

Right now we do error checking for dbi_base in platform specific code
and in pcie-designware-host.c:dw_pcie_host_init.

it's been done in dw_pcie_host_init not as an error checking but since it's
*optional* for certain platforms to populate dbi_base (i.e where dbi_base is
mapped to configuration space), host_init takes care of assigning dbi_base to
configuration space address.

Thanks
Kishon