[PATCH 0/8] Designware EP support and code clenan up

[PATCH 0/8] Designware EP support and code clenan up

[Gustavo Pimentel]

The patch set was made against the Bjorn's next branch.

Adds support Designware EP support.

Increases the maximum number of interrupts allowed for Designware IP
controller.

Does a code cleanup on Designware driver:
 - Replaces magic numbers without a easy meaning by a well known define
   that helps the human compreension.
 - Replaces a division by 2 by a simple right shift rotation of 1 bit.
 - Fixes all first letter characters on comments and debug messages to
   upper case to maintain coherency.

Gustavo Pimentel (8):
  bindings: PCI: designware: Example update
  PCI: dwc: designware: Add support for endpoint mode
  bindings: PCI: designware: Add support for the EP in designware driver
  misc: pci_endpoint_test: Add designware EP entry
  PCI: dwc: designware: Define maximum number of vectors
  PCI: dwc: Replace lower into upper case characters
  PCI: dwc: Small computation improvement
  PCI: dwc: Replace magic number by defines

 .../devicetree/bindings/pci/designware-pcie.txt    |  25 +++-
 drivers/misc/pci_endpoint_test.c                   |   1 +
 drivers/pci/dwc/Kconfig                            |  45 ++++--
 drivers/pci/dwc/pcie-designware-ep.c               |  16 +-
 drivers/pci/dwc/pcie-designware-host.c             |  77 +++++-----
 drivers/pci/dwc/pcie-designware-plat.c             | 163 +++++++++++++++++++--
 drivers/pci/dwc/pcie-designware.c                  |  22 +--
 drivers/pci/dwc/pcie-designware.h                  |   1 +
 drivers/pci/endpoint/functions/pci-epf-test.c      |   5 +
 9 files changed, 276 insertions(+), 79 deletions(-)

-- 
2.7.4

[PATCH 1/8] bindings: PCI: designware: Example update

[Gustavo Pimentel]

Changes the IP registers size to accommodate the ATU unroll space.

Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.

Replaces the pcie base address example by a real pcie base address in use.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 1da7ade..6300762 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -1,7 +1,8 @@
 * Synopsys DesignWare PCIe interface
 
 Required properties:
-- compatible: should contain "snps,dw-pcie" to identify the core.
+- compatible:
+	"snps,dw-pcie" for RC mode;
 - reg: Should contain the configuration address space.
 - reg-names: Must be "config" for the PCIe configuration space.
     (The old way of getting the configuration address space from "ranges"
@@ -41,11 +42,11 @@ EP mode:
 
 Example configuration:
 
-	pcie: pcie@dffff000 {
+	pcie: pcie@dfc00000 {
 		compatible = "snps,dw-pcie";
-		reg = <0xdffff000 0x1000>, /* Controller registers */
-		      <0xd0000000 0x2000>; /* PCI config space */
-		reg-names = "ctrlreg", "config";
+		reg = <0xdfc00000 0x302000>, /* IP registers */
+		      <0xd0000000 0x002000>; /* Configuration space */
+		reg-names = "dbi", "config";
 		#address-cells = <3>;
 		#size-cells = <2>;
 		device_type = "pci";
@@ -54,5 +55,4 @@ Example configuration:
 		interrupts = <25>, <24>;
 		#interrupt-cells = <1>;
 		num-lanes = <1>;
-		num-viewport = <3>;
 	};
-- 
2.7.4

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Changes the IP registers size to accommodate the ATU unroll space.
> 
> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
> 
> Replaces the pcie base address example by a real pcie base address in use.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 1da7ade..6300762 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -1,7 +1,8 @@
>  * Synopsys DesignWare PCIe interface
>  
>  Required properties:
> -- compatible: should contain "snps,dw-pcie" to identify the core.
> +- compatible:
> +	"snps,dw-pcie" for RC mode;

I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
identify the pcie core?
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -41,11 +42,11 @@ EP mode:
>  
>  Example configuration:
>  
> -	pcie: pcie@dffff000 {
> +	pcie: pcie@dfc00000 {
>  		compatible = "snps,dw-pcie";
> -		reg = <0xdffff000 0x1000>, /* Controller registers */
> -		      <0xd0000000 0x2000>; /* PCI config space */
> -		reg-names = "ctrlreg", "config";
> +		reg = <0xdfc00000 0x302000>, /* IP registers */

which version of synopsys IP is this. I think the ideal thing to do here is to
have a separate register space for iATU.

Thanks
Kishon

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Changes the IP registers size to accommodate the ATU unroll space.
> 
> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
> 
> Replaces the pcie base address example by a real pcie base address in use.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 1da7ade..6300762 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -1,7 +1,8 @@
>  * Synopsys DesignWare PCIe interface
>  
>  Required properties:
> -- compatible: should contain "snps,dw-pcie" to identify the core.
> +- compatible:
> +	"snps,dw-pcie" for RC mode;

I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
identify the pcie core?
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -41,11 +42,11 @@ EP mode:
>  
>  Example configuration:
>  
> -	pcie: pcie@dffff000 {
> +	pcie: pcie@dfc00000 {
>  		compatible = "snps,dw-pcie";
> -		reg = <0xdffff000 0x1000>, /* Controller registers */
> -		      <0xd0000000 0x2000>; /* PCI config space */
> -		reg-names = "ctrlreg", "config";
> +		reg = <0xdfc00000 0x302000>, /* IP registers */

which version of synopsys IP is this. I think the ideal thing to do here is to
have a separate register space for iATU.

Thanks
Kishon

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Gustavo Pimentel]

Hi Kishon,

On 02/04/2018 06:23, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>> Changes the IP registers size to accommodate the ATU unroll space.
>>
>> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
>>
>> Replaces the pcie base address example by a real pcie base address in use.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index 1da7ade..6300762 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -1,7 +1,8 @@
>>  * Synopsys DesignWare PCIe interface
>>  
>>  Required properties:
>> -- compatible: should contain "snps,dw-pcie" to identify the core.
>> +- compatible:
>> +	"snps,dw-pcie" for RC mode;
> 
> I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
> identify the pcie core?

I guess so. What you suggest? I was just folling the same guideline present
here: https://lkml.org/lkml/2017/11/3/310

>>  - reg: Should contain the configuration address space.
>>  - reg-names: Must be "config" for the PCIe configuration space.
>>      (The old way of getting the configuration address space from "ranges"
>> @@ -41,11 +42,11 @@ EP mode:
>>  
>>  Example configuration:
>>  
>> -	pcie: pcie@dffff000 {
>> +	pcie: pcie@dfc00000 {
>>  		compatible = "snps,dw-pcie";
>> -		reg = <0xdffff000 0x1000>, /* Controller registers */
>> -		      <0xd0000000 0x2000>; /* PCI config space */
>> -		reg-names = "ctrlreg", "config";
>> +		reg = <0xdfc00000 0x302000>, /* IP registers */
> 
> which version of synopsys IP is this. I think the ideal thing to do here is to
> have a separate register space for iATU.

I also agree with that. The unroll iATU was introduced on Synopsys IP version
4.80 and the kernel patch was release on 2016-08-10
https://patchwork.ozlabs.org/patch/657796/
However a separate register space for iATU implies some extra code do handle it
(and of course some tests) that don't fit into this patch series, in my point of
view. Can this task enter in the backlog in order to be done in another patch
series?

> 
> Thanks
> Kishon
> 

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Kishon Vijay Abraham I]

On Tuesday 03 April 2018 04:03 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 02/04/2018 06:23, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>> Changes the IP registers size to accommodate the ATU unroll space.
>>>
>>> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
>>>
>>> Replaces the pcie base address example by a real pcie base address in use.
>>>
>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> index 1da7ade..6300762 100644
>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> @@ -1,7 +1,8 @@
>>>  * Synopsys DesignWare PCIe interface
>>>  
>>>  Required properties:
>>> -- compatible: should contain "snps,dw-pcie" to identify the core.
>>> +- compatible:
>>> +	"snps,dw-pcie" for RC mode;
>>
>> I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
>> identify the pcie core?
> 
> I guess so. What you suggest? I was just folling the same guideline present
> here: https://lkml.org/lkml/2017/11/3/310

Okay, I think you should have
"snps,dw-pcie-rc", "snps,dw-pcie" for RC mode;

and in the later patch
"snps,dw-pcie-ep", "snps,dw-pcie" for EP mode;

> 
>>>  - reg: Should contain the configuration address space.
>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>      (The old way of getting the configuration address space from "ranges"
>>> @@ -41,11 +42,11 @@ EP mode:
>>>  
>>>  Example configuration:
>>>  
>>> -	pcie: pcie@dffff000 {
>>> +	pcie: pcie@dfc00000 {
>>>  		compatible = "snps,dw-pcie";
>>> -		reg = <0xdffff000 0x1000>, /* Controller registers */
>>> -		      <0xd0000000 0x2000>; /* PCI config space */
>>> -		reg-names = "ctrlreg", "config";
>>> +		reg = <0xdfc00000 0x302000>, /* IP registers */
>>
>> which version of synopsys IP is this. I think the ideal thing to do here is to
>> have a separate register space for iATU.
> 
> I also agree with that. The unroll iATU was introduced on Synopsys IP version
> 4.80 and the kernel patch was release on 2016-08-10
> https://patchwork.ozlabs.org/patch/657796/
> However a separate register space for iATU implies some extra code do handle it
> (and of course some tests) that don't fit into this patch series, in my point of
> view. Can this task enter in the backlog in order to be done in another patch
> series?

Yes sure. I think we should also make sure existing binding doesn't break.

Thanks
Kishon

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 April 2018 04:22 PM, Kishon Vijay Abraham I wrote:
> 
> 
> On Tuesday 03 April 2018 04:03 PM, Gustavo Pimentel wrote:
>> Hi Kishon,
>>
>> On 02/04/2018 06:23, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>>> Changes the IP registers size to accommodate the ATU unroll space.
>>>>
>>>> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
>>>>
>>>> Replaces the pcie base address example by a real pcie base address in use.
>>>>
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> index 1da7ade..6300762 100644
>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> @@ -1,7 +1,8 @@
>>>>  * Synopsys DesignWare PCIe interface
>>>>  
>>>>  Required properties:
>>>> -- compatible: should contain "snps,dw-pcie" to identify the core.
>>>> +- compatible:
>>>> +	"snps,dw-pcie" for RC mode;
>>>
>>> I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
>>> identify the pcie core?
>>
>> I guess so. What you suggest? I was just folling the same guideline present
>> here: https://lkml.org/lkml/2017/11/3/310
> 
> Okay, I think you should have
> "snps,dw-pcie-rc", "snps,dw-pcie" for RC mode;
> 
> and in the later patch
> "snps,dw-pcie-ep", "snps,dw-pcie" for EP mode;
> 
>>
>>>>  - reg: Should contain the configuration address space.
>>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>>      (The old way of getting the configuration address space from "ranges"
>>>> @@ -41,11 +42,11 @@ EP mode:
>>>>  
>>>>  Example configuration:
>>>>  
>>>> -	pcie: pcie@dffff000 {
>>>> +	pcie: pcie@dfc00000 {
>>>>  		compatible = "snps,dw-pcie";
>>>> -		reg = <0xdffff000 0x1000>, /* Controller registers */
>>>> -		      <0xd0000000 0x2000>; /* PCI config space */
>>>> -		reg-names = "ctrlreg", "config";
>>>> +		reg = <0xdfc00000 0x302000>, /* IP registers */
>>>
>>> which version of synopsys IP is this. I think the ideal thing to do here is to
>>> have a separate register space for iATU.
>>
>> I also agree with that. The unroll iATU was introduced on Synopsys IP version
>> 4.80 and the kernel patch was release on 2016-08-10
>> https://patchwork.ozlabs.org/patch/657796/
>> However a separate register space for iATU implies some extra code do handle it
>> (and of course some tests) that don't fit into this patch series, in my point of
>> view. Can this task enter in the backlog in order to be done in another patch
>> series?
> 
> Yes sure. I think we should also make sure existing binding doesn't break.

And IMO we should have a new compatible "snps,dw-pcie-4.80" in order to enable
iATU unroll.

Thanks
Kishon

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Gustavo Pimentel]

Hi Kishon,

On 03/04/2018 11:53, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 03 April 2018 04:22 PM, Kishon Vijay Abraham I wrote:
>>
>>
>> On Tuesday 03 April 2018 04:03 PM, Gustavo Pimentel wrote:
>>> Hi Kishon,
>>>
>>> On 02/04/2018 06:23, Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>>>> Changes the IP registers size to accommodate the ATU unroll space.
>>>>>
>>>>> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
>>>>>
>>>>> Replaces the pcie base address example by a real pcie base address in use.
>>>>>
>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> index 1da7ade..6300762 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> @@ -1,7 +1,8 @@
>>>>>  * Synopsys DesignWare PCIe interface
>>>>>  
>>>>>  Required properties:
>>>>> -- compatible: should contain "snps,dw-pcie" to identify the core.
>>>>> +- compatible:
>>>>> +	"snps,dw-pcie" for RC mode;
>>>>
>>>> I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
>>>> identify the pcie core?
>>>
>>> I guess so. What you suggest? I was just folling the same guideline present
>>> here: https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_11_3_310&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=G1MqB_DY6ZwWtvS60L9PZMHnMe6rClMnrakAyQT_hDc&s=BhQYkrcp6y3QkD23qn1I6lU882BDUfLiXjBVWQ91cmg&e=
>>
>> Okay, I think you should have
>> "snps,dw-pcie-rc", "snps,dw-pcie" for RC mode;
>>
>> and in the later patch
>> "snps,dw-pcie-ep", "snps,dw-pcie" for EP mode;
>>

Ok, I'll change it.

>>>
>>>>>  - reg: Should contain the configuration address space.
>>>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>>>      (The old way of getting the configuration address space from "ranges"
>>>>> @@ -41,11 +42,11 @@ EP mode:
>>>>>  
>>>>>  Example configuration:
>>>>>  
>>>>> -	pcie: pcie@dffff000 {
>>>>> +	pcie: pcie@dfc00000 {
>>>>>  		compatible = "snps,dw-pcie";
>>>>> -		reg = <0xdffff000 0x1000>, /* Controller registers */
>>>>> -		      <0xd0000000 0x2000>; /* PCI config space */
>>>>> -		reg-names = "ctrlreg", "config";
>>>>> +		reg = <0xdfc00000 0x302000>, /* IP registers */
>>>>
>>>> which version of synopsys IP is this. I think the ideal thing to do here is to
>>>> have a separate register space for iATU.
>>>
>>> I also agree with that. The unroll iATU was introduced on Synopsys IP version
>>> 4.80 and the kernel patch was release on 2016-08-10
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_657796_&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=G1MqB_DY6ZwWtvS60L9PZMHnMe6rClMnrakAyQT_hDc&s=EgKKDbg4ywCxu4-lG_scYAgPMnxirsjS0uSS7SzBTeM&e=
>>> However a separate register space for iATU implies some extra code do handle it
>>> (and of course some tests) that don't fit into this patch series, in my point of
>>> view. Can this task enter in the backlog in order to be done in another patch
>>> series?
>>
>> Yes sure. I think we should also make sure existing binding doesn't break.

I'll remove the any iATU unroll reference or change from this patch to avoid any
confusion.

> 
> And IMO we should have a new compatible "snps,dw-pcie-4.80" in order to enable
> iATU unroll.
> 
> Thanks
> Kishon
> 

Re: [PATCH 1/8] bindings: PCI: designware: Example update

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 April 2018 06:43 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 03/04/2018 11:53, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 03 April 2018 04:22 PM, Kishon Vijay Abraham I wrote:
>>>
>>>
>>> On Tuesday 03 April 2018 04:03 PM, Gustavo Pimentel wrote:
>>>> Hi Kishon,
>>>>
>>>> On 02/04/2018 06:23, Kishon Vijay Abraham I wrote:
>>>>> Hi,
>>>>>
>>>>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>>>>> Changes the IP registers size to accommodate the ATU unroll space.
>>>>>>
>>>>>> Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers.
>>>>>>
>>>>>> Replaces the pcie base address example by a real pcie base address in use.
>>>>>>
>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
>>>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>>> index 1da7ade..6300762 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>>> @@ -1,7 +1,8 @@
>>>>>>  * Synopsys DesignWare PCIe interface
>>>>>>  
>>>>>>  Required properties:
>>>>>> -- compatible: should contain "snps,dw-pcie" to identify the core.
>>>>>> +- compatible:
>>>>>> +	"snps,dw-pcie" for RC mode;
>>>>>
>>>>> I think irrespective of RC mode or EP mode, "snps,dw-pcie" can be used to
>>>>> identify the pcie core?
>>>>
>>>> I guess so. What you suggest? I was just folling the same guideline present
>>>> here: https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_11_3_310&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=G1MqB_DY6ZwWtvS60L9PZMHnMe6rClMnrakAyQT_hDc&s=BhQYkrcp6y3QkD23qn1I6lU882BDUfLiXjBVWQ91cmg&e=
>>>
>>> Okay, I think you should have
>>> "snps,dw-pcie-rc", "snps,dw-pcie" for RC mode;
>>>
>>> and in the later patch
>>> "snps,dw-pcie-ep", "snps,dw-pcie" for EP mode;
>>>
> 
> Ok, I'll change it.
> 
>>>>
>>>>>>  - reg: Should contain the configuration address space.
>>>>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>>>>      (The old way of getting the configuration address space from "ranges"
>>>>>> @@ -41,11 +42,11 @@ EP mode:
>>>>>>  
>>>>>>  Example configuration:
>>>>>>  
>>>>>> -	pcie: pcie@dffff000 {
>>>>>> +	pcie: pcie@dfc00000 {
>>>>>>  		compatible = "snps,dw-pcie";
>>>>>> -		reg = <0xdffff000 0x1000>, /* Controller registers */
>>>>>> -		      <0xd0000000 0x2000>; /* PCI config space */
>>>>>> -		reg-names = "ctrlreg", "config";
>>>>>> +		reg = <0xdfc00000 0x302000>, /* IP registers */
>>>>>
>>>>> which version of synopsys IP is this. I think the ideal thing to do here is to
>>>>> have a separate register space for iATU.
>>>>
>>>> I also agree with that. The unroll iATU was introduced on Synopsys IP version
>>>> 4.80 and the kernel patch was release on 2016-08-10
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_657796_&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=G1MqB_DY6ZwWtvS60L9PZMHnMe6rClMnrakAyQT_hDc&s=EgKKDbg4ywCxu4-lG_scYAgPMnxirsjS0uSS7SzBTeM&e=
>>>> However a separate register space for iATU implies some extra code do handle it
>>>> (and of course some tests) that don't fit into this patch series, in my point of
>>>> view. Can this task enter in the backlog in order to be done in another patch
>>>> series?
>>>
>>> Yes sure. I think we should also make sure existing binding doesn't break.
> 
> I'll remove the any iATU unroll reference or change from this patch to avoid any
> confusion.

No, I think it's fine to have iATU unroll reference (since kernel also seems to
support iATU unroll).

What I meant is, we should add more flexibility on defining iATU address space
(while making sure it doesn't break existing binding).

I'm okay with this patch once you fix the compatible comment but in general
this is what I'd like to have (in a future patch)

*)  Add a new compatible "snps,dw-pcie-4.80";
*) In the kernel "iatu_unroll_enabled" should be set based on the above compatible
*) Define the a new reg space for iATU in 4.80

And these have to be done in a way it doesn't break existing binding.

Thanks
Kishon

[PATCH 2/8] PCI: dwc: designware: Add support for endpoint mode

[Gustavo Pimentel]

The PCIe controller dual mode is capable of operating in host mode as well
as endpoint mode by configuration, therefore this patch aims to add
endpoint mode support to the designware driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/Kconfig                       |  45 ++++++--
 drivers/pci/dwc/pcie-designware-plat.c        | 157 ++++++++++++++++++++++++--
 drivers/pci/endpoint/functions/pci-epf-test.c |   5 +
 3 files changed, 187 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
index 2f3f5c5..3fd7daf 100644
--- a/drivers/pci/dwc/Kconfig
+++ b/drivers/pci/dwc/Kconfig
@@ -7,8 +7,7 @@ config PCIE_DW
 
 config PCIE_DW_HOST
         bool
-	depends on PCI
-	depends on PCI_MSI_IRQ_DOMAIN
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
         select PCIE_DW
 
 config PCIE_DW_EP
@@ -52,16 +51,42 @@ config PCI_DRA7XX_EP
 
 config PCIE_DW_PLAT
 	bool "Platform bus based DesignWare PCIe Controller"
-	depends on PCI
-	depends on PCI_MSI_IRQ_DOMAIN
-	select PCIE_DW_HOST
-	---help---
-	 This selects the DesignWare PCIe controller support. Select this if
-	 you have a PCIe controller on Platform bus.
+	help
+	  There are two instances of PCIe controller in Designware IP.
+	  This controller can work either as EP or RC. In order to enable
+	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
+	  order to enable device-specific features PCIE_DW_PLAT_EP must be
+	  selected.
 
-	 If you have a controller with this interface, say Y or M here.
+config PCIE_DW_PLAT_HOST
+	bool "Platform bus based DesignWare PCIe Controller - Host mode"
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	select PCIE_DW_PLAT
+	default y
+	help
+	  Enables support for the PCIe controller in the Designware IP to
+	  work in host mode. There are two instances of PCIe controller in
+	  Designware IP.
+	  This controller can work either as EP or RC. In order to enable
+	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
+	  order to enable device-specific features PCI_DW_PLAT_EP must be
+	  selected.
 
-	 If unsure, say N.
+config PCIE_DW_PLAT_EP
+	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	select PCIE_DW_PLAT
+	help
+	  Enables support for the PCIe controller in the Designware IP to
+	  work in endpoint mode. There are two instances of PCIe controller
+	  in Designware IP.
+	  This controller can work either as EP or RC. In order to enable
+	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
+	  order to enable device-specific features PCI_DW_PLAT_EP must be
+	  selected.
 
 config PCI_EXYNOS
 	bool "Samsung Exynos PCIe controller"
diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
index 5416aa8..921ab07 100644
--- a/drivers/pci/dwc/pcie-designware-plat.c
+++ b/drivers/pci/dwc/pcie-designware-plat.c
@@ -12,19 +12,29 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
 #include <linux/signal.h>
 #include <linux/types.h>
+#include <linux/regmap.h>
 
 #include "pcie-designware.h"
 
 struct dw_plat_pcie {
-	struct dw_pcie		*pci;
+	struct dw_pcie			*pci;
+	struct regmap			*regmap;
+	enum dw_pcie_device_mode	mode;
 };
 
+struct dw_plat_pcie_of_data {
+	enum dw_pcie_device_mode	mode;
+};
+
+static const struct of_device_id dw_plat_pcie_of_match[];
+
 static int dw_plat_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -42,9 +52,61 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
 	.host_init = dw_plat_pcie_host_init,
 };
 
-static int dw_plat_add_pcie_port(struct pcie_port *pp,
+static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
+{
+	dw_pcie_ep_linkup(&pci->ep);
+
+	return 0;
+}
+
+static void dw_plat_pcie_stop_link(struct dw_pcie *pci)
+{
+
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = dw_plat_pcie_establish_link,
+	.stop_link = dw_plat_pcie_stop_link,
+};
+
+static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = BAR_0; bar <= BAR_5; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				     enum pci_epc_irq_type type,
+				     u8 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
+		return -EINVAL;
+	case PCI_EPC_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+	}
+
+	return 0;
+}
+
+static struct dw_pcie_ep_ops pcie_ep_ops = {
+	.ep_init = dw_plat_pcie_ep_init,
+	.raise_irq = dw_plat_pcie_ep_raise_irq,
+};
+
+static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
 				 struct platform_device *pdev)
 {
+	struct dw_pcie *pci = dw_plat_pcie->pci;
+	struct pcie_port *pp = &pci->pp;
 	struct device *dev = &pdev->dev;
 	int ret;
 
@@ -63,15 +125,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
 
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
-		dev_err(dev, "failed to initialize host\n");
+		dev_err(dev, "Failed to initialize host\n");
 		return ret;
 	}
 
 	return 0;
 }
 
-static const struct dw_pcie_ops dw_pcie_ops = {
-};
+static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
+			       struct platform_device *pdev)
+{
+	int ret;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = dw_plat_pcie->pci;
+
+	ep = &pci->ep;
+	ep->ops = &pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
+	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(pci->dbi_base2))
+		return PTR_ERR(pci->dbi_base2);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+	if (!res)
+		return -EINVAL;
+
+	ep->phys_base = res->start;
+	ep->addr_size = resource_size(res);
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret) {
+		dev_err(dev, "Failed to initialize endpoint\n");
+		return ret;
+	}
+	return 0;
+}
 
 static int dw_plat_pcie_probe(struct platform_device *pdev)
 {
@@ -80,6 +171,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct resource *res;  /* Resource from DT */
 	int ret;
+	const struct of_device_id *match;
+	const struct dw_plat_pcie_of_data *data;
+	enum dw_pcie_device_mode mode;
+
+	match = of_match_device(dw_plat_pcie_of_match, dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct dw_plat_pcie_of_data *)match->data;
+	mode = (enum dw_pcie_device_mode)data->mode;
 
 	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
 	if (!dw_plat_pcie)
@@ -93,23 +194,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	dw_plat_pcie->pci = pci;
+	dw_plat_pcie->mode = mode;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	if (!res)
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pci->dbi_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
 	platform_set_drvdata(pdev, dw_plat_pcie);
 
-	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
-	if (ret < 0)
-		return ret;
+	switch (dw_plat_pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
+			return -ENODEV;
+
+		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	case DW_PCIE_EP_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
+			return -ENODEV;
+
+		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
+	}
 
 	return 0;
 }
 
+static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id dw_plat_pcie_of_match[] = {
-	{ .compatible = "snps,dw-pcie", },
+	{
+		.compatible = "snps,dw-pcie",
+		.data = &dw_plat_pcie_rc_of_data,
+	},
+	{
+		.compatible = "snps,dw-pcie-ep",
+		.data = &dw_plat_pcie_ep_of_data,
+	},
 	{},
 };
 
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 64d8a17..d7de684 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -451,9 +451,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	return 0;
 }
 
+static const struct pci_epf_test_data pci_epf_data = {
+	.linkup_notifier = false
+};
+
 static const struct pci_epf_device_id pci_epf_test_ids[] = {
 	{
 		.name = "pci_epf_test",
+		.driver_data = (kernel_ulong_t)&pci_epf_data,
 	},
 	{},
 };
-- 
2.7.4

Re: [PATCH 2/8] PCI: dwc: designware: Add support for endpoint mode

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> The PCIe controller dual mode is capable of operating in host mode as well
> as endpoint mode by configuration, therefore this patch aims to add
> endpoint mode support to the designware driver.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/dwc/Kconfig                       |  45 ++++++--
>  drivers/pci/dwc/pcie-designware-plat.c        | 157 ++++++++++++++++++++++++--
>  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +
>  3 files changed, 187 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
> index 2f3f5c5..3fd7daf 100644
> --- a/drivers/pci/dwc/Kconfig
> +++ b/drivers/pci/dwc/Kconfig
> @@ -7,8 +7,7 @@ config PCIE_DW
>  
>  config PCIE_DW_HOST
>          bool
> -	depends on PCI
> -	depends on PCI_MSI_IRQ_DOMAIN
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>          select PCIE_DW
>  
>  config PCIE_DW_EP
> @@ -52,16 +51,42 @@ config PCI_DRA7XX_EP
>  
>  config PCIE_DW_PLAT
>  	bool "Platform bus based DesignWare PCIe Controller"
> -	depends on PCI
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIE_DW_HOST
> -	---help---
> -	 This selects the DesignWare PCIe controller support. Select this if
> -	 you have a PCIe controller on Platform bus.
> +	help
> +	  There are two instances of PCIe controller in Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCIE_DW_PLAT_EP must be
> +	  selected.
>  
> -	 If you have a controller with this interface, say Y or M here.
> +config PCIE_DW_PLAT_HOST
> +	bool "Platform bus based DesignWare PCIe Controller - Host mode"
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	select PCIE_DW_PLAT
> +	default y
> +	help
> +	  Enables support for the PCIe controller in the Designware IP to
> +	  work in host mode. There are two instances of PCIe controller in
> +	  Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
> +	  selected.
>  
> -	 If unsure, say N.
> +config PCIE_DW_PLAT_EP
> +	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	select PCIE_DW_PLAT
> +	help
> +	  Enables support for the PCIe controller in the Designware IP to
> +	  work in endpoint mode. There are two instances of PCIe controller
> +	  in Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
> +	  selected.
>  
>  config PCI_EXYNOS
>  	bool "Samsung Exynos PCIe controller"
> diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
> index 5416aa8..921ab07 100644
> --- a/drivers/pci/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/dwc/pcie-designware-plat.c
> @@ -12,19 +12,29 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
>  #include <linux/signal.h>
>  #include <linux/types.h>
> +#include <linux/regmap.h>
>  
>  #include "pcie-designware.h"
>  
>  struct dw_plat_pcie {
> -	struct dw_pcie		*pci;
> +	struct dw_pcie			*pci;
> +	struct regmap			*regmap;
> +	enum dw_pcie_device_mode	mode;
>  };
>  
> +struct dw_plat_pcie_of_data {
> +	enum dw_pcie_device_mode	mode;
> +};
> +
> +static const struct of_device_id dw_plat_pcie_of_match[];
> +
>  static int dw_plat_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -42,9 +52,61 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>  	.host_init = dw_plat_pcie_host_init,
>  };
>  
> -static int dw_plat_add_pcie_port(struct pcie_port *pp,
> +static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
> +{
> +	dw_pcie_ep_linkup(&pci->ep);

.start_link ops is used incorrectly here. .start_link is used when all the
endpoint side configuration is done and "is ready" to establish a link with the
host. But dw_pcie_ep_linkup is used to inform the function devices that the
link "has been" established.
> +
> +	return 0;
> +}
> +
> +static void dw_plat_pcie_stop_link(struct dw_pcie *pci)
> +{
> +
> +}

Not necessary to have empty function here. pci-epc-core will not try to invoke
ops which are not populated.
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = dw_plat_pcie_establish_link,
> +	.stop_link = dw_plat_pcie_stop_link,
> +};
> +
> +static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	enum pci_barno bar;
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				     enum pci_epc_irq_type type,
> +				     u8 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
> +		return -EINVAL;
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static struct dw_pcie_ep_ops pcie_ep_ops = {
> +	.ep_init = dw_plat_pcie_ep_init,
> +	.raise_irq = dw_plat_pcie_ep_raise_irq,
> +};
> +
> +static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
>  				 struct platform_device *pdev)
>  {
> +	struct dw_pcie *pci = dw_plat_pcie->pci;
> +	struct pcie_port *pp = &pci->pp;
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> @@ -63,15 +125,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
>  
>  	ret = dw_pcie_host_init(pp);
>  	if (ret) {
> -		dev_err(dev, "failed to initialize host\n");
> +		dev_err(dev, "Failed to initialize host\n");
>  		return ret;
>  	}
>  
>  	return 0;
>  }
>  
> -static const struct dw_pcie_ops dw_pcie_ops = {
> -};
> +static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
> +			       struct platform_device *pdev)
> +{
> +	int ret;
> +	struct dw_pcie_ep *ep;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = dw_plat_pcie->pci;
> +
> +	ep = &pci->ep;
> +	ep->ops = &pcie_ep_ops;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> +	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(pci->dbi_base2))
> +		return PTR_ERR(pci->dbi_base2);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> +	if (!res)
> +		return -EINVAL;
> +
> +	ep->phys_base = res->start;
> +	ep->addr_size = resource_size(res);
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize endpoint\n");
> +		return ret;
> +	}
> +	return 0;
> +}
>  
>  static int dw_plat_pcie_probe(struct platform_device *pdev)
>  {
> @@ -80,6 +171,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>  	struct dw_pcie *pci;
>  	struct resource *res;  /* Resource from DT */
>  	int ret;
> +	const struct of_device_id *match;
> +	const struct dw_plat_pcie_of_data *data;
> +	enum dw_pcie_device_mode mode;
> +
> +	match = of_match_device(dw_plat_pcie_of_match, dev);
> +	if (!match)
> +		return -EINVAL;
> +
> +	data = (struct dw_plat_pcie_of_data *)match->data;
> +	mode = (enum dw_pcie_device_mode)data->mode;
>  
>  	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
>  	if (!dw_plat_pcie)
> @@ -93,23 +194,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>  	pci->ops = &dw_pcie_ops;
>  
>  	dw_plat_pcie->pci = pci;
> +	dw_plat_pcie->mode = mode;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	if (!res)
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pci->dbi_base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(pci->dbi_base))
>  		return PTR_ERR(pci->dbi_base);
>  
>  	platform_set_drvdata(pdev, dw_plat_pcie);
>  
> -	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
> -	if (ret < 0)
> -		return ret;
> +	switch (dw_plat_pcie->mode) {
> +	case DW_PCIE_RC_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
> +			return -ENODEV;
> +
> +		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case DW_PCIE_EP_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
> +			return -ENODEV;
> +
> +		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
> +	}
>  
>  	return 0;
>  }
>  
> +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static const struct of_device_id dw_plat_pcie_of_match[] = {
> -	{ .compatible = "snps,dw-pcie", },
> +	{
> +		.compatible = "snps,dw-pcie",
> +		.data = &dw_plat_pcie_rc_of_data,
> +	},
> +	{
> +		.compatible = "snps,dw-pcie-ep",
> +		.data = &dw_plat_pcie_ep_of_data,
> +	},
>  	{},
>  };
>  
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 64d8a17..d7de684 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -451,9 +451,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static const struct pci_epf_test_data pci_epf_data = {
> +	.linkup_notifier = false
> +};
> +
>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>  	{
>  		.name = "pci_epf_test",
> +		.driver_data = (kernel_ulong_t)&pci_epf_data,

This will disable linkup_notifier for existing devices. Please add a new entry
to the table to make any modifications to driver data. (I have a configfs fix
for this to work. Will post that today).

Thanks
Kishon

Re: [PATCH 2/8] PCI: dwc: designware: Add support for endpoint mode

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> The PCIe controller dual mode is capable of operating in host mode as well
> as endpoint mode by configuration, therefore this patch aims to add
> endpoint mode support to the designware driver.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/dwc/Kconfig                       |  45 ++++++--
>  drivers/pci/dwc/pcie-designware-plat.c        | 157 ++++++++++++++++++++++++--
>  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +
>  3 files changed, 187 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
> index 2f3f5c5..3fd7daf 100644
> --- a/drivers/pci/dwc/Kconfig
> +++ b/drivers/pci/dwc/Kconfig
> @@ -7,8 +7,7 @@ config PCIE_DW
>  
>  config PCIE_DW_HOST
>          bool
> -	depends on PCI
> -	depends on PCI_MSI_IRQ_DOMAIN
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>          select PCIE_DW
>  
>  config PCIE_DW_EP
> @@ -52,16 +51,42 @@ config PCI_DRA7XX_EP
>  
>  config PCIE_DW_PLAT
>  	bool "Platform bus based DesignWare PCIe Controller"
> -	depends on PCI
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIE_DW_HOST
> -	---help---
> -	 This selects the DesignWare PCIe controller support. Select this if
> -	 you have a PCIe controller on Platform bus.
> +	help
> +	  There are two instances of PCIe controller in Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCIE_DW_PLAT_EP must be
> +	  selected.
>  
> -	 If you have a controller with this interface, say Y or M here.
> +config PCIE_DW_PLAT_HOST
> +	bool "Platform bus based DesignWare PCIe Controller - Host mode"
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	select PCIE_DW_PLAT
> +	default y
> +	help
> +	  Enables support for the PCIe controller in the Designware IP to
> +	  work in host mode. There are two instances of PCIe controller in
> +	  Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
> +	  selected.
>  
> -	 If unsure, say N.
> +config PCIE_DW_PLAT_EP
> +	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	select PCIE_DW_PLAT
> +	help
> +	  Enables support for the PCIe controller in the Designware IP to
> +	  work in endpoint mode. There are two instances of PCIe controller
> +	  in Designware IP.
> +	  This controller can work either as EP or RC. In order to enable
> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
> +	  selected.
>  
>  config PCI_EXYNOS
>  	bool "Samsung Exynos PCIe controller"
> diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
> index 5416aa8..921ab07 100644
> --- a/drivers/pci/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/dwc/pcie-designware-plat.c
> @@ -12,19 +12,29 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
>  #include <linux/signal.h>
>  #include <linux/types.h>
> +#include <linux/regmap.h>
>  
>  #include "pcie-designware.h"
>  
>  struct dw_plat_pcie {
> -	struct dw_pcie		*pci;
> +	struct dw_pcie			*pci;
> +	struct regmap			*regmap;
> +	enum dw_pcie_device_mode	mode;
>  };
>  
> +struct dw_plat_pcie_of_data {
> +	enum dw_pcie_device_mode	mode;
> +};
> +
> +static const struct of_device_id dw_plat_pcie_of_match[];
> +
>  static int dw_plat_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -42,9 +52,61 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>  	.host_init = dw_plat_pcie_host_init,
>  };
>  
> -static int dw_plat_add_pcie_port(struct pcie_port *pp,
> +static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
> +{
> +	dw_pcie_ep_linkup(&pci->ep);

.start_link ops is used incorrectly here. .start_link is used when all the
endpoint side configuration is done and "is ready" to establish a link with the
host. But dw_pcie_ep_linkup is used to inform the function devices that the
link "has been" established.
> +
> +	return 0;
> +}
> +
> +static void dw_plat_pcie_stop_link(struct dw_pcie *pci)
> +{
> +
> +}

Not necessary to have empty function here. pci-epc-core will not try to invoke
ops which are not populated.
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = dw_plat_pcie_establish_link,
> +	.stop_link = dw_plat_pcie_stop_link,
> +};
> +
> +static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	enum pci_barno bar;
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				     enum pci_epc_irq_type type,
> +				     u8 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
> +		return -EINVAL;
> +	case PCI_EPC_IRQ_MSI:
> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +	default:
> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static struct dw_pcie_ep_ops pcie_ep_ops = {
> +	.ep_init = dw_plat_pcie_ep_init,
> +	.raise_irq = dw_plat_pcie_ep_raise_irq,
> +};
> +
> +static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
>  				 struct platform_device *pdev)
>  {
> +	struct dw_pcie *pci = dw_plat_pcie->pci;
> +	struct pcie_port *pp = &pci->pp;
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> @@ -63,15 +125,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
>  
>  	ret = dw_pcie_host_init(pp);
>  	if (ret) {
> -		dev_err(dev, "failed to initialize host\n");
> +		dev_err(dev, "Failed to initialize host\n");
>  		return ret;
>  	}
>  
>  	return 0;
>  }
>  
> -static const struct dw_pcie_ops dw_pcie_ops = {
> -};
> +static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
> +			       struct platform_device *pdev)
> +{
> +	int ret;
> +	struct dw_pcie_ep *ep;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = dw_plat_pcie->pci;
> +
> +	ep = &pci->ep;
> +	ep->ops = &pcie_ep_ops;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
> +	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(pci->dbi_base2))
> +		return PTR_ERR(pci->dbi_base2);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> +	if (!res)
> +		return -EINVAL;
> +
> +	ep->phys_base = res->start;
> +	ep->addr_size = resource_size(res);
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize endpoint\n");
> +		return ret;
> +	}
> +	return 0;
> +}
>  
>  static int dw_plat_pcie_probe(struct platform_device *pdev)
>  {
> @@ -80,6 +171,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>  	struct dw_pcie *pci;
>  	struct resource *res;  /* Resource from DT */
>  	int ret;
> +	const struct of_device_id *match;
> +	const struct dw_plat_pcie_of_data *data;
> +	enum dw_pcie_device_mode mode;
> +
> +	match = of_match_device(dw_plat_pcie_of_match, dev);
> +	if (!match)
> +		return -EINVAL;
> +
> +	data = (struct dw_plat_pcie_of_data *)match->data;
> +	mode = (enum dw_pcie_device_mode)data->mode;
>  
>  	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
>  	if (!dw_plat_pcie)
> @@ -93,23 +194,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>  	pci->ops = &dw_pcie_ops;
>  
>  	dw_plat_pcie->pci = pci;
> +	dw_plat_pcie->mode = mode;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	if (!res)
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pci->dbi_base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(pci->dbi_base))
>  		return PTR_ERR(pci->dbi_base);
>  
>  	platform_set_drvdata(pdev, dw_plat_pcie);
>  
> -	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
> -	if (ret < 0)
> -		return ret;
> +	switch (dw_plat_pcie->mode) {
> +	case DW_PCIE_RC_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
> +			return -ENODEV;
> +
> +		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case DW_PCIE_EP_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
> +			return -ENODEV;
> +
> +		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
> +	}
>  
>  	return 0;
>  }
>  
> +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static const struct of_device_id dw_plat_pcie_of_match[] = {
> -	{ .compatible = "snps,dw-pcie", },
> +	{
> +		.compatible = "snps,dw-pcie",
> +		.data = &dw_plat_pcie_rc_of_data,
> +	},
> +	{
> +		.compatible = "snps,dw-pcie-ep",
> +		.data = &dw_plat_pcie_ep_of_data,
> +	},
>  	{},
>  };
>  
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 64d8a17..d7de684 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -451,9 +451,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static const struct pci_epf_test_data pci_epf_data = {
> +	.linkup_notifier = false
> +};
> +
>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>  	{
>  		.name = "pci_epf_test",
> +		.driver_data = (kernel_ulong_t)&pci_epf_data,

This will disable linkup_notifier for existing devices. Please add a new entry
to the table to make any modifications to driver data. (I have a configfs fix
for this to work. Will post that today).

Thanks
Kishon

Re: [PATCH 2/8] PCI: dwc: designware: Add support for endpoint mode

[Gustavo Pimentel]

On 02/04/2018 06:34, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>> The PCIe controller dual mode is capable of operating in host mode as well
>> as endpoint mode by configuration, therefore this patch aims to add
>> endpoint mode support to the designware driver.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  drivers/pci/dwc/Kconfig                       |  45 ++++++--
>>  drivers/pci/dwc/pcie-designware-plat.c        | 157 ++++++++++++++++++++++++--
>>  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +
>>  3 files changed, 187 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>> index 2f3f5c5..3fd7daf 100644
>> --- a/drivers/pci/dwc/Kconfig
>> +++ b/drivers/pci/dwc/Kconfig
>> @@ -7,8 +7,7 @@ config PCIE_DW
>>  
>>  config PCIE_DW_HOST
>>          bool
>> -	depends on PCI
>> -	depends on PCI_MSI_IRQ_DOMAIN
>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>>          select PCIE_DW
>>  
>>  config PCIE_DW_EP
>> @@ -52,16 +51,42 @@ config PCI_DRA7XX_EP
>>  
>>  config PCIE_DW_PLAT
>>  	bool "Platform bus based DesignWare PCIe Controller"
>> -	depends on PCI
>> -	depends on PCI_MSI_IRQ_DOMAIN
>> -	select PCIE_DW_HOST
>> -	---help---
>> -	 This selects the DesignWare PCIe controller support. Select this if
>> -	 you have a PCIe controller on Platform bus.
>> +	help
>> +	  There are two instances of PCIe controller in Designware IP.
>> +	  This controller can work either as EP or RC. In order to enable
>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>> +	  order to enable device-specific features PCIE_DW_PLAT_EP must be
>> +	  selected.
>>  
>> -	 If you have a controller with this interface, say Y or M here.
>> +config PCIE_DW_PLAT_HOST
>> +	bool "Platform bus based DesignWare PCIe Controller - Host mode"
>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>> +	select PCIE_DW_HOST
>> +	select PCIE_DW_PLAT
>> +	default y
>> +	help
>> +	  Enables support for the PCIe controller in the Designware IP to
>> +	  work in host mode. There are two instances of PCIe controller in
>> +	  Designware IP.
>> +	  This controller can work either as EP or RC. In order to enable
>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
>> +	  selected.
>>  
>> -	 If unsure, say N.
>> +config PCIE_DW_PLAT_EP
>> +	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>> +	depends on PCI_ENDPOINT
>> +	select PCIE_DW_EP
>> +	select PCIE_DW_PLAT
>> +	help
>> +	  Enables support for the PCIe controller in the Designware IP to
>> +	  work in endpoint mode. There are two instances of PCIe controller
>> +	  in Designware IP.
>> +	  This controller can work either as EP or RC. In order to enable
>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
>> +	  selected.
>>  
>>  config PCI_EXYNOS
>>  	bool "Samsung Exynos PCIe controller"
>> diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
>> index 5416aa8..921ab07 100644
>> --- a/drivers/pci/dwc/pcie-designware-plat.c
>> +++ b/drivers/pci/dwc/pcie-designware-plat.c
>> @@ -12,19 +12,29 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/kernel.h>
>>  #include <linux/init.h>
>> +#include <linux/of_device.h>
>>  #include <linux/of_gpio.h>
>>  #include <linux/pci.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/resource.h>
>>  #include <linux/signal.h>
>>  #include <linux/types.h>
>> +#include <linux/regmap.h>
>>  
>>  #include "pcie-designware.h"
>>  
>>  struct dw_plat_pcie {
>> -	struct dw_pcie		*pci;
>> +	struct dw_pcie			*pci;
>> +	struct regmap			*regmap;
>> +	enum dw_pcie_device_mode	mode;
>>  };
>>  
>> +struct dw_plat_pcie_of_data {
>> +	enum dw_pcie_device_mode	mode;
>> +};
>> +
>> +static const struct of_device_id dw_plat_pcie_of_match[];
>> +
>>  static int dw_plat_pcie_host_init(struct pcie_port *pp)
>>  {
>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> @@ -42,9 +52,61 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>>  	.host_init = dw_plat_pcie_host_init,
>>  };
>>  
>> -static int dw_plat_add_pcie_port(struct pcie_port *pp,
>> +static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
>> +{
>> +	dw_pcie_ep_linkup(&pci->ep);
> 
> .start_link ops is used incorrectly here. .start_link is used when all the
> endpoint side configuration is done and "is ready" to establish a link with the
> host. But dw_pcie_ep_linkup is used to inform the function devices that the
> link "has been" established.

If I move the dw_pcie_ep_linkup call from the dw_plat_pcie_establish_link to the
dw_plat_pcie_ep_init function it would be more correct in your perspective? If
not, where you suggest?

>> +
>> +	return 0;
>> +}
>> +
>> +static void dw_plat_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> +
>> +}
> 
> Not necessary to have empty function here. pci-epc-core will not try to invoke
> ops which are not populated.

Ok, I'll remove it.

>> +
>> +static const struct dw_pcie_ops dw_pcie_ops = {
>> +	.start_link = dw_plat_pcie_establish_link,
>> +	.stop_link = dw_plat_pcie_stop_link,
>> +};
>> +
>> +static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	enum pci_barno bar;
>> +
>> +	for (bar = BAR_0; bar <= BAR_5; bar++)
>> +		dw_pcie_ep_reset_bar(pci, bar);
>> +}
>> +
>> +static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +				     enum pci_epc_irq_type type,
>> +				     u8 interrupt_num)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +
>> +	switch (type) {
>> +	case PCI_EPC_IRQ_LEGACY:
>> +		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
>> +		return -EINVAL;
>> +	case PCI_EPC_IRQ_MSI:
>> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> +	default:
>> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct dw_pcie_ep_ops pcie_ep_ops = {
>> +	.ep_init = dw_plat_pcie_ep_init,
>> +	.raise_irq = dw_plat_pcie_ep_raise_irq,
>> +};
>> +
>> +static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
>>  				 struct platform_device *pdev)
>>  {
>> +	struct dw_pcie *pci = dw_plat_pcie->pci;
>> +	struct pcie_port *pp = &pci->pp;
>>  	struct device *dev = &pdev->dev;
>>  	int ret;
>>  
>> @@ -63,15 +125,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
>>  
>>  	ret = dw_pcie_host_init(pp);
>>  	if (ret) {
>> -		dev_err(dev, "failed to initialize host\n");
>> +		dev_err(dev, "Failed to initialize host\n");
>>  		return ret;
>>  	}
>>  
>>  	return 0;
>>  }
>>  
>> -static const struct dw_pcie_ops dw_pcie_ops = {
>> -};
>> +static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
>> +			       struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct dw_pcie_ep *ep;
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +	struct dw_pcie *pci = dw_plat_pcie->pci;
>> +
>> +	ep = &pci->ep;
>> +	ep->ops = &pcie_ep_ops;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
>> +	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
>> +	if (IS_ERR(pci->dbi_base2))
>> +		return PTR_ERR(pci->dbi_base2);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	ep->phys_base = res->start;
>> +	ep->addr_size = resource_size(res);
>> +
>> +	ret = dw_pcie_ep_init(ep);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to initialize endpoint\n");
>> +		return ret;
>> +	}
>> +	return 0;
>> +}
>>  
>>  static int dw_plat_pcie_probe(struct platform_device *pdev)
>>  {
>> @@ -80,6 +171,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>>  	struct dw_pcie *pci;
>>  	struct resource *res;  /* Resource from DT */
>>  	int ret;
>> +	const struct of_device_id *match;
>> +	const struct dw_plat_pcie_of_data *data;
>> +	enum dw_pcie_device_mode mode;
>> +
>> +	match = of_match_device(dw_plat_pcie_of_match, dev);
>> +	if (!match)
>> +		return -EINVAL;
>> +
>> +	data = (struct dw_plat_pcie_of_data *)match->data;
>> +	mode = (enum dw_pcie_device_mode)data->mode;
>>  
>>  	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
>>  	if (!dw_plat_pcie)
>> @@ -93,23 +194,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>>  	pci->ops = &dw_pcie_ops;
>>  
>>  	dw_plat_pcie->pci = pci;
>> +	dw_plat_pcie->mode = mode;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>> +	if (!res)
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  
>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	pci->dbi_base = devm_ioremap_resource(dev, res);
>>  	if (IS_ERR(pci->dbi_base))
>>  		return PTR_ERR(pci->dbi_base);
>>  
>>  	platform_set_drvdata(pdev, dw_plat_pcie);
>>  
>> -	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
>> -	if (ret < 0)
>> -		return ret;
>> +	switch (dw_plat_pcie->mode) {
>> +	case DW_PCIE_RC_TYPE:
>> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
>> +			return -ENODEV;
>> +
>> +		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	case DW_PCIE_EP_TYPE:
>> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
>> +			return -ENODEV;
>> +
>> +		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +	default:
>> +		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
>> +	}
>>  
>>  	return 0;
>>  }
>>  
>> +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
>> +	.mode = DW_PCIE_RC_TYPE,
>> +};
>> +
>> +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
>> +	.mode = DW_PCIE_EP_TYPE,
>> +};
>> +
>>  static const struct of_device_id dw_plat_pcie_of_match[] = {
>> -	{ .compatible = "snps,dw-pcie", },
>> +	{
>> +		.compatible = "snps,dw-pcie",
>> +		.data = &dw_plat_pcie_rc_of_data,
>> +	},
>> +	{
>> +		.compatible = "snps,dw-pcie-ep",
>> +		.data = &dw_plat_pcie_ep_of_data,
>> +	},
>>  	{},
>>  };
>>  
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 64d8a17..d7de684 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -451,9 +451,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>  	return 0;
>>  }
>>  
>> +static const struct pci_epf_test_data pci_epf_data = {
>> +	.linkup_notifier = false
>> +};
>> +
>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>  	{
>>  		.name = "pci_epf_test",
>> +		.driver_data = (kernel_ulong_t)&pci_epf_data,
> 
> This will disable linkup_notifier for existing devices. Please add a new entry
> to the table to make any modifications to driver data. (I have a configfs fix
> for this to work. Will post that today).

Are you refering to this patch? https://patchwork.kernel.org/patch/10319695/
Let's see if I understood it right, this patch will expose the some configfs
(something like /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/...) that
ables me to disable the linkup notifier for existing devices without the need to
change the code like I did, right?

> 
> Thanks
> Kishon
> 

Re: [PATCH 2/8] PCI: dwc: designware: Add support for endpoint mode

[Kishon Vijay Abraham I]

Hi,

On Wednesday 04 April 2018 03:50 PM, Gustavo Pimentel wrote:
> On 02/04/2018 06:34, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>> The PCIe controller dual mode is capable of operating in host mode as well
>>> as endpoint mode by configuration, therefore this patch aims to add
>>> endpoint mode support to the designware driver.
>>>
>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> ---
>>>  drivers/pci/dwc/Kconfig                       |  45 ++++++--
>>>  drivers/pci/dwc/pcie-designware-plat.c        | 157 ++++++++++++++++++++++++--
>>>  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +
>>>  3 files changed, 187 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>>> index 2f3f5c5..3fd7daf 100644
>>> --- a/drivers/pci/dwc/Kconfig
>>> +++ b/drivers/pci/dwc/Kconfig
>>> @@ -7,8 +7,7 @@ config PCIE_DW
>>>  
>>>  config PCIE_DW_HOST
>>>          bool
>>> -	depends on PCI
>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>>>          select PCIE_DW
>>>  
>>>  config PCIE_DW_EP
>>> @@ -52,16 +51,42 @@ config PCI_DRA7XX_EP
>>>  
>>>  config PCIE_DW_PLAT
>>>  	bool "Platform bus based DesignWare PCIe Controller"
>>> -	depends on PCI
>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>> -	select PCIE_DW_HOST
>>> -	---help---
>>> -	 This selects the DesignWare PCIe controller support. Select this if
>>> -	 you have a PCIe controller on Platform bus.
>>> +	help
>>> +	  There are two instances of PCIe controller in Designware IP.
>>> +	  This controller can work either as EP or RC. In order to enable
>>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>>> +	  order to enable device-specific features PCIE_DW_PLAT_EP must be
>>> +	  selected.
>>>  
>>> -	 If you have a controller with this interface, say Y or M here.
>>> +config PCIE_DW_PLAT_HOST
>>> +	bool "Platform bus based DesignWare PCIe Controller - Host mode"
>>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>>> +	select PCIE_DW_HOST
>>> +	select PCIE_DW_PLAT
>>> +	default y
>>> +	help
>>> +	  Enables support for the PCIe controller in the Designware IP to
>>> +	  work in host mode. There are two instances of PCIe controller in
>>> +	  Designware IP.
>>> +	  This controller can work either as EP or RC. In order to enable
>>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>>> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
>>> +	  selected.
>>>  
>>> -	 If unsure, say N.
>>> +config PCIE_DW_PLAT_EP
>>> +	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
>>> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
>>> +	depends on PCI_ENDPOINT
>>> +	select PCIE_DW_EP
>>> +	select PCIE_DW_PLAT
>>> +	help
>>> +	  Enables support for the PCIe controller in the Designware IP to
>>> +	  work in endpoint mode. There are two instances of PCIe controller
>>> +	  in Designware IP.
>>> +	  This controller can work either as EP or RC. In order to enable
>>> +	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
>>> +	  order to enable device-specific features PCI_DW_PLAT_EP must be
>>> +	  selected.
>>>  
>>>  config PCI_EXYNOS
>>>  	bool "Samsung Exynos PCIe controller"
>>> diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
>>> index 5416aa8..921ab07 100644
>>> --- a/drivers/pci/dwc/pcie-designware-plat.c
>>> +++ b/drivers/pci/dwc/pcie-designware-plat.c
>>> @@ -12,19 +12,29 @@
>>>  #include <linux/interrupt.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/init.h>
>>> +#include <linux/of_device.h>
>>>  #include <linux/of_gpio.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/resource.h>
>>>  #include <linux/signal.h>
>>>  #include <linux/types.h>
>>> +#include <linux/regmap.h>
>>>  
>>>  #include "pcie-designware.h"
>>>  
>>>  struct dw_plat_pcie {
>>> -	struct dw_pcie		*pci;
>>> +	struct dw_pcie			*pci;
>>> +	struct regmap			*regmap;
>>> +	enum dw_pcie_device_mode	mode;
>>>  };
>>>  
>>> +struct dw_plat_pcie_of_data {
>>> +	enum dw_pcie_device_mode	mode;
>>> +};
>>> +
>>> +static const struct of_device_id dw_plat_pcie_of_match[];
>>> +
>>>  static int dw_plat_pcie_host_init(struct pcie_port *pp)
>>>  {
>>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> @@ -42,9 +52,61 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>>>  	.host_init = dw_plat_pcie_host_init,
>>>  };
>>>  
>>> -static int dw_plat_add_pcie_port(struct pcie_port *pp,
>>> +static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
>>> +{
>>> +	dw_pcie_ep_linkup(&pci->ep);
>>
>> .start_link ops is used incorrectly here. .start_link is used when all the
>> endpoint side configuration is done and "is ready" to establish a link with the
>> host. But dw_pcie_ep_linkup is used to inform the function devices that the
>> link "has been" established.
> 
> If I move the dw_pcie_ep_linkup call from the dw_plat_pcie_establish_link to the
> dw_plat_pcie_ep_init function it would be more correct in your perspective? If
> not, where you suggest?

No, dw_pcie_ep_linkup is an optional function. If your platform gives a
notification when the link to the host is established, then you have to invoke
this function.

If you keep "linkup_notifier = false" in pci-epf-test.c, dw_pcie_ep_linkup
should be required.
> 
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void dw_plat_pcie_stop_link(struct dw_pcie *pci)
>>> +{
>>> +
>>> +}
>>
>> Not necessary to have empty function here. pci-epc-core will not try to invoke
>> ops which are not populated.
> 
> Ok, I'll remove it.
> 
>>> +
>>> +static const struct dw_pcie_ops dw_pcie_ops = {
>>> +	.start_link = dw_plat_pcie_establish_link,
>>> +	.stop_link = dw_plat_pcie_stop_link,
>>> +};
>>> +
>>> +static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +	enum pci_barno bar;
>>> +
>>> +	for (bar = BAR_0; bar <= BAR_5; bar++)
>>> +		dw_pcie_ep_reset_bar(pci, bar);
>>> +}
>>> +
>>> +static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>> +				     enum pci_epc_irq_type type,
>>> +				     u8 interrupt_num)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +
>>> +	switch (type) {
>>> +	case PCI_EPC_IRQ_LEGACY:
>>> +		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
>>> +		return -EINVAL;
>>> +	case PCI_EPC_IRQ_MSI:
>>> +		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>>> +	default:
>>> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct dw_pcie_ep_ops pcie_ep_ops = {
>>> +	.ep_init = dw_plat_pcie_ep_init,
>>> +	.raise_irq = dw_plat_pcie_ep_raise_irq,
>>> +};
>>> +
>>> +static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
>>>  				 struct platform_device *pdev)
>>>  {
>>> +	struct dw_pcie *pci = dw_plat_pcie->pci;
>>> +	struct pcie_port *pp = &pci->pp;
>>>  	struct device *dev = &pdev->dev;
>>>  	int ret;
>>>  
>>> @@ -63,15 +125,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
>>>  
>>>  	ret = dw_pcie_host_init(pp);
>>>  	if (ret) {
>>> -		dev_err(dev, "failed to initialize host\n");
>>> +		dev_err(dev, "Failed to initialize host\n");
>>>  		return ret;
>>>  	}
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> -static const struct dw_pcie_ops dw_pcie_ops = {
>>> -};
>>> +static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
>>> +			       struct platform_device *pdev)
>>> +{
>>> +	int ret;
>>> +	struct dw_pcie_ep *ep;
>>> +	struct resource *res;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct dw_pcie *pci = dw_plat_pcie->pci;
>>> +
>>> +	ep = &pci->ep;
>>> +	ep->ops = &pcie_ep_ops;
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
>>> +	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res));
>>> +	if (IS_ERR(pci->dbi_base2))
>>> +		return PTR_ERR(pci->dbi_base2);
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>>> +	if (!res)
>>> +		return -EINVAL;
>>> +
>>> +	ep->phys_base = res->start;
>>> +	ep->addr_size = resource_size(res);
>>> +
>>> +	ret = dw_pcie_ep_init(ep);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to initialize endpoint\n");
>>> +		return ret;
>>> +	}
>>> +	return 0;
>>> +}
>>>  
>>>  static int dw_plat_pcie_probe(struct platform_device *pdev)
>>>  {
>>> @@ -80,6 +171,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>>>  	struct dw_pcie *pci;
>>>  	struct resource *res;  /* Resource from DT */
>>>  	int ret;
>>> +	const struct of_device_id *match;
>>> +	const struct dw_plat_pcie_of_data *data;
>>> +	enum dw_pcie_device_mode mode;
>>> +
>>> +	match = of_match_device(dw_plat_pcie_of_match, dev);
>>> +	if (!match)
>>> +		return -EINVAL;
>>> +
>>> +	data = (struct dw_plat_pcie_of_data *)match->data;
>>> +	mode = (enum dw_pcie_device_mode)data->mode;
>>>  
>>>  	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
>>>  	if (!dw_plat_pcie)
>>> @@ -93,23 +194,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
>>>  	pci->ops = &dw_pcie_ops;
>>>  
>>>  	dw_plat_pcie->pci = pci;
>>> +	dw_plat_pcie->mode = mode;
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
>>> +	if (!res)
>>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  
>>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  	pci->dbi_base = devm_ioremap_resource(dev, res);
>>>  	if (IS_ERR(pci->dbi_base))
>>>  		return PTR_ERR(pci->dbi_base);
>>>  
>>>  	platform_set_drvdata(pdev, dw_plat_pcie);
>>>  
>>> -	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
>>> -	if (ret < 0)
>>> -		return ret;
>>> +	switch (dw_plat_pcie->mode) {
>>> +	case DW_PCIE_RC_TYPE:
>>> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
>>> +			return -ENODEV;
>>> +
>>> +		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		break;
>>> +	case DW_PCIE_EP_TYPE:
>>> +		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
>>> +			return -ENODEV;
>>> +
>>> +		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
>>> +	}
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
>>> +	.mode = DW_PCIE_RC_TYPE,
>>> +};
>>> +
>>> +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
>>> +	.mode = DW_PCIE_EP_TYPE,
>>> +};
>>> +
>>>  static const struct of_device_id dw_plat_pcie_of_match[] = {
>>> -	{ .compatible = "snps,dw-pcie", },
>>> +	{
>>> +		.compatible = "snps,dw-pcie",
>>> +		.data = &dw_plat_pcie_rc_of_data,
>>> +	},
>>> +	{
>>> +		.compatible = "snps,dw-pcie-ep",
>>> +		.data = &dw_plat_pcie_ep_of_data,
>>> +	},
>>>  	{},
>>>  };
>>>  
>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> index 64d8a17..d7de684 100644
>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> @@ -451,9 +451,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>  	return 0;
>>>  }
>>>  
>>> +static const struct pci_epf_test_data pci_epf_data = {
>>> +	.linkup_notifier = false
>>> +};
>>> +
>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>  	{
>>>  		.name = "pci_epf_test",
>>> +		.driver_data = (kernel_ulong_t)&pci_epf_data,
>>
>> This will disable linkup_notifier for existing devices. Please add a new entry
>> to the table to make any modifications to driver data. (I have a configfs fix
>> for this to work. Will post that today).
> 
> Are you refering to this patch? https://patchwork.kernel.org/patch/10319695/
> Let's see if I understood it right, this patch will expose the some configfs
> (something like /sys/kernel/config/pci_ep/functions/pci_epf_test/func1/...) that
> ables me to disable the linkup notifier for existing devices without the need to
> change the code like I did, right?

Actually that patch doesn't add any configfs attribute entry. It just creates a
new configfs directory for every entry populated in pci_epf_device_id.

For example, If you have something like this

static const struct pci_epf_test_data k2g_data = {
	.test_reg_bar = BAR_1,
	.linkup_notifier = false
};

static const struct pci_epf_device_id pci_epf_test_ids[] = {
	{
		.name = "pci_epf_test",
	},
	{
		.name = "pci_epf_test_k2g",
		.driver_data = (kernel_ulong_t)&k2g_data,
	},
	{},
};

after [1], there should be 2 entries in /sys/kernel/config/pci_ep/functions/
pci_epf_test and pci_epf_test_k2g.

Here if you use pci_epf_test_k2g entry then the driver data associated with
pci_epf_test_k2g will be used.

Thanks
Kishon

[1] -> https://patchwork.kernel.org/patch/10319695/
> 
>>
>> Thanks
>> Kishon
>>
> 
> 

[PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Gustavo Pimentel]

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 6300762..4bb2e08 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -3,6 +3,7 @@
 Required properties:
 - compatible:
 	"snps,dw-pcie" for RC mode;
+	"snps,dw-pcie-ep" for EP mode;
 - reg: Should contain the configuration address space.
 - reg-names: Must be "config" for the PCIe configuration space.
     (The old way of getting the configuration address space from "ranges"
@@ -56,3 +57,15 @@ Example configuration:
 		#interrupt-cells = <1>;
 		num-lanes = <1>;
 	};
+or
+	pcie_ep: pcie_ep@dfc00000 {
+		compatible = "snps,dw-pcie-ep";
+		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
+		      <0xdfc01000 0x0001000>, /* IP registers 2 */
+		      <0xd0000000 0x2000000>; /* Configuration space */
+		reg-names = "dbi", "dbi2", "addr_space";
+		device_type = "pci";
+		num-ib-windows = <6>;
+		num-ob-windows = <2>;
+		num-lanes = <1>;
+	};
-- 
2.7.4

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Kishon Vijay Abraham I]

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Please add a commit message.
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 6300762..4bb2e08 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -3,6 +3,7 @@
>  Required properties:
>  - compatible:
>  	"snps,dw-pcie" for RC mode;
> +	"snps,dw-pcie-ep" for EP mode;
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -56,3 +57,15 @@ Example configuration:
>  		#interrupt-cells = <1>;
>  		num-lanes = <1>;
>  	};
> +or
> +	pcie_ep: pcie_ep@dfc00000 {
> +		compatible = "snps,dw-pcie-ep";
> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */

Doesn't this have iATU unroll space?

Thanks
Kishon

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Kishon Vijay Abraham I]

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

Please add a commit message.
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 6300762..4bb2e08 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -3,6 +3,7 @@
>  Required properties:
>  - compatible:
>  	"snps,dw-pcie" for RC mode;
> +	"snps,dw-pcie-ep" for EP mode;
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -56,3 +57,15 @@ Example configuration:
>  		#interrupt-cells = <1>;
>  		num-lanes = <1>;
>  	};
> +or
> +	pcie_ep: pcie_ep@dfc00000 {
> +		compatible = "snps,dw-pcie-ep";
> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */

Doesn't this have iATU unroll space?

Thanks
Kishon

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Gustavo Pimentel]

Hi Kishon,

On 02/04/2018 06:35, Kishon Vijay Abraham I wrote:
> 
> 
> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> 
> Please add a commit message.

Ok. I'll add. Thanks for noticing it.

>> ---
>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index 6300762..4bb2e08 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -3,6 +3,7 @@
>>  Required properties:
>>  - compatible:
>>  	"snps,dw-pcie" for RC mode;
>> +	"snps,dw-pcie-ep" for EP mode;
>>  - reg: Should contain the configuration address space.
>>  - reg-names: Must be "config" for the PCIe configuration space.
>>      (The old way of getting the configuration address space from "ranges"
>> @@ -56,3 +57,15 @@ Example configuration:
>>  		#interrupt-cells = <1>;
>>  		num-lanes = <1>;
>>  	};
>> +or
>> +	pcie_ep: pcie_ep@dfc00000 {
>> +		compatible = "snps,dw-pcie-ep";
>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
> 
> Doesn't this have iATU unroll space?

I don't think EP has it, but I'm no expert on this matter. Can you provide me
some example of having iATU unroll space mapping would be useful in EP scope?

> 
> Thanks
> Kishon
> 

Regards,
Gustavo

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 April 2018 04:13 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 02/04/2018 06:35, Kishon Vijay Abraham I wrote:
>>
>>
>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>
>> Please add a commit message.
> 
> Ok. I'll add. Thanks for noticing it.
> 
>>> ---
>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> index 6300762..4bb2e08 100644
>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> @@ -3,6 +3,7 @@
>>>  Required properties:
>>>  - compatible:
>>>  	"snps,dw-pcie" for RC mode;
>>> +	"snps,dw-pcie-ep" for EP mode;
>>>  - reg: Should contain the configuration address space.
>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>      (The old way of getting the configuration address space from "ranges"
>>> @@ -56,3 +57,15 @@ Example configuration:
>>>  		#interrupt-cells = <1>;
>>>  		num-lanes = <1>;
>>>  	};
>>> +or
>>> +	pcie_ep: pcie_ep@dfc00000 {
>>> +		compatible = "snps,dw-pcie-ep";
>>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
>>
>> Doesn't this have iATU unroll space?
> 
> I don't think EP has it, but I'm no expert on this matter. Can you provide me
> some example of having iATU unroll space mapping would be useful in EP scope?

I'm not sure. I thought if the dwc3 core version is 4.80, then it'll have a
separate ATU space irrespective of RC mode or EP mode.

Thanks
Kishon

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Gustavo Pimentel]

Hi Kishon,

On 03/04/2018 11:55, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 03 April 2018 04:13 PM, Gustavo Pimentel wrote:
>> Hi Kishon,
>>
>> On 02/04/2018 06:35, Kishon Vijay Abraham I wrote:
>>>
>>>
>>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>
>>> Please add a commit message.
>>
>> Ok. I'll add. Thanks for noticing it.
>>
>>>> ---
>>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> index 6300762..4bb2e08 100644
>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>> @@ -3,6 +3,7 @@
>>>>  Required properties:
>>>>  - compatible:
>>>>  	"snps,dw-pcie" for RC mode;
>>>> +	"snps,dw-pcie-ep" for EP mode;
>>>>  - reg: Should contain the configuration address space.
>>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>>      (The old way of getting the configuration address space from "ranges"
>>>> @@ -56,3 +57,15 @@ Example configuration:
>>>>  		#interrupt-cells = <1>;
>>>>  		num-lanes = <1>;
>>>>  	};
>>>> +or
>>>> +	pcie_ep: pcie_ep@dfc00000 {
>>>> +		compatible = "snps,dw-pcie-ep";
>>>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>>>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
>>>
>>> Doesn't this have iATU unroll space?
>>
>> I don't think EP has it, but I'm no expert on this matter. Can you provide me
>> some example of having iATU unroll space mapping would be useful in EP scope?
> 
> I'm not sure. I thought if the dwc3 core version is 4.80, then it'll have a
> separate ATU space irrespective of RC mode or EP mode.

As replied on patch 1, let's leave out  any reference of iATU unroll to avoid
any confusion. Agree?

> 
> Thanks
> Kishon
> 

Regards,
Gustavo

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 April 2018 06:50 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 03/04/2018 11:55, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 03 April 2018 04:13 PM, Gustavo Pimentel wrote:
>>> Hi Kishon,
>>>
>>> On 02/04/2018 06:35, Kishon Vijay Abraham I wrote:
>>>>
>>>>
>>>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>
>>>> Please add a commit message.
>>>
>>> Ok. I'll add. Thanks for noticing it.
>>>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> index 6300762..4bb2e08 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>>>> @@ -3,6 +3,7 @@
>>>>>  Required properties:
>>>>>  - compatible:
>>>>>  	"snps,dw-pcie" for RC mode;
>>>>> +	"snps,dw-pcie-ep" for EP mode;
>>>>>  - reg: Should contain the configuration address space.
>>>>>  - reg-names: Must be "config" for the PCIe configuration space.
>>>>>      (The old way of getting the configuration address space from "ranges"
>>>>> @@ -56,3 +57,15 @@ Example configuration:
>>>>>  		#interrupt-cells = <1>;
>>>>>  		num-lanes = <1>;
>>>>>  	};
>>>>> +or
>>>>> +	pcie_ep: pcie_ep@dfc00000 {
>>>>> +		compatible = "snps,dw-pcie-ep";
>>>>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>>>>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
>>>>
>>>> Doesn't this have iATU unroll space?
>>>
>>> I don't think EP has it, but I'm no expert on this matter. Can you provide me
>>> some example of having iATU unroll space mapping would be useful in EP scope?
>>
>> I'm not sure. I thought if the dwc3 core version is 4.80, then it'll have a
>> separate ATU space irrespective of RC mode or EP mode.
> 
> As replied on patch 1, let's leave out  any reference of iATU unroll to avoid
> any confusion. Agree?

Mentioning iATU is fine as long as you change the size field in the "reg" property.

Thanks
Kishon

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Lorenzo Pieralisi]

On Wed, Mar 28, 2018 at 12:38:33PM +0100, Gustavo Pimentel wrote:

Please always write a commit log even if it is trivial.

Thanks,
Lorenzo

> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 6300762..4bb2e08 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -3,6 +3,7 @@
>  Required properties:
>  - compatible:
>  	"snps,dw-pcie" for RC mode;
> +	"snps,dw-pcie-ep" for EP mode;
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -56,3 +57,15 @@ Example configuration:
>  		#interrupt-cells = <1>;
>  		num-lanes = <1>;
>  	};
> +or
> +	pcie_ep: pcie_ep@dfc00000 {
> +		compatible = "snps,dw-pcie-ep";
> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
> +		      <0xd0000000 0x2000000>; /* Configuration space */
> +		reg-names = "dbi", "dbi2", "addr_space";
> +		device_type = "pci";
> +		num-ib-windows = <6>;
> +		num-ob-windows = <2>;
> +		num-lanes = <1>;
> +	};
> -- 
> 2.7.4
> 
> 

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Gustavo Pimentel]

Hi Lorenzo,

On 04/04/2018 12:50, Lorenzo Pieralisi wrote:
> On Wed, Mar 28, 2018 at 12:38:33PM +0100, Gustavo Pimentel wrote:
> 
> Please always write a commit log even if it is trivial.

Ok, Kishon has also refered that. On next patch version it'll contain a log
description.

> 
> Thanks,
> Lorenzo
> 
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index 6300762..4bb2e08 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -3,6 +3,7 @@
>>  Required properties:
>>  - compatible:
>>  	"snps,dw-pcie" for RC mode;
>> +	"snps,dw-pcie-ep" for EP mode;
>>  - reg: Should contain the configuration address space.
>>  - reg-names: Must be "config" for the PCIe configuration space.
>>      (The old way of getting the configuration address space from "ranges"
>> @@ -56,3 +57,15 @@ Example configuration:
>>  		#interrupt-cells = <1>;
>>  		num-lanes = <1>;
>>  	};
>> +or
>> +	pcie_ep: pcie_ep@dfc00000 {
>> +		compatible = "snps,dw-pcie-ep";
>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
>> +		      <0xd0000000 0x2000000>; /* Configuration space */
>> +		reg-names = "dbi", "dbi2", "addr_space";
>> +		device_type = "pci";
>> +		num-ib-windows = <6>;
>> +		num-ob-windows = <2>;
>> +		num-lanes = <1>;
>> +	};
>> -- 
>> 2.7.4
>>
>>

Regards,
Gustavo

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Rob Herring]

On Wed, Mar 28, 2018 at 12:38:33PM +0100, Gustavo Pimentel wrote:
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 6300762..4bb2e08 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -3,6 +3,7 @@
>  Required properties:
>  - compatible:
>  	"snps,dw-pcie" for RC mode;
> +	"snps,dw-pcie-ep" for EP mode;
>  - reg: Should contain the configuration address space.
>  - reg-names: Must be "config" for the PCIe configuration space.
>      (The old way of getting the configuration address space from "ranges"
> @@ -56,3 +57,15 @@ Example configuration:
>  		#interrupt-cells = <1>;
>  		num-lanes = <1>;
>  	};
> +or
> +	pcie_ep: pcie_ep@dfc00000 {

pcie-ep@...

Or what others have used. We should define a standard name in the DT 
spec for this.

> +		compatible = "snps,dw-pcie-ep";
> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
> +		      <0xd0000000 0x2000000>; /* Configuration space */
> +		reg-names = "dbi", "dbi2", "addr_space";
> +		device_type = "pci";
> +		num-ib-windows = <6>;
> +		num-ob-windows = <2>;
> +		num-lanes = <1>;
> +	};
> -- 
> 2.7.4
> 
> 

Re: [PATCH 3/8] bindings: PCI: designware: Add support for the EP in designware driver

[Gustavo Pimentel]

Hi Rob,

On 09/04/2018 20:12, Rob Herring wrote:
> On Wed, Mar 28, 2018 at 12:38:33PM +0100, Gustavo Pimentel wrote:
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index 6300762..4bb2e08 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -3,6 +3,7 @@
>>  Required properties:
>>  - compatible:
>>  	"snps,dw-pcie" for RC mode;
>> +	"snps,dw-pcie-ep" for EP mode;
>>  - reg: Should contain the configuration address space.
>>  - reg-names: Must be "config" for the PCIe configuration space.
>>      (The old way of getting the configuration address space from "ranges"
>> @@ -56,3 +57,15 @@ Example configuration:
>>  		#interrupt-cells = <1>;
>>  		num-lanes = <1>;
>>  	};
>> +or
>> +	pcie_ep: pcie_ep@dfc00000 {
> 
> pcie-ep@...
> 
> Or what others have used. We should define a standard name in the DT 
> spec for this.
> 

By looking to [1] and [2], I think I should maintain the pcie name instead of
pcie-ep.

[1] ->
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pci/ti-pci.txt
[2] ->
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt

Thanks for noticing it.

>> +		compatible = "snps,dw-pcie-ep";
>> +		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
>> +		      <0xdfc01000 0x0001000>, /* IP registers 2 */
>> +		      <0xd0000000 0x2000000>; /* Configuration space */
>> +		reg-names = "dbi", "dbi2", "addr_space";
>> +		device_type = "pci";
>> +		num-ib-windows = <6>;
>> +		num-ob-windows = <2>;
>> +		num-lanes = <1>;
>> +	};
>> -- 
>> 2.7.4
>>
>>

Regards,
Gustavo

[PATCH 4/8] misc: pci_endpoint_test: Add designware EP entry

[Gustavo Pimentel]

Adds the designware EP device ID entry to pci_endpoint_test driver table
to allow this device to be recognize and handle by the pci_endpoint_test
driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/misc/pci_endpoint_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 320276f..e80c533 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -632,6 +632,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
-- 
2.7.4

Re: [PATCH 4/8] misc: pci_endpoint_test: Add designware EP entry

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Adds the designware EP device ID entry to pci_endpoint_test driver table
> to allow this device to be recognize and handle by the pci_endpoint_test
> driver.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 320276f..e80c533 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -632,6 +632,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>  static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },

Add device ids to include/linux/pci_ids.h and use the macro here.

Thanks
Kishon

Re: [PATCH 4/8] misc: pci_endpoint_test: Add designware EP entry

[Kishon Vijay Abraham I]

Hi,

On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
> Adds the designware EP device ID entry to pci_endpoint_test driver table
> to allow this device to be recognize and handle by the pci_endpoint_test
> driver.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 320276f..e80c533 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -632,6 +632,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>  static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },

Add device ids to include/linux/pci_ids.h and use the macro here.

Thanks
Kishon

Re: [PATCH 4/8] misc: pci_endpoint_test: Add designware EP entry

[Gustavo Pimentel]

Hi Kishon,

On 02/04/2018 06:36, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>> Adds the designware EP device ID entry to pci_endpoint_test driver table
>> to allow this device to be recognize and handle by the pci_endpoint_test
>> driver.
>>
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> ---
>>  drivers/misc/pci_endpoint_test.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index 320276f..e80c533 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -632,6 +632,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>>  static const struct pci_device_id pci_endpoint_test_tbl[] = {
>>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
>>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
> 
> Add device ids to include/linux/pci_ids.h and use the macro here.

Add device id request sent, as soon as I get a positive confirmation from them
I'll change for the macro. It works for you?

> 
> Thanks
> Kishon
> 

Regards,
Gustavo

Re: [PATCH 4/8] misc: pci_endpoint_test: Add designware EP entry

[Kishon Vijay Abraham I]

Hi,

On Tuesday 03 April 2018 03:41 PM, Gustavo Pimentel wrote:
> Hi Kishon,
> 
> On 02/04/2018 06:36, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 28 March 2018 05:08 PM, Gustavo Pimentel wrote:
>>> Adds the designware EP device ID entry to pci_endpoint_test driver table
>>> to allow this device to be recognize and handle by the pci_endpoint_test
>>> driver.
>>>
>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>> ---
>>>  drivers/misc/pci_endpoint_test.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>> index 320276f..e80c533 100644
>>> --- a/drivers/misc/pci_endpoint_test.c
>>> +++ b/drivers/misc/pci_endpoint_test.c
>>> @@ -632,6 +632,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>>>  static const struct pci_device_id pci_endpoint_test_tbl[] = {
>>>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
>>>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
>>> +	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
>>
>> Add device ids to include/linux/pci_ids.h and use the macro here.
> 
> Add device id request sent, as soon as I get a positive confirmation from them
> I'll change for the macro. It works for you?

IMO only confirmed device ID's should be used.

Thanks
Kishon

[PATCH 5/8] PCI: dwc: designware: Define maximum number of vectors

[Gustavo Pimentel]

Adds a callback that defines the maximum number of vectors that can be use
by the Root Complex.

Since this is a parameter associated to each SoC IP setting, makes sense to
be configurable and easily visible to future modifications.

The designware IP supports a maximum of 256 vectors.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
index 921ab07..11271bb 100644
--- a/drivers/pci/dwc/pcie-designware-plat.c
+++ b/drivers/pci/dwc/pcie-designware-plat.c
@@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static void dw_plat_set_num_vectors(struct pcie_port *pp)
+{
+	pp->num_vectors = MAX_MSI_IRQS;
+}
+
 static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
 	.host_init = dw_plat_pcie_host_init,
+	.set_num_vectors = dw_plat_set_num_vectors,
 };
 
 static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
-- 
2.7.4

[PATCH 6/8] PCI: dwc: Replace lower into upper case characters

[Gustavo Pimentel]

Replaces lower into upper case characters in comments and debug printks.

This is an attempt to keep the messages coherent within the designware
driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/pcie-designware-ep.c   | 16 ++++++++--------
 drivers/pci/dwc/pcie-designware-host.c | 35 ++++++++++++++++++----------------
 drivers/pci/dwc/pcie-designware.c      | 22 ++++++++++-----------
 3 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 9236b99..dcab4cf 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -65,7 +65,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar,
 
 	free_win = find_first_zero_bit(ep->ib_window_map, ep->num_ib_windows);
 	if (free_win >= ep->num_ib_windows) {
-		dev_err(pci->dev, "no free inbound window\n");
+		dev_err(pci->dev, "No free inbound window\n");
 		return -EINVAL;
 	}
 
@@ -90,7 +90,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
 
 	free_win = find_first_zero_bit(ep->ob_window_map, ep->num_ob_windows);
 	if (free_win >= ep->num_ob_windows) {
-		dev_err(pci->dev, "no free outbound window\n");
+		dev_err(pci->dev, "No free outbound window\n");
 		return -EINVAL;
 	}
 
@@ -184,7 +184,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no,
 
 	ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
 	if (ret) {
-		dev_err(pci->dev, "failed to enable address\n");
+		dev_err(pci->dev, "Failed to enable address\n");
 		return ret;
 	}
 
@@ -328,21 +328,21 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
-		dev_err(dev, "unable to read *num-ib-windows* property\n");
+		dev_err(dev, "Unable to read *num-ib-windows* property\n");
 		return ret;
 	}
 	if (ep->num_ib_windows > MAX_IATU_IN) {
-		dev_err(dev, "invalid *num-ib-windows*\n");
+		dev_err(dev, "Invalid *num-ib-windows*\n");
 		return -EINVAL;
 	}
 
 	ret = of_property_read_u32(np, "num-ob-windows", &ep->num_ob_windows);
 	if (ret < 0) {
-		dev_err(dev, "unable to read *num-ob-windows* property\n");
+		dev_err(dev, "Unable to read *num-ob-windows* property\n");
 		return ret;
 	}
 	if (ep->num_ob_windows > MAX_IATU_OUT) {
-		dev_err(dev, "invalid *num-ob-windows*\n");
+		dev_err(dev, "Invalid *num-ob-windows*\n");
 		return -EINVAL;
 	}
 
@@ -369,7 +369,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	epc = devm_pci_epc_create(dev, &epc_ops);
 	if (IS_ERR(epc)) {
-		dev_err(dev, "failed to create epc device\n");
+		dev_err(dev, "Failed to create epc device\n");
 		return PTR_ERR(epc);
 	}
 
diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 550fdbb..03e9b82 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -248,8 +248,10 @@ static void dw_pcie_irq_domain_free(struct irq_domain *domain,
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&pp->lock, flags);
+
 	bitmap_release_region(pp->msi_irq_in_use, data->hwirq,
 			      order_base_2(nr_irqs));
+
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
@@ -266,7 +268,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
 	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
 					       &dw_pcie_msi_domain_ops, pp);
 	if (!pp->irq_domain) {
-		dev_err(pci->dev, "failed to create IRQ domain\n");
+		dev_err(pci->dev, "Failed to create IRQ domain\n");
 		return -ENOMEM;
 	}
 
@@ -274,7 +276,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
 						   &dw_pcie_msi_domain_info,
 						   pp->irq_domain);
 	if (!pp->msi_domain) {
-		dev_err(pci->dev, "failed to create MSI domain\n");
+		dev_err(pci->dev, "Failed to create MSI domain\n");
 		irq_domain_remove(pp->irq_domain);
 		return -ENOMEM;
 	}
@@ -301,13 +303,13 @@ void dw_pcie_msi_init(struct pcie_port *pp)
 	page = alloc_page(GFP_KERNEL);
 	pp->msi_data = dma_map_page(dev, page, 0, PAGE_SIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(dev, pp->msi_data)) {
-		dev_err(dev, "failed to map MSI data\n");
+		dev_err(dev, "Failed to map MSI data\n");
 		__free_page(page);
 		return;
 	}
 	msi_target = (u64)pp->msi_data;
 
-	/* program the msi_data */
+	/* Program the msi_data */
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
 			    lower_32_bits(msi_target));
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
@@ -335,7 +337,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		pp->cfg0_base = cfg_res->start;
 		pp->cfg1_base = cfg_res->start + pp->cfg0_size;
 	} else if (!pp->va_cfg0_base) {
-		dev_err(dev, "missing *config* reg space\n");
+		dev_err(dev, "Missing *config* reg space\n");
 	}
 
 	bridge = pci_alloc_host_bridge(0);
@@ -357,7 +359,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		case IORESOURCE_IO:
 			ret = pci_remap_iospace(win->res, pp->io_base);
 			if (ret) {
-				dev_warn(dev, "error %d: failed to map resource %pR\n",
+				dev_warn(dev, "Error %d: failed to map resource %pR\n",
 					 ret, win->res);
 				resource_list_destroy_entry(win);
 			} else {
@@ -391,7 +393,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg->start,
 						resource_size(pp->cfg));
 		if (!pci->dbi_base) {
-			dev_err(dev, "error with ioremap\n");
+			dev_err(dev, "Error with ioremap\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -403,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
 					pp->cfg0_base, pp->cfg0_size);
 		if (!pp->va_cfg0_base) {
-			dev_err(dev, "error with ioremap in function\n");
+			dev_err(dev, "Error with ioremap in function\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -414,7 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg1_base,
 						pp->cfg1_size);
 		if (!pp->va_cfg1_base) {
-			dev_err(dev, "error with ioremap\n");
+			dev_err(dev, "Error with ioremap\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -586,7 +588,7 @@ static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus,
 			return 0;
 	}
 
-	/* access only one slot on each root port */
+	/* Access only one slot on each root port */
 	if (bus->number == pp->root_bus_nr && dev > 0)
 		return 0;
 
@@ -652,11 +654,12 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + (ctrl * 12), 4,
 				    &pp->irq_status[ctrl]);
-	/* setup RC BARs */
+
+	/* Setup RC BARs */
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000);
 
-	/* setup interrupt pins */
+	/* Setup interrupt pins */
 	dw_pcie_dbi_ro_wr_en(pci);
 	val = dw_pcie_readl_dbi(pci, PCI_INTERRUPT_LINE);
 	val &= 0xffff00ff;
@@ -664,13 +667,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
 	dw_pcie_dbi_ro_wr_dis(pci);
 
-	/* setup bus numbers */
+	/* Setup bus numbers */
 	val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
 	val &= 0xff000000;
 	val |= 0x00010100;
 	dw_pcie_writel_dbi(pci, PCI_PRIMARY_BUS, val);
 
-	/* setup command register */
+	/* Setup command register */
 	val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
 	val &= 0xffff0000;
 	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
@@ -683,7 +686,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	 * we should not program the ATU here.
 	 */
 	if (!pp->ops->rd_other_conf) {
-		/* get iATU unroll support */
+		/* Get iATU unroll support */
 		pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
 		dev_dbg(pci->dev, "iATU unroll: %s\n",
 			pci->iatu_unroll_enabled ? "enabled" : "disabled");
@@ -701,7 +704,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	/* Enable write permission for the DBI read-only register */
 	dw_pcie_dbi_ro_wr_en(pci);
-	/* program correct class for RC */
+	/* Program correct class for RC */
 	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
 	/* Better disable write permission right after the update */
 	dw_pcie_dbi_ro_wr_dis(pci);
diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
index 1b7282e..778c4f7 100644
--- a/drivers/pci/dwc/pcie-designware.c
+++ b/drivers/pci/dwc/pcie-designware.c
@@ -69,7 +69,7 @@ u32 __dw_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 
 	ret = dw_pcie_read(base + reg, size, &val);
 	if (ret)
-		dev_err(pci->dev, "read DBI address failed\n");
+		dev_err(pci->dev, "Read DBI address failed\n");
 
 	return val;
 }
@@ -86,7 +86,7 @@ void __dw_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 
 	ret = dw_pcie_write(base + reg, size, val);
 	if (ret)
-		dev_err(pci->dev, "write DBI address failed\n");
+		dev_err(pci->dev, "Write DBI address failed\n");
 }
 
 static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
@@ -137,7 +137,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "outbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
 }
 
 void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
@@ -180,7 +180,7 @@ void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "outbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
 }
 
 static u32 dw_pcie_readl_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg)
@@ -238,7 +238,7 @@ static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "inbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
 
 	return -EBUSY;
 }
@@ -284,7 +284,7 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "inbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
 
 	return -EBUSY;
 }
@@ -313,16 +313,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 {
 	int retries;
 
-	/* check if the link is up or not */
+	/* Check if the link is up or not */
 	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
 		if (dw_pcie_link_up(pci)) {
-			dev_info(pci->dev, "link up\n");
+			dev_info(pci->dev, "Link up\n");
 			return 0;
 		}
 		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
 	}
 
-	dev_err(pci->dev, "phy link never came up\n");
+	dev_err(pci->dev, "Phy link never came up\n");
 
 	return -ETIMEDOUT;
 }
@@ -351,7 +351,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	if (ret)
 		lanes = 0;
 
-	/* set the number of lanes */
+	/* Set the number of lanes */
 	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_MODE_MASK;
 	switch (lanes) {
@@ -373,7 +373,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	}
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
 
-	/* set link width speed control register */
+	/* Set link width speed control register */
 	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
 	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
 	switch (lanes) {
-- 
2.7.4

Re: [PATCH 6/8] PCI: dwc: Replace lower into upper case characters

[Fabio Estevam]

Hi Gustavo,

On Wed, Mar 28, 2018 at 8:38 AM, Gustavo Pimentel
<gustavo.pimentel@synopsys.com> wrote:

> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> index 550fdbb..03e9b82 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -248,8 +248,10 @@ static void dw_pcie_irq_domain_free(struct irq_domain *domain,
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&pp->lock, flags);
> +
>         bitmap_release_region(pp->msi_irq_in_use, data->hwirq,
>                               order_base_2(nr_irqs));
> +
>         raw_spin_unlock_irqrestore(&pp->lock, flags);
>  }

This looks like an unrelated change.

Re: [PATCH 6/8] PCI: dwc: Replace lower into upper case characters

[Gustavo Pimentel]

Hi Fabio,

On 28/03/2018 13:05, Fabio Estevam wrote:
> Hi Gustavo,
> 
> On Wed, Mar 28, 2018 at 8:38 AM, Gustavo Pimentel
> <gustavo.pimentel@synopsys.com> wrote:
> 
>> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
>> index 550fdbb..03e9b82 100644
>> --- a/drivers/pci/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/dwc/pcie-designware-host.c
>> @@ -248,8 +248,10 @@ static void dw_pcie_irq_domain_free(struct irq_domain *domain,
>>         unsigned long flags;
>>
>>         raw_spin_lock_irqsave(&pp->lock, flags);
>> +
>>         bitmap_release_region(pp->msi_irq_in_use, data->hwirq,
>>                               order_base_2(nr_irqs));
>> +
>>         raw_spin_unlock_irqrestore(&pp->lock, flags);
>>  }
> 
> This looks like an unrelated change.

Yes, it's not a replace character from lower to upper case. It's just a code
formatting style to be coherent with the rest of the driver code. I guess I can
added on the patch description a note about it.
It works for you?
Thanks.

> 
Regards,
Gustavo

Re: [PATCH 6/8] PCI: dwc: Replace lower into upper case characters

[Fabio Estevam]

On Wed, Mar 28, 2018 at 10:00 AM, Gustavo Pimentel
<gustavo.pimentel@synopsys.com> wrote:

> Yes, it's not a replace character from lower to upper case. It's just a code
> formatting style to be coherent with the rest of the driver code. I guess I can
> added on the patch description a note about it.
> It works for you?

Yes, that would be better, thanks.

[PATCH 7/8] PCI: dwc: Small computation improvement

[Gustavo Pimentel]

Replaces a simple division by 2 to a right shiftrotation of 1 bit.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/pcie-designware-host.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 03e9b82..8e6fed4 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
-		pp->cfg0_size = resource_size(cfg_res) / 2;
-		pp->cfg1_size = resource_size(cfg_res) / 2;
+		pp->cfg0_size = resource_size(cfg_res) >> 1;
+		pp->cfg1_size = resource_size(cfg_res) >> 1;
 		pp->cfg0_base = cfg_res->start;
 		pp->cfg1_base = cfg_res->start + pp->cfg0_size;
 	} else if (!pp->va_cfg0_base) {
@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 			break;
 		case 0:
 			pp->cfg = win->res;
-			pp->cfg0_size = resource_size(pp->cfg) / 2;
-			pp->cfg1_size = resource_size(pp->cfg) / 2;
+			pp->cfg0_size = resource_size(pp->cfg) >> 1;
+			pp->cfg1_size = resource_size(pp->cfg) >> 1;
 			pp->cfg0_base = pp->cfg->start;
 			pp->cfg1_base = pp->cfg->start + pp->cfg0_size;
 			break;
-- 
2.7.4

[PATCH 8/8] PCI: dwc: Replace magic number by defines

[Gustavo Pimentel]

Replace magic numbers by a well known define in order to make the code
human readable and also facilitate the code reusability.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/dwc/pcie-designware-host.c | 34 ++++++++++++++++++++--------------
 drivers/pci/dwc/pcie-designware.h      |  1 +
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 8e6fed4..ff43b0f 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -83,18 +83,23 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
 	for (i = 0; i < num_ctrls; i++) {
-		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				    &val);
+		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
+					(i * MSI_REG_CTRL_BLOCK_SIZE),
+				    4, &val);
 		if (!val)
 			continue;
 
 		ret = IRQ_HANDLED;
 		pos = 0;
-		while ((pos = find_next_bit((unsigned long *) &val, 32,
-					    pos)) != 32) {
-			irq = irq_find_mapping(pp->irq_domain, i * 32 + pos);
+		while ((pos = find_next_bit((unsigned long *) &val,
+					    MAX_MSI_IRQS_PER_CTRL,
+					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
+			irq = irq_find_mapping(pp->irq_domain,
+					       (i * MAX_MSI_IRQS_PER_CTRL) +
+					       pos);
 			generic_handle_irq(irq);
-			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12,
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS +
+						(i * MSI_REG_CTRL_BLOCK_SIZE),
 					    4, 1 << pos);
 			pos++;
 		}
@@ -157,9 +162,9 @@ static void dw_pci_bottom_mask(struct irq_data *data)
 	if (pp->ops->msi_clear_irq) {
 		pp->ops->msi_clear_irq(pp, data->hwirq);
 	} else {
-		ctrl = data->hwirq / 32;
-		res = ctrl * 12;
-		bit = data->hwirq % 32;
+		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
 		pp->irq_status[ctrl] &= ~(1 << bit);
 		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -180,9 +185,9 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
 	if (pp->ops->msi_set_irq) {
 		pp->ops->msi_set_irq(pp, data->hwirq);
 	} else {
-		ctrl = data->hwirq / 32;
-		res = ctrl * 12;
-		bit = data->hwirq % 32;
+		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
 		pp->irq_status[ctrl] |= 1 << bit;
 		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -652,8 +657,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	/* Initialize IRQ Status array */
 	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
-		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + (ctrl * 12), 4,
-				    &pp->irq_status[ctrl]);
+		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
+					(ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+				    4, &pp->irq_status[ctrl]);
 
 	/* Setup RC BARs */
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index fe811db..bee4e25 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -110,6 +110,7 @@
 #define MAX_MSI_IRQS			256
 #define MAX_MSI_IRQS_PER_CTRL		32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
+#define MSI_REG_CTRL_BLOCK_SIZE		12
 #define MSI_DEF_NUM_VECTORS		32
 
 /* Maximum number of inbound/outbound iATUs */
-- 
2.7.4