[PATCH net-next 0/9] mlxsw: Add support for physical hardware clock

[PATCH net-next 0/9] mlxsw: Add support for physical hardware clock

[Ido Schimmel]

From: Ido Schimmel <idosch@mellanox.com>

This is the first of about four patchsets that add PTP support in mlxsw
for the Spectrum-1 ASIC.

This patchset exposes the physical hardware clock, while subsequent
patchsets will add timestamping support and mlxsw tracepoints for
debugging and testing.

Shalom says:

This patchset adds support for physical hardware clock for Spectrum-1
ASIC only.

Patches #1, #2 and #3 add the ability to query the free running clock
PCI address.

Patches #4 and #5 add two new register, the Management UTC Register and
the Management Pulse Per Second Register.

Patch #6 publishes scaled_ppm_to_ppb() to allow drivers to use it.

Patch #7 adds the physical hardware clock operations.

Patch #8 initializes the physical hardware clock.

Patch #9 adds a selftest for testing the PTP physical hardware clock.

Shalom Toledo (9):
  mlxsw: cmd: Free running clock PCI BAR and offsets via query firmware
  mlxsw: core: Add a new interface for reading the hardware free running
    clock
  mlxsw: pci: Query free running clock PCI BAR and offsets
  mlxsw: reg: Add Management UTC Register
  mlxsw: reg: Add Management Pulse Per Second Register
  ptp: ptp_clock: Publish scaled_ppm_to_ppb
  mlxsw: spectrum_ptp: Add implementation for physical hardware clock
    operations
  mlxsw: spectrum: PTP physical hardware clock initialization
  selftests: ptp: Add Physical Hardware Clock test

 drivers/net/ethernet/mellanox/mlxsw/Makefile  |   1 +
 drivers/net/ethernet/mellanox/mlxsw/cmd.h     |  12 +
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  12 +
 drivers/net/ethernet/mellanox/mlxsw/core.h    |   8 +-
 drivers/net/ethernet/mellanox/mlxsw/pci.c     |  32 +++
 drivers/net/ethernet/mellanox/mlxsw/pci_hw.h  |   3 +
 drivers/net/ethernet/mellanox/mlxsw/reg.h     | 103 +++++++
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  36 +++
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   3 +
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    | 267 ++++++++++++++++++
 .../ethernet/mellanox/mlxsw/spectrum_ptp.h    |  44 +++
 drivers/ptp/ptp_clock.c                       |   5 +-
 include/linux/ptp_clock_kernel.h              |   8 +
 tools/testing/selftests/ptp/phc.sh            | 166 +++++++++++
 14 files changed, 697 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
 create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
 create mode 100755 tools/testing/selftests/ptp/phc.sh

-- 
2.20.1

[PATCH net-next 1/9] mlxsw: cmd: Free running clock PCI BAR and offsets via query firmware

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Add free running clock PCI BAR and offset to query firmware command.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/cmd.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/cmd.h b/drivers/net/ethernet/mellanox/mlxsw/cmd.h
index 0772e4339b33..5ffdfb532cb7 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/cmd.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/cmd.h
@@ -317,6 +317,18 @@ MLXSW_ITEM64(cmd_mbox, query_fw, doorbell_page_offset, 0x40, 0, 64);
  */
 MLXSW_ITEM32(cmd_mbox, query_fw, doorbell_page_bar, 0x48, 30, 2);
 
+/* cmd_mbox_query_fw_free_running_clock_offset
+ * The offset of the free running clock page
+ */
+MLXSW_ITEM64(cmd_mbox, query_fw, free_running_clock_offset, 0x50, 0, 64);
+
+/* cmd_mbox_query_fw_fr_rn_clk_bar
+ * PCI base address register (BAR) of the free running clock page
+ * 0: BAR 0
+ * 1: 64 bit BAR
+ */
+MLXSW_ITEM32(cmd_mbox, query_fw, fr_rn_clk_bar, 0x58, 30, 2);
+
 /* QUERY_BOARDINFO - Query Board Information
  * -----------------------------------------
  * OpMod == 0 (N/A), INMmod == 0 (N/A)
-- 
2.20.1

[PATCH net-next 2/9] mlxsw: core: Add a new interface for reading the hardware free running clock

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Add two new bus operations for reading the hardware free running clock.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 12 ++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/core.h |  8 +++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 182762898361..b49a6520386c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -2011,6 +2011,18 @@ int mlxsw_core_resources_query(struct mlxsw_core *mlxsw_core, char *mbox,
 }
 EXPORT_SYMBOL(mlxsw_core_resources_query);
 
+u32 mlxsw_core_read_frc_h(struct mlxsw_core *mlxsw_core)
+{
+	return mlxsw_core->bus->read_frc_h(mlxsw_core->bus_priv);
+}
+EXPORT_SYMBOL(mlxsw_core_read_frc_h);
+
+u32 mlxsw_core_read_frc_l(struct mlxsw_core *mlxsw_core)
+{
+	return mlxsw_core->bus->read_frc_l(mlxsw_core->bus_priv);
+}
+EXPORT_SYMBOL(mlxsw_core_read_frc_l);
+
 static int __init mlxsw_core_module_init(void)
 {
 	int err;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index e3832cb5bdda..02d1e580e44f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -306,6 +306,9 @@ int mlxsw_core_kvd_sizes_get(struct mlxsw_core *mlxsw_core,
 void mlxsw_core_fw_flash_start(struct mlxsw_core *mlxsw_core);
 void mlxsw_core_fw_flash_end(struct mlxsw_core *mlxsw_core);
 
+u32 mlxsw_core_read_frc_h(struct mlxsw_core *mlxsw_core);
+u32 mlxsw_core_read_frc_l(struct mlxsw_core *mlxsw_core);
+
 bool mlxsw_core_res_valid(struct mlxsw_core *mlxsw_core,
 			  enum mlxsw_res_id res_id);
 
@@ -336,6 +339,8 @@ struct mlxsw_bus {
 			char *in_mbox, size_t in_mbox_size,
 			char *out_mbox, size_t out_mbox_size,
 			u8 *p_status);
+	u32 (*read_frc_h)(void *bus_priv);
+	u32 (*read_frc_l)(void *bus_priv);
 	u8 features;
 };
 
@@ -353,7 +358,8 @@ struct mlxsw_bus_info {
 	struct mlxsw_fw_rev fw_rev;
 	u8 vsd[MLXSW_CMD_BOARDINFO_VSD_LEN];
 	u8 psid[MLXSW_CMD_BOARDINFO_PSID_LEN];
-	u8 low_frequency;
+	u8 low_frequency:1,
+	   read_frc_capable:1;
 };
 
 struct mlxsw_hwmon;
-- 
2.20.1

[PATCH net-next 3/9] mlxsw: pci: Query free running clock PCI BAR and offsets

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Query free running clock PCI BAR and offsets during the pci_init.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c    | 32 ++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/pci_hw.h |  3 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index b40455f8293d..6acb9bbfdf89 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -102,6 +102,7 @@ struct mlxsw_pci_queue_type_group {
 struct mlxsw_pci {
 	struct pci_dev *pdev;
 	u8 __iomem *hw_addr;
+	u64 free_running_clock_offset;
 	struct mlxsw_pci_queue_type_group queues[MLXSW_PCI_QUEUE_TYPE_COUNT];
 	u32 doorbell_offset;
 	struct mlxsw_core *core;
@@ -1414,6 +1415,15 @@ static int mlxsw_pci_init(void *bus_priv, struct mlxsw_core *mlxsw_core,
 	mlxsw_pci->doorbell_offset =
 		mlxsw_cmd_mbox_query_fw_doorbell_page_offset_get(mbox);
 
+	if (mlxsw_cmd_mbox_query_fw_fr_rn_clk_bar_get(mbox) != 0) {
+		dev_err(&pdev->dev, "Unsupported free running clock BAR queried from hw\n");
+		err = -EINVAL;
+		goto err_fr_rn_clk_bar;
+	}
+
+	mlxsw_pci->free_running_clock_offset =
+		mlxsw_cmd_mbox_query_fw_free_running_clock_offset_get(mbox);
+
 	num_pages = mlxsw_cmd_mbox_query_fw_fw_pages_get(mbox);
 	err = mlxsw_pci_fw_area_init(mlxsw_pci, mbox, num_pages);
 	if (err)
@@ -1469,6 +1479,7 @@ static int mlxsw_pci_init(void *bus_priv, struct mlxsw_core *mlxsw_core,
 err_boardinfo:
 	mlxsw_pci_fw_area_fini(mlxsw_pci);
 err_fw_area_init:
+err_fr_rn_clk_bar:
 err_doorbell_page_bar:
 err_iface_rev:
 err_query_fw:
@@ -1672,6 +1683,24 @@ static int mlxsw_pci_cmd_exec(void *bus_priv, u16 opcode, u8 opcode_mod,
 	return err;
 }
 
+static u32 mlxsw_pci_read_frc_h(void *bus_priv)
+{
+	struct mlxsw_pci *mlxsw_pci = bus_priv;
+	u64 frc_offset;
+
+	frc_offset = mlxsw_pci->free_running_clock_offset;
+	return mlxsw_pci_read32(mlxsw_pci, FREE_RUNNING_CLOCK_H(frc_offset));
+}
+
+static u32 mlxsw_pci_read_frc_l(void *bus_priv)
+{
+	struct mlxsw_pci *mlxsw_pci = bus_priv;
+	u64 frc_offset;
+
+	frc_offset = mlxsw_pci->free_running_clock_offset;
+	return mlxsw_pci_read32(mlxsw_pci, FREE_RUNNING_CLOCK_L(frc_offset));
+}
+
 static const struct mlxsw_bus mlxsw_pci_bus = {
 	.kind			= "pci",
 	.init			= mlxsw_pci_init,
@@ -1679,6 +1708,8 @@ static const struct mlxsw_bus mlxsw_pci_bus = {
 	.skb_transmit_busy	= mlxsw_pci_skb_transmit_busy,
 	.skb_transmit		= mlxsw_pci_skb_transmit,
 	.cmd_exec		= mlxsw_pci_cmd_exec,
+	.read_frc_h		= mlxsw_pci_read_frc_h,
+	.read_frc_l		= mlxsw_pci_read_frc_l,
 	.features		= MLXSW_BUS_F_TXRX | MLXSW_BUS_F_RESET,
 };
 
@@ -1740,6 +1771,7 @@ static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mlxsw_pci->bus_info.device_kind = driver_name;
 	mlxsw_pci->bus_info.device_name = pci_name(mlxsw_pci->pdev);
 	mlxsw_pci->bus_info.dev = &pdev->dev;
+	mlxsw_pci->bus_info.read_frc_capable = true;
 	mlxsw_pci->id = id;
 
 	err = mlxsw_core_bus_device_register(&mlxsw_pci->bus_info,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci_hw.h b/drivers/net/ethernet/mellanox/mlxsw/pci_hw.h
index 8648ca171254..e57e42e2d2b2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci_hw.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci_hw.h
@@ -43,6 +43,9 @@
 #define MLXSW_PCI_DOORBELL(offset, type_offset, num)	\
 	((offset) + (type_offset) + (num) * 4)
 
+#define MLXSW_PCI_FREE_RUNNING_CLOCK_H(offset)	(offset)
+#define MLXSW_PCI_FREE_RUNNING_CLOCK_L(offset)	((offset) + 4)
+
 #define MLXSW_PCI_CQS_MAX	96
 #define MLXSW_PCI_EQS_COUNT	2
 #define MLXSW_PCI_EQ_ASYNC_NUM	0
-- 
2.20.1

[PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

The MTUTC register configures the HW UTC counter.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h | 45 +++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index 7348c5a5ad6a..9ec154975cb2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -8691,6 +8691,50 @@ static inline void mlxsw_reg_mlcr_pack(char *payload, u8 local_port,
 					   MLXSW_REG_MLCR_DURATION_MAX : 0);
 }
 
+/* MTUTC - Management UTC Register
+ * -------------------------------
+ * Configures the HW UTC counter.
+ */
+#define MLXSW_REG_MTUTC_ID 0x9055
+#define MLXSW_REG_MTUTC_LEN 0x1C
+
+MLXSW_REG_DEFINE(mtutc, MLXSW_REG_MTUTC_ID, MLXSW_REG_MTUTC_LEN);
+
+enum mlxsw_reg_mtutc_operation {
+	MLXSW_REG_MTUTC_OPERATION_SET_TIME_AT_NEXT_SEC = 0,
+	MLXSW_REG_MTUTC_OPERATION_ADJUST_FREQ = 3,
+};
+
+/* reg_mtutc_operation
+ * Operation.
+ * Access: OP
+ */
+MLXSW_ITEM32(reg, mtutc, operation, 0x00, 0, 4);
+
+/* reg_mtutc_freq_adjustment
+ * Frequency adjustment: Every PPS the HW frequency will be
+ * adjusted by this value. Units of HW clock, where HW counts
+ * 10^9 HW clocks for 1 HW second.
+ * Access: RW
+ */
+MLXSW_ITEM32(reg, mtutc, freq_adjustment, 0x04, 0, 32);
+
+/* reg_mtutc_utc_sec
+ * UTC seconds.
+ * Access: WO
+ */
+MLXSW_ITEM32(reg, mtutc, utc_sec, 0x10, 0, 32);
+
+static inline void
+mlxsw_reg_mtutc_pack(char *payload, enum mlxsw_reg_mtutc_operation oper,
+		     u32 freq_adj, u32 utc_sec)
+{
+	MLXSW_REG_ZERO(mtutc, payload);
+	mlxsw_reg_mtutc_operation_set(payload, oper);
+	mlxsw_reg_mtutc_freq_adjustment_set(payload, freq_adj);
+	mlxsw_reg_mtutc_utc_sec_set(payload, utc_sec);
+}
+
 /* MCQI - Management Component Query Information
  * ---------------------------------------------
  * This register allows querying information about firmware components.
@@ -10105,6 +10149,7 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {
 	MLXSW_REG(mgir),
 	MLXSW_REG(mrsr),
 	MLXSW_REG(mlcr),
+	MLXSW_REG(mtutc),
 	MLXSW_REG(mpsc),
 	MLXSW_REG(mcqi),
 	MLXSW_REG(mcc),
-- 
2.20.1

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Richard Cochran]

On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
> From: Shalom Toledo <shalomt@mellanox.com>
> 
> The MTUTC register configures the HW UTC counter.

Why is this called the "UTC" counter?

The PTP time scale is TAI.  Is this counter intended to reflect the
Linux CLOCK_REALTIME system time?

> +/* MTUTC - Management UTC Register
> + * -------------------------------
> + * Configures the HW UTC counter.
> + */
> +#define MLXSW_REG_MTUTC_ID 0x9055
> +#define MLXSW_REG_MTUTC_LEN 0x1C

Thanks,
Richard

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Shalom Toledo]

On 04/06/2019 17:17, Richard Cochran wrote:
> On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
>> From: Shalom Toledo <shalomt@mellanox.com>
>>
>> The MTUTC register configures the HW UTC counter.
> 
> Why is this called the "UTC" counter?
> 
> The PTP time scale is TAI.  Is this counter intended to reflect the
> Linux CLOCK_REALTIME system time?

Exactly. The hardware doesn't have the ability to convert the FRC to UTC, so
we, as a driver, need to do it and align the hardware with this value.

> 
>> +/* MTUTC - Management UTC Register
>> + * -------------------------------
>> + * Configures the HW UTC counter.
>> + */
>> +#define MLXSW_REG_MTUTC_ID 0x9055
>> +#define MLXSW_REG_MTUTC_LEN 0x1C
> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Richard Cochran]

On Wed, Jun 05, 2019 at 11:30:06AM +0000, Shalom Toledo wrote:
> On 04/06/2019 17:17, Richard Cochran wrote:
> > On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
> >> From: Shalom Toledo <shalomt@mellanox.com>
> >>
> >> The MTUTC register configures the HW UTC counter.
> > 
> > Why is this called the "UTC" counter?
> > 
> > The PTP time scale is TAI.  Is this counter intended to reflect the
> > Linux CLOCK_REALTIME system time?
> 
> Exactly. The hardware doesn't have the ability to convert the FRC to UTC, so
> we, as a driver, need to do it and align the hardware with this value.

What does "FRC" mean?

Thanks,
Richard

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Shalom Toledo]

On 05/06/2019 20:23, Richard Cochran wrote:
> On Wed, Jun 05, 2019 at 11:30:06AM +0000, Shalom Toledo wrote:
>> On 04/06/2019 17:17, Richard Cochran wrote:
>>> On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
>>>> From: Shalom Toledo <shalomt@mellanox.com>
>>>>
>>>> The MTUTC register configures the HW UTC counter.
>>>
>>> Why is this called the "UTC" counter?
>>>
>>> The PTP time scale is TAI.  Is this counter intended to reflect the
>>> Linux CLOCK_REALTIME system time?
>>
>> Exactly. The hardware doesn't have the ability to convert the FRC to UTC, so
>> we, as a driver, need to do it and align the hardware with this value.
> 
> What does "FRC" mean?

Free Running Counter

> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Richard Cochran]

On Wed, Jun 05, 2019 at 06:55:18PM +0000, Shalom Toledo wrote:
> On 05/06/2019 20:23, Richard Cochran wrote:
> > On Wed, Jun 05, 2019 at 11:30:06AM +0000, Shalom Toledo wrote:
> >> On 04/06/2019 17:17, Richard Cochran wrote:
> >>> On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
> >>>> From: Shalom Toledo <shalomt@mellanox.com>
> >>>>
> >>>> The MTUTC register configures the HW UTC counter.
> >>>
> >>> Why is this called the "UTC" counter?
> >>>
> >>> The PTP time scale is TAI.  Is this counter intended to reflect the
> >>> Linux CLOCK_REALTIME system time?
> >>
> >> Exactly. The hardware doesn't have the ability to convert the FRC to UTC, so
> >> we, as a driver, need to do it and align the hardware with this value.
> > 
> > What does "FRC" mean?
> 
> Free Running Counter

Okay, so then you want to convert it into TAI (for PTP) rather than UTC.

Thanks,
Richard

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Shalom Toledo]

On 06/06/2019 5:37, Richard Cochran wrote:
> On Wed, Jun 05, 2019 at 06:55:18PM +0000, Shalom Toledo wrote:
>> On 05/06/2019 20:23, Richard Cochran wrote:
>>> On Wed, Jun 05, 2019 at 11:30:06AM +0000, Shalom Toledo wrote:
>>>> On 04/06/2019 17:17, Richard Cochran wrote:
>>>>> On Mon, Jun 03, 2019 at 03:12:39PM +0300, Ido Schimmel wrote:
>>>>>> From: Shalom Toledo <shalomt@mellanox.com>
>>>>>>
>>>>>> The MTUTC register configures the HW UTC counter.
>>>>>
>>>>> Why is this called the "UTC" counter?
>>>>>
>>>>> The PTP time scale is TAI.  Is this counter intended to reflect the
>>>>> Linux CLOCK_REALTIME system time?
>>>>
>>>> Exactly. The hardware doesn't have the ability to convert the FRC to UTC, so
>>>> we, as a driver, need to do it and align the hardware with this value.
>>>
>>> What does "FRC" mean?
>>
>> Free Running Counter
> 
> Okay, so then you want to convert it into TAI (for PTP) rather than UTC.

No, the HW interface is in UTC format. This is part of the HW machine that
responsible for adding the UTC time stamping on R-SPAN mirror packets.

> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 4/9] mlxsw: reg: Add Management UTC Register

[Petr Machata]

Shalom Toledo <shalomt@mellanox.com> writes:

> On 06/06/2019 5:37, Richard Cochran wrote:
>> Okay, so then you want to convert it into TAI (for PTP) rather than UTC.
>
> No, the HW interface is in UTC format. This is part of the HW machine that
> responsible for adding the UTC time stamping on R-SPAN mirror packets.

The way I understand it is that UTC is a misnomer. We are certainly not
updating the list of leap seconds in the ASIC so that it knows how to
get to UTC. (To be clear, no such interface even exists.) But it's
called UTC in the documentation, so we want to keep the name for that
reason.

[PATCH net-next 5/9] mlxsw: reg: Add Management Pulse Per Second Register

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

The MTPPS register provides the device PPS capabilities, configure the PPS
in and out modules and holds the PPS in time stamp.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h | 58 +++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index 9ec154975cb2..d8eb9ef01646 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -8691,6 +8691,63 @@ static inline void mlxsw_reg_mlcr_pack(char *payload, u8 local_port,
 					   MLXSW_REG_MLCR_DURATION_MAX : 0);
 }
 
+/* MTPPS - Management Pulse Per Second Register
+ * --------------------------------------------
+ * This register provides the device PPS capabilities, configure the PPS in and
+ * out modules and holds the PPS in time stamp.
+ */
+#define MLXSW_REG_MTPPS_ID 0x9053
+#define MLXSW_REG_MTPPS_LEN 0x3C
+
+MLXSW_REG_DEFINE(mtpps, MLXSW_REG_MTPPS_ID, MLXSW_REG_MTPPS_LEN);
+
+/* reg_mtpps_enable
+ * Enables the PPS functionality the specific pin.
+ * A boolean variable.
+ * Access: RW
+ */
+MLXSW_ITEM32(reg, mtpps, enable, 0x20, 31, 1);
+
+enum mlxsw_reg_mtpps_pin_mode {
+	MLXSW_REG_MTPPS_PIN_MODE_VIRTUAL_PIN = 0x2,
+};
+
+/* reg_mtpps_pin_mode
+ * Pin mode to be used. The mode must comply with the supported modes of the
+ * requested pin.
+ * Access: RW
+ */
+MLXSW_ITEM32(reg, mtpps, pin_mode, 0x20, 8, 4);
+
+#define MLXSW_REG_MTPPS_PIN_SP_VIRTUAL_PIN	7
+
+/* reg_mtpps_pin
+ * Pin to be configured or queried out of the supported pins.
+ * Access: Index
+ */
+MLXSW_ITEM32(reg, mtpps, pin, 0x20, 0, 8);
+
+/* reg_mtpps_time_stamp
+ * When pin_mode = pps_in, the latched device time when it was triggered from
+ * the external GPIO pin.
+ * When pin_mode = pps_out or virtual_pin or pps_out_and_virtual_pin, the target
+ * time to generate next output signal.
+ * Time is in units of device clock.
+ * Access: RW
+ */
+MLXSW_ITEM64(reg, mtpps, time_stamp, 0x28, 0, 64);
+
+static inline void
+mlxsw_reg_mtpps_vpin_pack(char *payload, u64 time_stamp)
+{
+	MLXSW_REG_ZERO(mtpps, payload);
+	mlxsw_reg_mtpps_pin_set(payload, MLXSW_REG_MTPPS_PIN_SP_VIRTUAL_PIN);
+	mlxsw_reg_mtpps_pin_mode_set(payload,
+				     MLXSW_REG_MTPPS_PIN_MODE_VIRTUAL_PIN);
+	mlxsw_reg_mtpps_enable_set(payload, true);
+	mlxsw_reg_mtpps_time_stamp_set(payload, time_stamp);
+}
+
 /* MTUTC - Management UTC Register
  * -------------------------------
  * Configures the HW UTC counter.
@@ -10149,6 +10206,7 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {
 	MLXSW_REG(mgir),
 	MLXSW_REG(mrsr),
 	MLXSW_REG(mlcr),
+	MLXSW_REG(mtpps),
 	MLXSW_REG(mtutc),
 	MLXSW_REG(mpsc),
 	MLXSW_REG(mcqi),
-- 
2.20.1

[PATCH net-next 6/9] ptp: ptp_clock: Publish scaled_ppm_to_ppb

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Publish scaled_ppm_to_ppb to allow drivers to use it.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/ptp/ptp_clock.c          | 5 +++--
 include/linux/ptp_clock_kernel.h | 8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index e189fa1be21e..f0893953b503 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -63,7 +63,7 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
 	spin_unlock_irqrestore(&queue->lock, flags);
 }
 
-static s32 scaled_ppm_to_ppb(long ppm)
+s32 ptp_clock_scaled_ppm_to_ppb(long ppm)
 {
 	/*
 	 * The 'freq' field in the 'struct timex' is in parts per
@@ -82,6 +82,7 @@ static s32 scaled_ppm_to_ppb(long ppm)
 	ppb >>= 13;
 	return (s32) ppb;
 }
+EXPORT_SYMBOL(ptp_clock_scaled_ppm_to_ppb);
 
 /* posix clock implementation */
 
@@ -137,7 +138,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 		delta = ktime_to_ns(kt);
 		err = ops->adjtime(ops, delta);
 	} else if (tx->modes & ADJ_FREQUENCY) {
-		s32 ppb = scaled_ppm_to_ppb(tx->freq);
+		s32 ppb = ptp_clock_scaled_ppm_to_ppb(tx->freq);
 		if (ppb > ops->max_adj || ppb < -ops->max_adj)
 			return -ERANGE;
 		if (ops->adjfine)
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 28eb9c792522..fc59d57640a5 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -212,6 +212,14 @@ extern void ptp_clock_event(struct ptp_clock *ptp,
 
 extern int ptp_clock_index(struct ptp_clock *ptp);
 
+/**
+ * ptp_clock_scaled_ppm_to_ppb() - convert scaled ppm to ppb
+ *
+ * @ppm:    Parts per million, but with a 16 bit binary fractional field
+ */
+
+extern s32 ptp_clock_scaled_ppm_to_ppb(long ppm);
+
 /**
  * ptp_find_pin() - obtain the pin index of a given auxiliary function
  *
-- 
2.20.1

Re: [PATCH net-next 6/9] ptp: ptp_clock: Publish scaled_ppm_to_ppb

[Richard Cochran]

On Mon, Jun 03, 2019 at 03:12:41PM +0300, Ido Schimmel wrote:
> From: Shalom Toledo <shalomt@mellanox.com>
> 
> Publish scaled_ppm_to_ppb to allow drivers to use it.

But why?

> @@ -63,7 +63,7 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
>  	spin_unlock_irqrestore(&queue->lock, flags);
>  }
>  
> -static s32 scaled_ppm_to_ppb(long ppm)
> +s32 ptp_clock_scaled_ppm_to_ppb(long ppm)

Six words is a little bit much for the name of a function.

Thanks,
Richard

Re: [PATCH net-next 6/9] ptp: ptp_clock: Publish scaled_ppm_to_ppb

[Shalom Toledo]

On 04/06/2019 17:21, Richard Cochran wrote:
> On Mon, Jun 03, 2019 at 03:12:41PM +0300, Ido Schimmel wrote:
>> From: Shalom Toledo <shalomt@mellanox.com>
>>
>> Publish scaled_ppm_to_ppb to allow drivers to use it.
> 
> But why?

See my comment in patch #7

> 
>> @@ -63,7 +63,7 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
>>  	spin_unlock_irqrestore(&queue->lock, flags);
>>  }
>>  
>> -static s32 scaled_ppm_to_ppb(long ppm)
>> +s32 ptp_clock_scaled_ppm_to_ppb(long ppm)
> 
> Six words is a little bit much for the name of a function.

I can make it scaled_ppm_to_ppb() as before. Is that ok? or maybe you have
something else in your mind?

> 
> Thanks,
> Richard
> 

[PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Implement physical hardware clock operations.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/Makefile  |   1 +
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    | 267 ++++++++++++++++++
 .../ethernet/mellanox/mlxsw/spectrum_ptp.h    |  44 +++
 3 files changed, 312 insertions(+)
 create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
 create mode 100644 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h

diff --git a/drivers/net/ethernet/mellanox/mlxsw/Makefile b/drivers/net/ethernet/mellanox/mlxsw/Makefile
index c4dc72e1ce63..171b36bd8a4e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/Makefile
+++ b/drivers/net/ethernet/mellanox/mlxsw/Makefile
@@ -31,5 +31,6 @@ mlxsw_spectrum-objs		:= spectrum.o spectrum_buffers.o \
 				   spectrum_nve.o spectrum_nve_vxlan.o \
 				   spectrum_dpipe.o
 mlxsw_spectrum-$(CONFIG_MLXSW_SPECTRUM_DCB)	+= spectrum_dcb.o
+mlxsw_spectrum-$(CONFIG_PTP_1588_CLOCK)		+= spectrum_ptp.o
 obj-$(CONFIG_MLXSW_MINIMAL)	+= mlxsw_minimal.o
 mlxsw_minimal-objs		:= minimal.o
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
new file mode 100644
index 000000000000..9bdbbba29400
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/* Copyright (c) 2019 Mellanox Technologies. All rights reserved */
+
+#include <linux/ptp_clock_kernel.h>
+#include <linux/clocksource.h>
+#include <linux/timecounter.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+
+#include "spectrum_ptp.h"
+#include "core.h"
+
+#define MLXSW_SP1_PTP_CLOCK_CYCLES_SHIFT	29
+#define MLXSW_SP1_PTP_CLOCK_FREQ_KHZ		156257 /* 6.4nSec */
+#define MLXSW_SP1_PTP_CLOCK_MASK		64
+
+struct mlxsw_sp_ptp_clock {
+	struct mlxsw_core *core;
+	spinlock_t lock; /* protect this structure */
+	struct cyclecounter cycles;
+	struct timecounter tc;
+	u32 nominal_c_mult;
+	struct ptp_clock *ptp;
+	struct ptp_clock_info ptp_info;
+	unsigned long overflow_period;
+	struct delayed_work overflow_work;
+};
+
+static u64 __mlxsw_sp1_ptp_read_frc(struct mlxsw_sp_ptp_clock *clock,
+				    struct ptp_system_timestamp *sts)
+{
+	struct mlxsw_core *mlxsw_core = clock->core;
+	u32 frc_h1, frc_h2, frc_l;
+
+	frc_h1 = mlxsw_core_read_frc_h(mlxsw_core);
+	ptp_read_system_prets(sts);
+	frc_l = mlxsw_core_read_frc_l(mlxsw_core);
+	ptp_read_system_postts(sts);
+	frc_h2 = mlxsw_core_read_frc_h(mlxsw_core);
+
+	if (frc_h1 != frc_h2) {
+		/* wrap around */
+		ptp_read_system_prets(sts);
+		frc_l = mlxsw_core_read_frc_l(mlxsw_core);
+		ptp_read_system_postts(sts);
+	}
+
+	return (u64) frc_l | (u64) frc_h2 << 32;
+}
+
+static u64 mlxsw_sp1_ptp_read_frc(const struct cyclecounter *cc)
+{
+	struct mlxsw_sp_ptp_clock *clock =
+		container_of(cc, struct mlxsw_sp_ptp_clock, cycles);
+
+	return __mlxsw_sp1_ptp_read_frc(clock, NULL) & cc->mask;
+}
+
+static int
+mlxsw_sp1_ptp_update_phc_adjfreq(struct mlxsw_sp_ptp_clock *clock, int freq_adj)
+{
+	struct mlxsw_core *mlxsw_core = clock->core;
+	char mtutc_pl[MLXSW_REG_MTUTC_LEN];
+
+	mlxsw_reg_mtutc_pack(mtutc_pl, MLXSW_REG_MTUTC_OPERATION_ADJUST_FREQ,
+			     freq_adj, 0);
+	return mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtutc), mtutc_pl);
+}
+
+static u64 mlxsw_sp1_ptp_ns2cycles(const struct timecounter *tc, u64 nsec)
+{
+	u64 cycles = (u64) nsec;
+
+	cycles <<= tc->cc->shift;
+	cycles = div_u64(cycles, tc->cc->mult);
+
+	return cycles;
+}
+
+static int
+mlxsw_sp1_ptp_update_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)
+{
+	struct mlxsw_core *mlxsw_core = clock->core;
+	char mtutc_pl[MLXSW_REG_MTUTC_LEN];
+	char mtpps_pl[MLXSW_REG_MTPPS_LEN];
+	u64 next_sec_in_nsec, cycles;
+	u32 next_sec;
+	int err;
+
+	next_sec = nsec / NSEC_PER_SEC + 1;
+	next_sec_in_nsec = next_sec * NSEC_PER_SEC;
+
+	spin_lock(&clock->lock);
+	cycles = mlxsw_sp1_ptp_ns2cycles(&clock->tc, next_sec_in_nsec);
+	spin_unlock(&clock->lock);
+
+	mlxsw_reg_mtpps_vpin_pack(mtpps_pl, cycles);
+	err = mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtpps), mtpps_pl);
+	if (err)
+		return err;
+
+	mlxsw_reg_mtutc_pack(mtutc_pl,
+			     MLXSW_REG_MTUTC_OPERATION_SET_TIME_AT_NEXT_SEC,
+			     0, next_sec);
+	return mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtutc), mtutc_pl);
+}
+
+static int mlxsw_sp1_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct mlxsw_sp_ptp_clock *clock =
+		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
+	int neg_adj = 0;
+	u32 diff;
+	u64 adj;
+	s32 ppb;
+
+	ppb = ptp_clock_scaled_ppm_to_ppb(scaled_ppm);
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;
+	}
+
+	adj = clock->nominal_c_mult;
+	adj *= ppb;
+	diff = div_u64(adj, NSEC_PER_SEC);
+
+	spin_lock(&clock->lock);
+	timecounter_read(&clock->tc);
+	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
+				       clock->nominal_c_mult + diff;
+	spin_unlock(&clock->lock);
+
+	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);
+}
+
+static int mlxsw_sp1_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct mlxsw_sp_ptp_clock *clock =
+		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
+	u64 nsec;
+
+	spin_lock(&clock->lock);
+	timecounter_adjtime(&clock->tc, delta);
+	nsec = timecounter_read(&clock->tc);
+	spin_unlock(&clock->lock);
+
+	return mlxsw_sp1_ptp_update_phc_settime(clock, nsec);
+}
+
+static int mlxsw_sp1_ptp_gettimex(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  struct ptp_system_timestamp *sts)
+{
+	struct mlxsw_sp_ptp_clock *clock =
+		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
+	u64 cycles, nsec;
+
+	spin_lock(&clock->lock);
+	cycles = __mlxsw_sp1_ptp_read_frc(clock, sts);
+	nsec = timecounter_cyc2time(&clock->tc, cycles);
+	spin_unlock(&clock->lock);
+
+	*ts = ns_to_timespec64(nsec);
+
+	return 0;
+}
+
+static int mlxsw_sp1_ptp_settime(struct ptp_clock_info *ptp,
+				 const struct timespec64 *ts)
+{
+	struct mlxsw_sp_ptp_clock *clock =
+		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
+	u64 nsec = timespec64_to_ns(ts);
+
+	spin_lock(&clock->lock);
+	timecounter_init(&clock->tc, &clock->cycles, nsec);
+	nsec = timecounter_read(&clock->tc);
+	spin_unlock(&clock->lock);
+
+	return mlxsw_sp1_ptp_update_phc_settime(clock, nsec);
+}
+
+static const struct ptp_clock_info mlxsw_sp1_ptp_clock_info = {
+	.owner		= THIS_MODULE,
+	.name		= "mlxsw_sp_clock",
+	.max_adj	= 100000000,
+	.adjfine	= mlxsw_sp1_ptp_adjfine,
+	.adjtime	= mlxsw_sp1_ptp_adjtime,
+	.gettimex64	= mlxsw_sp1_ptp_gettimex,
+	.settime64	= mlxsw_sp1_ptp_settime,
+};
+
+static void mlxsw_sp1_ptp_clock_overflow(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct mlxsw_sp_ptp_clock *clock;
+
+	clock = container_of(dwork, struct mlxsw_sp_ptp_clock, overflow_work);
+
+	spin_lock(&clock->lock);
+	timecounter_read(&clock->tc);
+	spin_unlock(&clock->lock);
+	mlxsw_core_schedule_dw(&clock->overflow_work, clock->overflow_period);
+}
+
+struct mlxsw_sp_ptp_clock *
+mlxsw_sp1_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev)
+{
+	u64 overflow_cycles, nsec, frac = 0;
+	struct mlxsw_sp_ptp_clock *clock;
+	int err;
+
+	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
+	if (!clock)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&clock->lock);
+	clock->cycles.read = mlxsw_sp1_ptp_read_frc;
+	clock->cycles.shift = MLXSW_SP1_PTP_CLOCK_CYCLES_SHIFT;
+	clock->cycles.mult = clocksource_khz2mult(MLXSW_SP1_PTP_CLOCK_FREQ_KHZ,
+						  clock->cycles.shift);
+	clock->nominal_c_mult = clock->cycles.mult;
+	clock->cycles.mask = CLOCKSOURCE_MASK(MLXSW_SP1_PTP_CLOCK_MASK);
+	clock->core = mlxsw_sp->core;
+
+	timecounter_init(&clock->tc, &clock->cycles,
+			 ktime_to_ns(ktime_get_real()));
+
+	/* Calculate period in seconds to call the overflow watchdog - to make
+	 * sure counter is checked at least twice every wrap around.
+	 * The period is calculated as the minimum between max HW cycles count
+	 * (The clock source mask) and max amount of cycles that can be
+	 * multiplied by clock multiplier where the result doesn't exceed
+	 * 64bits.
+	 */
+	overflow_cycles = div64_u64(~0ULL >> 1, clock->cycles.mult);
+	overflow_cycles = min(overflow_cycles, div_u64(clock->cycles.mask, 3));
+
+	nsec = cyclecounter_cyc2ns(&clock->cycles, overflow_cycles, 0, &frac);
+	clock->overflow_period = nsecs_to_jiffies(nsec);
+
+	INIT_DELAYED_WORK(&clock->overflow_work, mlxsw_sp1_ptp_clock_overflow);
+	mlxsw_core_schedule_dw(&clock->overflow_work, 0);
+
+	clock->ptp_info = mlxsw_sp1_ptp_clock_info;
+	clock->ptp = ptp_clock_register(&clock->ptp_info, dev);
+	if (IS_ERR(clock->ptp)) {
+		err = PTR_ERR(clock->ptp);
+		dev_err(dev, "ptp_clock_register failed %d\n", err);
+		goto err_ptp_clock_register;
+	}
+
+	return clock;
+
+err_ptp_clock_register:
+	cancel_delayed_work_sync(&clock->overflow_work);
+	kfree(clock);
+	return ERR_PTR(err);
+}
+
+void mlxsw_sp1_ptp_clock_fini(struct mlxsw_sp_ptp_clock *clock)
+{
+	ptp_clock_unregister(clock->ptp);
+	cancel_delayed_work_sync(&clock->overflow_work);
+	kfree(clock);
+}
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
new file mode 100644
index 000000000000..76fa00a4be75
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 */
+/* Copyright (c) 2019 Mellanox Technologies. All rights reserved */
+
+#ifndef _MLXSW_SPECTRUM_PTP_H
+#define _MLXSW_SPECTRUM_PTP_H
+
+#include <linux/device.h>
+
+#include "spectrum.h"
+
+struct mlxsw_sp_ptp_clock;
+
+#if IS_REACHABLE(CONFIG_PTP_1588_CLOCK)
+
+struct mlxsw_sp_ptp_clock *
+mlxsw_sp1_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev);
+
+void mlxsw_sp1_ptp_clock_fini(struct mlxsw_sp_ptp_clock *clock);
+
+#else
+
+static inline struct mlxsw_sp_ptp_clock *
+mlxsw_sp1_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev)
+{
+	return NULL;
+}
+
+static inline void mlxsw_sp1_ptp_clock_fini(struct mlxsw_sp_ptp_clock *clock)
+{
+}
+
+#endif
+
+static inline struct mlxsw_sp_ptp_clock *
+mlxsw_sp2_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev)
+{
+	return NULL;
+}
+
+static inline void mlxsw_sp2_ptp_clock_fini(struct mlxsw_sp_ptp_clock *clock)
+{
+}
+
+#endif
-- 
2.20.1

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:

> +static int
> +mlxsw_sp1_ptp_update_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)

Six words ^^^

What is wrong with "mlxsw_phc_settime" ?

> +{
> +	struct mlxsw_core *mlxsw_core = clock->core;
> +	char mtutc_pl[MLXSW_REG_MTUTC_LEN];
> +	char mtpps_pl[MLXSW_REG_MTPPS_LEN];
> +	u64 next_sec_in_nsec, cycles;
> +	u32 next_sec;
> +	int err;
> +
> +	next_sec = nsec / NSEC_PER_SEC + 1;
> +	next_sec_in_nsec = next_sec * NSEC_PER_SEC;
> +
> +	spin_lock(&clock->lock);
> +	cycles = mlxsw_sp1_ptp_ns2cycles(&clock->tc, next_sec_in_nsec);
> +	spin_unlock(&clock->lock);
> +
> +	mlxsw_reg_mtpps_vpin_pack(mtpps_pl, cycles);
> +	err = mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtpps), mtpps_pl);
> +	if (err)
> +		return err;
> +
> +	mlxsw_reg_mtutc_pack(mtutc_pl,
> +			     MLXSW_REG_MTUTC_OPERATION_SET_TIME_AT_NEXT_SEC,
> +			     0, next_sec);
> +	return mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtutc), mtutc_pl);
> +}
> +
> +static int mlxsw_sp1_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	struct mlxsw_sp_ptp_clock *clock =
> +		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
> +	int neg_adj = 0;
> +	u32 diff;
> +	u64 adj;
> +	s32 ppb;
> +
> +	ppb = ptp_clock_scaled_ppm_to_ppb(scaled_ppm);

Now I see why you did this.  Nice try.

The 'scaled_ppm' has a finer resolution than ppb.  Please make use of
the finer resolution in your calculation.  It does make a difference.

> +
> +	if (ppb < 0) {
> +		neg_adj = 1;
> +		ppb = -ppb;
> +	}
> +
> +	adj = clock->nominal_c_mult;
> +	adj *= ppb;
> +	diff = div_u64(adj, NSEC_PER_SEC);
> +
> +	spin_lock(&clock->lock);
> +	timecounter_read(&clock->tc);
> +	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
> +				       clock->nominal_c_mult + diff;
> +	spin_unlock(&clock->lock);
> +
> +	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);
> +}

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Jiri Pirko]

Tue, Jun 04, 2019 at 04:28:19PM CEST, richardcochran@gmail.com wrote:
>On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:
>
>> +static int
>> +mlxsw_sp1_ptp_update_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)
>
>Six words ^^^

It is aligned with the rest of mlxsw code.

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Wed, Jun 05, 2019 at 08:30:21AM +0200, Jiri Pirko wrote:
> Tue, Jun 04, 2019 at 04:28:19PM CEST, richardcochran@gmail.com wrote:
> >On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:
> >
> >> +static int
> >> +mlxsw_sp1_ptp_update_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)
> >
> >Six words ^^^
> 
> It is aligned with the rest of mlxsw code.

Those can be fixed, too.

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Shalom Toledo]

On 04/06/2019 17:28, Richard Cochran wrote:
> On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:
> 
>> +static int
>> +mlxsw_sp1_ptp_update_phc_settime(struct mlxsw_sp_ptp_clock *clock, u64 nsec)
> 
> Six words ^^^
> 
> What is wrong with "mlxsw_phc_settime" ?

I can drop the "update". But as Jiri mentioned, it is aligned with the rest of
mlxsw code.

> 
>> +{
>> +	struct mlxsw_core *mlxsw_core = clock->core;
>> +	char mtutc_pl[MLXSW_REG_MTUTC_LEN];
>> +	char mtpps_pl[MLXSW_REG_MTPPS_LEN];
>> +	u64 next_sec_in_nsec, cycles;
>> +	u32 next_sec;
>> +	int err;
>> +
>> +	next_sec = nsec / NSEC_PER_SEC + 1;
>> +	next_sec_in_nsec = next_sec * NSEC_PER_SEC;
>> +
>> +	spin_lock(&clock->lock);
>> +	cycles = mlxsw_sp1_ptp_ns2cycles(&clock->tc, next_sec_in_nsec);
>> +	spin_unlock(&clock->lock);
>> +
>> +	mlxsw_reg_mtpps_vpin_pack(mtpps_pl, cycles);
>> +	err = mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtpps), mtpps_pl);
>> +	if (err)
>> +		return err;
>> +
>> +	mlxsw_reg_mtutc_pack(mtutc_pl,
>> +			     MLXSW_REG_MTUTC_OPERATION_SET_TIME_AT_NEXT_SEC,
>> +			     0, next_sec);
>> +	return mlxsw_reg_write(mlxsw_core, MLXSW_REG(mtutc), mtutc_pl);
>> +}
>> +
>> +static int mlxsw_sp1_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>> +{
>> +	struct mlxsw_sp_ptp_clock *clock =
>> +		container_of(ptp, struct mlxsw_sp_ptp_clock, ptp_info);
>> +	int neg_adj = 0;
>> +	u32 diff;
>> +	u64 adj;
>> +	s32 ppb;
>> +
>> +	ppb = ptp_clock_scaled_ppm_to_ppb(scaled_ppm);
> 
> Now I see why you did this.  Nice try.

I didn't try anything.

The reason is that the hardware units is in ppb and not in scaled_ppm(or ppm),
so I just converted to ppb in order to set the hardware.

But I got your point, I will change my calculation to use scaled_ppm (to get a
more finer resolution) and not ppb, and convert to ppb just before setting the
hardware. Is that make sense?

But I'm still need to expose scaled_ppm_to_ppb.

> 
> The 'scaled_ppm' has a finer resolution than ppb.  Please make use of
> the finer resolution in your calculation.  It does make a difference.

Will change, thanks for that!

> 
>> +
>> +	if (ppb < 0) {
>> +		neg_adj = 1;
>> +		ppb = -ppb;
>> +	}
>> +
>> +	adj = clock->nominal_c_mult;
>> +	adj *= ppb;
>> +	diff = div_u64(adj, NSEC_PER_SEC);
>> +
>> +	spin_lock(&clock->lock);
>> +	timecounter_read(&clock->tc);
>> +	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
>> +				       clock->nominal_c_mult + diff;
>> +	spin_unlock(&clock->lock);
>> +
>> +	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);
>> +}
> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Wed, Jun 05, 2019 at 11:44:21AM +0000, Shalom Toledo wrote:
> On 04/06/2019 17:28, Richard Cochran wrote:
> > Now I see why you did this.  Nice try.
> 
> I didn't try anything.
> 
> The reason is that the hardware units is in ppb and not in scaled_ppm(or ppm),
> so I just converted to ppb in order to set the hardware.

Oh, I thought you were adapting code for the deprecated .adjfreq method.
 
> But I got your point, I will change my calculation to use scaled_ppm (to get a
> more finer resolution) and not ppb, and convert to ppb just before setting the
> hardware. Is that make sense?

So the HW actually accepts ppb adjustment values?  Fine.

But I don't understand this:

> >> +	if (ppb < 0) {
> >> +		neg_adj = 1;
> >> +		ppb = -ppb;
> >> +	}
> >> +
> >> +	adj = clock->nominal_c_mult;
> >> +	adj *= ppb;
> >> +	diff = div_u64(adj, NSEC_PER_SEC);
> >> +
> >> +	spin_lock(&clock->lock);
> >> +	timecounter_read(&clock->tc);
> >> +	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
> >> +				       clock->nominal_c_mult + diff;
> >> +	spin_unlock(&clock->lock);

You have a SW time counter here

> >> +	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);

and a hardware method here?

Why not choose one or the other?

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Shalom Toledo]

On 05/06/2019 20:40, Richard Cochran wrote:
> On Wed, Jun 05, 2019 at 11:44:21AM +0000, Shalom Toledo wrote:
>> On 04/06/2019 17:28, Richard Cochran wrote:
>>> Now I see why you did this.  Nice try.
>>
>> I didn't try anything.
>>
>> The reason is that the hardware units is in ppb and not in scaled_ppm(or ppm),
>> so I just converted to ppb in order to set the hardware.
> 
> Oh, I thought you were adapting code for the deprecated .adjfreq method.
>  
>> But I got your point, I will change my calculation to use scaled_ppm (to get a
>> more finer resolution) and not ppb, and convert to ppb just before setting the
>> hardware. Is that make sense?
> 
> So the HW actually accepts ppb adjustment values?  Fine.

So I leave the code as is.

> 
> But I don't understand this:
> 
>>>> +	if (ppb < 0) {
>>>> +		neg_adj = 1;
>>>> +		ppb = -ppb;
>>>> +	}
>>>> +
>>>> +	adj = clock->nominal_c_mult;
>>>> +	adj *= ppb;
>>>> +	diff = div_u64(adj, NSEC_PER_SEC);
>>>> +
>>>> +	spin_lock(&clock->lock);
>>>> +	timecounter_read(&clock->tc);
>>>> +	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
>>>> +				       clock->nominal_c_mult + diff;
>>>> +	spin_unlock(&clock->lock);
> 
> You have a SW time counter here

Yep.

> 
>>>> +	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);
> 
> and a hardware method here?

Yep.

> 
> Why not choose one or the other?

You are right, but there is a reason behind it of course.

The hardware has a free running counter that the SW can only read. This is
used when we do gettimex op. The rest ops, settime, adjfine and adjtime, we do
only in SW, since we can't control the HW free running counter.

Now, you are asking yourself, why the driver also update the HW? what does it
mean?

So, there is an HW machine which responsible for adding UTC timestamp on
R-SPAN mirror packets and there is no connection to the HW free running
counter. The SW can and should control this HW machine, from setting the UTC
time when doing settime for example. Or adjusting the frequency when doing
adjfine.

I hope it is more clear now.

> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Wed, Jun 05, 2019 at 07:28:38PM +0000, Shalom Toledo wrote:
> So, there is an HW machine which responsible for adding UTC timestamp on
> R-SPAN mirror packets and there is no connection to the HW free running
> counter.

If there is no connection, then the frequency adjustments to the HW
clock will not work as expected.

Perhaps the free running counter and the HW clock share the same clock
source?

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Shalom Toledo]

On 06/06/2019 5:43, Richard Cochran wrote:
> On Wed, Jun 05, 2019 at 07:28:38PM +0000, Shalom Toledo wrote:
>> So, there is an HW machine which responsible for adding UTC timestamp on
>> R-SPAN mirror packets and there is no connection to the HW free running
>> counter.
> 
> If there is no connection, then the frequency adjustments to the HW
> clock will not work as expected.

I mean that these are two different peaces of HW with different control
interfaces.

> 
> Perhaps the free running counter and the HW clock share the same clock
> source?

Yes, of course.

> 
> Thanks,
> Richard
> 

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Shalom Toledo]

On 06/06/2019 11:50, Shalom Toledo wrote:
> On 06/06/2019 5:43, Richard Cochran wrote:
>> On Wed, Jun 05, 2019 at 07:28:38PM +0000, Shalom Toledo wrote:
>>> So, there is an HW machine which responsible for adding UTC timestamp on
>>> R-SPAN mirror packets and there is no connection to the HW free running
>>> counter.
>>
>> If there is no connection, then the frequency adjustments to the HW
>> clock will not work as expected.
> 
> I mean that these are two different peaces of HW with different control
> interfaces.

pieces*

> 
>>
>> Perhaps the free running counter and the HW clock share the same clock
>> source?
> 
> Yes, of course.
> 
>>
>> Thanks,
>> Richard
>>
> 

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:
> +struct mlxsw_sp_ptp_clock *
> +mlxsw_sp1_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev)
> +{
> +	u64 overflow_cycles, nsec, frac = 0;
> +	struct mlxsw_sp_ptp_clock *clock;
> +	int err;
> +
> +	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
> +	if (!clock)
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock_init(&clock->lock);
> +	clock->cycles.read = mlxsw_sp1_ptp_read_frc;
> +	clock->cycles.shift = MLXSW_SP1_PTP_CLOCK_CYCLES_SHIFT;
> +	clock->cycles.mult = clocksource_khz2mult(MLXSW_SP1_PTP_CLOCK_FREQ_KHZ,
> +						  clock->cycles.shift);
> +	clock->nominal_c_mult = clock->cycles.mult;
> +	clock->cycles.mask = CLOCKSOURCE_MASK(MLXSW_SP1_PTP_CLOCK_MASK);
> +	clock->core = mlxsw_sp->core;
> +
> +	timecounter_init(&clock->tc, &clock->cycles,
> +			 ktime_to_ns(ktime_get_real()));
> +
> +	/* Calculate period in seconds to call the overflow watchdog - to make
> +	 * sure counter is checked at least twice every wrap around.
> +	 * The period is calculated as the minimum between max HW cycles count
> +	 * (The clock source mask) and max amount of cycles that can be
> +	 * multiplied by clock multiplier where the result doesn't exceed
> +	 * 64bits.
> +	 */
> +	overflow_cycles = div64_u64(~0ULL >> 1, clock->cycles.mult);
> +	overflow_cycles = min(overflow_cycles, div_u64(clock->cycles.mask, 3));
> +
> +	nsec = cyclecounter_cyc2ns(&clock->cycles, overflow_cycles, 0, &frac);
> +	clock->overflow_period = nsecs_to_jiffies(nsec);
> +
> +	INIT_DELAYED_WORK(&clock->overflow_work, mlxsw_sp1_ptp_clock_overflow);
> +	mlxsw_core_schedule_dw(&clock->overflow_work, 0);
> +
> +	clock->ptp_info = mlxsw_sp1_ptp_clock_info;
> +	clock->ptp = ptp_clock_register(&clock->ptp_info, dev);
> +	if (IS_ERR(clock->ptp)) {
> +		err = PTR_ERR(clock->ptp);
> +		dev_err(dev, "ptp_clock_register failed %d\n", err);
> +		goto err_ptp_clock_register;
> +	}
> +
> +	return clock;

You need to handle the case where ptp_clock_register() returns NULL...

/**
 * ptp_clock_register() - register a PTP hardware clock driver
 *
 * @info:   Structure describing the new clock.
 * @parent: Pointer to the parent device of the new clock.
 *
 * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
 * support is missing at the configuration level, this function
 * returns NULL, and drivers are expected to gracefully handle that
 * case separately.
 */

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Petr Machata]

Richard Cochran <richardcochran@gmail.com> writes:

> On Mon, Jun 03, 2019 at 03:12:42PM +0300, Ido Schimmel wrote:
>> +struct mlxsw_sp_ptp_clock *
>> +mlxsw_sp1_ptp_clock_init(struct mlxsw_sp *mlxsw_sp, struct device *dev)

[...]

>> +	clock->ptp_info = mlxsw_sp1_ptp_clock_info;
>> +	clock->ptp = ptp_clock_register(&clock->ptp_info, dev);
>> +	if (IS_ERR(clock->ptp)) {
>> +		err = PTR_ERR(clock->ptp);
>> +		dev_err(dev, "ptp_clock_register failed %d\n", err);
>> +		goto err_ptp_clock_register;
>> +	}
>> +
>> +	return clock;
>
> You need to handle the case where ptp_clock_register() returns NULL...
>
> /**
>  * ptp_clock_register() - register a PTP hardware clock driver
>  *
>  * @info:   Structure describing the new clock.
>  * @parent: Pointer to the parent device of the new clock.
>  *
>  * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
>  * support is missing at the configuration level, this function
>  * returns NULL, and drivers are expected to gracefully handle that
>  * case separately.
>  */

We don't build the PTP module at all unless CONFIG_PTP_1588_CLOCK is
enabled, and fall back to inline stubs unless it IS_REACHABLE. I believe
this should be OK.

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Richard Cochran]

On Wed, Jun 05, 2019 at 09:00:09AM +0000, Petr Machata wrote:
> We don't build the PTP module at all unless CONFIG_PTP_1588_CLOCK is
> enabled, and fall back to inline stubs unless it IS_REACHABLE. I believe
> this should be OK.

Please use "imply PTP_1588_CLOCK" in your kconfig, just like the other
PTP drivers do.

Thanks,
Richard

Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

[Petr Machata]

Richard Cochran <richardcochran@gmail.com> writes:

> On Wed, Jun 05, 2019 at 09:00:09AM +0000, Petr Machata wrote:
>> We don't build the PTP module at all unless CONFIG_PTP_1588_CLOCK is
>> enabled, and fall back to inline stubs unless it IS_REACHABLE. I believe
>> this should be OK.
>
> Please use "imply PTP_1588_CLOCK" in your kconfig, just like the other
> PTP drivers do.

All right.

[PATCH net-next 8/9] mlxsw: spectrum: PTP physical hardware clock initialization

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Initialize the PTP physical hardware clock.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 36 +++++++++++++++++++
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  3 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index dfe6b44baf63..ef86418bb217 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -41,6 +41,7 @@
 #include "spectrum_dpipe.h"
 #include "spectrum_acl_flex_actions.h"
 #include "spectrum_span.h"
+#include "spectrum_ptp.h"
 #include "../mlxfw/mlxfw.h"
 
 #define MLXSW_SP_FWREV_MINOR_TO_BRANCH(minor) ((minor) / 100)
@@ -4329,6 +4330,22 @@ static int mlxsw_sp_basic_trap_groups_set(struct mlxsw_core *mlxsw_core)
 	return mlxsw_reg_write(mlxsw_core, MLXSW_REG(htgt), htgt_pl);
 }
 
+struct mlxsw_sp_ptp_ops {
+	struct mlxsw_sp_ptp_clock *
+		(*clock_init)(struct mlxsw_sp *mlxsw_sp, struct device *dev);
+	void (*clock_fini)(struct mlxsw_sp_ptp_clock *clock);
+};
+
+static const struct mlxsw_sp_ptp_ops mlxsw_sp1_ptp_ops = {
+	.clock_init	= mlxsw_sp1_ptp_clock_init,
+	.clock_fini	= mlxsw_sp1_ptp_clock_fini,
+};
+
+static const struct mlxsw_sp_ptp_ops mlxsw_sp2_ptp_ops = {
+	.clock_init	= mlxsw_sp2_ptp_clock_init,
+	.clock_fini	= mlxsw_sp2_ptp_clock_fini,
+};
+
 static int mlxsw_sp_netdevice_event(struct notifier_block *unused,
 				    unsigned long event, void *ptr);
 
@@ -4426,6 +4443,18 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
 		goto err_router_init;
 	}
 
+	if (mlxsw_sp->bus_info->read_frc_capable) {
+		/* NULL is a valid return value from clock_init */
+		mlxsw_sp->clock =
+			mlxsw_sp->ptp_ops->clock_init(mlxsw_sp,
+						      mlxsw_sp->bus_info->dev);
+		if (IS_ERR(mlxsw_sp->clock)) {
+			err = PTR_ERR(mlxsw_sp->clock);
+			dev_err(mlxsw_sp->bus_info->dev, "Failed to init ptp clock\n");
+			goto err_ptp_clock_init;
+		}
+	}
+
 	/* Initialize netdevice notifier after router and SPAN is initialized,
 	 * so that the event handler can use router structures and call SPAN
 	 * respin.
@@ -4456,6 +4485,9 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
 err_dpipe_init:
 	unregister_netdevice_notifier(&mlxsw_sp->netdevice_nb);
 err_netdev_notifier:
+	if (mlxsw_sp->clock)
+		mlxsw_sp->ptp_ops->clock_fini(mlxsw_sp->clock);
+err_ptp_clock_init:
 	mlxsw_sp_router_fini(mlxsw_sp);
 err_router_init:
 	mlxsw_sp_acl_fini(mlxsw_sp);
@@ -4499,6 +4531,7 @@ static int mlxsw_sp1_init(struct mlxsw_core *mlxsw_core,
 	mlxsw_sp->rif_ops_arr = mlxsw_sp1_rif_ops_arr;
 	mlxsw_sp->sb_vals = &mlxsw_sp1_sb_vals;
 	mlxsw_sp->port_type_speed_ops = &mlxsw_sp1_port_type_speed_ops;
+	mlxsw_sp->ptp_ops = &mlxsw_sp1_ptp_ops;
 
 	return mlxsw_sp_init(mlxsw_core, mlxsw_bus_info);
 }
@@ -4518,6 +4551,7 @@ static int mlxsw_sp2_init(struct mlxsw_core *mlxsw_core,
 	mlxsw_sp->rif_ops_arr = mlxsw_sp2_rif_ops_arr;
 	mlxsw_sp->sb_vals = &mlxsw_sp2_sb_vals;
 	mlxsw_sp->port_type_speed_ops = &mlxsw_sp2_port_type_speed_ops;
+	mlxsw_sp->ptp_ops = &mlxsw_sp2_ptp_ops;
 
 	return mlxsw_sp_init(mlxsw_core, mlxsw_bus_info);
 }
@@ -4529,6 +4563,8 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
 	mlxsw_sp_ports_remove(mlxsw_sp);
 	mlxsw_sp_dpipe_fini(mlxsw_sp);
 	unregister_netdevice_notifier(&mlxsw_sp->netdevice_nb);
+	if (mlxsw_sp->clock)
+		mlxsw_sp->ptp_ops->clock_fini(mlxsw_sp->clock);
 	mlxsw_sp_router_fini(mlxsw_sp);
 	mlxsw_sp_acl_fini(mlxsw_sp);
 	mlxsw_sp_nve_fini(mlxsw_sp);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 8601b3041acd..ea4d56486ea3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -136,6 +136,7 @@ struct mlxsw_sp_acl_tcam_ops;
 struct mlxsw_sp_nve_ops;
 struct mlxsw_sp_sb_vals;
 struct mlxsw_sp_port_type_speed_ops;
+struct mlxsw_sp_ptp_ops;
 
 struct mlxsw_sp {
 	struct mlxsw_sp_port **ports;
@@ -155,6 +156,7 @@ struct mlxsw_sp {
 	struct mlxsw_sp_kvdl *kvdl;
 	struct mlxsw_sp_nve *nve;
 	struct notifier_block netdevice_nb;
+	struct mlxsw_sp_ptp_clock *clock;
 
 	struct mlxsw_sp_counter_pool *counter_pool;
 	struct {
@@ -172,6 +174,7 @@ struct mlxsw_sp {
 	const struct mlxsw_sp_rif_ops **rif_ops_arr;
 	const struct mlxsw_sp_sb_vals *sb_vals;
 	const struct mlxsw_sp_port_type_speed_ops *port_type_speed_ops;
+	const struct mlxsw_sp_ptp_ops *ptp_ops;
 };
 
 static inline struct mlxsw_sp_upper *
-- 
2.20.1

[PATCH net-next 9/9] selftests: ptp: Add Physical Hardware Clock test

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Test the PTP Physical Hardware Clock functionality using the "phc_ctl" (a
part of "linuxptp").

The test contains three sub-tests:
  * "settime" test
  * "adjtime" test
  * "adjfreq" test

"settime" test:
  * set the PHC time to 0 seconds.
  * wait for 120.5 seconds.
  * check if PHC time equal to 120.XX seconds.

"adjtime" test:
  * set the PHC time to 0 seconds.
  * adjust the time by 10 seconds.
  * check if PHC time equal to 10.XX seconds.

"adjfreq" test:
  * adjust the PHC frequency to be 1% faster.
  * set the PHC time to 0 seconds.
  * wait for 100.5 seconds.
  * check if PHC time equal to 101.XX seconds.

Usage:
  $ ./phc.sh /dev/ptp<X>

  It is possible to run a subset of the tests, for example:
    * To run only the "settime" test:
      $ TESTS="settime" ./phc.sh /dev/ptp<X>

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/ptp/phc.sh | 166 +++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100755 tools/testing/selftests/ptp/phc.sh

diff --git a/tools/testing/selftests/ptp/phc.sh b/tools/testing/selftests/ptp/phc.sh
new file mode 100755
index 000000000000..ac6e5a6e1d3a
--- /dev/null
+++ b/tools/testing/selftests/ptp/phc.sh
@@ -0,0 +1,166 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+	settime
+	adjtime
+	adjfreq
+"
+DEV=$1
+
+##############################################################################
+# Sanity checks
+
+if [[ "$(id -u)" -ne 0 ]]; then
+	echo "SKIP: need root privileges"
+	exit 0
+fi
+
+if [[ "$DEV" == "" ]]; then
+	echo "SKIP: PTP device not provided"
+	exit 0
+fi
+
+require_command()
+{
+	local cmd=$1; shift
+
+	if [[ ! -x "$(command -v "$cmd")" ]]; then
+		echo "SKIP: $cmd not installed"
+		exit 1
+	fi
+}
+
+phc_sanity()
+{
+	phc_ctl $DEV get &> /dev/null
+
+	if [ $? != 0 ]; then
+		echo "SKIP: unknown clock $DEV: No such device"
+		exit 1
+	fi
+}
+
+require_command phc_ctl
+phc_sanity
+
+##############################################################################
+# Helpers
+
+# Exit status to return at the end. Set in case one of the tests fails.
+EXIT_STATUS=0
+# Per-test return value. Clear at the beginning of each test.
+RET=0
+
+check_err()
+{
+	local err=$1
+
+	if [[ $RET -eq 0 && $err -ne 0 ]]; then
+		RET=$err
+	fi
+}
+
+log_test()
+{
+	local test_name=$1
+
+	if [[ $RET -ne 0 ]]; then
+		EXIT_STATUS=1
+		printf "TEST: %-60s  [FAIL]\n" "$test_name"
+		return 1
+	fi
+
+	printf "TEST: %-60s  [ OK ]\n" "$test_name"
+	return 0
+}
+
+tests_run()
+{
+	local current_test
+
+	for current_test in ${TESTS:-$ALL_TESTS}; do
+		$current_test
+	done
+}
+
+##############################################################################
+# Tests
+
+settime_do()
+{
+	local res
+
+	res=$(phc_ctl $DEV set 0 wait 120.5 get 2> /dev/null \
+		| awk '/clock time is/{print $5}' \
+		| awk -F. '{print $1}')
+
+	(( res == 120 ))
+}
+
+adjtime_do()
+{
+	local res
+
+	res=$(phc_ctl $DEV set 0 adj 10 get 2> /dev/null \
+		| awk '/clock time is/{print $5}' \
+		| awk -F. '{print $1}')
+
+	(( res == 10 ))
+}
+
+adjfreq_do()
+{
+	local res
+
+	# Set the clock to be 1% faster
+	res=$(phc_ctl $DEV freq 10000000 set 0 wait 100.5 get 2> /dev/null \
+		| awk '/clock time is/{print $5}' \
+		| awk -F. '{print $1}')
+
+	(( res == 101 ))
+}
+
+##############################################################################
+
+cleanup()
+{
+	phc_ctl $DEV freq 0.0 &> /dev/null
+	phc_ctl $DEV set &> /dev/null
+}
+
+settime()
+{
+	RET=0
+
+	settime_do
+	check_err $?
+	log_test "settime"
+	cleanup
+}
+
+adjtime()
+{
+	RET=0
+
+	adjtime_do
+	check_err $?
+	log_test "adjtime"
+	cleanup
+}
+
+adjfreq()
+{
+	RET=0
+
+	adjfreq_do
+	check_err $?
+	log_test "adjfreq"
+	cleanup
+}
+
+trap cleanup EXIT
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.20.1

Re: [PATCH net-next 9/9] selftests: ptp: Add Physical Hardware Clock test

[Richard Cochran]

On Mon, Jun 03, 2019 at 03:12:44PM +0300, Ido Schimmel wrote:
> From: Shalom Toledo <shalomt@mellanox.com>
> 
> Test the PTP Physical Hardware Clock functionality using the "phc_ctl" (a
> part of "linuxptp").
> 
> The test contains three sub-tests:
>   * "settime" test
>   * "adjtime" test
>   * "adjfreq" test

This looks useful!

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net-next 9/9] selftests: ptp: Add Physical Hardware Clock test

[Vladimir Oltean]

On Mon, 3 Jun 2019 at 15:25, Ido Schimmel <idosch@idosch.org> wrote:
>
> From: Shalom Toledo <shalomt@mellanox.com>
>
> Test the PTP Physical Hardware Clock functionality using the "phc_ctl" (a
> part of "linuxptp").
>
> The test contains three sub-tests:
>   * "settime" test
>   * "adjtime" test
>   * "adjfreq" test
>
> "settime" test:
>   * set the PHC time to 0 seconds.
>   * wait for 120.5 seconds.
>   * check if PHC time equal to 120.XX seconds.
>
> "adjtime" test:
>   * set the PHC time to 0 seconds.
>   * adjust the time by 10 seconds.
>   * check if PHC time equal to 10.XX seconds.
>
> "adjfreq" test:
>   * adjust the PHC frequency to be 1% faster.
>   * set the PHC time to 0 seconds.
>   * wait for 100.5 seconds.
>   * check if PHC time equal to 101.XX seconds.
>
> Usage:
>   $ ./phc.sh /dev/ptp<X>
>
>   It is possible to run a subset of the tests, for example:
>     * To run only the "settime" test:
>       $ TESTS="settime" ./phc.sh /dev/ptp<X>
>
> Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> Reviewed-by: Petr Machata <petrm@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  tools/testing/selftests/ptp/phc.sh | 166 +++++++++++++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100755 tools/testing/selftests/ptp/phc.sh
>
> diff --git a/tools/testing/selftests/ptp/phc.sh b/tools/testing/selftests/ptp/phc.sh
> new file mode 100755
> index 000000000000..ac6e5a6e1d3a
> --- /dev/null
> +++ b/tools/testing/selftests/ptp/phc.sh
> @@ -0,0 +1,166 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="
> +       settime
> +       adjtime
> +       adjfreq
> +"
> +DEV=$1
> +
> +##############################################################################
> +# Sanity checks
> +
> +if [[ "$(id -u)" -ne 0 ]]; then
> +       echo "SKIP: need root privileges"
> +       exit 0
> +fi
> +
> +if [[ "$DEV" == "" ]]; then
> +       echo "SKIP: PTP device not provided"
> +       exit 0
> +fi
> +
> +require_command()
> +{
> +       local cmd=$1; shift
> +
> +       if [[ ! -x "$(command -v "$cmd")" ]]; then
> +               echo "SKIP: $cmd not installed"
> +               exit 1
> +       fi
> +}
> +
> +phc_sanity()
> +{
> +       phc_ctl $DEV get &> /dev/null
> +
> +       if [ $? != 0 ]; then
> +               echo "SKIP: unknown clock $DEV: No such device"
> +               exit 1
> +       fi
> +}
> +
> +require_command phc_ctl
> +phc_sanity
> +
> +##############################################################################
> +# Helpers
> +
> +# Exit status to return at the end. Set in case one of the tests fails.
> +EXIT_STATUS=0
> +# Per-test return value. Clear at the beginning of each test.
> +RET=0
> +
> +check_err()
> +{
> +       local err=$1
> +
> +       if [[ $RET -eq 0 && $err -ne 0 ]]; then
> +               RET=$err
> +       fi
> +}
> +
> +log_test()
> +{
> +       local test_name=$1
> +
> +       if [[ $RET -ne 0 ]]; then
> +               EXIT_STATUS=1
> +               printf "TEST: %-60s  [FAIL]\n" "$test_name"
> +               return 1
> +       fi
> +
> +       printf "TEST: %-60s  [ OK ]\n" "$test_name"
> +       return 0
> +}
> +
> +tests_run()
> +{
> +       local current_test
> +
> +       for current_test in ${TESTS:-$ALL_TESTS}; do
> +               $current_test
> +       done
> +}
> +
> +##############################################################################
> +# Tests
> +
> +settime_do()
> +{
> +       local res
> +
> +       res=$(phc_ctl $DEV set 0 wait 120.5 get 2> /dev/null \
> +               | awk '/clock time is/{print $5}' \
> +               | awk -F. '{print $1}')
> +
> +       (( res == 120 ))
> +}
> +
> +adjtime_do()
> +{
> +       local res
> +
> +       res=$(phc_ctl $DEV set 0 adj 10 get 2> /dev/null \
> +               | awk '/clock time is/{print $5}' \
> +               | awk -F. '{print $1}')
> +
> +       (( res == 10 ))
> +}
> +
> +adjfreq_do()
> +{
> +       local res
> +
> +       # Set the clock to be 1% faster
> +       res=$(phc_ctl $DEV freq 10000000 set 0 wait 100.5 get 2> /dev/null \
> +               | awk '/clock time is/{print $5}' \
> +               | awk -F. '{print $1}')
> +
> +       (( res == 101 ))
> +}
> +
> +##############################################################################
> +
> +cleanup()
> +{
> +       phc_ctl $DEV freq 0.0 &> /dev/null
> +       phc_ctl $DEV set &> /dev/null
> +}
> +
> +settime()
> +{
> +       RET=0
> +
> +       settime_do
> +       check_err $?
> +       log_test "settime"
> +       cleanup
> +}
> +
> +adjtime()
> +{
> +       RET=0
> +
> +       adjtime_do
> +       check_err $?
> +       log_test "adjtime"
> +       cleanup
> +}
> +
> +adjfreq()
> +{
> +       RET=0
> +
> +       adjfreq_do
> +       check_err $?
> +       log_test "adjfreq"
> +       cleanup
> +}
> +
> +trap cleanup EXIT
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> --
> 2.20.1
>

Cool testing framework, thanks!
Some things to consider:
- Why the .5 in the wait commands?
- I suspect there's a huge margin of inaccuracy that the test is
missing by only looking at the 'seconds' portion of the PHC time after
the adjfreq operation (up to 10^9 - 1 ppb, in the worst case).

Tested-by: Vladimir Oltean <olteanv@gmail.com>

Regards,
-Vladimir

Re: [PATCH net-next 9/9] selftests: ptp: Add Physical Hardware Clock test

[Vladimir Oltean]

On 6/7/19 2:15 PM, Vladimir Oltean wrote:
> On Mon, 3 Jun 2019 at 15:25, Ido Schimmel <idosch@idosch.org> wrote:
>>
>> From: Shalom Toledo <shalomt@mellanox.com>
>>
>> Test the PTP Physical Hardware Clock functionality using the "phc_ctl" (a
>> part of "linuxptp").
>>
>> The test contains three sub-tests:
>>    * "settime" test
>>    * "adjtime" test
>>    * "adjfreq" test
>>
>> "settime" test:
>>    * set the PHC time to 0 seconds.
>>    * wait for 120.5 seconds.
>>    * check if PHC time equal to 120.XX seconds.
>>
>> "adjtime" test:
>>    * set the PHC time to 0 seconds.
>>    * adjust the time by 10 seconds.
>>    * check if PHC time equal to 10.XX seconds.
>>
>> "adjfreq" test:
>>    * adjust the PHC frequency to be 1% faster.
>>    * set the PHC time to 0 seconds.
>>    * wait for 100.5 seconds.
>>    * check if PHC time equal to 101.XX seconds.
>>
>> Usage:
>>    $ ./phc.sh /dev/ptp<X>
>>
>>    It is possible to run a subset of the tests, for example:
>>      * To run only the "settime" test:
>>        $ TESTS="settime" ./phc.sh /dev/ptp<X>
>>
>> Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
>> Reviewed-by: Petr Machata <petrm@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> ---
>>   tools/testing/selftests/ptp/phc.sh | 166 +++++++++++++++++++++++++++++
>>   1 file changed, 166 insertions(+)
>>   create mode 100755 tools/testing/selftests/ptp/phc.sh
>>
>> diff --git a/tools/testing/selftests/ptp/phc.sh b/tools/testing/selftests/ptp/phc.sh
>> new file mode 100755
>> index 000000000000..ac6e5a6e1d3a
>> --- /dev/null
>> +++ b/tools/testing/selftests/ptp/phc.sh
>> @@ -0,0 +1,166 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +ALL_TESTS="
>> +       settime
>> +       adjtime
>> +       adjfreq
>> +"
>> +DEV=$1
>> +
>> +##############################################################################
>> +# Sanity checks
>> +
>> +if [[ "$(id -u)" -ne 0 ]]; then
>> +       echo "SKIP: need root privileges"
>> +       exit 0
>> +fi
>> +
>> +if [[ "$DEV" == "" ]]; then
>> +       echo "SKIP: PTP device not provided"
>> +       exit 0
>> +fi
>> +
>> +require_command()
>> +{
>> +       local cmd=$1; shift
>> +
>> +       if [[ ! -x "$(command -v "$cmd")" ]]; then
>> +               echo "SKIP: $cmd not installed"
>> +               exit 1
>> +       fi
>> +}
>> +
>> +phc_sanity()
>> +{
>> +       phc_ctl $DEV get &> /dev/null
>> +
>> +       if [ $? != 0 ]; then
>> +               echo "SKIP: unknown clock $DEV: No such device"
>> +               exit 1
>> +       fi
>> +}
>> +
>> +require_command phc_ctl
>> +phc_sanity
>> +
>> +##############################################################################
>> +# Helpers
>> +
>> +# Exit status to return at the end. Set in case one of the tests fails.
>> +EXIT_STATUS=0
>> +# Per-test return value. Clear at the beginning of each test.
>> +RET=0
>> +
>> +check_err()
>> +{
>> +       local err=$1
>> +
>> +       if [[ $RET -eq 0 && $err -ne 0 ]]; then
>> +               RET=$err
>> +       fi
>> +}
>> +
>> +log_test()
>> +{
>> +       local test_name=$1
>> +
>> +       if [[ $RET -ne 0 ]]; then
>> +               EXIT_STATUS=1
>> +               printf "TEST: %-60s  [FAIL]\n" "$test_name"
>> +               return 1
>> +       fi
>> +
>> +       printf "TEST: %-60s  [ OK ]\n" "$test_name"
>> +       return 0
>> +}
>> +
>> +tests_run()
>> +{
>> +       local current_test
>> +
>> +       for current_test in ${TESTS:-$ALL_TESTS}; do
>> +               $current_test
>> +       done
>> +}
>> +
>> +##############################################################################
>> +# Tests
>> +
>> +settime_do()
>> +{
>> +       local res
>> +
>> +       res=$(phc_ctl $DEV set 0 wait 120.5 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 120 ))
>> +}
>> +
>> +adjtime_do()
>> +{
>> +       local res
>> +
>> +       res=$(phc_ctl $DEV set 0 adj 10 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 10 ))
>> +}
>> +
>> +adjfreq_do()
>> +{
>> +       local res
>> +
>> +       # Set the clock to be 1% faster
>> +       res=$(phc_ctl $DEV freq 10000000 set 0 wait 100.5 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 101 ))
>> +}
>> +
>> +##############################################################################
>> +
>> +cleanup()
>> +{
>> +       phc_ctl $DEV freq 0.0 &> /dev/null
>> +       phc_ctl $DEV set &> /dev/null
>> +}
>> +
>> +settime()
>> +{
>> +       RET=0
>> +
>> +       settime_do
>> +       check_err $?
>> +       log_test "settime"
>> +       cleanup
>> +}
>> +
>> +adjtime()
>> +{
>> +       RET=0
>> +
>> +       adjtime_do
>> +       check_err $?
>> +       log_test "adjtime"
>> +       cleanup
>> +}
>> +
>> +adjfreq()
>> +{
>> +       RET=0
>> +
>> +       adjfreq_do
>> +       check_err $?
>> +       log_test "adjfreq"
>> +       cleanup
>> +}
>> +
>> +trap cleanup EXIT
>> +
>> +tests_run
>> +
>> +exit $EXIT_STATUS
>> --
>> 2.20.1
>>
> 
> Cool testing framework, thanks!
> Some things to consider:
> - Why the .5 in the wait commands?
> - I suspect there's a huge margin of inaccuracy that the test is
> missing by only looking at the 'seconds' portion of the PHC time after
> the adjfreq operation (up to 10^9 - 1 ppb, in the worst case).
> 
> Tested-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Regards,
> -Vladimir
> 

Here, I was actually thinking about something like this:

check_with_tolerance()
{
	local res=$1
	local expected=$2
	local ppb=$3
	local expected_min=$(($expected - (($expected * $ppb) / 1000000000)))
	local expected_max=$(($expected + (($expected * $ppb) / 1000000000)))

	if [ $res -lt $expected_min ]; then
		printf "%d is more than %d ppb lower than expected %d (%d)\n" \
			$res $ppb $expected $expected_min
		return 1
	elif [ $res -gt $expected_max ]; then
		printf "%d is more than %d ppb higher than expected %d (%d)\n" \
			$res $ppb $expected $expected_max
		return 1;
	else
		printf "%d is within the +/-%d ppb tolerance of %d (%d - %d)\n" \
			$res $ppb $expected $expected_min $expected_max
		return 0;
	fi
}

settime_do()
{
	local res

	res=$(phc_ctl $DEV set 0 wait 120 get 2> /dev/null \
		 | awk '/clock time is/{print $5}' \
		 | awk -F. '{print $1 * 1000000000 + $2}')

	check_with_tolerance $res 120000000000 10000
}

adjtime_do()
{
	local res

	res=$(phc_ctl $DEV set 0 adj 10 get 2> /dev/null \
		 | awk '/clock time is/{print $5}' \
		 | awk -F. '{print $1 * 1000000000 + $2}')

	check_with_tolerance $res 10000000000 10000
}

adjfreq_do()
{
	local res

	# Set the clock to be 1% faster
	res=$(phc_ctl $DEV freq 10000000 set 0 wait 100 get 2> /dev/null \
		 | awk '/clock time is/{print $5}' \
		 | awk -F. '{print $1 * 1000000000 + $2}')

	check_with_tolerance $res 101000000000 10000
}


With the above changes:

SJA1105 hardware clock operations (not the timecounter ones that I 
submitted):

# ./phc.sh /dev/ptp1
119999611352 is within the +/-1000000 ppb tolerance of 120000000000 
(119880000000 - 120120000000)
TEST: settime                                                 [ OK ]
10001018472 is within the +/-1000000 ppb tolerance of 10000000000 
(9990000000 - 10010000000)
TEST: adjtime                                                 [ OK ]
100998300984 is within the +/-1000000 ppb tolerance of 101000000000 
(100899000000 - 101101000000)
TEST: adjfreq                                                 [ OK ]

But at a lower tolerance of 10000 ppb:

[root@OpenIL:~]# ./phc.sh /dev/ptp1
119998277344 is more than 10000 ppb lower than expected 120000000000 
(119998800000)
TEST: settime                                                 [FAIL]
10002033840 is more than 10000 ppb higher than expected 10000000000 
(10000100000)
TEST: adjtime                                                 [FAIL]
100998295304 is more than 10000 ppb lower than expected 101000000000 
(100998990000)
TEST: adjfreq                                                 [FAIL]

For reference, ptp_qoriq:

[root@OpenIL:~]# ./phc.sh /dev/ptp0
120000960470 is within the +/-10000 ppb tolerance of 120000000000 
(119998800000 - 120001200000)
TEST: settime                                                 [ OK ]
10000699770 is more than 10000 ppb higher than expected 10000000000 
(10000100000)
TEST: adjtime                                                 [FAIL]
101000211269 is within the +/-10000 ppb tolerance of 101000000000 
(100998990000 - 101001010000)
TEST: adjfreq                                                 [ OK ]

Regards,
-Vladimir

Re: [PATCH net-next 9/9] selftests: ptp: Add Physical Hardware Clock test

[Shalom Toledo]

On 07/06/2019 14:15, Vladimir Oltean wrote:
> On Mon, 3 Jun 2019 at 15:25, Ido Schimmel <idosch@idosch.org> wrote:
>>
>> From: Shalom Toledo <shalomt@mellanox.com>
>>
>> Test the PTP Physical Hardware Clock functionality using the "phc_ctl" (a
>> part of "linuxptp").
>>
>> The test contains three sub-tests:
>>   * "settime" test
>>   * "adjtime" test
>>   * "adjfreq" test
>>
>> "settime" test:
>>   * set the PHC time to 0 seconds.
>>   * wait for 120.5 seconds.
>>   * check if PHC time equal to 120.XX seconds.
>>
>> "adjtime" test:
>>   * set the PHC time to 0 seconds.
>>   * adjust the time by 10 seconds.
>>   * check if PHC time equal to 10.XX seconds.
>>
>> "adjfreq" test:
>>   * adjust the PHC frequency to be 1% faster.
>>   * set the PHC time to 0 seconds.
>>   * wait for 100.5 seconds.
>>   * check if PHC time equal to 101.XX seconds.
>>
>> Usage:
>>   $ ./phc.sh /dev/ptp<X>
>>
>>   It is possible to run a subset of the tests, for example:
>>     * To run only the "settime" test:
>>       $ TESTS="settime" ./phc.sh /dev/ptp<X>
>>
>> Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
>> Reviewed-by: Petr Machata <petrm@mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> ---
>>  tools/testing/selftests/ptp/phc.sh | 166 +++++++++++++++++++++++++++++
>>  1 file changed, 166 insertions(+)
>>  create mode 100755 tools/testing/selftests/ptp/phc.sh
>>
>> diff --git a/tools/testing/selftests/ptp/phc.sh b/tools/testing/selftests/ptp/phc.sh
>> new file mode 100755
>> index 000000000000..ac6e5a6e1d3a
>> --- /dev/null
>> +++ b/tools/testing/selftests/ptp/phc.sh
>> @@ -0,0 +1,166 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +ALL_TESTS="
>> +       settime
>> +       adjtime
>> +       adjfreq
>> +"
>> +DEV=$1
>> +
>> +##############################################################################
>> +# Sanity checks
>> +
>> +if [[ "$(id -u)" -ne 0 ]]; then
>> +       echo "SKIP: need root privileges"
>> +       exit 0
>> +fi
>> +
>> +if [[ "$DEV" == "" ]]; then
>> +       echo "SKIP: PTP device not provided"
>> +       exit 0
>> +fi
>> +
>> +require_command()
>> +{
>> +       local cmd=$1; shift
>> +
>> +       if [[ ! -x "$(command -v "$cmd")" ]]; then
>> +               echo "SKIP: $cmd not installed"
>> +               exit 1
>> +       fi
>> +}
>> +
>> +phc_sanity()
>> +{
>> +       phc_ctl $DEV get &> /dev/null
>> +
>> +       if [ $? != 0 ]; then
>> +               echo "SKIP: unknown clock $DEV: No such device"
>> +               exit 1
>> +       fi
>> +}
>> +
>> +require_command phc_ctl
>> +phc_sanity
>> +
>> +##############################################################################
>> +# Helpers
>> +
>> +# Exit status to return at the end. Set in case one of the tests fails.
>> +EXIT_STATUS=0
>> +# Per-test return value. Clear at the beginning of each test.
>> +RET=0
>> +
>> +check_err()
>> +{
>> +       local err=$1
>> +
>> +       if [[ $RET -eq 0 && $err -ne 0 ]]; then
>> +               RET=$err
>> +       fi
>> +}
>> +
>> +log_test()
>> +{
>> +       local test_name=$1
>> +
>> +       if [[ $RET -ne 0 ]]; then
>> +               EXIT_STATUS=1
>> +               printf "TEST: %-60s  [FAIL]\n" "$test_name"
>> +               return 1
>> +       fi
>> +
>> +       printf "TEST: %-60s  [ OK ]\n" "$test_name"
>> +       return 0
>> +}
>> +
>> +tests_run()
>> +{
>> +       local current_test
>> +
>> +       for current_test in ${TESTS:-$ALL_TESTS}; do
>> +               $current_test
>> +       done
>> +}
>> +
>> +##############################################################################
>> +# Tests
>> +
>> +settime_do()
>> +{
>> +       local res
>> +
>> +       res=$(phc_ctl $DEV set 0 wait 120.5 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 120 ))
>> +}
>> +
>> +adjtime_do()
>> +{
>> +       local res
>> +
>> +       res=$(phc_ctl $DEV set 0 adj 10 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 10 ))
>> +}
>> +
>> +adjfreq_do()
>> +{
>> +       local res
>> +
>> +       # Set the clock to be 1% faster
>> +       res=$(phc_ctl $DEV freq 10000000 set 0 wait 100.5 get 2> /dev/null \
>> +               | awk '/clock time is/{print $5}' \
>> +               | awk -F. '{print $1}')
>> +
>> +       (( res == 101 ))
>> +}
>> +
>> +##############################################################################
>> +
>> +cleanup()
>> +{
>> +       phc_ctl $DEV freq 0.0 &> /dev/null
>> +       phc_ctl $DEV set &> /dev/null
>> +}
>> +
>> +settime()
>> +{
>> +       RET=0
>> +
>> +       settime_do
>> +       check_err $?
>> +       log_test "settime"
>> +       cleanup
>> +}
>> +
>> +adjtime()
>> +{
>> +       RET=0
>> +
>> +       adjtime_do
>> +       check_err $?
>> +       log_test "adjtime"
>> +       cleanup
>> +}
>> +
>> +adjfreq()
>> +{
>> +       RET=0
>> +
>> +       adjfreq_do
>> +       check_err $?
>> +       log_test "adjfreq"
>> +       cleanup
>> +}
>> +
>> +trap cleanup EXIT
>> +
>> +tests_run
>> +
>> +exit $EXIT_STATUS
>> --
>> 2.20.1
>>
> 
> Cool testing framework, thanks!
> Some things to consider:
> - Why the .5 in the wait commands?

To make sure the clock get to the expected time. It's been tested on different
devices.

> - I suspect there's a huge margin of inaccuracy that the test is
> missing by only looking at the 'seconds' portion of the PHC time after
> the adjfreq operation (up to 10^9 - 1 ppb, in the worst case).

Correct, but this test does the work. It tests the basic functionality of PHC.

I'm just sending a v2 based on Richard comments, feel free to extended this
framework. All of us will get benefit from it.

> 
> Tested-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Regards,
> -Vladimir
> 

Re: [PATCH net-next 0/9] mlxsw: Add support for physical hardware clock

[David Miller]

From: Ido Schimmel <idosch@idosch.org>
Date: Mon,  3 Jun 2019 15:12:35 +0300

> This is the first of about four patchsets that add PTP support in mlxsw
> for the Spectrum-1 ASIC.

Richard, could you give this a quick review?

Thank you.