The CPTS driver is capable of configuring GENFy (Periodic Signal Generator
Function) present in the CPTS module, to generate periodic output signals
with a custom time period. In order to generate a PPS signal on the GENFy
output, the device-tree property "ti,pps" has to be used. The "ti,pps"
property is used to declare the mapping between the CPTS HWx_TS_PUSH
(Hardware Timestamp trigger) input and the GENFy output that is configured
to generate a PPS signal. The mapping is of the form:
<x-1 y>
where the value x corresponds to HWx_TS_PUSH input (1-based indexing) and
the value y corresponds to GENFy (0-based indexing).

To verify that the signal is a PPS signal, the GENFy output signal is fed
into the CPTS HWx_TS_PUSH input, which generates a timestamp event on the
rising edge of the GENFy signal. The GENFy output signal can be routed to
the HWx_TS_PUSH input by using the Time Sync Router. This is done by
mentioning the mapping between the GENFy output and the HWx_TS_PUSH input
within the "timesync_router" device-tree node.

The Input Sources to the Time Sync Router are documented at: [1]
The Output Destinations of the Time Sync Router are documented at: [2]

The PPS signal can be verified using testptp and ppstest tools as follows:
 # ./testptp -d /dev/ptp0 -P 1
 pps for system time request okay
 # ./ppstest /dev/pps0
 trying PPS source "/dev/pps0"
 found PPS source "/dev/pps0"
 ok, found 1 source(s), now start fetching data...
 source 0 - assert 48.000000013, sequence: 8 - clear  0.000000000, sequence: 0
 source 0 - assert 49.000000013, sequence: 9 - clear  0.000000000, sequence: 0
 source 0 - assert 50.000000013, sequence: 10 - clear  0.000000000, sequence: 0

Add an example in the device-tree, enabling PPS generation on GENF1. The
HW3_TS_PUSH Timestamp trigger input is used to verify the PPS signal.

[1]
Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-input-sources
[2]
Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
index 4f179b146cab..962a922cc94b 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
@@ -366,6 +366,10 @@ &cpsw3g {
 	pinctrl-names = "default";
 	pinctrl-0 = <&main_rgmii1_pins_default
 		     &main_rgmii2_pins_default>;
+
+	cpts@3d000 {
+		ti,pps = <2 1>;
+	};
 };
 
 &cpsw_port1 {
@@ -464,3 +468,19 @@ partition@3fc0000 {
 		};
 	};
 };
+
+#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
+
+&timesync_router {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&cpsw_cpts>;
+
+	/* Example of the timesync routing */
+	cpsw_cpts: cpsw-cpts {
+		pinctrl-single,pins = <
+			/* pps [cpsw cpts genf1] in17 -> out12 [cpsw cpts hw3_push] */
+			TS_OFFSET(12, 17)
+			>;
+	};
+};
-- 
2.25.1

Hi,

On 11/01/2023 13:44, Siddharth Vadapalli wrote:
> The CPTS driver is capable of configuring GENFy (Periodic Signal Generator
> Function) present in the CPTS module, to generate periodic output signals
> with a custom time period. In order to generate a PPS signal on the GENFy
> output, the device-tree property "ti,pps" has to be used. The "ti,pps"
> property is used to declare the mapping between the CPTS HWx_TS_PUSH
> (Hardware Timestamp trigger) input and the GENFy output that is configured
> to generate a PPS signal. The mapping is of the form:
> <x-1 y>
> where the value x corresponds to HWx_TS_PUSH input (1-based indexing) and
> the value y corresponds to GENFy (0-based indexing).

You mean there is no HWx_TX_PUSH0 pin? so user needs to use 0 for HWx_TX_PUSH1 pin?

Can you please define macros for HWx_TS_PUSH and GENFy so we avoid
human error with this different indexing methods?

DT should contain the name exactly in hardware.

So if pin is called HWx_TX_PUSH1 in hardware then DT should contain HWx_TX_PUSH(1).

> 
> To verify that the signal is a PPS signal, the GENFy output signal is fed
> into the CPTS HWx_TS_PUSH input, which generates a timestamp event on the
> rising edge of the GENFy signal. The GENFy output signal can be routed to
> the HWx_TS_PUSH input by using the Time Sync Router. This is done by
> mentioning the mapping between the GENFy output and the HWx_TS_PUSH input
> within the "timesync_router" device-tree node.
> 
> The Input Sources to the Time Sync Router are documented at: [1]
> The Output Destinations of the Time Sync Router are documented at: [2]
> 
> The PPS signal can be verified using testptp and ppstest tools as follows:
>  # ./testptp -d /dev/ptp0 -P 1
>  pps for system time request okay
>  # ./ppstest /dev/pps0
>  trying PPS source "/dev/pps0"
>  found PPS source "/dev/pps0"
>  ok, found 1 source(s), now start fetching data...
>  source 0 - assert 48.000000013, sequence: 8 - clear  0.000000000, sequence: 0
>  source 0 - assert 49.000000013, sequence: 9 - clear  0.000000000, sequence: 0
>  source 0 - assert 50.000000013, sequence: 10 - clear  0.000000000, sequence: 0
> 
> Add an example in the device-tree, enabling PPS generation on GENF1. The
> HW3_TS_PUSH Timestamp trigger input is used to verify the PPS signal.
> 
> [1]
> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-input-sources
> [2]
> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
> index 4f179b146cab..962a922cc94b 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
> @@ -366,6 +366,10 @@ &cpsw3g {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&main_rgmii1_pins_default
>  		     &main_rgmii2_pins_default>;
> +
> +	cpts@3d000 {
> +		ti,pps = <2 1>;
> +	};
>  };
>  
>  &cpsw_port1 {
> @@ -464,3 +468,19 @@ partition@3fc0000 {
>  		};
>  	};
>  };
> +
> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)

Should this go in ./include/dt-bindings/pinctrl/k3.h ?
That way every board DT file doesn't have to define it.

The name should be made more platform specific.
e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
If not then please add Platform name instead of K3.

> +
> +&timesync_router {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&cpsw_cpts>;
> +
> +	/* Example of the timesync routing */
> +	cpsw_cpts: cpsw-cpts {
> +		pinctrl-single,pins = <
> +			/* pps [cpsw cpts genf1] in17 -> out12 [cpsw cpts hw3_push] */
> +			TS_OFFSET(12, 17)
> +			>;
> +	};
> +};

cheers,
-roger

Roger,

On 13/01/23 15:48, Roger Quadros wrote:
> Hi,
> 
> On 11/01/2023 13:44, Siddharth Vadapalli wrote:
>> The CPTS driver is capable of configuring GENFy (Periodic Signal Generator
>> Function) present in the CPTS module, to generate periodic output signals
>> with a custom time period. In order to generate a PPS signal on the GENFy
>> output, the device-tree property "ti,pps" has to be used. The "ti,pps"
>> property is used to declare the mapping between the CPTS HWx_TS_PUSH
>> (Hardware Timestamp trigger) input and the GENFy output that is configured
>> to generate a PPS signal. The mapping is of the form:
>> <x-1 y>
>> where the value x corresponds to HWx_TS_PUSH input (1-based indexing) and
>> the value y corresponds to GENFy (0-based indexing).
> 
> You mean there is no HWx_TX_PUSH0 pin? so user needs to use 0 for HWx_TX_PUSH1 pin?

The HWx_TX_PUSH pins correspond to the cpts_hw1_push, cpts_hw2_push,...,
cpts_hw8_push pins. The names are documented at:

Link:
https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations

Thus, considering that the documentation uses 1-based indexing, I wanted to
indicate that the driver expects 0-based indexing, and therefore the user would
have to provide (x-1) for cpsw_hwx_push pin.

> 
> Can you please define macros for HWx_TS_PUSH and GENFy so we avoid
> human error with this different indexing methods?
> 
> DT should contain the name exactly in hardware.
> 
> So if pin is called HWx_TX_PUSH1 in hardware then DT should contain HWx_TX_PUSH(1).

The pins are called HW1_TX_PUSH, HW2_TX_PUSH and so on. This 1-based indexing is
followed in the Technical Reference Manual. Similarly, the documentation in the
link above also uses 1-based indexing: cpts_hw1_push, cpts_hw2_push, and so on.

However, for the GENFy pins, the documentation consistently uses 0-based
indexing. Thus, the driver expects indices that are 0-based and the user is
expected to convert the x to x-1 for the HWx_TX_PUSH pins while the y in GENFy
pins can be used directly as it is already 0-based indexing.

> 
>>
>> To verify that the signal is a PPS signal, the GENFy output signal is fed
>> into the CPTS HWx_TS_PUSH input, which generates a timestamp event on the
>> rising edge of the GENFy signal. The GENFy output signal can be routed to
>> the HWx_TS_PUSH input by using the Time Sync Router. This is done by
>> mentioning the mapping between the GENFy output and the HWx_TS_PUSH input
>> within the "timesync_router" device-tree node.
>>
>> The Input Sources to the Time Sync Router are documented at: [1]
>> The Output Destinations of the Time Sync Router are documented at: [2]
>>
>> The PPS signal can be verified using testptp and ppstest tools as follows:
>>  # ./testptp -d /dev/ptp0 -P 1
>>  pps for system time request okay
>>  # ./ppstest /dev/pps0
>>  trying PPS source "/dev/pps0"
>>  found PPS source "/dev/pps0"
>>  ok, found 1 source(s), now start fetching data...
>>  source 0 - assert 48.000000013, sequence: 8 - clear  0.000000000, sequence: 0
>>  source 0 - assert 49.000000013, sequence: 9 - clear  0.000000000, sequence: 0
>>  source 0 - assert 50.000000013, sequence: 10 - clear  0.000000000, sequence: 0
>>
>> Add an example in the device-tree, enabling PPS generation on GENF1. The
>> HW3_TS_PUSH Timestamp trigger input is used to verify the PPS signal.
>>
>> [1]
>> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-input-sources
>> [2]
>> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>> index 4f179b146cab..962a922cc94b 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>> @@ -366,6 +366,10 @@ &cpsw3g {
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>  		     &main_rgmii2_pins_default>;
>> +
>> +	cpts@3d000 {
>> +		ti,pps = <2 1>;
>> +	};
>>  };
>>  
>>  &cpsw_port1 {
>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>  		};
>>  	};
>>  };
>> +
>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
> 
> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
> That way every board DT file doesn't have to define it.
> 
> The name should be made more platform specific.
> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
> If not then please add Platform name instead of K3.

The offsets are board specific. If it is acceptable, I will add board specific
macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
file. Please let me know.

Regards,
Siddharth.

On 16/01/2023 09:12, Siddharth Vadapalli wrote:
> Roger,
> 
> On 13/01/23 15:48, Roger Quadros wrote:
>> Hi,
>>
>> On 11/01/2023 13:44, Siddharth Vadapalli wrote:
>>> The CPTS driver is capable of configuring GENFy (Periodic Signal Generator
>>> Function) present in the CPTS module, to generate periodic output signals
>>> with a custom time period. In order to generate a PPS signal on the GENFy
>>> output, the device-tree property "ti,pps" has to be used. The "ti,pps"
>>> property is used to declare the mapping between the CPTS HWx_TS_PUSH
>>> (Hardware Timestamp trigger) input and the GENFy output that is configured
>>> to generate a PPS signal. The mapping is of the form:
>>> <x-1 y>
>>> where the value x corresponds to HWx_TS_PUSH input (1-based indexing) and
>>> the value y corresponds to GENFy (0-based indexing).
>>
>> You mean there is no HWx_TX_PUSH0 pin? so user needs to use 0 for HWx_TX_PUSH1 pin?
> 
> The HWx_TX_PUSH pins correspond to the cpts_hw1_push, cpts_hw2_push,...,
> cpts_hw8_push pins. The names are documented at:
> 
> Link:
> https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations
> 
> Thus, considering that the documentation uses 1-based indexing, I wanted to
> indicate that the driver expects 0-based indexing, and therefore the user would
> have to provide (x-1) for cpsw_hwx_push pin.
> 
>>
>> Can you please define macros for HWx_TS_PUSH and GENFy so we avoid
>> human error with this different indexing methods?
>>
>> DT should contain the name exactly in hardware.
>>
>> So if pin is called HWx_TX_PUSH1 in hardware then DT should contain HWx_TX_PUSH(1).
> 
> The pins are called HW1_TX_PUSH, HW2_TX_PUSH and so on. This 1-based indexing is
> followed in the Technical Reference Manual. Similarly, the documentation in the
> link above also uses 1-based indexing: cpts_hw1_push, cpts_hw2_push, and so on.
> 
> However, for the GENFy pins, the documentation consistently uses 0-based
> indexing. Thus, the driver expects indices that are 0-based and the user is
> expected to convert the x to x-1 for the HWx_TX_PUSH pins while the y in GENFy
> pins can be used directly as it is already 0-based indexing.
> 
>>
>>>
>>> To verify that the signal is a PPS signal, the GENFy output signal is fed
>>> into the CPTS HWx_TS_PUSH input, which generates a timestamp event on the
>>> rising edge of the GENFy signal. The GENFy output signal can be routed to
>>> the HWx_TS_PUSH input by using the Time Sync Router. This is done by
>>> mentioning the mapping between the GENFy output and the HWx_TS_PUSH input
>>> within the "timesync_router" device-tree node.
>>>
>>> The Input Sources to the Time Sync Router are documented at: [1]
>>> The Output Destinations of the Time Sync Router are documented at: [2]
>>>
>>> The PPS signal can be verified using testptp and ppstest tools as follows:
>>>  # ./testptp -d /dev/ptp0 -P 1
>>>  pps for system time request okay
>>>  # ./ppstest /dev/pps0
>>>  trying PPS source "/dev/pps0"
>>>  found PPS source "/dev/pps0"
>>>  ok, found 1 source(s), now start fetching data...
>>>  source 0 - assert 48.000000013, sequence: 8 - clear  0.000000000, sequence: 0
>>>  source 0 - assert 49.000000013, sequence: 9 - clear  0.000000000, sequence: 0
>>>  source 0 - assert 50.000000013, sequence: 10 - clear  0.000000000, sequence: 0
>>>
>>> Add an example in the device-tree, enabling PPS generation on GENF1. The
>>> HW3_TS_PUSH Timestamp trigger input is used to verify the PPS signal.
>>>
>>> [1]
>>> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-input-sources
>>> [2]
>>> Link: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>> index 4f179b146cab..962a922cc94b 100644
>>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>> @@ -366,6 +366,10 @@ &cpsw3g {
>>>  	pinctrl-names = "default";
>>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>>  		     &main_rgmii2_pins_default>;
>>> +
>>> +	cpts@3d000 {
>>> +		ti,pps = <2 1>;
>>> +	};
>>>  };
>>>  
>>>  &cpsw_port1 {
>>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>>  		};
>>>  	};
>>>  };
>>> +
>>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>>
>> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
>> That way every board DT file doesn't have to define it.
>>
>> The name should be made more platform specific.
>> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
>> If not then please add Platform name instead of K3.
> 
> The offsets are board specific. If it is acceptable, I will add board specific
> macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
> file. Please let me know.

If it is board specific then it should remain in the board file.

cheers,
-roger

On 16/01/23 9:35 pm, Roger Quadros wrote:
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>> index 4f179b146cab..962a922cc94b 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>> @@ -366,6 +366,10 @@ &cpsw3g {
>>>>  	pinctrl-names = "default";
>>>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>>>  		     &main_rgmii2_pins_default>;
>>>> +
>>>> +	cpts@3d000 {
>>>> +		ti,pps = <2 1>;
>>>> +	};
>>>>  };
>>>>  
>>>>  &cpsw_port1 {
>>>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>>>  		};
>>>>  	};
>>>>  };
>>>> +
>>>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>>> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
>>> That way every board DT file doesn't have to define it.
>>>
>>> The name should be made more platform specific.
>>> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
>>> If not then please add Platform name instead of K3.
>> The offsets are board specific. If it is acceptable, I will add board specific
>> macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
>> file. Please let me know.
> If it is board specific then it should remain in the board file.


The values you pass to macro maybe board specific. But the macro
definition itself same for a given SoC right? Also, is its same across
K3 family ?

Please use SoC specific prefix like AM62X_TS_OFFSET() or K3_TS_OFFSET()
accordingly.

Regards
Vignesh

Vignesh,

On 16/01/23 22:00, Vignesh Raghavendra wrote:
> 
> 
> On 16/01/23 9:35 pm, Roger Quadros wrote:
>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>> index 4f179b146cab..962a922cc94b 100644
>>>>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>> @@ -366,6 +366,10 @@ &cpsw3g {
>>>>>  	pinctrl-names = "default";
>>>>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>>>>  		     &main_rgmii2_pins_default>;
>>>>> +
>>>>> +	cpts@3d000 {
>>>>> +		ti,pps = <2 1>;
>>>>> +	};
>>>>>  };
>>>>>  
>>>>>  &cpsw_port1 {
>>>>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>>>>  		};
>>>>>  	};
>>>>>  };
>>>>> +
>>>>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>>>> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
>>>> That way every board DT file doesn't have to define it.
>>>>
>>>> The name should be made more platform specific.
>>>> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
>>>> If not then please add Platform name instead of K3.
>>> The offsets are board specific. If it is acceptable, I will add board specific
>>> macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
>>> file. Please let me know.
>> If it is board specific then it should remain in the board file.
> 
> 
> The values you pass to macro maybe board specific. But the macro
> definition itself same for a given SoC right? Also, is its same across
> K3 family ?
> 
> Please use SoC specific prefix like AM62X_TS_OFFSET() or K3_TS_OFFSET()
> accordingly.

For certain SoCs including AM62X, the macro is:
#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
while for other SoCs (refer [0]), the macro is:
#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x80000000 | val)

Therefore, I will use SoC specific prefix in the macro. Please let me know if
the SoC specific macro can be added to the ./include/dt-bindings/pinctrl/k3.h
file for each SoC. If not, I will add the SoC specific macro in the board file
itself.

[0] https://lwn.net/Articles/819313/

Regards,
Siddharth.

On 17/01/2023 07:28, Siddharth Vadapalli wrote:
> Vignesh,
> 
> On 16/01/23 22:00, Vignesh Raghavendra wrote:
>>
>>
>> On 16/01/23 9:35 pm, Roger Quadros wrote:
>>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>> index 4f179b146cab..962a922cc94b 100644
>>>>>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>> @@ -366,6 +366,10 @@ &cpsw3g {
>>>>>>  	pinctrl-names = "default";
>>>>>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>>>>>  		     &main_rgmii2_pins_default>;
>>>>>> +
>>>>>> +	cpts@3d000 {
>>>>>> +		ti,pps = <2 1>;
>>>>>> +	};
>>>>>>  };
>>>>>>  
>>>>>>  &cpsw_port1 {
>>>>>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>>>>>  		};
>>>>>>  	};
>>>>>>  };
>>>>>> +
>>>>>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>>>>> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
>>>>> That way every board DT file doesn't have to define it.
>>>>>
>>>>> The name should be made more platform specific.
>>>>> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
>>>>> If not then please add Platform name instead of K3.
>>>> The offsets are board specific. If it is acceptable, I will add board specific
>>>> macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
>>>> file. Please let me know.
>>> If it is board specific then it should remain in the board file.
>>
>>
>> The values you pass to macro maybe board specific. But the macro
>> definition itself same for a given SoC right? Also, is its same across
>> K3 family ?
>>

I misunderstood then. I agree with Vignesh.

>> Please use SoC specific prefix like AM62X_TS_OFFSET() or K3_TS_OFFSET()
>> accordingly.
> 
> For certain SoCs including AM62X, the macro is:
> #define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
> while for other SoCs (refer [0]), the macro is:
> #define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x80000000 | val)
> 
> Therefore, I will use SoC specific prefix in the macro. Please let me know if
> the SoC specific macro can be added to the ./include/dt-bindings/pinctrl/k3.h
> file for each SoC. If not, I will add the SoC specific macro in the board file
> itself.

Not in board file please. It should go in ./include/dt-bindings/pinctrl/k3.h

> 
> [0] https://lwn.net/Articles/819313/
> 
> Regards,
> Siddharth.

cheers,
-roger

Roger,

On 17/01/23 15:00, Roger Quadros wrote:
> On 17/01/2023 07:28, Siddharth Vadapalli wrote:
>> Vignesh,
>>
>> On 16/01/23 22:00, Vignesh Raghavendra wrote:
>>>
>>>
>>> On 16/01/23 9:35 pm, Roger Quadros wrote:
>>>>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-sk.dts b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>>> index 4f179b146cab..962a922cc94b 100644
>>>>>>> --- a/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>>> +++ b/arch/arm64/boot/dts/ti/k3-am625-sk.dts
>>>>>>> @@ -366,6 +366,10 @@ &cpsw3g {
>>>>>>>  	pinctrl-names = "default";
>>>>>>>  	pinctrl-0 = <&main_rgmii1_pins_default
>>>>>>>  		     &main_rgmii2_pins_default>;
>>>>>>> +
>>>>>>> +	cpts@3d000 {
>>>>>>> +		ti,pps = <2 1>;
>>>>>>> +	};
>>>>>>>  };
>>>>>>>  
>>>>>>>  &cpsw_port1 {
>>>>>>> @@ -464,3 +468,19 @@ partition@3fc0000 {
>>>>>>>  		};
>>>>>>>  	};
>>>>>>>  };
>>>>>>> +
>>>>>>> +#define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>>>>>> Should this go in ./include/dt-bindings/pinctrl/k3.h ?
>>>>>> That way every board DT file doesn't have to define it.
>>>>>>
>>>>>> The name should be made more platform specific.
>>>>>> e.g. K3_TS_OFFSET if it is the same for all K3 platforms.
>>>>>> If not then please add Platform name instead of K3.
>>>>> The offsets are board specific. If it is acceptable, I will add board specific
>>>>> macro for the TS_OFFSET definition in the ./include/dt-bindings/pinctrl/k3.h
>>>>> file. Please let me know.
>>>> If it is board specific then it should remain in the board file.
>>>
>>>
>>> The values you pass to macro maybe board specific. But the macro
>>> definition itself same for a given SoC right? Also, is its same across
>>> K3 family ?
>>>
> 
> I misunderstood then. I agree with Vignesh.
> 
>>> Please use SoC specific prefix like AM62X_TS_OFFSET() or K3_TS_OFFSET()
>>> accordingly.
>>
>> For certain SoCs including AM62X, the macro is:
>> #define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x10000 | val)
>> while for other SoCs (refer [0]), the macro is:
>> #define TS_OFFSET(pa, val)	(0x4+(pa)*4) (0x80000000 | val)
>>
>> Therefore, I will use SoC specific prefix in the macro. Please let me know if
>> the SoC specific macro can be added to the ./include/dt-bindings/pinctrl/k3.h
>> file for each SoC. If not, I will add the SoC specific macro in the board file
>> itself.
> 
> Not in board file please. It should go in ./include/dt-bindings/pinctrl/k3.h

Thank you for letting me know. I will do so in the device-tree series that I
will post once the bindings and driver patches get merged.

The v2 series for the bindings and driver patches that I am referring to is at:
https://lore.kernel.org/r/20230116085534.440820-1-s-vadapalli@ti.com/

Regards,
Siddharth.