Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

Hi Niklas,

On Friday 10 March 2017 09:17 PM, Niklas Cassel wrote:
> 
> 
> On 03/10/2017 01:56 PM, Kishon Vijay Abraham I wrote:
>> 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:
 Hi,

 On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> 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.
> 
> What I'm afraid of is that we might get a NULL ptr dereference
> when using dw_pcie_writel_dbi2, if platform specific code has
> not populated dbi_base2.
> 
> Having a NULL check in generic code is just a fail safe if some
> platform specific code failed to NULL check.
> 
> The code in dw_pcie_host_init might have been written just
> to populate dbi_base when dbi is mapped to config space,
> but the end result is that if platform specific code did not
> populate dbi_base (and did not populate pp->cfg),
> we will return -ENOMEM.
> Which means that we can never get a NULL ptr dereference
> when using dw_pcie_writel_dbi.
> 
> It might be a good idea to have a NULL check in generic code,
> just as a fail safe, also for dw_pcie_ep_init.
> That way we know that we will not get a NULL ptr dereference
> when using dw_pcie_writel_dbi2.

All right, will add it then.

Thanks
Kishon

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

Hi Niklas,

On Friday 10 March 2017 09:17 PM, Niklas Cassel wrote:
> 
> 
> On 03/10/2017 01:56 PM, Kishon Vijay Abraham I wrote:
>> 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:
 Hi,

 On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> 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.
> 
> What I'm afraid of is that we might get a NULL ptr dereference
> when using dw_pcie_writel_dbi2, if platform specific code has
> not populated dbi_base2.
> 
> Having a NULL check in generic code is just a fail safe if some
> platform specific code failed to NULL check.
> 
> The code in dw_pcie_host_init might have been written just
> to populate dbi_base when dbi is mapped to config space,
> but the end result is that if platform specific code did not
> populate dbi_base (and did not populate pp->cfg),
> we will return -ENOMEM.
> Which means that we can never get a NULL ptr dereference
> when using dw_pcie_writel_dbi.
> 
> It might be a good idea to have a NULL check in generic code,
> just as a fail safe, also for dw_pcie_ep_init.
> That way we know that we will not get a NULL ptr dereference
> when using dw_pcie_writel_dbi2.

All right, will add it then.

Thanks
Kishon

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 01:56 PM, Kishon Vijay Abraham I wrote:
> 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:
>>> Hi,
>>>
>>> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
 On 03/09/2017 03:48 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> 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.

What I'm afraid of is that we might get a NULL ptr dereference
when using dw_pcie_writel_dbi2, if platform specific code has
not populated dbi_base2.

Having a NULL check in generic code is just a fail safe if some
platform specific code failed to NULL check.

The code in dw_pcie_host_init might have been written just
to populate dbi_base when dbi is mapped to config space,
but the end result is that if platform specific code did not
populate dbi_base (and did not populate pp->cfg),
we will return -ENOMEM.
Which means that we can never get a NULL ptr dereference
when using dw_pcie_writel_dbi.

It might be a good idea to have a NULL check in generic code,
just as a fail safe, also for dw_pcie_ep_init.
That way we know that we will not get a NULL ptr dereference
when using dw_pcie_writel_dbi2.

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 01:56 PM, Kishon Vijay Abraham I wrote:
> 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:
>>> Hi,
>>>
>>> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
 On 03/09/2017 03:48 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> 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.

What I'm afraid of is that we might get a NULL ptr dereference
when using dw_pcie_writel_dbi2, if platform specific code has
not populated dbi_base2.

Having a NULL check in generic code is just a fail safe if some
platform specific code failed to NULL check.

The code in dw_pcie_host_init might have been written just
to populate dbi_base when dbi is mapped to config space,
but the end result is that if platform specific code did not
populate dbi_base (and did not populate pp->cfg),
we will return -ENOMEM.
Which means that we can never get a NULL ptr dereference
when using dw_pcie_writel_dbi.

It might be a good idea to have a NULL check in generic code,
just as a fail safe, also for dw_pcie_ep_init.
That way we know that we will not get a NULL ptr dereference
when using dw_pcie_writel_dbi2.

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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:
>> Hi,
>>
>> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>>
>>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
 On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> 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

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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:
>> Hi,
>>
>> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>>
>>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
 On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> 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

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Joao Pinto]

Às 11:36 AM de 3/10/2017, Kishon Vijay Abraham I escreveu:
> Hi,
> 
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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?

I agree with Kishon.

Thanks,
Joao

> 
> Thanks
> Kishon
> 

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Joao Pinto]

Às 11:36 AM de 3/10/2017, Kishon Vijay Abraham I escreveu:
> Hi,
> 
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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?

I agree with Kishon.

Thanks,
Joao

> 
> Thanks
> Kishon
> 

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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);x‘W¡R

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/10/2017 12:36 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
>>
>> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 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);x‘W¡R

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

Hi,

On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
> 
> 
> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> 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?

Thanks
Kishon

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

Hi,

On Thursday 09 March 2017 08:35 PM, Niklas Cassel wrote:
> 
> 
> On 03/09/2017 03:48 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> 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?

Thanks
Kishon

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/09/2017 03:48 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> 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.

>
> That way we don't have to change every existing read/write.
> I'm assuming that synopsys won't add a dbi3 in the next version of the IP :p
>
>> Cc: Jingoo Han 
>> Cc: Richard Zhu 
>> Cc: Lucas Stach 
>> Cc: Murali Karicheri 
>> Cc: Thomas Petazzoni 
>> Cc: Niklas Cassel 
>> Cc: Jesper Nilsson 
>> Cc: Joao Pinto 
>> Cc: Zhou Wang 
>> Cc: Gabriele Paoloni 
>> Acked-by: Joao Pinto 
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
>>  drivers/pci/dwc/pci-exynos.c   |   10 +++--
>>  drivers/pci/dwc/pci-imx6.c |   62 +++---
>>  drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
>>  drivers/pci/dwc/pcie-armada8k.c|   39 +---
>>  drivers/pci/dwc/pcie-artpec6.c |7 +--
>>  drivers/pci/dwc/pcie-designware-host.c |   20 +
>>  drivers/pci/dwc/pcie-designware.c  |   76 
>> ++--
>>  drivers/pci/dwc/pcie-designware.h  |   10 +++--
>>  drivers/pci/dwc/pcie-hisi.c|   17 ---
>>  10 files changed, 152 insertions(+), 114 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index 07c45ec..3708bd6 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
>>  {
>>  struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>>  struct dw_pcie *pci = dra7xx->pci;
>> +void __iomem *base = pci->dbi_base;
>>  u32 val;
>>  
>>  /* clear MSE */
>> -val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
>> +val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>>  val &= ~PCI_COMMAND_MEMORY;
>> -dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
>> +dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>>  
>>  return 0;
>>  }
>> @@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
>>  {
>>  struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>>  struct dw_pcie *pci = dra7xx->pci;
>> +void __iomem *base = pci->dbi_base;
>>  u32 val;
>>  
>>  /* set MSE */
>> -val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
>> +val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>>  val |= PCI_COMMAND_MEMORY;
>> -dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
>> +dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>>  
>>  return 0;
>>  }
>> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
>> index 993b650..a0d40f7 100644
>> --- a/drivers/pci/dwc/pci-exynos.c
>> +++ b/drivers/pci/dwc/pci-exynos.c
>> @@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
>> exynos_pcie *ep)
>>  exynos_pcie_msi_init(ep);
>>  }
>>  
>> -static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
>> +static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg)
>>  {
>>  struct exynos_pcie *ep = to_exynos_pcie(pci);
>>  u32 val;
>>  
>>  exynos_pcie_sideband_dbi_r_mode(ep, true);
>> -val = readl(pci->dbi_base + reg);
>> +val = readl(base + reg);
>>  exynos_pcie_sideband_dbi_r_mode(ep, false);
>>  return val;
>>  }
>>  
>> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
>> +static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
>> +   u32 reg, u32 val)
>>  {
>>  struct exynos_pcie *ep = to_exynos_pcie(pci);
>>  
>>  

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/09/2017 03:48 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> 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.

>
> That way we don't have to change every existing read/write.
> I'm assuming that synopsys won't add a dbi3 in the next version of the IP :p
>
>> Cc: Jingoo Han 
>> Cc: Richard Zhu 
>> Cc: Lucas Stach 
>> Cc: Murali Karicheri 
>> Cc: Thomas Petazzoni 
>> Cc: Niklas Cassel 
>> Cc: Jesper Nilsson 
>> Cc: Joao Pinto 
>> Cc: Zhou Wang 
>> Cc: Gabriele Paoloni 
>> Acked-by: Joao Pinto 
>> Signed-off-by: Kishon Vijay Abraham I 
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
>>  drivers/pci/dwc/pci-exynos.c   |   10 +++--
>>  drivers/pci/dwc/pci-imx6.c |   62 +++---
>>  drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
>>  drivers/pci/dwc/pcie-armada8k.c|   39 +---
>>  drivers/pci/dwc/pcie-artpec6.c |7 +--
>>  drivers/pci/dwc/pcie-designware-host.c |   20 +
>>  drivers/pci/dwc/pcie-designware.c  |   76 
>> ++--
>>  drivers/pci/dwc/pcie-designware.h  |   10 +++--
>>  drivers/pci/dwc/pcie-hisi.c|   17 ---
>>  10 files changed, 152 insertions(+), 114 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index 07c45ec..3708bd6 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
>>  {
>>  struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>>  struct dw_pcie *pci = dra7xx->pci;
>> +void __iomem *base = pci->dbi_base;
>>  u32 val;
>>  
>>  /* clear MSE */
>> -val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
>> +val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>>  val &= ~PCI_COMMAND_MEMORY;
>> -dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
>> +dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>>  
>>  return 0;
>>  }
>> @@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
>>  {
>>  struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>>  struct dw_pcie *pci = dra7xx->pci;
>> +void __iomem *base = pci->dbi_base;
>>  u32 val;
>>  
>>  /* set MSE */
>> -val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
>> +val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>>  val |= PCI_COMMAND_MEMORY;
>> -dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
>> +dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>>  
>>  return 0;
>>  }
>> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
>> index 993b650..a0d40f7 100644
>> --- a/drivers/pci/dwc/pci-exynos.c
>> +++ b/drivers/pci/dwc/pci-exynos.c
>> @@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
>> exynos_pcie *ep)
>>  exynos_pcie_msi_init(ep);
>>  }
>>  
>> -static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
>> +static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
>> + u32 reg)
>>  {
>>  struct exynos_pcie *ep = to_exynos_pcie(pci);
>>  u32 val;
>>  
>>  exynos_pcie_sideband_dbi_r_mode(ep, true);
>> -val = readl(pci->dbi_base + reg);
>> +val = readl(base + reg);
>>  exynos_pcie_sideband_dbi_r_mode(ep, false);
>>  return val;
>>  }
>>  
>> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
>> +static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
>> +   u32 reg, u32 val)
>>  {
>>  struct exynos_pcie *ep = to_exynos_pcie(pci);
>>  
>>  exynos_pcie_sideband_dbi_w_mode(ep, true);
>> -writel(val, pci->dbi_base + reg);
>> +writel(val, base + reg);
>>  exynos_pcie_sideband_dbi_w_mode(ep, false);
>>  }
>>  
>> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
>> index 801e46c..85dd901 100644
>> --- 

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> 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)

That way we don't have to change every existing read/write.
I'm assuming that synopsys won't add a dbi3 in the next version of the IP :p

> Cc: Jingoo Han 
> Cc: Richard Zhu 
> Cc: Lucas Stach 
> Cc: Murali Karicheri 
> Cc: Thomas Petazzoni 
> Cc: Niklas Cassel 
> Cc: Jesper Nilsson 
> Cc: Joao Pinto 
> Cc: Zhou Wang 
> Cc: Gabriele Paoloni 
> Acked-by: Joao Pinto 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
>  drivers/pci/dwc/pci-exynos.c   |   10 +++--
>  drivers/pci/dwc/pci-imx6.c |   62 +++---
>  drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
>  drivers/pci/dwc/pcie-armada8k.c|   39 +---
>  drivers/pci/dwc/pcie-artpec6.c |7 +--
>  drivers/pci/dwc/pcie-designware-host.c |   20 +
>  drivers/pci/dwc/pcie-designware.c  |   76 
> ++--
>  drivers/pci/dwc/pcie-designware.h  |   10 +++--
>  drivers/pci/dwc/pcie-hisi.c|   17 ---
>  10 files changed, 152 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 07c45ec..3708bd6 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
>  {
>   struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>   struct dw_pcie *pci = dra7xx->pci;
> + void __iomem *base = pci->dbi_base;
>   u32 val;
>  
>   /* clear MSE */
> - val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
> + val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>   val &= ~PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
> + dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>  
>   return 0;
>  }
> @@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
>  {
>   struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>   struct dw_pcie *pci = dra7xx->pci;
> + void __iomem *base = pci->dbi_base;
>   u32 val;
>  
>   /* set MSE */
> - val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
> + val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>   val |= PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
> + dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>  
>   return 0;
>  }
> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
> index 993b650..a0d40f7 100644
> --- a/drivers/pci/dwc/pci-exynos.c
> +++ b/drivers/pci/dwc/pci-exynos.c
> @@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
> exynos_pcie *ep)
>   exynos_pcie_msi_init(ep);
>  }
>  
> -static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
> +static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
> +  u32 reg)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>   u32 val;
>  
>   exynos_pcie_sideband_dbi_r_mode(ep, true);
> - val = readl(pci->dbi_base + reg);
> + val = readl(base + reg);
>   exynos_pcie_sideband_dbi_r_mode(ep, false);
>   return val;
>  }
>  
> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
> +static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
> +u32 reg, u32 val)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>  
>   exynos_pcie_sideband_dbi_w_mode(ep, true);
> - writel(val, pci->dbi_base + reg);
> + writel(val, base + reg);
>   exynos_pcie_sideband_dbi_w_mode(ep, false);
>  }
>  
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 801e46c..85dd901 100644
> --- 

Re: [RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Niklas Cassel]

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> 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)

That way we don't have to change every existing read/write.
I'm assuming that synopsys won't add a dbi3 in the next version of the IP :p

> Cc: Jingoo Han 
> Cc: Richard Zhu 
> Cc: Lucas Stach 
> Cc: Murali Karicheri 
> Cc: Thomas Petazzoni 
> Cc: Niklas Cassel 
> Cc: Jesper Nilsson 
> Cc: Joao Pinto 
> Cc: Zhou Wang 
> Cc: Gabriele Paoloni 
> Acked-by: Joao Pinto 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
>  drivers/pci/dwc/pci-exynos.c   |   10 +++--
>  drivers/pci/dwc/pci-imx6.c |   62 +++---
>  drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
>  drivers/pci/dwc/pcie-armada8k.c|   39 +---
>  drivers/pci/dwc/pcie-artpec6.c |7 +--
>  drivers/pci/dwc/pcie-designware-host.c |   20 +
>  drivers/pci/dwc/pcie-designware.c  |   76 
> ++--
>  drivers/pci/dwc/pcie-designware.h  |   10 +++--
>  drivers/pci/dwc/pcie-hisi.c|   17 ---
>  10 files changed, 152 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 07c45ec..3708bd6 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
>  {
>   struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>   struct dw_pcie *pci = dra7xx->pci;
> + void __iomem *base = pci->dbi_base;
>   u32 val;
>  
>   /* clear MSE */
> - val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
> + val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>   val &= ~PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
> + dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>  
>   return 0;
>  }
> @@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
>  {
>   struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
>   struct dw_pcie *pci = dra7xx->pci;
> + void __iomem *base = pci->dbi_base;
>   u32 val;
>  
>   /* set MSE */
> - val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
> + val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
>   val |= PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
> + dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
>  
>   return 0;
>  }
> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
> index 993b650..a0d40f7 100644
> --- a/drivers/pci/dwc/pci-exynos.c
> +++ b/drivers/pci/dwc/pci-exynos.c
> @@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
> exynos_pcie *ep)
>   exynos_pcie_msi_init(ep);
>  }
>  
> -static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
> +static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
> +  u32 reg)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>   u32 val;
>  
>   exynos_pcie_sideband_dbi_r_mode(ep, true);
> - val = readl(pci->dbi_base + reg);
> + val = readl(base + reg);
>   exynos_pcie_sideband_dbi_r_mode(ep, false);
>   return val;
>  }
>  
> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
> +static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
> +u32 reg, u32 val)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>  
>   exynos_pcie_sideband_dbi_w_mode(ep, true);
> - writel(val, pci->dbi_base + reg);
> + writel(val, base + reg);
>   exynos_pcie_sideband_dbi_w_mode(ep, false);
>  }
>  
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 801e46c..85dd901 100644
> --- a/drivers/pci/dwc/pci-imx6.c
> +++ b/drivers/pci/dwc/pci-imx6.c
> @@ -98,12 +98,13 @@ struct imx6_pcie {
>  static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
>  {
>   struct dw_pcie *pci = imx6_pcie->pci;
> + void __iomem *base = pci->dbi_base;
>   u32 val;
>   u32 

[RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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.

Cc: Jingoo Han 
Cc: Richard Zhu 
Cc: Lucas Stach 
Cc: Murali Karicheri 
Cc: Thomas Petazzoni 
Cc: Niklas Cassel 
Cc: Jesper Nilsson 
Cc: Joao Pinto 
Cc: Zhou Wang 
Cc: Gabriele Paoloni 
Acked-by: Joao Pinto 
Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
 drivers/pci/dwc/pci-exynos.c   |   10 +++--
 drivers/pci/dwc/pci-imx6.c |   62 +++---
 drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
 drivers/pci/dwc/pcie-armada8k.c|   39 +---
 drivers/pci/dwc/pcie-artpec6.c |7 +--
 drivers/pci/dwc/pcie-designware-host.c |   20 +
 drivers/pci/dwc/pcie-designware.c  |   76 ++--
 drivers/pci/dwc/pcie-designware.h  |   10 +++--
 drivers/pci/dwc/pcie-hisi.c|   17 ---
 10 files changed, 152 insertions(+), 114 deletions(-)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 07c45ec..3708bd6 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* clear MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val &= ~PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
@@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* set MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val |= PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
index 993b650..a0d40f7 100644
--- a/drivers/pci/dwc/pci-exynos.c
+++ b/drivers/pci/dwc/pci-exynos.c
@@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
exynos_pcie *ep)
exynos_pcie_msi_init(ep);
 }
 
-static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
+static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
+u32 reg)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
u32 val;
 
exynos_pcie_sideband_dbi_r_mode(ep, true);
-   val = readl(pci->dbi_base + reg);
+   val = readl(base + reg);
exynos_pcie_sideband_dbi_r_mode(ep, false);
return val;
 }
 
-static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
+static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
+  u32 reg, u32 val)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
 
exynos_pcie_sideband_dbi_w_mode(ep, true);
-   writel(val, pci->dbi_base + reg);
+   writel(val, base + reg);
exynos_pcie_sideband_dbi_w_mode(ep, false);
 }
 
diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index 801e46c..85dd901 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -98,12 +98,13 @@ struct imx6_pcie {
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
u32 max_iterations = 10;
u32 wait_counter = 0;
 
do {
-   val = dw_pcie_readl_dbi(pci, PCIE_PHY_STAT);
+   val = dw_pcie_readl_dbi(pci, base, PCIE_PHY_STAT);
val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1;
wait_counter++;
 
@@ -119,21 +120,22 @@ static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, 
int exp_val)
 static int pcie_phy_wait_ack(struct imx6_pcie *imx6_pcie, int addr)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
int ret;
 
val = addr << PCIE_PHY_CTRL_DATA_LOC;
-   dw_pcie_writel_dbi(pci, 

[RESEND PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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.

Cc: Jingoo Han 
Cc: Richard Zhu 
Cc: Lucas Stach 
Cc: Murali Karicheri 
Cc: Thomas Petazzoni 
Cc: Niklas Cassel 
Cc: Jesper Nilsson 
Cc: Joao Pinto 
Cc: Zhou Wang 
Cc: Gabriele Paoloni 
Acked-by: Joao Pinto 
Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
 drivers/pci/dwc/pci-exynos.c   |   10 +++--
 drivers/pci/dwc/pci-imx6.c |   62 +++---
 drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
 drivers/pci/dwc/pcie-armada8k.c|   39 +---
 drivers/pci/dwc/pcie-artpec6.c |7 +--
 drivers/pci/dwc/pcie-designware-host.c |   20 +
 drivers/pci/dwc/pcie-designware.c  |   76 ++--
 drivers/pci/dwc/pcie-designware.h  |   10 +++--
 drivers/pci/dwc/pcie-hisi.c|   17 ---
 10 files changed, 152 insertions(+), 114 deletions(-)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 07c45ec..3708bd6 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* clear MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val &= ~PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
@@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* set MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val |= PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
index 993b650..a0d40f7 100644
--- a/drivers/pci/dwc/pci-exynos.c
+++ b/drivers/pci/dwc/pci-exynos.c
@@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
exynos_pcie *ep)
exynos_pcie_msi_init(ep);
 }
 
-static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
+static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
+u32 reg)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
u32 val;
 
exynos_pcie_sideband_dbi_r_mode(ep, true);
-   val = readl(pci->dbi_base + reg);
+   val = readl(base + reg);
exynos_pcie_sideband_dbi_r_mode(ep, false);
return val;
 }
 
-static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
+static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
+  u32 reg, u32 val)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
 
exynos_pcie_sideband_dbi_w_mode(ep, true);
-   writel(val, pci->dbi_base + reg);
+   writel(val, base + reg);
exynos_pcie_sideband_dbi_w_mode(ep, false);
 }
 
diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index 801e46c..85dd901 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -98,12 +98,13 @@ struct imx6_pcie {
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
u32 max_iterations = 10;
u32 wait_counter = 0;
 
do {
-   val = dw_pcie_readl_dbi(pci, PCIE_PHY_STAT);
+   val = dw_pcie_readl_dbi(pci, base, PCIE_PHY_STAT);
val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1;
wait_counter++;
 
@@ -119,21 +120,22 @@ static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, 
int exp_val)
 static int pcie_phy_wait_ack(struct imx6_pcie *imx6_pcie, int addr)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
int ret;
 
val = addr << PCIE_PHY_CTRL_DATA_LOC;
-   dw_pcie_writel_dbi(pci, PCIE_PHY_CTRL, val);
+   dw_pcie_writel_dbi(pci, base, PCIE_PHY_CTRL, val);
 
val |= (0x1 << PCIE_PHY_CTRL_CAP_ADR_LOC);
-   dw_pcie_writel_dbi(pci, PCIE_PHY_CTRL, val);
+   dw_pcie_writel_dbi(pci, base, PCIE_PHY_CTRL, val);
 
ret = pcie_phy_poll_ack(imx6_pcie, 

[PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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.

Cc: Jingoo Han 
Cc: Richard Zhu 
Cc: Lucas Stach 
Cc: Murali Karicheri 
Cc: Thomas Petazzoni 
Cc: Niklas Cassel 
Cc: Jesper Nilsson 
Cc: Joao Pinto 
Cc: Zhou Wang 
Cc: Gabriele Paoloni 
Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
 drivers/pci/dwc/pci-exynos.c   |   10 +++--
 drivers/pci/dwc/pci-imx6.c |   62 +++---
 drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
 drivers/pci/dwc/pcie-armada8k.c|   39 +---
 drivers/pci/dwc/pcie-artpec6.c |7 +--
 drivers/pci/dwc/pcie-designware-host.c |   20 +
 drivers/pci/dwc/pcie-designware.c  |   76 ++--
 drivers/pci/dwc/pcie-designware.h  |   10 +++--
 drivers/pci/dwc/pcie-hisi.c|   17 ---
 10 files changed, 152 insertions(+), 114 deletions(-)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 07c45ec..3708bd6 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* clear MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val &= ~PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
@@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* set MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val |= PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
index 993b650..a0d40f7 100644
--- a/drivers/pci/dwc/pci-exynos.c
+++ b/drivers/pci/dwc/pci-exynos.c
@@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
exynos_pcie *ep)
exynos_pcie_msi_init(ep);
 }
 
-static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
+static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
+u32 reg)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
u32 val;
 
exynos_pcie_sideband_dbi_r_mode(ep, true);
-   val = readl(pci->dbi_base + reg);
+   val = readl(base + reg);
exynos_pcie_sideband_dbi_r_mode(ep, false);
return val;
 }
 
-static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
+static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
+  u32 reg, u32 val)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
 
exynos_pcie_sideband_dbi_w_mode(ep, true);
-   writel(val, pci->dbi_base + reg);
+   writel(val, base + reg);
exynos_pcie_sideband_dbi_w_mode(ep, false);
 }
 
diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index 801e46c..85dd901 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -98,12 +98,13 @@ struct imx6_pcie {
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
u32 max_iterations = 10;
u32 wait_counter = 0;
 
do {
-   val = dw_pcie_readl_dbi(pci, PCIE_PHY_STAT);
+   val = dw_pcie_readl_dbi(pci, base, PCIE_PHY_STAT);
val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1;
wait_counter++;
 
@@ -119,21 +120,22 @@ static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, 
int exp_val)
 static int pcie_phy_wait_ack(struct imx6_pcie *imx6_pcie, int addr)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
int ret;
 
val = addr << PCIE_PHY_CTRL_DATA_LOC;
-   dw_pcie_writel_dbi(pci, PCIE_PHY_CTRL, val);
+   

[PATCH v3 4/7] PCI: dwc: all: Modify dbi accessors to take dbi_base as argument

[Kishon Vijay Abraham I]

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.

Cc: Jingoo Han 
Cc: Richard Zhu 
Cc: Lucas Stach 
Cc: Murali Karicheri 
Cc: Thomas Petazzoni 
Cc: Niklas Cassel 
Cc: Jesper Nilsson 
Cc: Joao Pinto 
Cc: Zhou Wang 
Cc: Gabriele Paoloni 
Signed-off-by: Kishon Vijay Abraham I 
---
 drivers/pci/dwc/pci-dra7xx.c   |   10 +++--
 drivers/pci/dwc/pci-exynos.c   |   10 +++--
 drivers/pci/dwc/pci-imx6.c |   62 +++---
 drivers/pci/dwc/pci-keystone-dw.c  |   15 ---
 drivers/pci/dwc/pcie-armada8k.c|   39 +---
 drivers/pci/dwc/pcie-artpec6.c |7 +--
 drivers/pci/dwc/pcie-designware-host.c |   20 +
 drivers/pci/dwc/pcie-designware.c  |   76 ++--
 drivers/pci/dwc/pcie-designware.h  |   10 +++--
 drivers/pci/dwc/pcie-hisi.c|   17 ---
 10 files changed, 152 insertions(+), 114 deletions(-)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 07c45ec..3708bd6 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -495,12 +495,13 @@ static int dra7xx_pcie_suspend(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* clear MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val &= ~PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
@@ -509,12 +510,13 @@ static int dra7xx_pcie_resume(struct device *dev)
 {
struct dra7xx_pcie *dra7xx = dev_get_drvdata(dev);
struct dw_pcie *pci = dra7xx->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
 
/* set MSE */
-   val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+   val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
val |= PCI_COMMAND_MEMORY;
-   dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
+   dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
 
return 0;
 }
diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
index 993b650..a0d40f7 100644
--- a/drivers/pci/dwc/pci-exynos.c
+++ b/drivers/pci/dwc/pci-exynos.c
@@ -521,23 +521,25 @@ static void exynos_pcie_enable_interrupts(struct 
exynos_pcie *ep)
exynos_pcie_msi_init(ep);
 }
 
-static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, u32 reg)
+static u32 exynos_pcie_readl_dbi(struct dw_pcie *pci, void __iomem *base,
+u32 reg)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
u32 val;
 
exynos_pcie_sideband_dbi_r_mode(ep, true);
-   val = readl(pci->dbi_base + reg);
+   val = readl(base + reg);
exynos_pcie_sideband_dbi_r_mode(ep, false);
return val;
 }
 
-static void exynos_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
+static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
+  u32 reg, u32 val)
 {
struct exynos_pcie *ep = to_exynos_pcie(pci);
 
exynos_pcie_sideband_dbi_w_mode(ep, true);
-   writel(val, pci->dbi_base + reg);
+   writel(val, base + reg);
exynos_pcie_sideband_dbi_w_mode(ep, false);
 }
 
diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index 801e46c..85dd901 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -98,12 +98,13 @@ struct imx6_pcie {
 static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, int exp_val)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
u32 max_iterations = 10;
u32 wait_counter = 0;
 
do {
-   val = dw_pcie_readl_dbi(pci, PCIE_PHY_STAT);
+   val = dw_pcie_readl_dbi(pci, base, PCIE_PHY_STAT);
val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1;
wait_counter++;
 
@@ -119,21 +120,22 @@ static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, 
int exp_val)
 static int pcie_phy_wait_ack(struct imx6_pcie *imx6_pcie, int addr)
 {
struct dw_pcie *pci = imx6_pcie->pci;
+   void __iomem *base = pci->dbi_base;
u32 val;
int ret;
 
val = addr << PCIE_PHY_CTRL_DATA_LOC;
-   dw_pcie_writel_dbi(pci, PCIE_PHY_CTRL, val);
+   dw_pcie_writel_dbi(pci, base, PCIE_PHY_CTRL, val);
 
val |= (0x1 << PCIE_PHY_CTRL_CAP_ADR_LOC);
-   dw_pcie_writel_dbi(pci, PCIE_PHY_CTRL, val);
+   dw_pcie_writel_dbi(pci, base, PCIE_PHY_CTRL, val);
 
ret = pcie_phy_poll_ack(imx6_pcie, 1);
if (ret)