Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/10/2017 02:04 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 10 March 2017 06:26 PM, Niklas Cassel wrote:
>> On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
 On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> Previously dbi accessors can be used to access data of size 4
> bytes. But there might be situations (like accessing
> MSI_MESSAGE_CONTROL in order to set/get the number of required
> MSI interrupts in EP mode) where dbi accessors must
> be used to access data of size 2. This is in preparation for
> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
 a read/write of length == 32?

 How about adding some defines to pcie-designware.h:

 #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, 
 base, reg, 0x4, val)
 #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
 0x4)

 That way we don't have to change every existing read/write.



 Is there a reason why we can't just do:

 vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
>>> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
>>> can
>>> do a readl that crosses the alignment boundary in all platforms. The other
>>> option is to readl from "MSI capability offset + 0" and extract the last 16
>>> bits. I felt this is more clear since we are interested only in the
>>> MSI_MESSAGE_CONTROL.
>>>
 
 dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);

 Or are we going to be doing read/writes of length != 32 so often that
 you think that it's cleaner to have this abstraction?
>>> it's used mainly for accessing configuration space header fields. Even the 
>>> pci
>>> core uses *pci_read_config_word* for accessing such fields.
>> I see. Adding an extra size argument is a good thing then,
>> since it's consistent with the pci generic code.
>>
>> However, I still think that having defines for writel/readl is a
>> good thing :)
> sure, having defines is fine. How about something like below (readl, readw: to
> differentiate 4byte and 2 byte access?)
>
> #define dw_pcie_readl_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
> 0x4)
> #define dw_pcie_readw_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
> 0x2)

Looks good to me.
But if we add readw, we might as well add readb :P

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/10/2017 02:04 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 10 March 2017 06:26 PM, Niklas Cassel wrote:
>> On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
 On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> Previously dbi accessors can be used to access data of size 4
> bytes. But there might be situations (like accessing
> MSI_MESSAGE_CONTROL in order to set/get the number of required
> MSI interrupts in EP mode) where dbi accessors must
> be used to access data of size 2. This is in preparation for
> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
 a read/write of length == 32?

 How about adding some defines to pcie-designware.h:

 #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, 
 base, reg, 0x4, val)
 #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
 0x4)

 That way we don't have to change every existing read/write.



 Is there a reason why we can't just do:

 vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
>>> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
>>> can
>>> do a readl that crosses the alignment boundary in all platforms. The other
>>> option is to readl from "MSI capability offset + 0" and extract the last 16
>>> bits. I felt this is more clear since we are interested only in the
>>> MSI_MESSAGE_CONTROL.
>>>
 
 dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);

 Or are we going to be doing read/writes of length != 32 so often that
 you think that it's cleaner to have this abstraction?
>>> it's used mainly for accessing configuration space header fields. Even the 
>>> pci
>>> core uses *pci_read_config_word* for accessing such fields.
>> I see. Adding an extra size argument is a good thing then,
>> since it's consistent with the pci generic code.
>>
>> However, I still think that having defines for writel/readl is a
>> good thing :)
> sure, having defines is fine. How about something like below (readl, readw: to
> differentiate 4byte and 2 byte access?)
>
> #define dw_pcie_readl_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
> 0x4)
> #define dw_pcie_readw_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
> 0x2)

Looks good to me.
But if we add readw, we might as well add readb :P

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Kishon Vijay Abraham I]

Hi,

On Friday 10 March 2017 06:26 PM, Niklas Cassel wrote:
> On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 Previously dbi accessors can be used to access data of size 4
 bytes. But there might be situations (like accessing
 MSI_MESSAGE_CONTROL in order to set/get the number of required
 MSI interrupts in EP mode) where dbi accessors must
 be used to access data of size 2. This is in preparation for
 adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
>>> a read/write of length == 32?
>>>
>>> How about adding some defines to pcie-designware.h:
>>>
>>> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, 
>>> base, reg, 0x4, val)
>>> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
>>> 0x4)
>>>
>>> That way we don't have to change every existing read/write.
>>>
>>>
>>>
>>> Is there a reason why we can't just do:
>>>
>>> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
>> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
>> can
>> do a readl that crosses the alignment boundary in all platforms. The other
>> option is to readl from "MSI capability offset + 0" and extract the last 16
>> bits. I felt this is more clear since we are interested only in the
>> MSI_MESSAGE_CONTROL.
>>
>>> 
>>> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
>>>
>>> Or are we going to be doing read/writes of length != 32 so often that
>>> you think that it's cleaner to have this abstraction?
>> it's used mainly for accessing configuration space header fields. Even the 
>> pci
>> core uses *pci_read_config_word* for accessing such fields.
> 
> I see. Adding an extra size argument is a good thing then,
> since it's consistent with the pci generic code.
> 
> However, I still think that having defines for writel/readl is a
> good thing :)

sure, having defines is fine. How about something like below (readl, readw: to
differentiate 4byte and 2 byte access?)

#define dw_pcie_readl_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
0x4)
#define dw_pcie_readw_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
0x2)

Thanks
Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Kishon Vijay Abraham I]

Hi,

On Friday 10 March 2017 06:26 PM, Niklas Cassel wrote:
> On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
>>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
 Previously dbi accessors can be used to access data of size 4
 bytes. But there might be situations (like accessing
 MSI_MESSAGE_CONTROL in order to set/get the number of required
 MSI interrupts in EP mode) where dbi accessors must
 be used to access data of size 2. This is in preparation for
 adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
>>> a read/write of length == 32?
>>>
>>> How about adding some defines to pcie-designware.h:
>>>
>>> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, 
>>> base, reg, 0x4, val)
>>> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
>>> 0x4)
>>>
>>> That way we don't have to change every existing read/write.
>>>
>>>
>>>
>>> Is there a reason why we can't just do:
>>>
>>> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
>> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
>> can
>> do a readl that crosses the alignment boundary in all platforms. The other
>> option is to readl from "MSI capability offset + 0" and extract the last 16
>> bits. I felt this is more clear since we are interested only in the
>> MSI_MESSAGE_CONTROL.
>>
>>> 
>>> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
>>>
>>> Or are we going to be doing read/writes of length != 32 so often that
>>> you think that it's cleaner to have this abstraction?
>> it's used mainly for accessing configuration space header fields. Even the 
>> pci
>> core uses *pci_read_config_word* for accessing such fields.
> 
> I see. Adding an extra size argument is a good thing then,
> since it's consistent with the pci generic code.
> 
> However, I still think that having defines for writel/readl is a
> good thing :)

sure, having defines is fine. How about something like below (readl, readw: to
differentiate 4byte and 2 byte access?)

#define dw_pcie_readl_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
0x4)
#define dw_pcie_readw_dbi(pci, base, reg) __dw_pcie_read_dbi(pci, base, reg, 
0x2)

Thanks
Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> Previously dbi accessors can be used to access data of size 4
>>> bytes. But there might be situations (like accessing
>>> MSI_MESSAGE_CONTROL in order to set/get the number of required
>>> MSI interrupts in EP mode) where dbi accessors must
>>> be used to access data of size 2. This is in preparation for
>>> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
>> a read/write of length == 32?
>>
>> How about adding some defines to pcie-designware.h:
>>
>> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
>> reg, 0x4, val)
>> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
>> 0x4)
>>
>> That way we don't have to change every existing read/write.
>>
>>
>>
>> Is there a reason why we can't just do:
>>
>> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
> can
> do a readl that crosses the alignment boundary in all platforms. The other
> option is to readl from "MSI capability offset + 0" and extract the last 16
> bits. I felt this is more clear since we are interested only in the
> MSI_MESSAGE_CONTROL.
>
>> 
>> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
>>
>> Or are we going to be doing read/writes of length != 32 so often that
>> you think that it's cleaner to have this abstraction?
> it's used mainly for accessing configuration space header fields. Even the pci
> core uses *pci_read_config_word* for accessing such fields.

I see. Adding an extra size argument is a good thing then,
since it's consistent with the pci generic code.

However, I still think that having defines for writel/readl is a
good thing :)


>
> Thanks
> Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/10/2017 01:04 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
>> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>>> Previously dbi accessors can be used to access data of size 4
>>> bytes. But there might be situations (like accessing
>>> MSI_MESSAGE_CONTROL in order to set/get the number of required
>>> MSI interrupts in EP mode) where dbi accessors must
>>> be used to access data of size 2. This is in preparation for
>>> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
>> a read/write of length == 32?
>>
>> How about adding some defines to pcie-designware.h:
>>
>> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
>> reg, 0x4, val)
>> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
>> 0x4)
>>
>> That way we don't have to change every existing read/write.
>>
>>
>>
>> Is there a reason why we can't just do:
>>
>> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);
> MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we 
> can
> do a readl that crosses the alignment boundary in all platforms. The other
> option is to readl from "MSI capability offset + 0" and extract the last 16
> bits. I felt this is more clear since we are interested only in the
> MSI_MESSAGE_CONTROL.
>
>> 
>> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
>>
>> Or are we going to be doing read/writes of length != 32 so often that
>> you think that it's cleaner to have this abstraction?
> it's used mainly for accessing configuration space header fields. Even the pci
> core uses *pci_read_config_word* for accessing such fields.

I see. Adding an extra size argument is a good thing then,
since it's consistent with the pci generic code.

However, I still think that having defines for writel/readl is a
good thing :)


>
> Thanks
> Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Kishon Vijay Abraham I]

Hi,

On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> Previously dbi accessors can be used to access data of size 4
>> bytes. But there might be situations (like accessing
>> MSI_MESSAGE_CONTROL in order to set/get the number of required
>> MSI interrupts in EP mode) where dbi accessors must
>> be used to access data of size 2. This is in preparation for
>> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
> a read/write of length == 32?
> 
> How about adding some defines to pcie-designware.h:
> 
> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
> reg, 0x4, val)
> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
> 0x4)
> 
> That way we don't have to change every existing read/write.
> 
> 
> 
> Is there a reason why we can't just do:
> 
> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);

MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we can
do a readl that crosses the alignment boundary in all platforms. The other
option is to readl from "MSI capability offset + 0" and extract the last 16
bits. I felt this is more clear since we are interested only in the
MSI_MESSAGE_CONTROL.

> 
> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
> 
> Or are we going to be doing read/writes of length != 32 so often that
> you think that it's cleaner to have this abstraction?

it's used mainly for accessing configuration space header fields. Even the pci
core uses *pci_read_config_word* for accessing such fields.

Thanks
Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Kishon Vijay Abraham I]

Hi,

On Thursday 09 March 2017 08:18 PM, Niklas Cassel wrote:
> On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
>> Previously dbi accessors can be used to access data of size 4
>> bytes. But there might be situations (like accessing
>> MSI_MESSAGE_CONTROL in order to set/get the number of required
>> MSI interrupts in EP mode) where dbi accessors must
>> be used to access data of size 2. This is in preparation for
>> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
> a read/write of length == 32?
> 
> How about adding some defines to pcie-designware.h:
> 
> #define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
> reg, 0x4, val)
> #define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 
> 0x4)
> 
> That way we don't have to change every existing read/write.
> 
> 
> 
> Is there a reason why we can't just do:
> 
> vial = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);

MSI_MESSAGE_CONTROL is 0x52 (MSI capability offset + 2). I'm not sure if we can
do a readl that crosses the alignment boundary in all platforms. The other
option is to readl from "MSI capability offset + 0" and extract the last 16
bits. I felt this is more clear since we are interested only in the
MSI_MESSAGE_CONTROL.

> 
> dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);
> 
> Or are we going to be doing read/writes of length != 32 so often that
> you think that it's cleaner to have this abstraction?

it's used mainly for accessing configuration space header fields. Even the pci
core uses *pci_read_config_word* for accessing such fields.

Thanks
Kishon

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> Previously dbi accessors can be used to access data of size 4
> bytes. But there might be situations (like accessing
> MSI_MESSAGE_CONTROL in order to set/get the number of required
> MSI interrupts in EP mode) where dbi accessors must
> be used to access data of size 2. This is in preparation for
> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
a read/write of length == 32?

How about adding some defines to pcie-designware.h:

#define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
reg, 0x4, val)
#define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 0x4)

That way we don't have to change every existing read/write.



Is there a reason why we can't just do:

val = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);

dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);

Or are we going to be doing read/writes of length != 32 so often that
you think that it's cleaner to have this abstraction?

>
> 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   |8 ++--
>  drivers/pci/dwc/pci-exynos.c   |   16 +++
>  drivers/pci/dwc/pci-imx6.c |   54 +++---
>  drivers/pci/dwc/pci-keystone-dw.c  |   13 +++---
>  drivers/pci/dwc/pcie-armada8k.c|   38 
>  drivers/pci/dwc/pcie-artpec6.c |6 +--
>  drivers/pci/dwc/pcie-designware-host.c |   18 
>  drivers/pci/dwc/pcie-designware.c  |   77 
> +++-
>  drivers/pci/dwc/pcie-designware.h  |   14 +++---
>  drivers/pci/dwc/pcie-hisi.c|   14 +++---
>  10 files changed, 138 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 3708bd6..c6fef0a 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -499,9 +499,9 @@ static int dra7xx_pcie_suspend(struct device *dev)
>   u32 val;
>  
>   /* clear MSE */
> - val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
> + val = dw_pcie_read_dbi(pci, base, PCI_COMMAND, 0x4);
>   val &= ~PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
> + dw_pcie_write_dbi(pci, base, PCI_COMMAND, 0x4, val);
>  
>   return 0;
>  }
> @@ -514,9 +514,9 @@ static int dra7xx_pcie_resume(struct device *dev)
>   u32 val;
>  
>   /* set MSE */
> - val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
> + val = dw_pcie_read_dbi(pci, base, PCI_COMMAND, 0x4);
>   val |= PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
> + dw_pcie_write_dbi(pci, base, PCI_COMMAND, 0x4, val);
>  
>   return 0;
>  }
> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
> index a0d40f7..37d6d2b 100644
> --- a/drivers/pci/dwc/pci-exynos.c
> +++ b/drivers/pci/dwc/pci-exynos.c
> @@ -521,25 +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, void __iomem *base,
> -  u32 reg)
> +static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>   u32 val;
>  
>   exynos_pcie_sideband_dbi_r_mode(ep, true);
> - val = readl(base + reg);
> + dw_pcie_read(base + reg, size, );
>   exynos_pcie_sideband_dbi_r_mode(ep, false);
>   return val;
>  }
>  
> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
> -u32 reg, u32 val)
> +static void exynos_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +   u32 reg, size_t size, u32 val)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>  
>   exynos_pcie_sideband_dbi_w_mode(ep, true);
> - writel(val, base + reg);
> + dw_pcie_write(base + reg, size, val);
>   exynos_pcie_sideband_dbi_w_mode(ep, false);
>  }
>  
> @@ -646,8 +646,8 @@ static int __init exynos_add_pcie_port(struct exynos_pcie 
> *ep,
>  }
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> - .readl_dbi = 

Re: [RESEND PATCH v3 5/7] PCI: dwc: all: Modify dbi accessors to access data of 4/2/1 bytes

[Niklas Cassel]

On 03/09/2017 07:39 AM, Kishon Vijay Abraham I wrote:
> Previously dbi accessors can be used to access data of size 4
> bytes. But there might be situations (like accessing
> MSI_MESSAGE_CONTROL in order to set/get the number of required
> MSI interrupts in EP mode) where dbi accessors must
> be used to access data of size 2. This is in preparation for
> adding endpoint mode support to designware 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 of length != 32 be quite uncommon compared to
a read/write of length == 32?

How about adding some defines to pcie-designware.h:

#define dw_pcie_writel_dbi(pci, base, reg, val) dw_pcie_write_dbi(pci, base, 
reg, 0x4, val)
#define dw_pcie_readl_dbi(pci, base, reg) dw_pcie_read_dbi(pci, base, reg, 0x4)

That way we don't have to change every existing read/write.



Is there a reason why we can't just do:

val = dw_pcie_readl_dbi(pci, base, MSI_MESSAGE_CONTROL);

dw_pcie_writel_dbi(pci, base, MSI_MESSAGE_CONTROL, val);

Or are we going to be doing read/writes of length != 32 so often that
you think that it's cleaner to have this abstraction?

>
> 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   |8 ++--
>  drivers/pci/dwc/pci-exynos.c   |   16 +++
>  drivers/pci/dwc/pci-imx6.c |   54 +++---
>  drivers/pci/dwc/pci-keystone-dw.c  |   13 +++---
>  drivers/pci/dwc/pcie-armada8k.c|   38 
>  drivers/pci/dwc/pcie-artpec6.c |6 +--
>  drivers/pci/dwc/pcie-designware-host.c |   18 
>  drivers/pci/dwc/pcie-designware.c  |   77 
> +++-
>  drivers/pci/dwc/pcie-designware.h  |   14 +++---
>  drivers/pci/dwc/pcie-hisi.c|   14 +++---
>  10 files changed, 138 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 3708bd6..c6fef0a 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -499,9 +499,9 @@ static int dra7xx_pcie_suspend(struct device *dev)
>   u32 val;
>  
>   /* clear MSE */
> - val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
> + val = dw_pcie_read_dbi(pci, base, PCI_COMMAND, 0x4);
>   val &= ~PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
> + dw_pcie_write_dbi(pci, base, PCI_COMMAND, 0x4, val);
>  
>   return 0;
>  }
> @@ -514,9 +514,9 @@ static int dra7xx_pcie_resume(struct device *dev)
>   u32 val;
>  
>   /* set MSE */
> - val = dw_pcie_readl_dbi(pci, base, PCI_COMMAND);
> + val = dw_pcie_read_dbi(pci, base, PCI_COMMAND, 0x4);
>   val |= PCI_COMMAND_MEMORY;
> - dw_pcie_writel_dbi(pci, base, PCI_COMMAND, val);
> + dw_pcie_write_dbi(pci, base, PCI_COMMAND, 0x4, val);
>  
>   return 0;
>  }
> diff --git a/drivers/pci/dwc/pci-exynos.c b/drivers/pci/dwc/pci-exynos.c
> index a0d40f7..37d6d2b 100644
> --- a/drivers/pci/dwc/pci-exynos.c
> +++ b/drivers/pci/dwc/pci-exynos.c
> @@ -521,25 +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, void __iomem *base,
> -  u32 reg)
> +static u32 exynos_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> + u32 reg, size_t size)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>   u32 val;
>  
>   exynos_pcie_sideband_dbi_r_mode(ep, true);
> - val = readl(base + reg);
> + dw_pcie_read(base + reg, size, );
>   exynos_pcie_sideband_dbi_r_mode(ep, false);
>   return val;
>  }
>  
> -static void exynos_pcie_writel_dbi(struct dw_pcie *pci, void __iomem *base,
> -u32 reg, u32 val)
> +static void exynos_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +   u32 reg, size_t size, u32 val)
>  {
>   struct exynos_pcie *ep = to_exynos_pcie(pci);
>  
>   exynos_pcie_sideband_dbi_w_mode(ep, true);
> - writel(val, base + reg);
> + dw_pcie_write(base + reg, size, val);
>   exynos_pcie_sideband_dbi_w_mode(ep, false);
>  }
>  
> @@ -646,8 +646,8 @@ static int __init exynos_add_pcie_port(struct exynos_pcie 
> *ep,
>  }
>  
>  static const struct dw_pcie_ops dw_pcie_ops = {
> - .readl_dbi = exynos_pcie_readl_dbi,
> - .writel_dbi = exynos_pcie_writel_dbi,
> + .read_dbi = exynos_pcie_read_dbi,
> + .write_dbi = exynos_pcie_write_dbi,
>   .link_up = exynos_pcie_link_up,
>  };
>  
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index 85dd901..e58ca7a 100644
> ---