Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
> >> +static void set_default_function_pointers(struct idtcm *idtcm)
> >> +{
> >> + idtcm->_idtcm_gettime = _idtcm_gettime;
> >> + idtcm->_idtcm_settime = _idtcm_settime;
> >> + idtcm->_idtcm_rdwr = idtcm_rdwr;
> >> + idtcm->_sync_pll_output = sync_pll_output;
> >> +}
> >
> >Why does this indirection? Are the SPI versions of the silicon?
>
> The indirection is to enable us to replace those functions in
> our unit tests with mocked functions.
Due to Spectra/meltdown etc, indirection is now expensive. But i guess
the I2C operations are a lot more expensive.
But in general, we try to keep the code KISS. Have you tried other
ways of doing this. Have your unit test framework implement
i2c_transfer()?
> I read somewhere that I should leave a week between sending a
> revised patch series. Is this a good rule to follow?
There are different 'timers'. One is how long to wait for review
comments, and reposting when you don't receiver any comments. netdev
for example is fast, a couple of days. Other subsystems, you need to
wait two weeks. Another 'timer' is how often to post new versions. In
general, never more than once per day. And the slower the subsystem is
for making reviews, the longer you should wait for additional review
comments.
What also plays a role is that the merge window is currently open. So
most subsystems won't accept patches at the moment. You need to wait
until it closes before submitting patches you expect to be good enough
to be accepted.
Andrew
Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
On Fri, Sep 27, 2019 at 08:25:18AM EDT, Andrew Lunn wrote:
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> + u8 regaddr,
>> + u8 *buf,
>> + u16 count,
>> + bool write)
>> +{
>> +struct i2c_client *client = idtcm->client;
>> +struct i2c_msg msg[2];
>> +s32 cnt;
>> +
>> +msg[0].addr = client->addr;
>> +msg[0].flags = 0;
>> +msg[0].len = 1;
>> +msg[0].buf =
>> +
>> +msg[1].addr = client->addr;
>> +msg[1].flags = write ? 0 : I2C_M_RD;
>> +msg[1].len = count;
>> +msg[1].buf = buf;
>> +
>> +cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> +if (cnt < 0) {
>> +dev_err(>dev, "i2c_transfer returned %d\n", cnt);
>> +return cnt;
>> +} else if (cnt != 2) {
>> +dev_err(>dev,
>> +"i2c_transfer sent only %d of %d messages\n", cnt, 2);
>> +return -EIO;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
>> +{
>> +u8 buf[4];
>> +s32 err;
>
>Hi Vincent
Hi Andrew,
Thank-you for looking at the patch.
>All your functions return s32, rather than the usual int. err is an
>s32. i2c_transfer() will return an int, which you then assign to an
>s32. I've no idea, but maybe the static code checkers like smatch
>will complain about this, especially on 64 bit systems? I suspect on
>64 bit machines, the compiler will be generating worse code, masking
>registers? Maybe use int, not s32?
Oops. You are correct, I messed up when trying to standardize
on linux types.h. I will go through the code to ensure int is used
for error codes and return values.
>> +case OUTPUT_MASK_PLL2_ADDR + 1:
>> +SET_U16_MSB(idtcm->channel[2].output_mask, val);
>> +break;
>> +case OUTPUT_MASK_PLL3_ADDR:
>> +SET_U16_LSB(idtcm->channel[3].output_mask, val);
>> +break;
>> +case OUTPUT_MASK_PLL3_ADDR + 1:
>> +SET_U16_MSB(idtcm->channel[3].output_mask, val);
>> +break;
>> +default:
>> +err = -1;
>
>EINVAL?
Yes, will replace with -EINVAL. Thanks.
>> +static void set_default_function_pointers(struct idtcm *idtcm)
>> +{
>> +idtcm->_idtcm_gettime = _idtcm_gettime;
>> +idtcm->_idtcm_settime = _idtcm_settime;
>> +idtcm->_idtcm_rdwr = idtcm_rdwr;
>> +idtcm->_sync_pll_output = sync_pll_output;
>> +}
>
>Why does this indirection? Are the SPI versions of the silicon?
The indirection is to enable us to replace those functions in
our unit tests with mocked functions.
I read somewhere that I should leave a week between sending a
revised patch series. Is this a good rule to follow?
Regards,
Vincent
Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
> +static s32 idtcm_xfer(struct idtcm *idtcm,
> + u8 regaddr,
> + u8 *buf,
> + u16 count,
> + bool write)
> +{
> + struct i2c_client *client = idtcm->client;
> + struct i2c_msg msg[2];
> + s32 cnt;
> +
> + msg[0].addr = client->addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf =
> +
> + msg[1].addr = client->addr;
> + msg[1].flags = write ? 0 : I2C_M_RD;
> + msg[1].len = count;
> + msg[1].buf = buf;
> +
> + cnt = i2c_transfer(client->adapter, msg, 2);
> +
> + if (cnt < 0) {
> + dev_err(>dev, "i2c_transfer returned %d\n", cnt);
> + return cnt;
> + } else if (cnt != 2) {
> + dev_err(>dev,
> + "i2c_transfer sent only %d of %d messages\n", cnt, 2);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
> +{
> + u8 buf[4];
> + s32 err;
Hi Vincent
All your functions return s32, rather than the usual int. err is an
s32. i2c_transfer() will return an int, which you then assign to an
s32. I've no idea, but maybe the static code checkers like smatch
will complain about this, especially on 64 bit systems? I suspect on
64 bit machines, the compiler will be generating worse code, masking
registers? Maybe use int, not s32?
> +static s32 set_pll_output_mask(struct idtcm *idtcm, u16 addr, u8 val)
> +{
> + s32 err = 0;
> +
> + switch (addr) {
> + case OUTPUT_MASK_PLL0_ADDR:
> + SET_U16_LSB(idtcm->channel[0].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL0_ADDR + 1:
> + SET_U16_MSB(idtcm->channel[0].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL1_ADDR:
> + SET_U16_LSB(idtcm->channel[1].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL1_ADDR + 1:
> + SET_U16_MSB(idtcm->channel[1].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL2_ADDR:
> + SET_U16_LSB(idtcm->channel[2].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL2_ADDR + 1:
> + SET_U16_MSB(idtcm->channel[2].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL3_ADDR:
> + SET_U16_LSB(idtcm->channel[3].output_mask, val);
> + break;
> + case OUTPUT_MASK_PLL3_ADDR + 1:
> + SET_U16_MSB(idtcm->channel[3].output_mask, val);
> + break;
> + default:
> + err = -1;
EINVAL?
> + break;
> + }
> +
> + return err;
> +}
> +static void set_default_function_pointers(struct idtcm *idtcm)
> +{
> + idtcm->_idtcm_gettime = _idtcm_gettime;
> + idtcm->_idtcm_settime = _idtcm_settime;
> + idtcm->_idtcm_rdwr = idtcm_rdwr;
> + idtcm->_sync_pll_output = sync_pll_output;
> +}
Why does this indirection? Are the SPI versions of the silicon?
Andrew
[PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
From: Vincent Cheng The IDT ClockMatrix (TM) family includes integrated devices that provide eight PLL channels. Each PLL channel can be independently configured as a frequency synthesizer, jitter attenuator, digitally controlled oscillator (DCO), or a digital phase lock loop (DPLL). Typically these devices are used as timing references and clock sources for PTP applications. This patch adds support for the device. Co-developed-by: Richard Cochran Signed-off-by: Richard Cochran Signed-off-by: Vincent Cheng --- Changes since v1: - Reported-by: kbuild test robot Fix ARCH=i386 build failure: ERROR: "__divdi3" undefined! - As suggested by Andrew Lunn: 1. Replace pr_err with dev_err because we are an i2c device 2. Replace set_current_state()+schedule_timeout() with msleep_interruptable() 3. Downgrade pr_info to dev_dbg where appropriate --- drivers/ptp/Kconfig | 12 + drivers/ptp/Makefile |1 + drivers/ptp/idt8a340_reg.h| 659 drivers/ptp/ptp_clockmatrix.c | 1385 + drivers/ptp/ptp_clockmatrix.h | 123 5 files changed, 2180 insertions(+) create mode 100644 drivers/ptp/idt8a340_reg.h create mode 100644 drivers/ptp/ptp_clockmatrix.c create mode 100644 drivers/ptp/ptp_clockmatrix.h diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index 960961f..16c7c90 100644 --- a/drivers/ptp/Kconfig +++ b/drivers/ptp/Kconfig @@ -119,4 +119,16 @@ config PTP_1588_CLOCK_KVM To compile this driver as a module, choose M here: the module will be called ptp_kvm. +config PTP_1588_CLOCK_IDTCM + tristate "IDT CLOCKMATRIX as PTP clock" + select PTP_1588_CLOCK + default n + help + This driver adds support for using IDT CLOCKMATRIX(TM) as a PTP + clock. This clock is only useful if your time stamping MAC + is connected to the IDT chip. + + To compile this driver as a module, choose M here: the module + will be called ptp_clockmatrix. + endmenu diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index 677d1d1..69a06f8 100644 --- a/drivers/ptp/Makefile +++ b/drivers/ptp/Makefile @@ -12,3 +12,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_KVM) += ptp_kvm.o obj-$(CONFIG_PTP_1588_CLOCK_QORIQ) += ptp-qoriq.o ptp-qoriq-y+= ptp_qoriq.o ptp-qoriq-$(CONFIG_DEBUG_FS) += ptp_qoriq_debugfs.o +obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o \ No newline at end of file diff --git a/drivers/ptp/idt8a340_reg.h b/drivers/ptp/idt8a340_reg.h new file mode 100644 index 000..9263bc3 --- /dev/null +++ b/drivers/ptp/idt8a340_reg.h @@ -0,0 +1,659 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* idt8a340_reg.h + * + * Originally generated by regen.tcl on Thu Feb 14 19:23:44 PST 2019 + * https://github.com/richardcochran/regen + * + * Hand modified to include some HW registers. + * Based on 4.8.0, SCSR rev C commit a03c7ae5 + */ +#ifndef HAVE_IDT8A340_REG +#define HAVE_IDT8A340_REG + +#define PAGE_ADDR_BASE0x +#define PAGE_ADDR 0x00fc + +#define HW_REVISION 0x8180 +#define REV_ID0x007a + +#define HW_DPLL_0 (0x8a00) +#define HW_DPLL_1 (0x8b00) +#define HW_DPLL_2 (0x8c00) +#define HW_DPLL_3 (0x8d00) + +#define HW_DPLL_TOD_SW_TRIG_ADDR__0 (0x080) +#define HW_DPLL_TOD_CTRL_1(0x089) +#define HW_DPLL_TOD_CTRL_2(0x08A) +#define HW_DPLL_TOD_OVR__0(0x098) +#define HW_DPLL_TOD_OUT_0__0 (0x0B0) + +#define HW_Q0_Q1_CH_SYNC_CTRL_0 (0xa740) +#define HW_Q0_Q1_CH_SYNC_CTRL_1 (0xa741) +#define HW_Q2_Q3_CH_SYNC_CTRL_0 (0xa742) +#define HW_Q2_Q3_CH_SYNC_CTRL_1 (0xa743) +#define HW_Q4_Q5_CH_SYNC_CTRL_0 (0xa744) +#define HW_Q4_Q5_CH_SYNC_CTRL_1 (0xa745) +#define HW_Q6_Q7_CH_SYNC_CTRL_0 (0xa746) +#define HW_Q6_Q7_CH_SYNC_CTRL_1 (0xa747) +#define HW_Q8_CH_SYNC_CTRL_0 (0xa748) +#define HW_Q8_CH_SYNC_CTRL_1 (0xa749) +#define HW_Q9_CH_SYNC_CTRL_0 (0xa74a) +#define HW_Q9_CH_SYNC_CTRL_1 (0xa74b) +#define HW_Q10_CH_SYNC_CTRL_0 (0xa74c) +#define HW_Q10_CH_SYNC_CTRL_1 (0xa74d) +#define HW_Q11_CH_SYNC_CTRL_0 (0xa74e) +#define HW_Q11_CH_SYNC_CTRL_1 (0xa74f) + +#define SYNC_SOURCE_DPLL0_TOD_PPS 0x14 +#define SYNC_SOURCE_DPLL1_TOD_PPS 0x15 +#define SYNC_SOURCE_DPLL2_TOD_PPS 0x16 +#define SYNC_SOURCE_DPLL3_TOD_PPS 0x17 + +#define SYNCTRL1_MASTER_SYNC_RST BIT(7) +#define SYNCTRL1_MASTER_SYNC_TRIG BIT(5) +#define SYNCTRL1_TOD_SYNC_TRIG BIT(4) +#define SYNCTRL1_FBDIV_FRAME_SYNC_TRIG BIT(3) +#define SYNCTRL1_FBDIV_SYNC_TRIG BIT(2) +#define
