[PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Bharat Kumar Gogada]

Describe cpm reg property before cfg reg property to align with
node name.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 .../devicetree/bindings/pci/xilinx-versal-cpm.yaml     | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
index 32f4641085bc..cca395317a4c 100644
--- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
+++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
@@ -18,13 +18,13 @@ properties:
 
   reg:
     items:
-      - description: Configuration space region and bridge registers.
       - description: CPM system level control and status registers.
+      - description: Configuration space region and bridge registers.
 
   reg-names:
     items:
-      - const: cfg
       - const: cpm_slcr
+      - const: cfg
 
   interrupts:
     maxItems: 1
@@ -86,9 +86,9 @@ examples:
                        ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
                                 <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
                        msi-map = <0x0 &its_gic 0x0 0x10000>;
-                       reg = <0x6 0x00000000 0x0 0x10000000>,
-                             <0x0 0xfca10000 0x0 0x1000>;
-                       reg-names = "cfg", "cpm_slcr";
+                       reg = <0x0 0xfca10000 0x0 0x1000>,
+                             <0x6 0x00000000 0x0 0x10000000>;
+                       reg-names = "cpm_slcr", "cfg";
                        pcie_intc_0: interrupt-controller {
                                #address-cells = <0>;
                                #interrupt-cells = <1>;
-- 
2.17.1

Re: [PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Michal Simek]

On 4/29/22 15:02, Bharat Kumar Gogada wrote:
> Describe cpm reg property before cfg reg property to align with
> node name.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>   .../devicetree/bindings/pci/xilinx-versal-cpm.yaml     | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index 32f4641085bc..cca395317a4c 100644
> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -18,13 +18,13 @@ properties:
>   
>     reg:
>       items:
> -      - description: Configuration space region and bridge registers.
>         - description: CPM system level control and status registers.
> +      - description: Configuration space region and bridge registers.
>   
>     reg-names:
>       items:
> -      - const: cfg
>         - const: cpm_slcr
> +      - const: cfg
>   
>     interrupts:
>       maxItems: 1
> @@ -86,9 +86,9 @@ examples:
>                          ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
>                                   <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
>                          msi-map = <0x0 &its_gic 0x0 0x10000>;
> -                       reg = <0x6 0x00000000 0x0 0x10000000>,
> -                             <0x0 0xfca10000 0x0 0x1000>;
> -                       reg-names = "cfg", "cpm_slcr";
> +                       reg = <0x0 0xfca10000 0x0 0x1000>,
> +                             <0x6 0x00000000 0x0 0x10000000>;
> +                       reg-names = "cpm_slcr", "cfg";
>                          pcie_intc_0: interrupt-controller {
>                                  #address-cells = <0>;
>                                  #interrupt-cells = <1>;

Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Rob Herring]

On Fri, Apr 29, 2022 at 8:02 AM Bharat Kumar Gogada
<bharat.kumar.gogada@xilinx.com> wrote:
>
> Describe cpm reg property before cfg reg property to align with
> node name.

The order is an ABI. If breaking it is okay, explain why here.

>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  .../devicetree/bindings/pci/xilinx-versal-cpm.yaml     | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index 32f4641085bc..cca395317a4c 100644
> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -18,13 +18,13 @@ properties:
>
>    reg:
>      items:
> -      - description: Configuration space region and bridge registers.
>        - description: CPM system level control and status registers.
> +      - description: Configuration space region and bridge registers.
>
>    reg-names:
>      items:
> -      - const: cfg
>        - const: cpm_slcr
> +      - const: cfg
>
>    interrupts:
>      maxItems: 1
> @@ -86,9 +86,9 @@ examples:
>                         ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
>                                  <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
>                         msi-map = <0x0 &its_gic 0x0 0x10000>;
> -                       reg = <0x6 0x00000000 0x0 0x10000000>,
> -                             <0x0 0xfca10000 0x0 0x1000>;
> -                       reg-names = "cfg", "cpm_slcr";
> +                       reg = <0x0 0xfca10000 0x0 0x1000>,
> +                             <0x6 0x00000000 0x0 0x10000000>;
> +                       reg-names = "cpm_slcr", "cfg";
>                         pcie_intc_0: interrupt-controller {
>                                 #address-cells = <0>;
>                                 #interrupt-cells = <1>;
> --
> 2.17.1
>

Re: [PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Michal Simek]

On 5/4/22 17:43, Rob Herring wrote:
> On Fri, Apr 29, 2022 at 8:02 AM Bharat Kumar Gogada
> <bharat.kumar.gogada@xilinx.com> wrote:
>>
>> Describe cpm reg property before cfg reg property to align with
>> node name.
> 
> The order is an ABI. If breaking it is okay, explain why here.

I didn't push any description for versal to upstream u-boot or linux yet but 
xilinx is using this order for years. DT binding order wasn't aligned to it.

For example: (Xilinx Linux is in sync with this).
https://github.com/Xilinx/u-boot-xlnx/blob/master/arch/arm/dts/versal.dtsi

Driver itself is working with reg-names and order of regs doesn't matter. It 
means changed order doesn't break any functionality.
Right now reg order really matter in binding doc but would be good in these 
examples to record that both ways are fine.
Would it be better to describe that both ways are fine?

   reg-names:
     oneOf:
     - items:
       - const: cfg
       - const: cpm_slcr
     - items:
       - const: cpm_slcr
       - const: cfg


Another small reason is that all hard IPs in Versal are normally placed below 
4GB address range. And there are some others which also have mapping above. This 
is one of that example and we normally aligned with 32bit address.

And the biggest reason is that current node name is pcie@fca10000 which should 
be aligned with the first register base which is before this patch 0x600000000 
but name suggest that the first reg should be cpm_slcr instead of cfg. That's 
why I consider this patch as a fix and the patch should contain fixed tag.

Thanks,
Michal

Re: [PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Rob Herring]

On Tue, May 10, 2022 at 2:29 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
>
>
> On 5/4/22 17:43, Rob Herring wrote:
> > On Fri, Apr 29, 2022 at 8:02 AM Bharat Kumar Gogada
> > <bharat.kumar.gogada@xilinx.com> wrote:
> >>
> >> Describe cpm reg property before cfg reg property to align with
> >> node name.
> >
> > The order is an ABI. If breaking it is okay, explain why here.
>
> I didn't push any description for versal to upstream u-boot or linux yet but
> xilinx is using this order for years. DT binding order wasn't aligned to it.
>
> For example: (Xilinx Linux is in sync with this).
> https://github.com/Xilinx/u-boot-xlnx/blob/master/arch/arm/dts/versal.dtsi

Good to know, but if there are upstream dts files, what do they use?

> Driver itself is working with reg-names and order of regs doesn't matter. It
> means changed order doesn't break any functionality.

While in general I consider the order part of the ABI, if that's
enough to avoid breakage on anything you care about, then just state
that.

> Right now reg order really matter in binding doc but would be good in these
> examples to record that both ways are fine.
> Would it be better to describe that both ways are fine?

Only if that's what you need. If you are fine with the order changing,
then make the change and fix all the dts files.

>
>    reg-names:
>      oneOf:
>      - items:
>        - const: cfg
>        - const: cpm_slcr
>      - items:
>        - const: cpm_slcr
>        - const: cfg
>
>
> Another small reason is that all hard IPs in Versal are normally placed below
> 4GB address range. And there are some others which also have mapping above. This
> is one of that example and we normally aligned with 32bit address.
>
> And the biggest reason is that current node name is pcie@fca10000 which should
> be aligned with the first register base which is before this patch 0x600000000
> but name suggest that the first reg should be cpm_slcr instead of cfg. That's
> why I consider this patch as a fix and the patch should contain fixed tag.

I don't disagree. I'm only asking for a better commit message.

Rob

Re: [PATCH] dt-bindings: PCI: xilinx-cpm: Change reg property order

[Michal Simek]

Hi Rob,

On 5/10/22 18:00, Rob Herring wrote:
> On Tue, May 10, 2022 at 2:29 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>
>>
>>
>> On 5/4/22 17:43, Rob Herring wrote:
>>> On Fri, Apr 29, 2022 at 8:02 AM Bharat Kumar Gogada
>>> <bharat.kumar.gogada@xilinx.com> wrote:
>>>>
>>>> Describe cpm reg property before cfg reg property to align with
>>>> node name.
>>>
>>> The order is an ABI. If breaking it is okay, explain why here.
>>
>> I didn't push any description for versal to upstream u-boot or linux yet but
>> xilinx is using this order for years. DT binding order wasn't aligned to it.
>>
>> For example: (Xilinx Linux is in sync with this).
>> https://github.com/Xilinx/u-boot-xlnx/blob/master/arch/arm/dts/versal.dtsi
> 
> Good to know, but if there are upstream dts files, what do they use?

I didn't push any versal DTs. But origin source are my xilinx repos where we 
have this order:

reg = <0x0 0xfca10000 0x0 0x1000>, <0x6 0x00000000 0x0 0x1000000>;


> 
>> Driver itself is working with reg-names and order of regs doesn't matter. It
>> means changed order doesn't break any functionality.
> 
> While in general I consider the order part of the ABI, if that's
> enough to avoid breakage on anything you care about, then just state
> that.

right.

> 
>> Right now reg order really matter in binding doc but would be good in these
>> examples to record that both ways are fine.
>> Would it be better to describe that both ways are fine?
> 
> Only if that's what you need. If you are fine with the order changing,
> then make the change and fix all the dts files.

I structure DT files in a way that we have versal.dtsi as description for all 
fixed peripherals. All platforms, boards are just sourcing it. That's why change 
is simple but this issue is really just in dt binding doc than our dts file 
which all uses order mentioned above.


>>
>>     reg-names:
>>       oneOf:
>>       - items:
>>         - const: cfg
>>         - const: cpm_slcr
>>       - items:
>>         - const: cpm_slcr
>>         - const: cfg
>>
>>
>> Another small reason is that all hard IPs in Versal are normally placed below
>> 4GB address range. And there are some others which also have mapping above. This
>> is one of that example and we normally aligned with 32bit address.
>>
>> And the biggest reason is that current node name is pcie@fca10000 which should
>> be aligned with the first register base which is before this patch 0x600000000
>> but name suggest that the first reg should be cpm_slcr instead of cfg. That's
>> why I consider this patch as a fix and the patch should contain fixed tag.
> 
> I don't disagree. I'm only asking for a better commit message.

Great.
Bharat: Please update commit message and sent new version.

Thanks,
Michal