diff mbox series

[v5,1/5] arm64: dts: ti: k3-j784s4-main: Add system controller and SERDES lane mux

Message ID 20230710101705.154119-2-j-choudhary@ti.com
State New
Headers
Series Add peripherals for J784S4 |

Commit Message

Jayesh Choudhary July 10, 2023, 10:17 a.m. UTC 
  From: Siddharth Vadapalli <s-vadapalli@ti.com>

The system controller node manages the CTRL_MMR0 region.
Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
[j-choudhary@ti.com: Add reg property to fix dtc warning]
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Krzysztof Kozlowski July 10, 2023, 11:43 a.m. UTC | #1 
On 10/07/2023 12:17, Jayesh Choudhary wrote:
> From: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> The system controller node manages the CTRL_MMR0 region.
> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> [j-choudhary@ti.com: Add reg property to fix dtc warning]
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> index 2ea0adae6832..68cc2fa053e7 100644
> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> @@ -5,6 +5,9 @@
>   * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>   */
>  
> +#include <dt-bindings/mux/mux.h>
> +#include <dt-bindings/mux/ti-serdes.h>

Why? What do you use from that binding?

Best regards,
Krzysztof
Jayesh Choudhary July 11, 2023, 6:31 a.m. UTC | #2 
On 10/07/23 17:13, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:17, Jayesh Choudhary wrote:
>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>
>> The system controller node manages the CTRL_MMR0 region.
>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> [j-choudhary@ti.com: Add reg property to fix dtc warning]
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>> index 2ea0adae6832..68cc2fa053e7 100644
>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>> @@ -5,6 +5,9 @@
>>    * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>    */
>>   
>> +#include <dt-bindings/mux/mux.h>
>> +#include <dt-bindings/mux/ti-serdes.h>
> 
> Why? What do you use from that binding?
> 

Missed idle-state in the mux-controller node here for default values.
I will wait for more feedback and then re-spin the series.

Thanks,
-Jayesh
Nishanth Menon July 11, 2023, 3:31 p.m. UTC | #3 
On 12:01-20230711, Jayesh Choudhary wrote:
> 
> 
> On 10/07/23 17:13, Krzysztof Kozlowski wrote:
> > On 10/07/2023 12:17, Jayesh Choudhary wrote:
> > > From: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > 
> > > The system controller node manages the CTRL_MMR0 region.
> > > Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
> > > 
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > [j-choudhary@ti.com: Add reg property to fix dtc warning]
> > > Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> > > ---
> > >   arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
> > >   1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > index 2ea0adae6832..68cc2fa053e7 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
> > > @@ -5,6 +5,9 @@
> > >    * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
> > >    */
> > > +#include <dt-bindings/mux/mux.h>
> > > +#include <dt-bindings/mux/ti-serdes.h>
> > 
> > Why? What do you use from that binding?
> > 
> 
> Missed idle-state in the mux-controller node here for default values.
> I will wait for more feedback and then re-spin the series.

btw, I am wondering if ti-serdes.h should even exist in dt-bindings -
are any of the macros used in the driver? or should this follow the
pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti
?
Krzysztof Kozlowski July 12, 2023, 5:44 a.m. UTC | #4 
On 11/07/2023 17:31, Nishanth Menon wrote:
> On 12:01-20230711, Jayesh Choudhary wrote:
>>
>>
>> On 10/07/23 17:13, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 12:17, Jayesh Choudhary wrote:
>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>
>>>> The system controller node manages the CTRL_MMR0 region.
>>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>> [j-choudhary@ti.com: Add reg property to fix dtc warning]
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>   arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
>>>>   1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>> index 2ea0adae6832..68cc2fa053e7 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>> @@ -5,6 +5,9 @@
>>>>    * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>>>    */
>>>> +#include <dt-bindings/mux/mux.h>
>>>> +#include <dt-bindings/mux/ti-serdes.h>
>>>
>>> Why? What do you use from that binding?
>>>
>>
>> Missed idle-state in the mux-controller node here for default values.
>> I will wait for more feedback and then re-spin the series.
> 
> btw, I am wondering if ti-serdes.h should even exist in dt-bindings -
> are any of the macros used in the driver? or should this follow the
> pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti
> ?

I don't see any usage in drivers, which is a clear indication that it
might not be suitable for bindings. What are these values? Look like
some register values, which there is little sense in making a binding.

Best regards,
Krzysztof
Roger Quadros July 12, 2023, 11:21 a.m. UTC | #5 
On 12/07/2023 08:44, Krzysztof Kozlowski wrote:
> On 11/07/2023 17:31, Nishanth Menon wrote:
>> On 12:01-20230711, Jayesh Choudhary wrote:
>>>
>>>
>>> On 10/07/23 17:13, Krzysztof Kozlowski wrote:
>>>> On 10/07/2023 12:17, Jayesh Choudhary wrote:
>>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>
>>>>> The system controller node manages the CTRL_MMR0 region.
>>>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
>>>>>
>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>> [j-choudhary@ti.com: Add reg property to fix dtc warning]
>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>> ---
>>>>>   arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
>>>>>   1 file changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>> index 2ea0adae6832..68cc2fa053e7 100644
>>>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>> @@ -5,6 +5,9 @@
>>>>>    * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>>>>    */
>>>>> +#include <dt-bindings/mux/mux.h>
>>>>> +#include <dt-bindings/mux/ti-serdes.h>
>>>>
>>>> Why? What do you use from that binding?
>>>>
>>>
>>> Missed idle-state in the mux-controller node here for default values.
>>> I will wait for more feedback and then re-spin the series.
>>
>> btw, I am wondering if ti-serdes.h should even exist in dt-bindings -
>> are any of the macros used in the driver? or should this follow the
>> pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti
>> ?
> 
> I don't see any usage in drivers, which is a clear indication that it
> might not be suitable for bindings. What are these values? Look like
> some register values, which there is little sense in making a binding.
> 
> Best regards,
> Krzysztof
> 
> 

You are right. They are constants not used in the driver directly.
mmio-mux driver uses it to set the idle state of the mux via the
'idle-states' property.

I agree with Nishanth that they should be moved to arch/arm64/boot/dts/ti
Jayesh Choudhary July 12, 2023, 12:43 p.m. UTC | #6 
On 12/07/23 16:51, Roger Quadros wrote:
> 
> 
> On 12/07/2023 08:44, Krzysztof Kozlowski wrote:
>> On 11/07/2023 17:31, Nishanth Menon wrote:
>>> On 12:01-20230711, Jayesh Choudhary wrote:
>>>>
>>>>
>>>> On 10/07/23 17:13, Krzysztof Kozlowski wrote:
>>>>> On 10/07/2023 12:17, Jayesh Choudhary wrote:
>>>>>> From: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>>
>>>>>> The system controller node manages the CTRL_MMR0 region.
>>>>>> Add serdes_ln_ctrl node which is used for controlling the SERDES lane mux.
>>>>>>
>>>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>>>> [j-choudhary@ti.com: Add reg property to fix dtc warning]
>>>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>>>> ---
>>>>>>    arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi | 23 ++++++++++++++++++++++
>>>>>>    1 file changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>>> index 2ea0adae6832..68cc2fa053e7 100644
>>>>>> --- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
>>>>>> @@ -5,6 +5,9 @@
>>>>>>     * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
>>>>>>     */
>>>>>> +#include <dt-bindings/mux/mux.h>
>>>>>> +#include <dt-bindings/mux/ti-serdes.h>
>>>>>
>>>>> Why? What do you use from that binding?
>>>>>
>>>>
>>>> Missed idle-state in the mux-controller node here for default values.
>>>> I will wait for more feedback and then re-spin the series.
>>>
>>> btw, I am wondering if ti-serdes.h should even exist in dt-bindings -
>>> are any of the macros used in the driver? or should this follow the
>>> pinctrl style macros that could happily reside in arch/arm64/boot/dts/ti
>>> ?
>>
>> I don't see any usage in drivers, which is a clear indication that it
>> might not be suitable for bindings. What are these values? Look like
>> some register values, which there is little sense in making a binding.
>>
>> Best regards,
>> Krzysztof
>>
>>
> 
> You are right. They are constants not used in the driver directly.
> mmio-mux driver uses it to set the idle state of the mux via the
> 'idle-states' property.
> 
> I agree with Nishanth that they should be moved to arch/arm64/boot/dts/ti
> 

Then I will do the cleanup for all platforms and then post the dependent
series before spinning v6.

Thanks and Warm regards,
-Jayesh
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
index 2ea0adae6832..68cc2fa053e7 100644
--- a/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j784s4-main.dtsi
@@ -5,6 +5,9 @@ 
  * Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
  */
 
+#include <dt-bindings/mux/mux.h>
+#include <dt-bindings/mux/ti-serdes.h>
+
 &cbass_main {
 	msmc_ram: sram@70000000 {
 		compatible = "mmio-sram";
@@ -26,6 +29,26 @@  l3cache-sram@200000 {
 		};
 	};
 
+	scm_conf: syscon@100000 {
+		compatible = "ti,j721e-system-controller", "syscon", "simple-mfd";
+		reg = <0x00 0x00100000 0x00 0x1c000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x00 0x00 0x00100000 0x1c000>;
+
+		serdes_ln_ctrl: mux-controller@4080 {
+			compatible = "mmio-mux";
+			reg = <0x00004080 0x30>;
+			#mux-control-cells = <1>;
+			mux-reg-masks = <0x4080 0x3>, <0x4084 0x3>, /* SERDES0 lane0/1 select */
+					<0x4088 0x3>, <0x408c 0x3>, /* SERDES0 lane2/3 select */
+					<0x4090 0x3>, <0x4094 0x3>, /* SERDES1 lane0/1 select */
+					<0x4098 0x3>, <0x409c 0x3>, /* SERDES1 lane2/3 select */
+					<0x40a0 0x3>, <0x40a4 0x3>, /* SERDES2 lane0/1 select */
+					<0x40a8 0x3>, <0x40ac 0x3>; /* SERDES2 lane2/3 select */
+		};
+	};
+
 	gic500: interrupt-controller@1800000 {
 		compatible = "arm,gic-v3";
 		#address-cells = <2>;