[PATCH v2 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[PATCH v2 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[Kishon Vijay Abraham I]

This series is a result of comments given by Rob Herring @ [1].
Patch series changes the DT bindings and makes the corresponding driver
changes.

[1] -> http://lore.kernel.org/r/20200219202700.GA21908@bogus

Changes from v1:
1) Added Reviewed-by: Rob Herring <robh@kernel.org> for dt-binding patch
2) Fixed nitpick comments from Bjorn Helgaas
3) Added a patch to read 32-bit Vendor ID/Device ID property from DT

Kishon Vijay Abraham I (4):
  dt-bindings: PCI: cadence: Deprecate inbound/outbound specific
    bindings
  PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits"
    property
  PCI: cadence: Remove "cdns,max-outbound-regions" DT property
  PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

 .../bindings/pci/cdns,cdns-pcie-ep.yaml       |  2 +-
 .../bindings/pci/cdns,cdns-pcie-host.yaml     |  3 +--
 .../devicetree/bindings/pci/cdns-pcie-ep.yaml | 25 +++++++++++++++++++
 .../bindings/pci/cdns-pcie-host.yaml          | 10 ++++++++
 .../devicetree/bindings/pci/cdns-pcie.yaml    |  8 ------
 .../controller/cadence/pcie-cadence-host.c    | 21 +++++++++-------
 drivers/pci/controller/cadence/pcie-cadence.h |  6 ++---
 7 files changed, 51 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: PCI: cadence: Deprecate inbound/outbound specific bindings

[Kishon Vijay Abraham I]

Deprecate cdns,max-outbound-regions and cdns,no-bar-match-nbits for
host mode as both these could be derived from "ranges" and "dma-ranges"
property. "cdns,max-outbound-regions" property would still be required
for EP mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/pci/cdns,cdns-pcie-ep.yaml       |  2 +-
 .../bindings/pci/cdns,cdns-pcie-host.yaml     |  3 +--
 .../devicetree/bindings/pci/cdns-pcie-ep.yaml | 25 +++++++++++++++++++
 .../bindings/pci/cdns-pcie-host.yaml          | 10 ++++++++
 .../devicetree/bindings/pci/cdns-pcie.yaml    |  8 ------
 5 files changed, 37 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
index 2996f8d4777c..50ce5d79d2c7 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
@@ -10,7 +10,7 @@ maintainers:
   - Tom Joseph <tjoseph@cadence.com>
 
 allOf:
-  - $ref: "cdns-pcie.yaml#"
+  - $ref: "cdns-pcie-ep.yaml#"
   - $ref: "pci-ep.yaml#"
 
 properties:
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
index cabbe46ff578..84a8f095d031 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
@@ -45,8 +45,6 @@ examples:
             #size-cells = <2>;
             bus-range = <0x0 0xff>;
             linux,pci-domain = <0>;
-            cdns,max-outbound-regions = <16>;
-            cdns,no-bar-match-nbits = <32>;
             vendor-id = <0x17cd>;
             device-id = <0x0200>;
 
@@ -57,6 +55,7 @@ examples:
 
             ranges = <0x02000000 0x0 0x42000000  0x0 0x42000000  0x0 0x1000000>,
                      <0x01000000 0x0 0x43000000  0x0 0x43000000  0x0 0x0010000>;
+            dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x1 0x00000000>;
 
             #interrupt-cells = <0x1>;
 
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
new file mode 100644
index 000000000000..6150a7a7bdbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/cdns-pcie-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Cadence PCIe Device
+
+maintainers:
+  - Tom Joseph <tjoseph@cadence.com>
+
+allOf:
+  - $ref: "cdns-pcie.yaml#"
+
+properties:
+  cdns,max-outbound-regions:
+    description: maximum number of outbound regions
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+    default: 32
+
+required:
+  - cdns,max-outbound-regions
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
index ab6e43b636ec..3d64f85aeb39 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
@@ -14,6 +14,15 @@ allOf:
   - $ref: "cdns-pcie.yaml#"
 
 properties:
+  cdns,max-outbound-regions:
+    description: maximum number of outbound regions
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+    default: 32
+    deprecated: true
+
   cdns,no-bar-match-nbits:
     description:
       Set into the no BAR match register to configure the number of least
@@ -23,5 +32,6 @@ properties:
     minimum: 0
     maximum: 64
     default: 32
+    deprecated: true
 
   msi-parent: true
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
index 6887ccc339cc..02553d5e6c51 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie.yaml
@@ -10,14 +10,6 @@ maintainers:
   - Tom Joseph <tjoseph@cadence.com>
 
 properties:
-  cdns,max-outbound-regions:
-    description: maximum number of outbound regions
-    allOf:
-      - $ref: /schemas/types.yaml#/definitions/uint32
-    minimum: 1
-    maximum: 32
-    default: 32
-
   phys:
     description:
       One per lane if more than one in the list. If only one PHY listed it must
-- 
2.17.1

[PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
property to configure the number of bits passed through from PCIe
address to internal address in Inbound Address Translation register.

However standard PCI dt-binding already defines "dma-ranges" to
describe the address range accessible by PCIe controller. Parse
"dma-ranges" property to configure the number of bits passed
through from PCIe address to internal address in Inbound Address
Translation register.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 9b1c3966414b..60f912a657b9 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	struct device *dev = rc->pcie.dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
+	struct of_pci_range_parser parser;
 	struct pci_host_bridge *bridge;
 	struct list_head resources;
+	struct of_pci_range range;
 	struct cdns_pcie *pcie;
 	struct resource *res;
 	int ret;
@@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	rc->max_regions = 32;
 	of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
 
-	rc->no_bar_nbits = 32;
-	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
+	if (!of_pci_dma_range_parser_init(&parser, np))
+		if (of_pci_range_parser_one(&parser, &range))
+			rc->no_bar_nbits = ilog2(range.size);
+
+	if (!rc->no_bar_nbits) {
+		rc->no_bar_nbits = 32;
+		of_property_read_u32(np, "cdns,no-bar-match-nbits",
+				     &rc->no_bar_nbits);
+	}
 
 	rc->vendor_id = 0xffff;
 	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
-- 
2.17.1

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Lorenzo Pieralisi]

[+Robin - to check on dma-ranges intepretation]

I would need RobH and Robin to review this.

Also, An ACK from Tom is required - for the whole series.

On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
> property to configure the number of bits passed through from PCIe
> address to internal address in Inbound Address Translation register.
> 
> However standard PCI dt-binding already defines "dma-ranges" to
> describe the address range accessible by PCIe controller. Parse
> "dma-ranges" property to configure the number of bits passed
> through from PCIe address to internal address in Inbound Address
> Translation register.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 9b1c3966414b..60f912a657b9 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	struct device *dev = rc->pcie.dev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct device_node *np = dev->of_node;
> +	struct of_pci_range_parser parser;
>  	struct pci_host_bridge *bridge;
>  	struct list_head resources;
> +	struct of_pci_range range;
>  	struct cdns_pcie *pcie;
>  	struct resource *res;
>  	int ret;
> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	rc->max_regions = 32;
>  	of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
>  
> -	rc->no_bar_nbits = 32;
> -	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
> +	if (!of_pci_dma_range_parser_init(&parser, np))
> +		if (of_pci_range_parser_one(&parser, &range))
> +			rc->no_bar_nbits = ilog2(range.size);
> +
> +	if (!rc->no_bar_nbits) {
> +		rc->no_bar_nbits = 32;
> +		of_property_read_u32(np, "cdns,no-bar-match-nbits",
> +				     &rc->no_bar_nbits);
> +	}
>  
>  	rc->vendor_id = 0xffff;
>  	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Robin Murphy]

On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
> [+Robin - to check on dma-ranges intepretation]
> 
> I would need RobH and Robin to review this.
> 
> Also, An ACK from Tom is required - for the whole series.
> 
> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>> property to configure the number of bits passed through from PCIe
>> address to internal address in Inbound Address Translation register.
>>
>> However standard PCI dt-binding already defines "dma-ranges" to
>> describe the address range accessible by PCIe controller. Parse
>> "dma-ranges" property to configure the number of bits passed
>> through from PCIe address to internal address in Inbound Address
>> Translation register.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>   drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 9b1c3966414b..60f912a657b9 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>   	struct device *dev = rc->pcie.dev;
>>   	struct platform_device *pdev = to_platform_device(dev);
>>   	struct device_node *np = dev->of_node;
>> +	struct of_pci_range_parser parser;
>>   	struct pci_host_bridge *bridge;
>>   	struct list_head resources;
>> +	struct of_pci_range range;
>>   	struct cdns_pcie *pcie;
>>   	struct resource *res;
>>   	int ret;
>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>   	rc->max_regions = 32;
>>   	of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
>>   
>> -	rc->no_bar_nbits = 32;
>> -	of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>> +	if (!of_pci_dma_range_parser_init(&parser, np))
>> +		if (of_pci_range_parser_one(&parser, &range))
>> +			rc->no_bar_nbits = ilog2(range.size);

You probably want "range.pci_addr + range.size" here just in case the 
bottom of the window is ever non-zero. Is there definitely only ever a 
single inbound window to consider?

I believe that pci_parse_request_of_pci_ranges() could do the actual 
parsing for you, but I suppose plumbing that in plus processing the 
resulting dma_ranges resource probably ends up a bit messier than the 
concise open-coding here.

Robin.

>> +
>> +	if (!rc->no_bar_nbits) {
>> +		rc->no_bar_nbits = 32;
>> +		of_property_read_u32(np, "cdns,no-bar-match-nbits",
>> +				     &rc->no_bar_nbits);
>> +	}
>>   
>>   	rc->vendor_id = 0xffff;
>>   	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Robin,

On 5/1/2020 9:24 PM, Robin Murphy wrote:
> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>> [+Robin - to check on dma-ranges intepretation]
>>
>> I would need RobH and Robin to review this.
>>
>> Also, An ACK from Tom is required - for the whole series.
>>
>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>> property to configure the number of bits passed through from PCIe
>>> address to internal address in Inbound Address Translation register.
>>>
>>> However standard PCI dt-binding already defines "dma-ranges" to
>>> describe the address range accessible by PCIe controller. Parse
>>> "dma-ranges" property to configure the number of bits passed
>>> through from PCIe address to internal address in Inbound Address
>>> Translation register.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>   drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> index 9b1c3966414b..60f912a657b9 100644
>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>       struct device *dev = rc->pcie.dev;
>>>       struct platform_device *pdev = to_platform_device(dev);
>>>       struct device_node *np = dev->of_node;
>>> +    struct of_pci_range_parser parser;
>>>       struct pci_host_bridge *bridge;
>>>       struct list_head resources;
>>> +    struct of_pci_range range;
>>>       struct cdns_pcie *pcie;
>>>       struct resource *res;
>>>       int ret;
>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>       rc->max_regions = 32;
>>>       of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
>>>   -    rc->no_bar_nbits = 32;
>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>> +        if (of_pci_range_parser_one(&parser, &range))
>>> +            rc->no_bar_nbits = ilog2(range.size);
> 
> You probably want "range.pci_addr + range.size" here just in case the bottom of
> the window is ever non-zero. Is there definitely only ever a single inbound
> window to consider?

Cadence IP has 3 inbound address translation registers, however we use only 1
inbound address translation register to map the entire 32 bit or 64 bit address
region.
> 
> I believe that pci_parse_request_of_pci_ranges() could do the actual parsing
> for you, but I suppose plumbing that in plus processing the resulting
> dma_ranges resource probably ends up a bit messier than the concise open-coding
> here.

right, pci_parse_request_of_pci_ranges() parses "ranges" property and is used
for outbound configuration, whereas here we parse "dma-ranges" property and is
used for inbound configuration.

Thanks
Kishon

> 
> Robin.
> 
>>> +
>>> +    if (!rc->no_bar_nbits) {
>>> +        rc->no_bar_nbits = 32;
>>> +        of_property_read_u32(np, "cdns,no-bar-match-nbits",
>>> +                     &rc->no_bar_nbits);
>>> +    }
>>>         rc->vendor_id = 0xffff;
>>>       of_property_read_u16(np, "vendor-id", &rc->vendor_id);
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Robin Murphy]

On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
> Hi Robin,
> 
> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>> [+Robin - to check on dma-ranges intepretation]
>>>
>>> I would need RobH and Robin to review this.
>>>
>>> Also, An ACK from Tom is required - for the whole series.
>>>
>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>> property to configure the number of bits passed through from PCIe
>>>> address to internal address in Inbound Address Translation register.
>>>>
>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>> describe the address range accessible by PCIe controller. Parse
>>>> "dma-ranges" property to configure the number of bits passed
>>>> through from PCIe address to internal address in Inbound Address
>>>> Translation register.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> index 9b1c3966414b..60f912a657b9 100644
>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>        struct device *dev = rc->pcie.dev;
>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>        struct device_node *np = dev->of_node;
>>>> +    struct of_pci_range_parser parser;
>>>>        struct pci_host_bridge *bridge;
>>>>        struct list_head resources;
>>>> +    struct of_pci_range range;
>>>>        struct cdns_pcie *pcie;
>>>>        struct resource *res;
>>>>        int ret;
>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>        rc->max_regions = 32;
>>>>        of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
>>>>    -    rc->no_bar_nbits = 32;
>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>
>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>> the window is ever non-zero. Is there definitely only ever a single inbound
>> window to consider?
> 
> Cadence IP has 3 inbound address translation registers, however we use only 1
> inbound address translation register to map the entire 32 bit or 64 bit address
> region.

OK, if anything that further strengthens the argument for deprecating a 
single "number of bits" property in favour of ranges that accurately 
describe the window(s). However it also suggests that other users in 
future might have some expectation that specifying "dma-ranges" with up 
to 3 entries should work to allow a more restrictive inbound 
configuration. Thus it would be desirable to make the code a little more 
robust here - even if we don't support multiple windows straight off, it 
would still be better to implement it in a way that can be cleanly 
extended later, and at least say something if more ranges are specified 
rather than just silently ignoring them.

>> I believe that pci_parse_request_of_pci_ranges() could do the actual parsing
>> for you, but I suppose plumbing that in plus processing the resulting
>> dma_ranges resource probably ends up a bit messier than the concise open-coding
>> here.
> 
> right, pci_parse_request_of_pci_ranges() parses "ranges" property and is used
> for outbound configuration, whereas here we parse "dma-ranges" property and is
> used for inbound configuration.

If you give it a valid third argument it *also* parses "dma-ranges" into 
a list of inbound regions. This is already used by various other drivers 
for equivalent inbound window setup, which is what I was hinting at 
before, but given the extensibility argument above I'm now going to 
actively suggest following that pattern for consistency.

Robin.

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Robin,

On 5/4/2020 4:24 PM, Robin Murphy wrote:
> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>> Hi Robin,
>>
>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>> [+Robin - to check on dma-ranges intepretation]
>>>>
>>>> I would need RobH and Robin to review this.
>>>>
>>>> Also, An ACK from Tom is required - for the whole series.
>>>>
>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>> property to configure the number of bits passed through from PCIe
>>>>> address to internal address in Inbound Address Translation register.
>>>>>
>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>> describe the address range accessible by PCIe controller. Parse
>>>>> "dma-ranges" property to configure the number of bits passed
>>>>> through from PCIe address to internal address in Inbound Address
>>>>> Translation register.
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>        struct device *dev = rc->pcie.dev;
>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>        struct device_node *np = dev->of_node;
>>>>> +    struct of_pci_range_parser parser;
>>>>>        struct pci_host_bridge *bridge;
>>>>>        struct list_head resources;
>>>>> +    struct of_pci_range range;
>>>>>        struct cdns_pcie *pcie;
>>>>>        struct resource *res;
>>>>>        int ret;
>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>        rc->max_regions = 32;
>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>> &rc->max_regions);
>>>>>    -    rc->no_bar_nbits = 32;
>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>
>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>> window to consider?
>>
>> Cadence IP has 3 inbound address translation registers, however we use only 1
>> inbound address translation register to map the entire 32 bit or 64 bit address
>> region.
> 
> OK, if anything that further strengthens the argument for deprecating a single
> "number of bits" property in favour of ranges that accurately describe the
> window(s). However it also suggests that other users in future might have some
> expectation that specifying "dma-ranges" with up to 3 entries should work to
> allow a more restrictive inbound configuration. Thus it would be desirable to
> make the code a little more robust here - even if we don't support multiple
> windows straight off, it would still be better to implement it in a way that
> can be cleanly extended later, and at least say something if more ranges are
> specified rather than just silently ignoring them.

I looked at this further in the Cadence user doc. The three inbound ATU entries
are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
BAR when there is no matching found in RC BARs. Right now we always configure
the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
> 
>>> I believe that pci_parse_request_of_pci_ranges() could do the actual parsing
>>> for you, but I suppose plumbing that in plus processing the resulting
>>> dma_ranges resource probably ends up a bit messier than the concise open-coding
>>> here.
>>
>> right, pci_parse_request_of_pci_ranges() parses "ranges" property and is used
>> for outbound configuration, whereas here we parse "dma-ranges" property and is
>> used for inbound configuration.
> 
> If you give it a valid third argument it *also* parses "dma-ranges" into a list
> of inbound regions. This is already used by various other drivers for
> equivalent inbound window setup, which is what I was hinting at before, but
> given the extensibility argument above I'm now going to actively suggest
> following that pattern for consistency.
yeah, just got to know about this.

Thanks
Kishon

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Robin,

On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
> Hi Robin,
> 
> On 5/4/2020 4:24 PM, Robin Murphy wrote:
>> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>>> Hi Robin,
>>>
>>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>>> [+Robin - to check on dma-ranges intepretation]
>>>>>
>>>>> I would need RobH and Robin to review this.
>>>>>
>>>>> Also, An ACK from Tom is required - for the whole series.
>>>>>
>>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>>> property to configure the number of bits passed through from PCIe
>>>>>> address to internal address in Inbound Address Translation register.
>>>>>>
>>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>>> describe the address range accessible by PCIe controller. Parse
>>>>>> "dma-ranges" property to configure the number of bits passed
>>>>>> through from PCIe address to internal address in Inbound Address
>>>>>> Translation register.
>>>>>>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> ---
>>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>        struct device *dev = rc->pcie.dev;
>>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>>        struct device_node *np = dev->of_node;
>>>>>> +    struct of_pci_range_parser parser;
>>>>>>        struct pci_host_bridge *bridge;
>>>>>>        struct list_head resources;
>>>>>> +    struct of_pci_range range;
>>>>>>        struct cdns_pcie *pcie;
>>>>>>        struct resource *res;
>>>>>>        int ret;
>>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>        rc->max_regions = 32;
>>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>>> &rc->max_regions);
>>>>>>    -    rc->no_bar_nbits = 32;
>>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>>
>>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>>> window to consider?
>>>
>>> Cadence IP has 3 inbound address translation registers, however we use only 1
>>> inbound address translation register to map the entire 32 bit or 64 bit address
>>> region.
>>
>> OK, if anything that further strengthens the argument for deprecating a single
>> "number of bits" property in favour of ranges that accurately describe the
>> window(s). However it also suggests that other users in future might have some
>> expectation that specifying "dma-ranges" with up to 3 entries should work to
>> allow a more restrictive inbound configuration. Thus it would be desirable to
>> make the code a little more robust here - even if we don't support multiple
>> windows straight off, it would still be better to implement it in a way that
>> can be cleanly extended later, and at least say something if more ranges are
>> specified rather than just silently ignoring them.
> 
> I looked at this further in the Cadence user doc. The three inbound ATU entries
> are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
> BAR when there is no matching found in RC BARs. Right now we always configure
> the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?

I was thinking if I could use something like
dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
	     <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
	     <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR

This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
doesn't have valid values as we use only the NO MATCH BAR.

However I'm not able to use for_each_of_pci_range() in Cadence driver to get
the configuration for each BAR, since the for loop gets invoked only once since
of_pci_range_parser_one() merges contiguous addresses.

Do you think I should extend the flags cell to differentiate between BAR0, BAR1
and NO MATCH BAR? Can you suggest any other alternatives?

Thanks
Kishon

>>
>>>> I believe that pci_parse_request_of_pci_ranges() could do the actual parsing
>>>> for you, but I suppose plumbing that in plus processing the resulting
>>>> dma_ranges resource probably ends up a bit messier than the concise open-coding
>>>> here.
>>>
>>> right, pci_parse_request_of_pci_ranges() parses "ranges" property and is used
>>> for outbound configuration, whereas here we parse "dma-ranges" property and is
>>> used for inbound configuration.
>>
>> If you give it a valid third argument it *also* parses "dma-ranges" into a list
>> of inbound regions. This is already used by various other drivers for
>> equivalent inbound window setup, which is what I was hinting at before, but
>> given the extensibility argument above I'm now going to actively suggest
>> following that pattern for consistency.
> yeah, just got to know about this.
> 
> Thanks
> Kishon
> 

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Rob Herring]

On Wed, May 06, 2020 at 08:52:13AM +0530, Kishon Vijay Abraham I wrote:
> Hi Robin,
> 
> On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
> > Hi Robin,
> > 
> > On 5/4/2020 4:24 PM, Robin Murphy wrote:
> >> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
> >>> Hi Robin,
> >>>
> >>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
> >>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
> >>>>> [+Robin - to check on dma-ranges intepretation]
> >>>>>
> >>>>> I would need RobH and Robin to review this.
> >>>>>
> >>>>> Also, An ACK from Tom is required - for the whole series.
> >>>>>
> >>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
> >>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
> >>>>>> property to configure the number of bits passed through from PCIe
> >>>>>> address to internal address in Inbound Address Translation register.
> >>>>>>
> >>>>>> However standard PCI dt-binding already defines "dma-ranges" to
> >>>>>> describe the address range accessible by PCIe controller. Parse
> >>>>>> "dma-ranges" property to configure the number of bits passed
> >>>>>> through from PCIe address to internal address in Inbound Address
> >>>>>> Translation register.
> >>>>>>
> >>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>>> ---
> >>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
> >>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>>>> index 9b1c3966414b..60f912a657b9 100644
> >>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> >>>>>>        struct device *dev = rc->pcie.dev;
> >>>>>>        struct platform_device *pdev = to_platform_device(dev);
> >>>>>>        struct device_node *np = dev->of_node;
> >>>>>> +    struct of_pci_range_parser parser;
> >>>>>>        struct pci_host_bridge *bridge;
> >>>>>>        struct list_head resources;
> >>>>>> +    struct of_pci_range range;
> >>>>>>        struct cdns_pcie *pcie;
> >>>>>>        struct resource *res;
> >>>>>>        int ret;
> >>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> >>>>>>        rc->max_regions = 32;
> >>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
> >>>>>> &rc->max_regions);
> >>>>>>    -    rc->no_bar_nbits = 32;
> >>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
> >>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
> >>>>>> +        if (of_pci_range_parser_one(&parser, &range))
> >>>>>> +            rc->no_bar_nbits = ilog2(range.size);
> >>>>
> >>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
> >>>> the window is ever non-zero. Is there definitely only ever a single inbound
> >>>> window to consider?
> >>>
> >>> Cadence IP has 3 inbound address translation registers, however we use only 1
> >>> inbound address translation register to map the entire 32 bit or 64 bit address
> >>> region.
> >>
> >> OK, if anything that further strengthens the argument for deprecating a single
> >> "number of bits" property in favour of ranges that accurately describe the
> >> window(s). However it also suggests that other users in future might have some
> >> expectation that specifying "dma-ranges" with up to 3 entries should work to
> >> allow a more restrictive inbound configuration. Thus it would be desirable to
> >> make the code a little more robust here - even if we don't support multiple
> >> windows straight off, it would still be better to implement it in a way that
> >> can be cleanly extended later, and at least say something if more ranges are
> >> specified rather than just silently ignoring them.
> > 
> > I looked at this further in the Cadence user doc. The three inbound ATU entries
> > are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
> > BAR when there is no matching found in RC BARs. Right now we always configure
> > the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
> 
> I was thinking if I could use something like
> dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
> 	     <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
> 	     <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR
> 
> This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
> last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
> doesn't have valid values as we use only the NO MATCH BAR.
> 
> However I'm not able to use for_each_of_pci_range() in Cadence driver to get
> the configuration for each BAR, since the for loop gets invoked only once since
> of_pci_range_parser_one() merges contiguous addresses.

NO_MATCH_BAR could just be the last entry no matter how many? Who cares 
if they get merged? Maybe each BAR has max size and dma-ranges could 
exceed that, but if so you have to handle that and split them again.

> Do you think I should extend the flags cell to differentiate between BAR0, BAR1
> and NO MATCH BAR? Can you suggest any other alternatives?

If you just have 1 region, then just 1 entry makes sense to me. Why 
can't you use BAR0 in that case?

Rob

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Rob,

On 5/8/2020 1:56 AM, Rob Herring wrote:
> On Wed, May 06, 2020 at 08:52:13AM +0530, Kishon Vijay Abraham I wrote:
>> Hi Robin,
>>
>> On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
>>> Hi Robin,
>>>
>>> On 5/4/2020 4:24 PM, Robin Murphy wrote:
>>>> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>>>>> Hi Robin,
>>>>>
>>>>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>>>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>>>>> [+Robin - to check on dma-ranges intepretation]
>>>>>>>
>>>>>>> I would need RobH and Robin to review this.
>>>>>>>
>>>>>>> Also, An ACK from Tom is required - for the whole series.
>>>>>>>
>>>>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>>>>> property to configure the number of bits passed through from PCIe
>>>>>>>> address to internal address in Inbound Address Translation register.
>>>>>>>>
>>>>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>>>>> describe the address range accessible by PCIe controller. Parse
>>>>>>>> "dma-ranges" property to configure the number of bits passed
>>>>>>>> through from PCIe address to internal address in Inbound Address
>>>>>>>> Translation register.
>>>>>>>>
>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>> ---
>>>>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>        struct device *dev = rc->pcie.dev;
>>>>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>>>>        struct device_node *np = dev->of_node;
>>>>>>>> +    struct of_pci_range_parser parser;
>>>>>>>>        struct pci_host_bridge *bridge;
>>>>>>>>        struct list_head resources;
>>>>>>>> +    struct of_pci_range range;
>>>>>>>>        struct cdns_pcie *pcie;
>>>>>>>>        struct resource *res;
>>>>>>>>        int ret;
>>>>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>        rc->max_regions = 32;
>>>>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>>>>> &rc->max_regions);
>>>>>>>>    -    rc->no_bar_nbits = 32;
>>>>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>>>>
>>>>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>>>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>>>>> window to consider?
>>>>>
>>>>> Cadence IP has 3 inbound address translation registers, however we use only 1
>>>>> inbound address translation register to map the entire 32 bit or 64 bit address
>>>>> region.
>>>>
>>>> OK, if anything that further strengthens the argument for deprecating a single
>>>> "number of bits" property in favour of ranges that accurately describe the
>>>> window(s). However it also suggests that other users in future might have some
>>>> expectation that specifying "dma-ranges" with up to 3 entries should work to
>>>> allow a more restrictive inbound configuration. Thus it would be desirable to
>>>> make the code a little more robust here - even if we don't support multiple
>>>> windows straight off, it would still be better to implement it in a way that
>>>> can be cleanly extended later, and at least say something if more ranges are
>>>> specified rather than just silently ignoring them.
>>>
>>> I looked at this further in the Cadence user doc. The three inbound ATU entries
>>> are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
>>> BAR when there is no matching found in RC BARs. Right now we always configure
>>> the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
>>
>> I was thinking if I could use something like
>> dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
>> 	     <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
>> 	     <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR
>>
>> This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
>> last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
>> doesn't have valid values as we use only the NO MATCH BAR.
>>
>> However I'm not able to use for_each_of_pci_range() in Cadence driver to get
>> the configuration for each BAR, since the for loop gets invoked only once since
>> of_pci_range_parser_one() merges contiguous addresses.
> 
> NO_MATCH_BAR could just be the last entry no matter how many? Who cares 
> if they get merged? Maybe each BAR has max size and dma-ranges could 
> exceed that, but if so you have to handle that and split them again.

Each of RP_BAR0, RP_BAR1 and RP_NO_BAR has separate register to be configured.
If they get merged, we'll loose info on which of the registers to be
configured. Cadence IP specifies maximum size of BAR0 as 256GB, maximum size of
BAR1 as 2 GB. However when I specify dma-ranges like below and use
for_each_of_pci_range(&parser, &range), the first range itself is 258.

dma-ranges = <0x02000000 0x00 0x0 0x00 0x0 0x40 0x00000000>, /* BAR0 256 GB */
	     <0x02000000 0x40 0x0 0x40 0x0 0x00 0x80000000>; /* BAR1 2 GB */
> 
>> Do you think I should extend the flags cell to differentiate between BAR0, BAR1
>> and NO MATCH BAR? Can you suggest any other alternatives?
> 
> If you just have 1 region, then just 1 entry makes sense to me. Why 
> can't you use BAR0 in that case?

Well, Cadence has specified a max size for each BAR. I think we could specify a
single region (48 bits in my case) in dma-ranges and let the driver decide how
to split it among BAR0, BAR1 and NO_MATCH_BAR?

Thanks
Kishon

Re: [PATCH v2 2/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property

[Kishon Vijay Abraham I]

Hi Rob, Robin,

On 5/8/2020 2:19 PM, Kishon Vijay Abraham I wrote:
> Hi Rob,
> 
> On 5/8/2020 1:56 AM, Rob Herring wrote:
>> On Wed, May 06, 2020 at 08:52:13AM +0530, Kishon Vijay Abraham I wrote:
>>> Hi Robin,
>>>
>>> On 5/4/2020 6:23 PM, Kishon Vijay Abraham I wrote:
>>>> Hi Robin,
>>>>
>>>> On 5/4/2020 4:24 PM, Robin Murphy wrote:
>>>>> On 2020-05-04 9:44 am, Kishon Vijay Abraham I wrote:
>>>>>> Hi Robin,
>>>>>>
>>>>>> On 5/1/2020 9:24 PM, Robin Murphy wrote:
>>>>>>> On 2020-05-01 3:46 pm, Lorenzo Pieralisi wrote:
>>>>>>>> [+Robin - to check on dma-ranges intepretation]
>>>>>>>>
>>>>>>>> I would need RobH and Robin to review this.
>>>>>>>>
>>>>>>>> Also, An ACK from Tom is required - for the whole series.
>>>>>>>>
>>>>>>>> On Fri, Apr 17, 2020 at 05:13:20PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>>>>>>>>> property to configure the number of bits passed through from PCIe
>>>>>>>>> address to internal address in Inbound Address Translation register.
>>>>>>>>>
>>>>>>>>> However standard PCI dt-binding already defines "dma-ranges" to
>>>>>>>>> describe the address range accessible by PCIe controller. Parse
>>>>>>>>> "dma-ranges" property to configure the number of bits passed
>>>>>>>>> through from PCIe address to internal address in Inbound Address
>>>>>>>>> Translation register.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>> ---
>>>>>>>>>    drivers/pci/controller/cadence/pcie-cadence-host.c | 13 +++++++++++--
>>>>>>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> index 9b1c3966414b..60f912a657b9 100644
>>>>>>>>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>>>>>>>>> @@ -206,8 +206,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>>        struct device *dev = rc->pcie.dev;
>>>>>>>>>        struct platform_device *pdev = to_platform_device(dev);
>>>>>>>>>        struct device_node *np = dev->of_node;
>>>>>>>>> +    struct of_pci_range_parser parser;
>>>>>>>>>        struct pci_host_bridge *bridge;
>>>>>>>>>        struct list_head resources;
>>>>>>>>> +    struct of_pci_range range;
>>>>>>>>>        struct cdns_pcie *pcie;
>>>>>>>>>        struct resource *res;
>>>>>>>>>        int ret;
>>>>>>>>> @@ -222,8 +224,15 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>>>>>>>>        rc->max_regions = 32;
>>>>>>>>>        of_property_read_u32(np, "cdns,max-outbound-regions",
>>>>>>>>> &rc->max_regions);
>>>>>>>>>    -    rc->no_bar_nbits = 32;
>>>>>>>>> -    of_property_read_u32(np, "cdns,no-bar-match-nbits", &rc->no_bar_nbits);
>>>>>>>>> +    if (!of_pci_dma_range_parser_init(&parser, np))
>>>>>>>>> +        if (of_pci_range_parser_one(&parser, &range))
>>>>>>>>> +            rc->no_bar_nbits = ilog2(range.size);
>>>>>>>
>>>>>>> You probably want "range.pci_addr + range.size" here just in case the bottom of
>>>>>>> the window is ever non-zero. Is there definitely only ever a single inbound
>>>>>>> window to consider?
>>>>>>
>>>>>> Cadence IP has 3 inbound address translation registers, however we use only 1
>>>>>> inbound address translation register to map the entire 32 bit or 64 bit address
>>>>>> region.
>>>>>
>>>>> OK, if anything that further strengthens the argument for deprecating a single
>>>>> "number of bits" property in favour of ranges that accurately describe the
>>>>> window(s). However it also suggests that other users in future might have some
>>>>> expectation that specifying "dma-ranges" with up to 3 entries should work to
>>>>> allow a more restrictive inbound configuration. Thus it would be desirable to
>>>>> make the code a little more robust here - even if we don't support multiple
>>>>> windows straight off, it would still be better to implement it in a way that
>>>>> can be cleanly extended later, and at least say something if more ranges are
>>>>> specified rather than just silently ignoring them.
>>>>
>>>> I looked at this further in the Cadence user doc. The three inbound ATU entries
>>>> are for BAR0, BAR1 in RC configuration space and the third one is for NO MATCH
>>>> BAR when there is no matching found in RC BARs. Right now we always configure
>>>> the NO MATCH BAR. Would it be possible describe at BAR granularity in dma-ranges?
>>>
>>> I was thinking if I could use something like
>>> dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR0 IB mapping
>>> 	     <0x02000000 0x0 0x0 0x0 0x0 0x00000 0x0>, //For BAR1 IB mapping
>>> 	     <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>; //NO MATCH BAR
>>>
>>> This way the driver can tell the 1st tuple is for BAR0, 2nd is for BAR1 and
>>> last is for NO MATCH. In the above case both BAR0 and BAR1 is just empty and
>>> doesn't have valid values as we use only the NO MATCH BAR.
>>>
>>> However I'm not able to use for_each_of_pci_range() in Cadence driver to get
>>> the configuration for each BAR, since the for loop gets invoked only once since
>>> of_pci_range_parser_one() merges contiguous addresses.
>>
>> NO_MATCH_BAR could just be the last entry no matter how many? Who cares 
>> if they get merged? Maybe each BAR has max size and dma-ranges could 
>> exceed that, but if so you have to handle that and split them again.
> 
> Each of RP_BAR0, RP_BAR1 and RP_NO_BAR has separate register to be configured.
> If they get merged, we'll loose info on which of the registers to be
> configured. Cadence IP specifies maximum size of BAR0 as 256GB, maximum size of
> BAR1 as 2 GB. However when I specify dma-ranges like below and use
> for_each_of_pci_range(&parser, &range), the first range itself is 258.
> 
> dma-ranges = <0x02000000 0x00 0x0 0x00 0x0 0x40 0x00000000>, /* BAR0 256 GB */
> 	     <0x02000000 0x40 0x0 0x40 0x0 0x00 0x80000000>; /* BAR1 2 GB */
>>
>>> Do you think I should extend the flags cell to differentiate between BAR0, BAR1
>>> and NO MATCH BAR? Can you suggest any other alternatives?
>>
>> If you just have 1 region, then just 1 entry makes sense to me. Why 
>> can't you use BAR0 in that case?
> 
> Well, Cadence has specified a max size for each BAR. I think we could specify a
> single region (48 bits in my case) in dma-ranges and let the driver decide how
> to split it among BAR0, BAR1 and NO_MATCH_BAR?

Okay, I'll add support in driver for parsing multiple dma-ranges (non
consecutive regions) and driver splitting the regions based on maximum size
supported by each BAR.

This means, we will not directly use NO_MATCH_BAR, but wil first fill up BAR0,
BAR1 and then only the remaining space in NO_MATCH_BAR.

Thanks
Kishon

[PATCH v2 3/4] PCI: cadence: Remove "cdns,max-outbound-regions" DT property

[Kishon Vijay Abraham I]

"cdns,max-outbound-regions" device tree property provides the
maximum number of outbound regions supported by the Host PCIe
controller. However the outbound regions are configured based
on what is populated in the "ranges" DT property.

Avoid using two properties for configuring outbound regions and
use only "ranges" property instead.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 6 ------
 drivers/pci/controller/cadence/pcie-cadence.h      | 2 --
 2 files changed, 8 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 60f912a657b9..8f72967f298f 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -140,9 +140,6 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 	for_each_of_pci_range(&parser, &range) {
 		bool is_io;
 
-		if (r >= rc->max_regions)
-			break;
-
 		if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM)
 			is_io = false;
 		else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
@@ -221,9 +218,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	pcie = &rc->pcie;
 	pcie->is_rc = true;
 
-	rc->max_regions = 32;
-	of_property_read_u32(np, "cdns,max-outbound-regions", &rc->max_regions);
-
 	if (!of_pci_dma_range_parser_init(&parser, np))
 		if (of_pci_range_parser_one(&parser, &range))
 			rc->no_bar_nbits = ilog2(range.size);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index a2b28b912ca4..6bd89a21bb1c 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -251,7 +251,6 @@ struct cdns_pcie {
  * @bus_range: first/last buses behind the PCIe host controller
  * @cfg_base: IO mapped window to access the PCI configuration space of a
  *            single function at a time
- * @max_regions: maximum number of regions supported by the hardware
  * @no_bar_nbits: Number of bits to keep for inbound (PCIe -> CPU) address
  *                translation (nbits sets into the "no BAR match" register)
  * @vendor_id: PCI vendor ID
@@ -262,7 +261,6 @@ struct cdns_pcie_rc {
 	struct resource		*cfg_res;
 	struct resource		*bus_range;
 	void __iomem		*cfg_base;
-	u32			max_regions;
 	u32			no_bar_nbits;
 	u16			vendor_id;
 	u16			device_id;
-- 
2.17.1

Re: [PATCH v2 3/4] PCI: cadence: Remove "cdns,max-outbound-regions" DT property

[Rob Herring]

On Fri, 17 Apr 2020 17:13:21 +0530, Kishon Vijay Abraham I wrote:
> "cdns,max-outbound-regions" device tree property provides the
> maximum number of outbound regions supported by the Host PCIe
> controller. However the outbound regions are configured based
> on what is populated in the "ranges" DT property.
> 
> Avoid using two properties for configuring outbound regions and
> use only "ranges" property instead.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 6 ------
>  drivers/pci/controller/cadence/pcie-cadence.h      | 2 --
>  2 files changed, 8 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 4/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Kishon Vijay Abraham I]

The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
defines both Vendor ID and Device ID to be 32-bits. Fix
pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
properties from device tree.

[1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
 drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 8f72967f298f..31e67c9c88cf 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -229,10 +229,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	}
 
 	rc->vendor_id = 0xffff;
-	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
+	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
 
 	rc->device_id = 0xffff;
-	of_property_read_u16(np, "device-id", &rc->device_id);
+	of_property_read_u32(np, "device-id", &rc->device_id);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
 	pcie->reg_base = devm_ioremap_resource(dev, res);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 6bd89a21bb1c..df14ad002fe9 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -262,8 +262,8 @@ struct cdns_pcie_rc {
 	struct resource		*bus_range;
 	void __iomem		*cfg_base;
 	u32			no_bar_nbits;
-	u16			vendor_id;
-	u16			device_id;
+	u32			vendor_id;
+	u32			device_id;
 };
 
 /**
-- 
2.17.1

Re: [PATCH v2 4/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Lorenzo Pieralisi]

On Fri, Apr 17, 2020 at 05:13:22PM +0530, Kishon Vijay Abraham I wrote:
> The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
> defines both Vendor ID and Device ID to be 32-bits. Fix
> pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
> properties from device tree.
> 
> [1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
>  drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

I don't see how you would use a 32-bit value for a 16-bit register so
certainly the struct cdns_pcie_rc fields size is questionable anyway.

I *assume* you are referring to 4.1.2.1 and the property list
encoded as "encode-int".

I would like to get RobH's opinion on this - I don't know myself
whether the PCI OF bindings you added are still relevant and how
they should be interpreted.

Thanks
Lorenzo

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 8f72967f298f..31e67c9c88cf 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -229,10 +229,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	}
>  
>  	rc->vendor_id = 0xffff;
> -	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
> +	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
>  
>  	rc->device_id = 0xffff;
> -	of_property_read_u16(np, "device-id", &rc->device_id);
> +	of_property_read_u32(np, "device-id", &rc->device_id);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
>  	pcie->reg_base = devm_ioremap_resource(dev, res);
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 6bd89a21bb1c..df14ad002fe9 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -262,8 +262,8 @@ struct cdns_pcie_rc {
>  	struct resource		*bus_range;
>  	void __iomem		*cfg_base;
>  	u32			no_bar_nbits;
> -	u16			vendor_id;
> -	u16			device_id;
> +	u32			vendor_id;
> +	u32			device_id;
>  };
>  
>  /**
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 5/1/2020 8:41 PM, Lorenzo Pieralisi wrote:
> On Fri, Apr 17, 2020 at 05:13:22PM +0530, Kishon Vijay Abraham I wrote:
>> The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
>> defines both Vendor ID and Device ID to be 32-bits. Fix
>> pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
>> properties from device tree.
>>
>> [1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
>>  drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> I don't see how you would use a 32-bit value for a 16-bit register so
> certainly the struct cdns_pcie_rc fields size is questionable anyway.
> 
> I *assume* you are referring to 4.1.2.1 and the property list
> encoded as "encode-int".
> 
> I would like to get RobH's opinion on this - I don't know myself
> whether the PCI OF bindings you added are still relevant and how
> they should be interpreted.

This change was made due to RobH's comment below [1]

[1] ->
https://lore.kernel.org/r/CAL_JsqLYScxGySy8xaN-UB6URfw8K_jSiuSXwVoTU9-RdJecww@mail.gmail.com/

Thanks
Kishon

> 
> Thanks
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 8f72967f298f..31e67c9c88cf 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -229,10 +229,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>  	}
>>  
>>  	rc->vendor_id = 0xffff;
>> -	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
>> +	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
>>  
>>  	rc->device_id = 0xffff;
>> -	of_property_read_u16(np, "device-id", &rc->device_id);
>> +	of_property_read_u32(np, "device-id", &rc->device_id);
>>  
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
>>  	pcie->reg_base = devm_ioremap_resource(dev, res);
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index 6bd89a21bb1c..df14ad002fe9 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -262,8 +262,8 @@ struct cdns_pcie_rc {
>>  	struct resource		*bus_range;
>>  	void __iomem		*cfg_base;
>>  	u32			no_bar_nbits;
>> -	u16			vendor_id;
>> -	u16			device_id;
>> +	u32			vendor_id;
>> +	u32			device_id;
>>  };
>>  
>>  /**
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 4/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Lorenzo Pieralisi]

On Mon, May 04, 2020 at 02:22:30PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 5/1/2020 8:41 PM, Lorenzo Pieralisi wrote:
> > On Fri, Apr 17, 2020 at 05:13:22PM +0530, Kishon Vijay Abraham I wrote:
> >> The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
> >> defines both Vendor ID and Device ID to be 32-bits. Fix
> >> pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
> >> properties from device tree.
> >>
> >> [1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
> >>  drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > I don't see how you would use a 32-bit value for a 16-bit register so
> > certainly the struct cdns_pcie_rc fields size is questionable anyway.
> > 
> > I *assume* you are referring to 4.1.2.1 and the property list
> > encoded as "encode-int".
> > 
> > I would like to get RobH's opinion on this - I don't know myself
> > whether the PCI OF bindings you added are still relevant and how
> > they should be interpreted.
> 
> This change was made due to RobH's comment below [1]
> 
> [1] ->
> https://lore.kernel.org/r/CAL_JsqLYScxGySy8xaN-UB6URfw8K_jSiuSXwVoTU9-RdJecww@mail.gmail.com/

Thanks for the pointer - that's what I needed to proceed with this
patch.

Lorenzo

> Thanks
> Kishon
> 
> > 
> > Thanks
> > Lorenzo
> > 
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> index 8f72967f298f..31e67c9c88cf 100644
> >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> @@ -229,10 +229,10 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> >>  	}
> >>  
> >>  	rc->vendor_id = 0xffff;
> >> -	of_property_read_u16(np, "vendor-id", &rc->vendor_id);
> >> +	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
> >>  
> >>  	rc->device_id = 0xffff;
> >> -	of_property_read_u16(np, "device-id", &rc->device_id);
> >> +	of_property_read_u32(np, "device-id", &rc->device_id);
> >>  
> >>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
> >>  	pcie->reg_base = devm_ioremap_resource(dev, res);
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> >> index 6bd89a21bb1c..df14ad002fe9 100644
> >> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> >> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> >> @@ -262,8 +262,8 @@ struct cdns_pcie_rc {
> >>  	struct resource		*bus_range;
> >>  	void __iomem		*cfg_base;
> >>  	u32			no_bar_nbits;
> >> -	u16			vendor_id;
> >> -	u16			device_id;
> >> +	u32			vendor_id;
> >> +	u32			device_id;
> >>  };
> >>  
> >>  /**
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v2 4/4] PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT

[Rob Herring]

On Fri, 17 Apr 2020 17:13:22 +0530, Kishon Vijay Abraham I wrote:
> The PCI Bus Binding specification (IEEE Std 1275-1994 Revision 2.1 [1])
> defines both Vendor ID and Device ID to be 32-bits. Fix
> pcie-cadence-host.c driver to read 32-bit Vendor ID and Device ID
> properties from device tree.
> 
> [1] -> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 4 ++--
>  drivers/pci/controller/cadence/pcie-cadence.h      | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

RE: [PATCH v2 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings

[Tom Joseph]

> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@ti.com>
> Sent: 17 April 2020 12:43
> To: Tom Joseph <tjoseph@cadence.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Rob Herring <robh+dt@kernel.org>; Lorenzo
> Pieralisi <lorenzo.pieralisi@arm.com>; Andrew Murray
> <amurray@thegoodpenguin.co.uk>
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v2 0/4] PCI: cadence: Deprecate inbound/outbound specific
> bindings
> 
> 
> This series is a result of comments given by Rob Herring @ [1].
> Patch series changes the DT bindings and makes the corresponding driver
> changes.
> 
> [1] ->
> https://urldefense.com/v3/__http://lore.kernel.org/r/20200219202700.GA2
> 1908@bogus__;!!EHscmS1ygiU1lA!WloWcIaUFQabEO5FFWQOtNXLI_LZm6w
> 5hMqRP7KjVX7QEGHBX7W13D1hEXnRbEg$
> 
> Changes from v1:
> 1) Added Reviewed-by: Rob Herring <robh@kernel.org> for dt-binding patch
> 2) Fixed nitpick comments from Bjorn Helgaas
> 3) Added a patch to read 32-bit Vendor ID/Device ID property from DT
> 
> Kishon Vijay Abraham I (4):
>   dt-bindings: PCI: cadence: Deprecate inbound/outbound specific
>     bindings
>   PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits"
>     property
>   PCI: cadence: Remove "cdns,max-outbound-regions" DT property
>   PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT
> 
>  .../bindings/pci/cdns,cdns-pcie-ep.yaml       |  2 +-
>  .../bindings/pci/cdns,cdns-pcie-host.yaml     |  3 +--
>  .../devicetree/bindings/pci/cdns-pcie-ep.yaml | 25 +++++++++++++++++++
>  .../bindings/pci/cdns-pcie-host.yaml          | 10 ++++++++
>  .../devicetree/bindings/pci/cdns-pcie.yaml    |  8 ------
>  .../controller/cadence/pcie-cadence-host.c    | 21 +++++++++-------
>  drivers/pci/controller/cadence/pcie-cadence.h |  6 ++---
>  7 files changed, 51 insertions(+), 24 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-
> ep.yaml
> 
> --
> 2.17.1

Acked-by: Tom Joseph <tjoseph@cadence.com>