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

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.

For dbi_base2, you've added error checking in platform specific code
(pci-dra7xx.c), but I don't see any error checking in your patch proposal
pcie-designware-ep.c:dw_pcie_ep_init.
If you add error checking in dw_pcie_ep_init, then I agree,
we don't need any error checking in dw_pcie_writel_dbi2.

Thanks
Kishon