[v2,2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

Message ID 20171219085823.8695-3-kishon@ti.com
State New
Headers show
Series
  • PCI: dra7xx: Support PCIe x2 lane mode
Related show

Commit Message

Kishon Vijay Abraham I Dec. 19, 2017, 8:58 a.m.
Add syscon properties required for configuring PCIe in x2 lane mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Rob Herring Dec. 20, 2017, 6:57 p.m. | #1
On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> Add syscon properties required for configuring PCIe in x2 lane mode.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> index 82cb875e4cec..bfbc77ac7355 100644
> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>  	       where <X> is the instance number of the pcie from the HW spec.
>   - num-lanes as specified in ../designware-pcie.txt
> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> +			 module and the register offset to specify 1 lane or
> +			 2 lane.
> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> +			module and the register offset to specify lane
> +			selection.

Adding a property for every syscon register doesn't really scale and 
doesn't work if the register layout changes.

Rob
Kishon Vijay Abraham I Dec. 22, 2017, 6:03 a.m. | #2
Hi Rob,

On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>> Add syscon properties required for configuring PCIe in x2 lane mode.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> index 82cb875e4cec..bfbc77ac7355 100644
>> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>>  	       where <X> is the instance number of the pcie from the HW spec.
>>   - num-lanes as specified in ../designware-pcie.txt
>> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>> +			 module and the register offset to specify 1 lane or
>> +			 2 lane.
>> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>> +			module and the register offset to specify lane
>> +			selection.
> 
> Adding a property for every syscon register doesn't really scale and 
> doesn't work if the register layout changes.

The register layout doesn't really change between silicon revisions and for new
SoCs, the phandle and the register offset for that SoC will have to be
populated again.

Having said that, I'm not aware of any other alternative here.

Thanks
Kishon
Tony Lindgren Dec. 22, 2017, 6:24 p.m. | #3
* Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
> Hi Rob,
> 
> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> >> Add syscon properties required for configuring PCIe in x2 lane mode.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> >> ---
> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> index 82cb875e4cec..bfbc77ac7355 100644
> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
> >>  	       where <X> is the instance number of the pcie from the HW spec.
> >>   - num-lanes as specified in ../designware-pcie.txt
> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> >> +			 module and the register offset to specify 1 lane or
> >> +			 2 lane.
> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> >> +			module and the register offset to specify lane
> >> +			selection.
> > 
> > Adding a property for every syscon register doesn't really scale and 
> > doesn't work if the register layout changes.
> 
> The register layout doesn't really change between silicon revisions and for new
> SoCs, the phandle and the register offset for that SoC will have to be
> populated again.
> 
> Having said that, I'm not aware of any other alternative here.

Sorry I did not realize this is still open. Sounds like I need to
revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
enable PCIe x2 lane mode"), let me know if that is not the case.

Regards,

Tony
Rob Herring Dec. 26, 2017, 5:46 p.m. | #4
On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
>> Hi Rob,
>>
>> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
>> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>> >> Add syscon properties required for configuring PCIe in x2 lane mode.
>> >>
>> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> index 82cb875e4cec..bfbc77ac7355 100644
>> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>> >>           where <X> is the instance number of the pcie from the HW spec.
>> >>   - num-lanes as specified in ../designware-pcie.txt
>> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>> >> +                   module and the register offset to specify 1 lane or
>> >> +                   2 lane.
>> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>> >> +                  module and the register offset to specify lane
>> >> +                  selection.
>> >
>> > Adding a property for every syscon register doesn't really scale and
>> > doesn't work if the register layout changes.
>>
>> The register layout doesn't really change between silicon revisions and for new
>> SoCs, the phandle and the register offset for that SoC will have to be
>> populated again.

And what about SoCs that don't exist yet?

>> Having said that, I'm not aware of any other alternative here.

What would you do if you had 20 different syscon registers to
configure? Add 20 properties? No, you would have per SoC functions in
the driver to handle the different cases.

> Sorry I did not realize this is still open. Sounds like I need to
> revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
> enable PCIe x2 lane mode"), let me know if that is not the case.

It's fine, I guess. Keep adding more syscon phandles and then I'll NAK
it (if I remember :)).

Rob
Tony Lindgren Dec. 26, 2017, 6:10 p.m. | #5
* Rob Herring <robh@kernel.org> [171226 17:49]:
> On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
> >> Hi Rob,
> >>
> >> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> >> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> >> >> Add syscon properties required for configuring PCIe in x2 lane mode.
> >> >>
> >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> >> >> ---
> >> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> index 82cb875e4cec..bfbc77ac7355 100644
> >> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
> >> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
> >> >>           where <X> is the instance number of the pcie from the HW spec.
> >> >>   - num-lanes as specified in ../designware-pcie.txt
> >> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> >> >> +                   module and the register offset to specify 1 lane or
> >> >> +                   2 lane.
> >> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> >> >> +                  module and the register offset to specify lane
> >> >> +                  selection.
> >> >
> >> > Adding a property for every syscon register doesn't really scale and
> >> > doesn't work if the register layout changes.
> >>
> >> The register layout doesn't really change between silicon revisions and for new
> >> SoCs, the phandle and the register offset for that SoC will have to be
> >> populated again.
> 
> And what about SoCs that don't exist yet?
> 
> >> Having said that, I'm not aware of any other alternative here.
> 
> What would you do if you had 20 different syscon registers to
> configure? Add 20 properties? No, you would have per SoC functions in
> the driver to handle the different cases.

Ideally these syscon registers would be managed by some Linux
generic framework such as clock/regulator/mux/phy.

But yeah, if that does not work, then setting a SoC specific
configuration function based on the compatible value makes sense
to me.

> > Sorry I did not realize this is still open. Sounds like I need to
> > revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
> > enable PCIe x2 lane mode"), let me know if that is not the case.
> 
> It's fine, I guess. Keep adding more syscon phandles and then I'll NAK
> it (if I remember :)).

Already reverted, thanks for the comments.

Regards,

Tony
Kishon Vijay Abraham I Dec. 27, 2017, 6:40 a.m. | #6
Hi,

On Tuesday 26 December 2017 11:40 PM, Tony Lindgren wrote:
> * Rob Herring <robh@kernel.org> [171226 17:49]:
>> On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony@atomide.com> wrote:
>>> * Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
>>>> Hi Rob,
>>>>
>>>> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
>>>>> On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>>>>>> Add syscon properties required for configuring PCIe in x2 lane mode.
>>>>>>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> index 82cb875e4cec..bfbc77ac7355 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>>>>>>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>>>>>>           where <X> is the instance number of the pcie from the HW spec.
>>>>>>   - num-lanes as specified in ../designware-pcie.txt
>>>>>> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>>>>>> +                   module and the register offset to specify 1 lane or
>>>>>> +                   2 lane.
>>>>>> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>>>>>> +                  module and the register offset to specify lane
>>>>>> +                  selection.
>>>>>
>>>>> Adding a property for every syscon register doesn't really scale and
>>>>> doesn't work if the register layout changes.
>>>>
>>>> The register layout doesn't really change between silicon revisions and for new
>>>> SoCs, the phandle and the register offset for that SoC will have to be
>>>> populated again.
>>
>> And what about SoCs that don't exist yet?
>>
>>>> Having said that, I'm not aware of any other alternative here.
>>
>> What would you do if you had 20 different syscon registers to
>> configure? Add 20 properties? No, you would have per SoC functions in
>> the driver to handle the different cases.
> 
> Ideally these syscon registers would be managed by some Linux
> generic framework such as clock/regulator/mux/phy.
> 
> But yeah, if that does not work, then setting a SoC specific
> configuration function based on the compatible value makes sense
> to me.

All right. I'll add a function for syscon configuration in pci-dra7xx.

Thanks
Kishon

Patch

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 82cb875e4cec..bfbc77ac7355 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -13,6 +13,12 @@  PCIe DesignWare Controller
  - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
 	       where <X> is the instance number of the pcie from the HW spec.
  - num-lanes as specified in ../designware-pcie.txt
+ - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
+			 module and the register offset to specify 1 lane or
+			 2 lane.
+ - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
+			module and the register offset to specify lane
+			selection.
 
 HOST MODE
 =========