The CPTS hardware doesn't support PPS signal generation. Using the GenFx
(periodic signal generator) function, it is possible to model a PPS signal
followed by routing it via the time sync router to the CPTS_HWy_TS_PUSH
(hardware time stamp) input, in order to generate timestamps at 1 second
intervals.
This series adds driver support for enabling PPS signal generation.
Additionally, the documentation for the am65-cpts driver is updated with
the bindings for the "ti,pps" property, which is used to inform the
pair [CPTS_HWy_TS_PUSH, GenFx] to the cpts driver. The PPS example is
enabled for AM625-SK board by default, by adding the timesync_router node
to the AM62x SoC, and configuring it for PPS in the AM625-SK board dts.
Grygorii Strashko (3):
dt-binding: net: ti: am65x-cpts: add 'ti,pps' property
net: ethernet: ti: am65-cpts: add pps support
net: ethernet: ti: am65-cpts: adjust pps following ptp changes
Siddharth Vadapalli (2):
arm64: dts: ti: k3-am62-main: Add timesync router node
arm64: dts: ti: k3-am625-sk: Add cpsw3g cpts PPS support
.../bindings/net/ti,k3-am654-cpts.yaml | 8 +
arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 9 ++
arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 +++
drivers/net/ethernet/ti/am65-cpts.c | 144 ++++++++++++++++--
4 files changed, 166 insertions(+), 15 deletions(-)
--
2.25.1
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 <[email protected]>
---
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)
+
+×ync_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
From: Grygorii Strashko <[email protected]>
CPTS doesn't have HW support for PPS ("pulse per second”) signal
generation, but it can be modeled by using Time Sync Router and routing
GenFx (periodic signal generator) output to CPTS_HWy_TS_PUSH (hardware time
stamp) input, and configuring GenFx to generate 1sec pulses.
+------------------------+
| CPTS |
| |
+--->CPTS_HW4_PUSH GENFx+---+
| | | |
| +------------------------+ |
| |
+--------------------------------+
Add corresponding support to am65-cpts driver. The DT property "ti,pps"
has to be used to enable PPS support and configure pair
[CPTS_HWy_TS_PUSH, GenFx].
Once enabled, PPS can be tested using ppstest tool:
# ./ppstest /dev/pps0
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpts.c | 85 +++++++++++++++++++++++++++--
1 file changed, 80 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index 9535396b28cd..6a0f09b497d1 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -176,6 +176,10 @@ struct am65_cpts {
u32 genf_enable;
u32 hw_ts_enable;
struct sk_buff_head txq;
+ bool pps_enabled;
+ bool pps_present;
+ u32 pps_hw_ts_idx;
+ u32 pps_genf_idx;
/* context save/restore */
u64 sr_cpts_ns;
u64 sr_ktime_ns;
@@ -319,8 +323,15 @@ static int am65_cpts_fifo_read(struct am65_cpts *cpts)
case AM65_CPTS_EV_HW:
pevent.index = am65_cpts_event_get_port(event) - 1;
pevent.timestamp = event->timestamp;
- pevent.type = PTP_CLOCK_EXTTS;
- dev_dbg(cpts->dev, "AM65_CPTS_EV_HW p:%d t:%llu\n",
+ if (cpts->pps_enabled && pevent.index == cpts->pps_hw_ts_idx) {
+ pevent.type = PTP_CLOCK_PPSUSR;
+ pevent.pps_times.ts_real = ns_to_timespec64(pevent.timestamp);
+ } else {
+ pevent.type = PTP_CLOCK_EXTTS;
+ }
+ dev_dbg(cpts->dev, "AM65_CPTS_EV_HW:%s p:%d t:%llu\n",
+ pevent.type == PTP_CLOCK_EXTTS ?
+ "extts" : "pps",
pevent.index, event->timestamp);
ptp_clock_event(cpts->ptp_clock, &pevent);
@@ -507,7 +518,13 @@ static void am65_cpts_extts_enable_hw(struct am65_cpts *cpts, u32 index, int on)
static int am65_cpts_extts_enable(struct am65_cpts *cpts, u32 index, int on)
{
- if (!!(cpts->hw_ts_enable & BIT(index)) == !!on)
+ if (index >= cpts->ptp_info.n_ext_ts)
+ return -ENXIO;
+
+ if (cpts->pps_present && index == cpts->pps_hw_ts_idx)
+ return -EINVAL;
+
+ if (((cpts->hw_ts_enable & BIT(index)) >> index) == on)
return 0;
mutex_lock(&cpts->ptp_clk_lock);
@@ -591,6 +608,12 @@ static void am65_cpts_perout_enable_hw(struct am65_cpts *cpts,
static int am65_cpts_perout_enable(struct am65_cpts *cpts,
struct ptp_perout_request *req, int on)
{
+ if (req->index >= cpts->ptp_info.n_per_out)
+ return -ENXIO;
+
+ if (cpts->pps_present && req->index == cpts->pps_genf_idx)
+ return -EINVAL;
+
if (!!(cpts->genf_enable & BIT(req->index)) == !!on)
return 0;
@@ -604,6 +627,48 @@ static int am65_cpts_perout_enable(struct am65_cpts *cpts,
return 0;
}
+static int am65_cpts_pps_enable(struct am65_cpts *cpts, int on)
+{
+ int ret = 0;
+ struct timespec64 ts;
+ struct ptp_clock_request rq;
+ u64 ns;
+
+ if (!cpts->pps_present)
+ return -EINVAL;
+
+ if (cpts->pps_enabled == !!on)
+ return 0;
+
+ mutex_lock(&cpts->ptp_clk_lock);
+
+ if (on) {
+ am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
+
+ ns = am65_cpts_gettime(cpts, NULL);
+ ts = ns_to_timespec64(ns);
+ rq.perout.period.sec = 1;
+ rq.perout.period.nsec = 0;
+ rq.perout.start.sec = ts.tv_sec + 2;
+ rq.perout.start.nsec = 0;
+ rq.perout.index = cpts->pps_genf_idx;
+
+ am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
+ cpts->pps_enabled = true;
+ } else {
+ rq.perout.index = cpts->pps_genf_idx;
+ am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
+ am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
+ cpts->pps_enabled = false;
+ }
+
+ mutex_unlock(&cpts->ptp_clk_lock);
+
+ dev_dbg(cpts->dev, "%s: pps: %s\n",
+ __func__, on ? "enabled" : "disabled");
+ return ret;
+}
+
static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
struct ptp_clock_request *rq, int on)
{
@@ -614,6 +679,8 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
return am65_cpts_extts_enable(cpts, rq->extts.index, on);
case PTP_CLK_REQ_PEROUT:
return am65_cpts_perout_enable(cpts, &rq->perout, on);
+ case PTP_CLK_REQ_PPS:
+ return am65_cpts_pps_enable(cpts, on);
default:
break;
}
@@ -926,6 +993,12 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
if (!of_property_read_u32(node, "ti,cpts-periodic-outputs", &prop[0]))
cpts->genf_num = prop[0];
+ if (!of_property_read_u32_array(node, "ti,pps", prop, 2)) {
+ cpts->pps_present = true;
+ cpts->pps_hw_ts_idx = prop[0];
+ cpts->pps_genf_idx = prop[1];
+ }
+
return cpts_of_mux_clk_setup(cpts, node);
}
@@ -993,6 +1066,8 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
cpts->ptp_info.n_ext_ts = cpts->ext_ts_inputs;
if (cpts->genf_num)
cpts->ptp_info.n_per_out = cpts->genf_num;
+ if (cpts->pps_present)
+ cpts->ptp_info.pps = 1;
am65_cpts_set_add_val(cpts);
@@ -1028,9 +1103,9 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
return ERR_PTR(ret);
}
- dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u\n",
+ dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u pps:%d\n",
am65_cpts_read32(cpts, idver),
- cpts->refclk_freq, cpts->ts_add_val);
+ cpts->refclk_freq, cpts->ts_add_val, cpts->pps_present);
return cpts;
--
2.25.1
From: Grygorii Strashko <[email protected]>
When CPTS clock is sync/adjusted by running linuxptp (ptp4l) it will cause
PPS jitter as Genf running PPS is not adjusted.
The same PPM adjustment has to be applied to GenF as to PHC clock to
correct PPS length and keep them in sync.
Testing:
Master:
ptp4l -P -2 -H -i eth0 -l 6 -m -q -p /dev/ptp1 -f ptp.cfg &
testptp -d /dev/ptp1 -P 1
ppstest /dev/pps0
Slave:
linuxptp/ptp4l -P -2 -H -i eth0 -l 6 -m -q -p /dev/ptp1 -f ptp1.cfg -s &
<port 1: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED;>
testptp -d /dev/ptp1 -P 1
ppstest /dev/pps0
Master log:
source 0 - assert 620.000000689, sequence: 530
source 0 - assert 621.000000689, sequence: 531
source 0 - assert 622.000000689, sequence: 532
source 0 - assert 623.000000689, sequence: 533
source 0 - assert 624.000000689, sequence: 534
source 0 - assert 625.000000689, sequence: 535
source 0 - assert 626.000000689, sequence: 536
source 0 - assert 627.000000689, sequence: 537
source 0 - assert 628.000000689, sequence: 538
source 0 - assert 629.000000689, sequence: 539
source 0 - assert 630.000000689, sequence: 540
source 0 - assert 631.000000689, sequence: 541
source 0 - assert 632.000000689, sequence: 542
source 0 - assert 633.000000689, sequence: 543
source 0 - assert 634.000000689, sequence: 544
source 0 - assert 635.000000689, sequence: 545
Slave log:
source 0 - assert 620.000000706, sequence: 252
source 0 - assert 621.000000709, sequence: 253
source 0 - assert 622.000000707, sequence: 254
source 0 - assert 623.000000707, sequence: 255
source 0 - assert 624.000000706, sequence: 256
source 0 - assert 625.000000705, sequence: 257
source 0 - assert 626.000000709, sequence: 258
source 0 - assert 627.000000709, sequence: 259
source 0 - assert 628.000000707, sequence: 260
source 0 - assert 629.000000706, sequence: 261
source 0 - assert 630.000000710, sequence: 262
source 0 - assert 631.000000708, sequence: 263
source 0 - assert 632.000000705, sequence: 264
source 0 - assert 633.000000710, sequence: 265
source 0 - assert 634.000000708, sequence: 266
source 0 - assert 635.000000707, sequence: 267
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/ethernet/ti/am65-cpts.c | 59 ++++++++++++++++++++++++-----
1 file changed, 49 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index 6a0f09b497d1..8d76ae28e238 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -405,10 +405,13 @@ static irqreturn_t am65_cpts_interrupt(int irq, void *dev_id)
static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
{
struct am65_cpts *cpts = container_of(ptp, struct am65_cpts, ptp_info);
+ u32 pps_ctrl_val = 0, pps_ppm_hi = 0, pps_ppm_low = 0;
s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
+ int pps_index = cpts->pps_genf_idx;
+ u64 adj_period, pps_adj_period;
+ u32 ctrl_val, ppm_hi, ppm_low;
+ unsigned long flags;
int neg_adj = 0;
- u64 adj_period;
- u32 val;
if (ppb < 0) {
neg_adj = 1;
@@ -428,17 +431,53 @@ static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
mutex_lock(&cpts->ptp_clk_lock);
- val = am65_cpts_read32(cpts, control);
+ ctrl_val = am65_cpts_read32(cpts, control);
if (neg_adj)
- val |= AM65_CPTS_CONTROL_TS_PPM_DIR;
+ ctrl_val |= AM65_CPTS_CONTROL_TS_PPM_DIR;
else
- val &= ~AM65_CPTS_CONTROL_TS_PPM_DIR;
- am65_cpts_write32(cpts, val, control);
+ ctrl_val &= ~AM65_CPTS_CONTROL_TS_PPM_DIR;
+
+ ppm_hi = upper_32_bits(adj_period) & 0x3FF;
+ ppm_low = lower_32_bits(adj_period);
+
+ if (cpts->pps_enabled) {
+ pps_ctrl_val = am65_cpts_read32(cpts, genf[pps_index].control);
+ if (neg_adj)
+ pps_ctrl_val &= ~BIT(1);
+ else
+ pps_ctrl_val |= BIT(1);
+
+ /* GenF PPM will do correction using cpts refclk tick which is
+ * (cpts->ts_add_val + 1) ns, so GenF length PPM adj period
+ * need to be corrected.
+ */
+ pps_adj_period = adj_period * (cpts->ts_add_val + 1);
+ pps_ppm_hi = upper_32_bits(pps_adj_period) & 0x3FF;
+ pps_ppm_low = lower_32_bits(pps_adj_period);
+ }
+
+ spin_lock_irqsave(&cpts->lock, flags);
- val = upper_32_bits(adj_period) & 0x3FF;
- am65_cpts_write32(cpts, val, ts_ppm_hi);
- val = lower_32_bits(adj_period);
- am65_cpts_write32(cpts, val, ts_ppm_low);
+ /* All below writes must be done extremely fast:
+ * - delay between PPM dir and PPM value changes can cause err due old
+ * PPM correction applied in wrong direction
+ * - delay between CPTS-clock PPM cfg and GenF PPM cfg can cause err
+ * due CPTS-clock PPM working with new cfg while GenF PPM cfg still
+ * with old for short period of time
+ */
+
+ am65_cpts_write32(cpts, ctrl_val, control);
+ am65_cpts_write32(cpts, ppm_hi, ts_ppm_hi);
+ am65_cpts_write32(cpts, ppm_low, ts_ppm_low);
+
+ if (cpts->pps_enabled) {
+ am65_cpts_write32(cpts, pps_ctrl_val, genf[pps_index].control);
+ am65_cpts_write32(cpts, pps_ppm_hi, genf[pps_index].ppm_hi);
+ am65_cpts_write32(cpts, pps_ppm_low, genf[pps_index].ppm_low);
+ }
+
+ /* All GenF/EstF can be updated here the same way */
+ spin_unlock_irqrestore(&cpts->lock, flags);
mutex_unlock(&cpts->ptp_clk_lock);
--
2.25.1
TI's AM62x SoC has a Time Sync Event Router, which enables routing a single
input signal to multiple recipients. This facilitates syncing all the
peripherals or processor cores to the input signal which acts as a master
clock.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
index 072903649d6e..4ce59170b6a7 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
@@ -649,6 +649,15 @@ cpts@3d000 {
};
};
+ timesync_router: pinctrl@a40000 {
+ compatible = "pinctrl-single";
+ reg = <0x0 0xa40000 0x0 0x800>;
+ #pinctrl-cells = <1>;
+ pinctrl-single,register-width = <32>;
+ pinctrl-single,function-mask = <0x000107ff>;
+ status = "disabled";
+ };
+
hwspinlock: spinlock@2a000000 {
compatible = "ti,am64-hwspinlock";
reg = <0x00 0x2a000000 0x00 0x1000>;
--
2.25.1
Siddharth,
On 11/01/2023 13:44, Siddharth Vadapalli wrote:
> The CPTS hardware doesn't support PPS signal generation. Using the GenFx
> (periodic signal generator) function, it is possible to model a PPS signal
> followed by routing it via the time sync router to the CPTS_HWy_TS_PUSH
> (hardware time stamp) input, in order to generate timestamps at 1 second
> intervals.
>
> This series adds driver support for enabling PPS signal generation.
> Additionally, the documentation for the am65-cpts driver is updated with
> the bindings for the "ti,pps" property, which is used to inform the
> pair [CPTS_HWy_TS_PUSH, GenFx] to the cpts driver. The PPS example is
> enabled for AM625-SK board by default, by adding the timesync_router node
> to the AM62x SoC, and configuring it for PPS in the AM625-SK board dts.
>
> Grygorii Strashko (3):
> dt-binding: net: ti: am65x-cpts: add 'ti,pps' property
> net: ethernet: ti: am65-cpts: add pps support
> net: ethernet: ti: am65-cpts: adjust pps following ptp changes
>
> Siddharth Vadapalli (2):
> arm64: dts: ti: k3-am62-main: Add timesync router node
> arm64: dts: ti: k3-am625-sk: Add cpsw3g cpts PPS support
Device tree patches need to be sent separately. You don't need to involve
net maintainers for that.
If you introduce a new binding then that needs to be in maintainer's
tree before you can send a related device tree patch.
>
> .../bindings/net/ti,k3-am654-cpts.yaml | 8 +
> arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 9 ++
> arch/arm64/boot/dts/ti/k3-am625-sk.dts | 20 +++
> drivers/net/ethernet/ti/am65-cpts.c | 144 ++++++++++++++++--
> 4 files changed, 166 insertions(+), 15 deletions(-)
>
cheers,
-roger
Hi,
On 11/01/2023 13:44, Siddharth Vadapalli wrote:
> From: Grygorii Strashko <[email protected]>
>
> CPTS doesn't have HW support for PPS ("pulse per second”) signal
> generation, but it can be modeled by using Time Sync Router and routing
> GenFx (periodic signal generator) output to CPTS_HWy_TS_PUSH (hardware time
> stamp) input, and configuring GenFx to generate 1sec pulses.
>
> +------------------------+
> | CPTS |
> | |
> +--->CPTS_HW4_PUSH GENFx+---+
> | | | |
> | +------------------------+ |
> | |
> +--------------------------------+
>
> Add corresponding support to am65-cpts driver. The DT property "ti,pps"
> has to be used to enable PPS support and configure pair
> [CPTS_HWy_TS_PUSH, GenFx].
>
> Once enabled, PPS can be tested using ppstest tool:
> # ./ppstest /dev/pps0
>
> Signed-off-by: Grygorii Strashko <[email protected]>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
> drivers/net/ethernet/ti/am65-cpts.c | 85 +++++++++++++++++++++++++++--
> 1 file changed, 80 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index 9535396b28cd..6a0f09b497d1 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -176,6 +176,10 @@ struct am65_cpts {
> u32 genf_enable;
> u32 hw_ts_enable;
> struct sk_buff_head txq;
> + bool pps_enabled;
> + bool pps_present;
> + u32 pps_hw_ts_idx;
> + u32 pps_genf_idx;
> /* context save/restore */
> u64 sr_cpts_ns;
> u64 sr_ktime_ns;
> @@ -319,8 +323,15 @@ static int am65_cpts_fifo_read(struct am65_cpts *cpts)
> case AM65_CPTS_EV_HW:
> pevent.index = am65_cpts_event_get_port(event) - 1;
> pevent.timestamp = event->timestamp;
> - pevent.type = PTP_CLOCK_EXTTS;
> - dev_dbg(cpts->dev, "AM65_CPTS_EV_HW p:%d t:%llu\n",
> + if (cpts->pps_enabled && pevent.index == cpts->pps_hw_ts_idx) {
> + pevent.type = PTP_CLOCK_PPSUSR;
> + pevent.pps_times.ts_real = ns_to_timespec64(pevent.timestamp);
> + } else {
> + pevent.type = PTP_CLOCK_EXTTS;
> + }
> + dev_dbg(cpts->dev, "AM65_CPTS_EV_HW:%s p:%d t:%llu\n",
> + pevent.type == PTP_CLOCK_EXTTS ?
> + "extts" : "pps",
> pevent.index, event->timestamp);
>
> ptp_clock_event(cpts->ptp_clock, &pevent);
> @@ -507,7 +518,13 @@ static void am65_cpts_extts_enable_hw(struct am65_cpts *cpts, u32 index, int on)
>
> static int am65_cpts_extts_enable(struct am65_cpts *cpts, u32 index, int on)
> {
> - if (!!(cpts->hw_ts_enable & BIT(index)) == !!on)
> + if (index >= cpts->ptp_info.n_ext_ts)
> + return -ENXIO;
> +
> + if (cpts->pps_present && index == cpts->pps_hw_ts_idx)
> + return -EINVAL;
> +
> + if (((cpts->hw_ts_enable & BIT(index)) >> index) == on)
> return 0;
>
> mutex_lock(&cpts->ptp_clk_lock);
> @@ -591,6 +608,12 @@ static void am65_cpts_perout_enable_hw(struct am65_cpts *cpts,
> static int am65_cpts_perout_enable(struct am65_cpts *cpts,
> struct ptp_perout_request *req, int on)
> {
> + if (req->index >= cpts->ptp_info.n_per_out)
> + return -ENXIO;
> +
> + if (cpts->pps_present && req->index == cpts->pps_genf_idx)
> + return -EINVAL;
> +
> if (!!(cpts->genf_enable & BIT(req->index)) == !!on)
> return 0;
>
> @@ -604,6 +627,48 @@ static int am65_cpts_perout_enable(struct am65_cpts *cpts,
> return 0;
> }
>
> +static int am65_cpts_pps_enable(struct am65_cpts *cpts, int on)
> +{
> + int ret = 0;
> + struct timespec64 ts;
> + struct ptp_clock_request rq;
> + u64 ns;
> +
> + if (!cpts->pps_present)
> + return -EINVAL;
> +
> + if (cpts->pps_enabled == !!on)
> + return 0;
> +
> + mutex_lock(&cpts->ptp_clk_lock);
> +
> + if (on) {
> + am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
> +
> + ns = am65_cpts_gettime(cpts, NULL);
> + ts = ns_to_timespec64(ns);
> + rq.perout.period.sec = 1;
> + rq.perout.period.nsec = 0;
> + rq.perout.start.sec = ts.tv_sec + 2;
> + rq.perout.start.nsec = 0;
> + rq.perout.index = cpts->pps_genf_idx;
> +
> + am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
> + cpts->pps_enabled = true;
> + } else {
> + rq.perout.index = cpts->pps_genf_idx;
> + am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
> + am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
> + cpts->pps_enabled = false;
> + }
> +
> + mutex_unlock(&cpts->ptp_clk_lock);
> +
> + dev_dbg(cpts->dev, "%s: pps: %s\n",
> + __func__, on ? "enabled" : "disabled");
> + return ret;
> +}
> +
> static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
> struct ptp_clock_request *rq, int on)
> {
> @@ -614,6 +679,8 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
> return am65_cpts_extts_enable(cpts, rq->extts.index, on);
> case PTP_CLK_REQ_PEROUT:
> return am65_cpts_perout_enable(cpts, &rq->perout, on);
> + case PTP_CLK_REQ_PPS:
> + return am65_cpts_pps_enable(cpts, on);
> default:
> break;
> }
> @@ -926,6 +993,12 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
> if (!of_property_read_u32(node, "ti,cpts-periodic-outputs", &prop[0]))
> cpts->genf_num = prop[0];
>
> + if (!of_property_read_u32_array(node, "ti,pps", prop, 2)) {
> + cpts->pps_present = true;
> + cpts->pps_hw_ts_idx = prop[0];
> + cpts->pps_genf_idx = prop[1];
What happens if DT provides an invalid value. e.g. out of range?
Better to do a sanity check?
> + }
> +
> return cpts_of_mux_clk_setup(cpts, node);
> }
>
> @@ -993,6 +1066,8 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
> cpts->ptp_info.n_ext_ts = cpts->ext_ts_inputs;
> if (cpts->genf_num)
> cpts->ptp_info.n_per_out = cpts->genf_num;
> + if (cpts->pps_present)
> + cpts->ptp_info.pps = 1;
>
> am65_cpts_set_add_val(cpts);
>
> @@ -1028,9 +1103,9 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
> return ERR_PTR(ret);
> }
>
> - dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u\n",
> + dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u pps:%d\n",
> am65_cpts_read32(cpts, idver),
> - cpts->refclk_freq, cpts->ts_add_val);
> + cpts->refclk_freq, cpts->ts_add_val, cpts->pps_present);
>
> return cpts;
>
cheers,
-roger
Hello Roger,
On 13/01/23 15:18, Roger Quadros wrote:
> Siddharth,
>
> On 11/01/2023 13:44, Siddharth Vadapalli wrote:
>> The CPTS hardware doesn't support PPS signal generation. Using the GenFx
>> (periodic signal generator) function, it is possible to model a PPS signal
>> followed by routing it via the time sync router to the CPTS_HWy_TS_PUSH
>> (hardware time stamp) input, in order to generate timestamps at 1 second
>> intervals.
>>
>> This series adds driver support for enabling PPS signal generation.
>> Additionally, the documentation for the am65-cpts driver is updated with
>> the bindings for the "ti,pps" property, which is used to inform the
>> pair [CPTS_HWy_TS_PUSH, GenFx] to the cpts driver. The PPS example is
>> enabled for AM625-SK board by default, by adding the timesync_router node
>> to the AM62x SoC, and configuring it for PPS in the AM625-SK board dts.
>>
>> Grygorii Strashko (3):
>> dt-binding: net: ti: am65x-cpts: add 'ti,pps' property
>> net: ethernet: ti: am65-cpts: add pps support
>> net: ethernet: ti: am65-cpts: adjust pps following ptp changes
>>
>> Siddharth Vadapalli (2):
>> arm64: dts: ti: k3-am62-main: Add timesync router node
>> arm64: dts: ti: k3-am625-sk: Add cpsw3g cpts PPS support
>
> Device tree patches need to be sent separately. You don't need to involve
> net maintainers for that.
>
> If you introduce a new binding then that needs to be in maintainer's
> tree before you can send a related device tree patch.
Thank you for letting me know. Would I need to resend the series in order for it
to be reviewed? I was hoping that if I get feedback for this series, I will
implement it and post just the bindings and driver patches as the v2 series,
dropping the device tree patches. Please let me know.
Regards,
Siddharth.
On 13/01/2023 11:56, Siddharth Vadapalli wrote:
> Hello Roger,
>
> On 13/01/23 15:18, Roger Quadros wrote:
>> Siddharth,
>>
>> On 11/01/2023 13:44, Siddharth Vadapalli wrote:
>>> The CPTS hardware doesn't support PPS signal generation. Using the GenFx
>>> (periodic signal generator) function, it is possible to model a PPS signal
>>> followed by routing it via the time sync router to the CPTS_HWy_TS_PUSH
>>> (hardware time stamp) input, in order to generate timestamps at 1 second
>>> intervals.
>>>
>>> This series adds driver support for enabling PPS signal generation.
>>> Additionally, the documentation for the am65-cpts driver is updated with
>>> the bindings for the "ti,pps" property, which is used to inform the
>>> pair [CPTS_HWy_TS_PUSH, GenFx] to the cpts driver. The PPS example is
>>> enabled for AM625-SK board by default, by adding the timesync_router node
>>> to the AM62x SoC, and configuring it for PPS in the AM625-SK board dts.
>>>
>>> Grygorii Strashko (3):
>>> dt-binding: net: ti: am65x-cpts: add 'ti,pps' property
>>> net: ethernet: ti: am65-cpts: add pps support
>>> net: ethernet: ti: am65-cpts: adjust pps following ptp changes
>>>
>>> Siddharth Vadapalli (2):
>>> arm64: dts: ti: k3-am62-main: Add timesync router node
>>> arm64: dts: ti: k3-am625-sk: Add cpsw3g cpts PPS support
>>
>> Device tree patches need to be sent separately. You don't need to involve
>> net maintainers for that.
>>
>> If you introduce a new binding then that needs to be in maintainer's
>> tree before you can send a related device tree patch.
>
> Thank you for letting me know. Would I need to resend the series in order for it
> to be reviewed? I was hoping that if I get feedback for this series, I will
> implement it and post just the bindings and driver patches as the v2 series,
> dropping the device tree patches. Please let me know.
You could wait a couple of days for more comments here before spinning off a v2 ;)
cheers,
-roger
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 <[email protected]>
> ---
> 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.
> +
> +×ync_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
Hello Roger,
On 13/01/23 15:27, Roger Quadros wrote:
> Hi,
>
> On 11/01/2023 13:44, Siddharth Vadapalli wrote:
>> From: Grygorii Strashko <[email protected]>
>>
>> CPTS doesn't have HW support for PPS ("pulse per second”) signal
>> generation, but it can be modeled by using Time Sync Router and routing
>> GenFx (periodic signal generator) output to CPTS_HWy_TS_PUSH (hardware time
>> stamp) input, and configuring GenFx to generate 1sec pulses.
>>
>> +------------------------+
>> | CPTS |
>> | |
>> +--->CPTS_HW4_PUSH GENFx+---+
>> | | | |
>> | +------------------------+ |
>> | |
>> +--------------------------------+
>>
>> Add corresponding support to am65-cpts driver. The DT property "ti,pps"
>> has to be used to enable PPS support and configure pair
>> [CPTS_HWy_TS_PUSH, GenFx].
>>
>> Once enabled, PPS can be tested using ppstest tool:
>> # ./ppstest /dev/pps0
>>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> Signed-off-by: Siddharth Vadapalli <[email protected]>
>> ---
>> drivers/net/ethernet/ti/am65-cpts.c | 85 +++++++++++++++++++++++++++--
>> 1 file changed, 80 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>> index 9535396b28cd..6a0f09b497d1 100644
>> --- a/drivers/net/ethernet/ti/am65-cpts.c
>> +++ b/drivers/net/ethernet/ti/am65-cpts.c
>> @@ -176,6 +176,10 @@ struct am65_cpts {
>> u32 genf_enable;
>> u32 hw_ts_enable;
>> struct sk_buff_head txq;
>> + bool pps_enabled;
>> + bool pps_present;
>> + u32 pps_hw_ts_idx;
>> + u32 pps_genf_idx;
>> /* context save/restore */
>> u64 sr_cpts_ns;
>> u64 sr_ktime_ns;
>> @@ -319,8 +323,15 @@ static int am65_cpts_fifo_read(struct am65_cpts *cpts)
>> case AM65_CPTS_EV_HW:
>> pevent.index = am65_cpts_event_get_port(event) - 1;
>> pevent.timestamp = event->timestamp;
>> - pevent.type = PTP_CLOCK_EXTTS;
>> - dev_dbg(cpts->dev, "AM65_CPTS_EV_HW p:%d t:%llu\n",
>> + if (cpts->pps_enabled && pevent.index == cpts->pps_hw_ts_idx) {
>> + pevent.type = PTP_CLOCK_PPSUSR;
>> + pevent.pps_times.ts_real = ns_to_timespec64(pevent.timestamp);
>> + } else {
>> + pevent.type = PTP_CLOCK_EXTTS;
>> + }
>> + dev_dbg(cpts->dev, "AM65_CPTS_EV_HW:%s p:%d t:%llu\n",
>> + pevent.type == PTP_CLOCK_EXTTS ?
>> + "extts" : "pps",
>> pevent.index, event->timestamp);
>>
>> ptp_clock_event(cpts->ptp_clock, &pevent);
>> @@ -507,7 +518,13 @@ static void am65_cpts_extts_enable_hw(struct am65_cpts *cpts, u32 index, int on)
>>
>> static int am65_cpts_extts_enable(struct am65_cpts *cpts, u32 index, int on)
>> {
>> - if (!!(cpts->hw_ts_enable & BIT(index)) == !!on)
>> + if (index >= cpts->ptp_info.n_ext_ts)
>> + return -ENXIO;
>> +
>> + if (cpts->pps_present && index == cpts->pps_hw_ts_idx)
>> + return -EINVAL;
>> +
>> + if (((cpts->hw_ts_enable & BIT(index)) >> index) == on)
>> return 0;
>>
>> mutex_lock(&cpts->ptp_clk_lock);
>> @@ -591,6 +608,12 @@ static void am65_cpts_perout_enable_hw(struct am65_cpts *cpts,
>> static int am65_cpts_perout_enable(struct am65_cpts *cpts,
>> struct ptp_perout_request *req, int on)
>> {
>> + if (req->index >= cpts->ptp_info.n_per_out)
>> + return -ENXIO;
>> +
>> + if (cpts->pps_present && req->index == cpts->pps_genf_idx)
>> + return -EINVAL;
>> +
>> if (!!(cpts->genf_enable & BIT(req->index)) == !!on)
>> return 0;
>>
>> @@ -604,6 +627,48 @@ static int am65_cpts_perout_enable(struct am65_cpts *cpts,
>> return 0;
>> }
>>
>> +static int am65_cpts_pps_enable(struct am65_cpts *cpts, int on)
>> +{
>> + int ret = 0;
>> + struct timespec64 ts;
>> + struct ptp_clock_request rq;
>> + u64 ns;
>> +
>> + if (!cpts->pps_present)
>> + return -EINVAL;
>> +
>> + if (cpts->pps_enabled == !!on)
>> + return 0;
>> +
>> + mutex_lock(&cpts->ptp_clk_lock);
>> +
>> + if (on) {
>> + am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
>> +
>> + ns = am65_cpts_gettime(cpts, NULL);
>> + ts = ns_to_timespec64(ns);
>> + rq.perout.period.sec = 1;
>> + rq.perout.period.nsec = 0;
>> + rq.perout.start.sec = ts.tv_sec + 2;
>> + rq.perout.start.nsec = 0;
>> + rq.perout.index = cpts->pps_genf_idx;
>> +
>> + am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
>> + cpts->pps_enabled = true;
>> + } else {
>> + rq.perout.index = cpts->pps_genf_idx;
>> + am65_cpts_perout_enable_hw(cpts, &rq.perout, on);
>> + am65_cpts_extts_enable_hw(cpts, cpts->pps_hw_ts_idx, on);
>> + cpts->pps_enabled = false;
>> + }
>> +
>> + mutex_unlock(&cpts->ptp_clk_lock);
>> +
>> + dev_dbg(cpts->dev, "%s: pps: %s\n",
>> + __func__, on ? "enabled" : "disabled");
>> + return ret;
>> +}
>> +
>> static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
>> struct ptp_clock_request *rq, int on)
>> {
>> @@ -614,6 +679,8 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,
>> return am65_cpts_extts_enable(cpts, rq->extts.index, on);
>> case PTP_CLK_REQ_PEROUT:
>> return am65_cpts_perout_enable(cpts, &rq->perout, on);
>> + case PTP_CLK_REQ_PPS:
>> + return am65_cpts_pps_enable(cpts, on);
>> default:
>> break;
>> }
>> @@ -926,6 +993,12 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
>> if (!of_property_read_u32(node, "ti,cpts-periodic-outputs", &prop[0]))
>> cpts->genf_num = prop[0];
>>
>> + if (!of_property_read_u32_array(node, "ti,pps", prop, 2)) {
>> + cpts->pps_present = true;
>> + cpts->pps_hw_ts_idx = prop[0];
>> + cpts->pps_genf_idx = prop[1];
>
> What happens if DT provides an invalid value. e.g. out of range?
> Better to do a sanity check?
Thank you for pointing it out. The pps_hw_ts_idx values range from 0 to 7, while
the pps_genf_idx values range from 0 to 1. I will implement this check and
default to index 0 if an invalid value is provided, along with a dev_err print
to inform this error.
Regards,
Siddharth.
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 <[email protected]>
>> ---
>> 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/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
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 <[email protected]>
>>> ---
>>> 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
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/[email protected]/
Regards,
Siddharth.
On 17:14-20230111, Siddharth Vadapalli wrote:
> TI's AM62x SoC has a Time Sync Event Router, which enables routing a single
> input signal to multiple recipients. This facilitates syncing all the
> peripherals or processor cores to the input signal which acts as a master
> clock.
>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
> arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> index 072903649d6e..4ce59170b6a7 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> @@ -649,6 +649,15 @@ cpts@3d000 {
> };
> };
>
> + timesync_router: pinctrl@a40000 {
> + compatible = "pinctrl-single";
While I understand that the timesync router is essentially a mux,
pinctrl-single is a specific mux model that is used to model external
facing pins to internal signals - pin mux sections of control module
which is already in place is an example of the same.
Using the pinctrl-single scheme for timesync router is, IMHO, wrong
and limiting to potential functions that timesync router could need
enabling.
Is there a reason for using pinctrl-single rather than writing a
mux-controller / consumer model driver instead or rather simpler a
reg-mux node?
> + reg = <0x0 0xa40000 0x0 0x800>;
> + #pinctrl-cells = <1>;
> + pinctrl-single,register-width = <32>;
> + pinctrl-single,function-mask = <0x000107ff>;
> + status = "disabled";
> + };
> +
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D