Re: [PATCH net-next v5 2/2] ptp: add FemtoClock3 Wireless as ptp hardware clock

From: Lee Jones
Date: Thu Dec 21 2023 - 11:16:46 EST

On Wed, 20 Dec 2023, Min Li wrote:

> From: Min Li <min.li.xe@xxxxxxxxxxx>
>
> The RENESAS FemtoClock3 Wireless is a high-performance jitter attenuator,
> frequency translator, and clock synthesizer. The device is comprised of 3
> digital PLLs (DPLL) to track CLKIN inputs and three independent low phase
> noise fractional output dividers (FOD) that output low phase noise clocks.
>
> FemtoClock3 supports one Time Synchronization (Time Sync) channel to enable
> an external processor to control the phase and frequency of the Time Sync
> channel and to take phase measurements using the TDC. Intended applications
> are synchronization using the precision time protocol (PTP) and
> synchronization with 0.5 Hz and 1 Hz signals from GNSS.
>
> Signed-off-by: Min Li <min.li.xe@xxxxxxxxxxx>
> ---
> drivers/ptp/Kconfig | 12 +
> drivers/ptp/Makefile | 1 +
> drivers/ptp/ptp_fc3.c | 1036 ++++++++++++++++++++++++++++
> drivers/ptp/ptp_fc3.h | 45 ++
> include/linux/mfd/idtRC38xxx_reg.h | 273 ++++++++

Why MFD? Where else is this being used?

> 5 files changed, 1367 insertions(+)
> create mode 100644 drivers/ptp/ptp_fc3.c
> create mode 100644 drivers/ptp/ptp_fc3.h
> create mode 100644 include/linux/mfd/idtRC38xxx_reg.h
>
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 5dd5f188e..604541dcb 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -155,6 +155,18 @@ config PTP_1588_CLOCK_IDTCM
> To compile this driver as a module, choose M here: the module
> will be called ptp_clockmatrix.
>
> +config PTP_1588_CLOCK_FC3W
> + tristate "RENESAS FemtoClock3 Wireless as PTP clock"
> + depends on PTP_1588_CLOCK && I2C
> + default n
> + help
> + This driver adds support for using Renesas FemtoClock3 Wireless
> + as a PTP clock. This clock is only useful if your time stamping
> + MAC is connected to the RENESAS chip.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called ptp_fc3.
> +
> config PTP_1588_CLOCK_MOCK
> tristate "Mock-up PTP clock"
> depends on PTP_1588_CLOCK
> diff --git a/drivers/ptp/ptp_fc3.h b/drivers/ptp/ptp_fc3.h
> new file mode 100644
> index 000000000..897101579
> --- /dev/null
> +++ b/drivers/ptp/ptp_fc3.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * PTP hardware clock driver for the FemtoClock3 family of timing and
> + * synchronization devices.
> + *
> + * Copyright (C) 2023 Integrated Device Technology, Inc., a Renesas Company.
> + */
> +#ifndef PTP_IDTFC3_H
> +#define PTP_IDTFC3_H
> +
> +#include <linux/ktime.h>
> +#include <linux/ptp_clock.h>
> +#include <linux/regmap.h>
> +
> +#define FW_FILENAME "idtfc3.bin"
> +
> +#define MAX_FFO_PPB (244000)
> +#define TDC_GET_PERIOD (10)
> +
> +struct idtfc3 {
> + struct ptp_clock_info caps;
> + struct ptp_clock *ptp_clock;
> + struct device *dev;
> + /* Mutex to protect operations from being interrupted */
> + struct mutex *lock;
> + struct device *mfd;

parent

Also, you can derive this from 'dev' above. You don't need both.

> + struct regmap *regmap;
> + struct idtfc3_hw_param hw_param;
> + u32 sub_sync_count;
> + u32 ns_per_sync;
> + int tdc_offset_sign;
> + u64 tdc_apll_freq;
> + u32 time_ref_freq;
> + u16 fod_n;
> + u8 lpf_mode;
> + /* Time counter */
> + u32 last_counter;
> + s64 ns;
> + u32 ns_per_counter;
> + u32 tc_update_period;
> + u32 tc_write_timeout;
> + s64 tod_write_overhead;
> +};
> +
> +#endif /* PTP_IDTFC3_H */
> diff --git a/include/linux/mfd/idtRC38xxx_reg.h b/include/linux/mfd/idtRC38xxx_reg.h
> new file mode 100644
> index 000000000..edddaae61
> --- /dev/null
> +++ b/include/linux/mfd/idtRC38xxx_reg.h
> @@ -0,0 +1,273 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Register Map - Based on PolarBear_CSRs.RevA.xlsx (2023-04-21)
> + *
> + * Copyright (C) 2023 Integrated Device Technology, Inc., a Renesas Company.
> + */
> +#ifndef HAVE_IDTRC38XXX_REG
> +#define HAVE_IDTRC38XXX_REG

MFD not HAVE.

> +
> +/* GLOBAL */

Everything in here is global?

> +#define SOFT_RESET_CTRL (0x15) /* Specific to FC3W */
> +#define MISC_CTRL (0x14) /* Specific to FC3A */
> +#define APLL_REINIT BIT(1)
> +#define APLL_REINIT_VFC3A BIT(2)
> +
> +#define DEVICE_ID (0x2)
> +#define DEVICE_ID_MASK (0x1000) /* Bit 12 is 1 if FC3W and 0 if FC3A */
> +#define DEVICE_ID_SHIFT (12)
> +
> +/* FOD */
> +#define FOD_0 (0x300)
> +#define FOD_0_VFC3A (0x400)
> +#define FOD_1 (0x340)
> +#define FOD_1_VFC3A (0x440)
> +#define FOD_2 (0x380)
> +#define FOD_2_VFC3A (0x480)
> +
> +/* TDCAPLL */
> +#define TDC_CTRL (0x44a) /* Specific to FC3W */
> +#define TDC_ENABLE_CTRL (0x169) /* Specific to FC3A */
> +#define TDC_DAC_CAL_CTRL (0x16a) /* Specific to FC3A */
> +#define TDC_EN BIT(0)
> +#define TDC_DAC_RECAL_REQ BIT(1)
> +#define TDC_DAC_RECAL_REQ_VFC3A BIT(0)
> +
> +#define TDC_FB_DIV_INT_CNFG (0x442)
> +#define TDC_FB_DIV_INT_CNFG_VFC3A (0x162)
> +#define TDC_FB_DIV_INT_MASK GENMASK(7, 0)
> +#define TDC_REF_DIV_CNFG (0x443)
> +#define TDC_REF_DIV_CNFG_VFC3A (0x163)
> +#define TDC_REF_DIV_CONFIG_MASK GENMASK(2, 0)
> +
> +/* TIME SYNC CHANNEL */
> +#define TIME_CLOCK_SRC (0xa01) /* Specific to FC3W */
> +#define TIME_CLOCK_COUNT (0xa00) /* Specific to FC3W */
> +#define TIME_CLOCK_COUNT_MASK GENMASK(5, 0)
> +
> +#define SUB_SYNC_GEN_CNFG (0xa04)
> +
> +#define TOD_COUNTER_READ_REQ (0xa5f)
> +#define TOD_COUNTER_READ_REQ_VFC3A (0x6df)
> +#define TOD_SYNC_LOAD_VAL_CTRL (0xa10)
> +#define TOD_SYNC_LOAD_VAL_CTRL_VFC3A (0x690)
> +#define SYNC_COUNTER_MASK GENMASK(51, 0)
> +#define SUB_SYNC_COUNTER_MASK GENMASK(30, 0)
> +#define TOD_SYNC_LOAD_REQ_CTRL (0xa21)
> +#define TOD_SYNC_LOAD_REQ_CTRL_VFC3A (0x6a1)
> +#define SYNC_LOAD_ENABLE BIT(1)
> +#define SUB_SYNC_LOAD_ENABLE BIT(0)
> +#define SYNC_LOAD_REQ BIT(0)
> +
> +#define LPF_MODE_CNFG (0xa80)
> +#define LPF_MODE_CNFG_VFC3A (0x700)
> +enum lpf_mode {
> + LPF_DISABLED = 0,
> + LPF_WP = 1,
> + LPF_HOLDOVER = 2,
> + LPF_WF = 3,
> + LPF_INVALID = 4
> +};
> +#define LPF_CTRL (0xa98)
> +#define LPF_CTRL_VFC3A (0x718)
> +#define LPF_EN BIT(0)
> +
> +#define LPF_BW_CNFG (0xa81)
> +#define LPF_BW_SHIFT GENMASK(7, 3)
> +#define LPF_BW_MULT GENMASK(2, 0)
> +#define LPF_BW_SHIFT_DEFAULT (0xb)
> +#define LPF_BW_MULT_DEFAULT (0x0)
> +#define LPF_BW_SHIFT_1PPS (0x5)
> +
> +#define LPF_WR_PHASE_CTRL (0xaa8)
> +#define LPF_WR_PHASE_CTRL_VFC3A (0x728)
> +#define LPF_WR_FREQ_CTRL (0xab0)
> +#define LPF_WR_FREQ_CTRL_VFC3A (0x730)
> +
> +#define TIME_CLOCK_TDC_FANOUT_CNFG (0xB00)
> +#define TIME_SYNC_TO_TDC_EN BIT(0)
> +#define SIG1_MUX_SEL_MASK GENMASK(7, 4)
> +#define SIG2_MUX_SEL_MASK GENMASK(11, 8)
> +enum tdc_mux_sel {
> + REF0 = 0,
> + REF1 = 1,
> + REF2 = 2,
> + REF3 = 3,
> + REF_CLK5 = 4,
> + REF_CLK6 = 5,
> + DPLL_FB_TO_TDC = 6,
> + DPLL_FB_DIVIDED_TO_TDC = 7,
> + TIME_CLK_DIVIDED = 8,
> + TIME_SYNC = 9,
> +};
> +
> +#define TIME_CLOCK_MEAS_CNFG (0xB04)
> +#define TDC_MEAS_MODE BIT(0)
> +enum tdc_meas_mode {
> + CONTINUOUS = 0,
> + ONE_SHOT = 1,
> + MEAS_MODE_INVALID = 2,
> +};
> +
> +#define TIME_CLOCK_MEAS_DIV_CNFG (0xB08)
> +#define TIME_REF_DIV_MASK GENMASK(29, 24)
> +
> +#define TIME_CLOCK_MEAS_CTRL (0xB10)
> +#define TDC_MEAS_EN BIT(0)
> +#define TDC_MEAS_START BIT(1)
> +
> +#define TDC_FIFO_READ_REQ (0xB2F)
> +#define TDC_FIFO_READ (0xB30)
> +#define COARSE_MEAS_MASK GENMASK(39, 13)
> +#define FINE_MEAS_MASK GENMASK(12, 0)
> +
> +#define TDC_FIFO_CTRL (0xB12)
> +#define FIFO_CLEAR BIT(0)
> +#define TDC_FIFO_STS (0xB38)
> +#define FIFO_FULL BIT(1)
> +#define FIFO_EMPTY BIT(0)
> +#define TDC_FIFO_EVENT (0xB39)
> +#define FIFO_OVERRUN BIT(1)
> +
> +/* DPLL */
> +#define MAX_REFERENCE_INDEX (3)
> +#define MAX_NUM_REF_PRIORITY (4)
> +
> +#define MAX_DPLL_INDEX (2)
> +
> +#define DPLL_STS (0x580)
> +#define DPLL_STS_VFC3A (0x571)
> +#define DPLL_STATE_STS_MASK (0x70)
> +#define DPLL_STATE_STS_SHIFT (4)
> +#define DPLL_REF_SEL_STS_MASK (0x6)
> +#define DPLL_REF_SEL_STS_SHIFT (1)
> +
> +#define DPLL_REF_PRIORITY_CNFG (0x502)
> +#define DPLL_REFX_PRIORITY_DISABLE_MASK (0xf)
> +#define DPLL_REF0_PRIORITY_ENABLE_AND_SET_MASK (0x31)
> +#define DPLL_REF1_PRIORITY_ENABLE_AND_SET_MASK (0xc2)
> +#define DPLL_REF2_PRIORITY_ENABLE_AND_SET_MASK (0x304)
> +#define DPLL_REF3_PRIORITY_ENABLE_AND_SET_MASK (0xc08)
> +#define DPLL_REF0_PRIORITY_SHIFT (4)
> +#define DPLL_REF1_PRIORITY_SHIFT (6)
> +#define DPLL_REF2_PRIORITY_SHIFT (8)
> +#define DPLL_REF3_PRIORITY_SHIFT (10)
> +
> +enum dpll_state {
> + DPLL_STATE_MIN = 0,
> + DPLL_STATE_FREERUN = DPLL_STATE_MIN,
> + DPLL_STATE_LOCKED = 1,
> + DPLL_STATE_HOLDOVER = 2,
> + DPLL_STATE_WRITE_FREQUENCY = 3,
> + DPLL_STATE_ACQUIRE = 4,
> + DPLL_STATE_HITLESS_SWITCH = 5,
> + DPLL_STATE_MAX = DPLL_STATE_HITLESS_SWITCH
> +};
> +
> +/* REFMON */
> +#define LOSMON_STS_0 (0x81e)
> +#define LOSMON_STS_0_VFC3A (0x18e)
> +#define LOSMON_STS_1 (0x82e)
> +#define LOSMON_STS_1_VFC3A (0x19e)
> +#define LOSMON_STS_2 (0x83e)
> +#define LOSMON_STS_2_VFC3A (0x1ae)
> +#define LOSMON_STS_3 (0x84e)
> +#define LOSMON_STS_3_VFC3A (0x1be)
> +#define LOS_STS_MASK (0x1)
> +
> +#define FREQMON_STS_0 (0x874)
> +#define FREQMON_STS_0_VFC3A (0x1d4)
> +#define FREQMON_STS_1 (0x894)
> +#define FREQMON_STS_1_VFC3A (0x1f4)
> +#define FREQMON_STS_2 (0x8b4)
> +#define FREQMON_STS_2_VFC3A (0x214)
> +#define FREQMON_STS_3 (0x8d4)
> +#define FREQMON_STS_3_VFC3A (0x234)
> +#define FREQ_FAIL_STS_SHIFT (31)
> +
> +/* Firmware interface */
> +#define TIME_CLK_FREQ_ADDR (0xffa0)
> +#define XTAL_FREQ_ADDR (0xffa1)
> +
> +/*
> + * Return register address and field mask based on passed in firmware version
> + */
> +#define IDTFC3_FW_REG(FW, VER, REG) (((FW) < (VER)) ? (REG) : (REG##_##VER))
> +#define IDTFC3_FW_FIELD(FW, VER, FIELD) (((FW) < (VER)) ? (FIELD) : (FIELD##_##VER))
> +enum fw_version {
> + V_DEFAULT = 0,
> + VFC3W = 1,
> + VFC3A = 2
> +};
> +
> +/* XTAL_FREQ_ADDR/TIME_CLK_FREQ_ADDR */
> +enum {
> + FREQ_MIN = 0,
> + FREQ_25M = 1,
> + FREQ_49_152M = 2,
> + FREQ_50M = 3,
> + FREQ_100M = 4,
> + FREQ_125M = 5,
> + FREQ_250M = 6,
> + FREQ_MAX
> +};
> +
> +struct idtfc3_hw_param {
> + u32 xtal_freq;
> + u32 time_clk_freq;
> +};
> +
> +struct idtfc3_fwrc {
> + u8 hiaddr;
> + u8 loaddr;
> + u8 value;
> + u8 reserved;
> +} __packed;

How much of the last 300 lines are actually being used?

> +static inline void idtfc3_default_hw_param(struct idtfc3_hw_param *hw_param)
> +{
> + hw_param->xtal_freq = 49152000;
> + hw_param->time_clk_freq = 25000000;
> +}

This is an odd use of an inline function.

Why not define them and use them else where?

> +static inline int idtfc3_set_hw_param(struct idtfc3_hw_param *hw_param,
> + u16 addr, u8 val)
> +{
> + if (addr == XTAL_FREQ_ADDR)
> + switch (val) {
> + case FREQ_49_152M:
> + hw_param->xtal_freq = 49152000;
> + break;
> + case FREQ_50M:
> + hw_param->xtal_freq = 50000000;
> + break;
> + default:
> + return -EINVAL;
> + }
> + else if (addr == TIME_CLK_FREQ_ADDR)
> + switch (val) {
> + case FREQ_25M:
> + hw_param->time_clk_freq = 25000000;
> + break;
> + case FREQ_50M:
> + hw_param->time_clk_freq = 50000000;
> + break;
> + case FREQ_100M:
> + hw_param->time_clk_freq = 100000000;
> + break;
> + case FREQ_125M:
> + hw_param->time_clk_freq = 125000000;
> + break;
> + case FREQ_250M:
> + hw_param->time_clk_freq = 250000000;
> + break;
> + default:
> + return -EINVAL;
> + }
> + else
> + return -EFAULT;
> +
> + return 0;
> +}

As above. Why is this being shoved in here?

> +#endif

--
Lee Jones [李琼斯]