* [PATCH net-next 1/3] ptp: idt82p33: add adjphase support
@ 2020-11-04 16:01 min.li.xe
2020-11-04 16:01 ` [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write min.li.xe
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: min.li.xe @ 2020-11-04 16:01 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Add idt82p33_adjphase() to support PHC write phase mode.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_idt82p33.c | 226 ++++++++++++++++++++++++++++++++-------------
drivers/ptp/ptp_idt82p33.h | 2 +
2 files changed, 164 insertions(+), 64 deletions(-)
diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index 179f6c4..556cf6c 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -21,6 +21,7 @@ MODULE_DESCRIPTION("Driver for IDT 82p33xxx clock devices");
MODULE_AUTHOR("IDT support-1588 <IDT-support-1588@lm.renesas.com>");
MODULE_VERSION("1.0");
MODULE_LICENSE("GPL");
+MODULE_FIRMWARE(FW_FILENAME);
/* Module Parameters */
static u32 sync_tod_timeout = SYNC_TOD_TIMEOUT_SEC;
@@ -116,7 +117,7 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
if (idt82p33->page_offset == val)
return 0;
- err = idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
+ err = _idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
if (err)
dev_err(&idt82p33->client->dev,
"failed to set page offset %d\n", val);
@@ -129,11 +130,12 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
static int idt82p33_rdwr(struct idt82p33 *idt82p33, unsigned int regaddr,
unsigned char *buf, unsigned int count, bool write)
{
- u8 offset, page;
int err;
+ u8 page;
+ u8 offset;
page = _PAGE(regaddr);
- offset = _OFFSET(regaddr);
+ offset = _OFFSET(regaddr);
err = idt82p33_page_offset(idt82p33, page);
if (err)
@@ -145,13 +147,13 @@ static int idt82p33_rdwr(struct idt82p33 *idt82p33, unsigned int regaddr,
}
static int idt82p33_read(struct idt82p33 *idt82p33, unsigned int regaddr,
- unsigned char *buf, unsigned int count)
+ unsigned char *buf, unsigned int count)
{
return idt82p33_rdwr(idt82p33, regaddr, buf, count, false);
}
static int idt82p33_write(struct idt82p33 *idt82p33, unsigned int regaddr,
- unsigned char *buf, unsigned int count)
+ unsigned char *buf, unsigned int count)
{
return idt82p33_rdwr(idt82p33, regaddr, buf, count, true);
}
@@ -448,8 +450,11 @@ static int idt82p33_measure_tod_write_overhead(struct idt82p33_channel *channel)
err = idt82p33_measure_settime_gettime_gap_overhead(channel, &gap_ns);
- if (err)
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
return err;
+ }
err = idt82p33_measure_one_byte_write_overhead(channel,
&one_byte_write_ns);
@@ -518,13 +523,10 @@ static int idt82p33_sync_tod(struct idt82p33_channel *channel, bool enable)
u8 sync_cnfg;
int err;
- if (enable == channel->sync_tod_on) {
- if (enable && sync_tod_timeout) {
- mod_delayed_work(system_wq, &channel->sync_tod_work,
- sync_tod_timeout * HZ);
- }
- return 0;
- }
+ /* Turn it off after sync_tod_timeout seconds */
+ if (enable && sync_tod_timeout)
+ ptp_schedule_worker(channel->ptp_clock,
+ sync_tod_timeout * HZ);
err = idt82p33_read(idt82p33, channel->dpll_sync_cnfg,
&sync_cnfg, sizeof(sync_cnfg));
@@ -541,20 +543,13 @@ static int idt82p33_sync_tod(struct idt82p33_channel *channel, bool enable)
if (err)
return err;
- channel->sync_tod_on = enable;
-
- if (enable && sync_tod_timeout) {
- mod_delayed_work(system_wq, &channel->sync_tod_work,
- sync_tod_timeout * HZ);
- }
-
return 0;
}
-static void idt82p33_sync_tod_work_handler(struct work_struct *work)
+static long idt82p33_sync_tod_work_handler(struct ptp_clock_info *ptp)
{
struct idt82p33_channel *channel =
- container_of(work, struct idt82p33_channel, sync_tod_work.work);
+ container_of(ptp, struct idt82p33_channel, caps);
struct idt82p33 *idt82p33 = channel->idt82p33;
mutex_lock(&idt82p33->reg_lock);
@@ -562,35 +557,51 @@ static void idt82p33_sync_tod_work_handler(struct work_struct *work)
(void)idt82p33_sync_tod(channel, false);
mutex_unlock(&idt82p33->reg_lock);
+
+ /* Return a negative value here to not reschedule */
+ return -1;
}
-static int idt82p33_pps_enable(struct idt82p33_channel *channel, bool enable)
+static int idt82p33_output_enable(struct idt82p33_channel *channel,
+ bool enable, unsigned int outn)
{
struct idt82p33 *idt82p33 = channel->idt82p33;
- u8 mask, outn, val;
int err;
+ u8 val;
+
+ err = idt82p33_read(idt82p33, OUT_MUX_CNFG(outn), &val, sizeof(val));
+
+ if (err)
+ return err;
+
+ if (enable)
+ val &= ~SQUELCH_ENABLE;
+ else
+ val |= SQUELCH_ENABLE;
+
+ return idt82p33_write(idt82p33, OUT_MUX_CNFG(outn), &val, sizeof(val));
+}
+
+static int idt82p33_output_mask_enable(struct idt82p33_channel *channel,
+ bool enable)
+{
+ u16 mask;
+ int err;
+ u8 outn;
mask = channel->output_mask;
outn = 0;
while (mask) {
- if (mask & 0x1) {
- err = idt82p33_read(idt82p33, OUT_MUX_CNFG(outn),
- &val, sizeof(val));
- if (err)
- return err;
- if (enable)
- val &= ~SQUELCH_ENABLE;
- else
- val |= SQUELCH_ENABLE;
+ if (mask & 0x1) {
- err = idt82p33_write(idt82p33, OUT_MUX_CNFG(outn),
- &val, sizeof(val));
+ err = idt82p33_output_enable(channel, enable, outn);
if (err)
return err;
}
+
mask >>= 0x1;
outn++;
}
@@ -598,6 +609,20 @@ static int idt82p33_pps_enable(struct idt82p33_channel *channel, bool enable)
return 0;
}
+static int idt82p33_perout_enable(struct idt82p33_channel *channel,
+ bool enable,
+ struct ptp_perout_request *perout)
+{
+ unsigned int flags = perout->flags;
+
+ /* Enable/disable output based on output_mask */
+ if (flags == PEROUT_ENABLE_OUTPUT_MASK)
+ return idt82p33_output_mask_enable(channel, enable);
+
+ /* Enable/disable individual output instead */
+ return idt82p33_output_enable(channel, enable, perout->index);
+}
+
static int idt82p33_enable_tod(struct idt82p33_channel *channel)
{
struct idt82p33 *idt82p33 = channel->idt82p33;
@@ -611,15 +636,13 @@ static int idt82p33_enable_tod(struct idt82p33_channel *channel)
if (err)
return err;
- err = idt82p33_pps_enable(channel, false);
-
- if (err)
- return err;
-
err = idt82p33_measure_tod_write_overhead(channel);
- if (err)
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
return err;
+ }
err = _idt82p33_settime(channel, &ts);
@@ -638,10 +661,8 @@ static void idt82p33_ptp_clock_unregister_all(struct idt82p33 *idt82p33)
channel = &idt82p33->channel[i];
- if (channel->ptp_clock) {
+ if (channel->ptp_clock)
ptp_clock_unregister(channel->ptp_clock);
- cancel_delayed_work_sync(&channel->sync_tod_work);
- }
}
}
@@ -659,14 +680,16 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
if (rq->type == PTP_CLK_REQ_PEROUT) {
if (!on)
- err = idt82p33_pps_enable(channel, false);
+ err = idt82p33_perout_enable(channel, false,
+ &rq->perout);
/* Only accept a 1-PPS aligned to the second. */
else if (rq->perout.start.nsec || rq->perout.period.sec != 1 ||
rq->perout.period.nsec) {
err = -ERANGE;
} else
- err = idt82p33_pps_enable(channel, true);
+ err = idt82p33_perout_enable(channel, true,
+ &rq->perout);
}
mutex_unlock(&idt82p33->reg_lock);
@@ -674,6 +697,49 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
return err;
}
+static int idt82p33_adjwritephase(struct ptp_clock_info *ptp, s32 offsetNs)
+{
+ struct idt82p33_channel *channel =
+ container_of(ptp, struct idt82p33_channel, caps);
+ struct idt82p33 *idt82p33 = channel->idt82p33;
+ s64 offsetInFs;
+ s64 offsetRegVal;
+ u8 val[4] = {0};
+ int err;
+
+ offsetInFs = (s64)(-offsetNs) * 1000000;
+
+ if (offsetInFs > WRITE_PHASE_OFFSET_LIMIT)
+ offsetInFs = WRITE_PHASE_OFFSET_LIMIT;
+ else if (offsetInFs < -WRITE_PHASE_OFFSET_LIMIT)
+ offsetInFs = -WRITE_PHASE_OFFSET_LIMIT;
+
+ /* Convert from phaseOffsetInFs to register value */
+ offsetRegVal = div_s64(offsetInFs * 1000, IDT_T0DPLL_PHASE_RESOL);
+
+ val[0] = offsetRegVal & 0xFF;
+ val[1] = (offsetRegVal >> 8) & 0xFF;
+ val[2] = (offsetRegVal >> 16) & 0xFF;
+ val[3] = (offsetRegVal >> 24) & 0x1F;
+ val[3] |= PH_OFFSET_EN;
+
+ mutex_lock(&idt82p33->reg_lock);
+
+ err = idt82p33_dpll_set_mode(channel, PLL_MODE_WPH);
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
+ goto out;
+ }
+
+ err = idt82p33_write(idt82p33, channel->dpll_phase_cnfg, val,
+ sizeof(val));
+
+out:
+ mutex_unlock(&idt82p33->reg_lock);
+ return err;
+}
+
static int idt82p33_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
{
struct idt82p33_channel *channel =
@@ -683,6 +749,9 @@ static int idt82p33_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
mutex_lock(&idt82p33->reg_lock);
err = _idt82p33_adjfine(channel, scaled_ppm);
+ if (err)
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
mutex_unlock(&idt82p33->reg_lock);
return err;
@@ -706,10 +775,15 @@ static int idt82p33_adjtime(struct ptp_clock_info *ptp, s64 delta_ns)
if (err) {
mutex_unlock(&idt82p33->reg_lock);
+ dev_err(&idt82p33->client->dev,
+ "Adjtime failed in %s with err %d!\n", __func__, err);
return err;
}
err = idt82p33_sync_tod(channel, true);
+ if (err)
+ dev_err(&idt82p33->client->dev,
+ "Sync_tod failed in %s with err %d!\n", __func__, err);
mutex_unlock(&idt82p33->reg_lock);
@@ -725,6 +799,9 @@ static int idt82p33_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
mutex_lock(&idt82p33->reg_lock);
err = _idt82p33_gettime(channel, ts);
+ if (err)
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
mutex_unlock(&idt82p33->reg_lock);
return err;
@@ -740,6 +817,9 @@ static int idt82p33_settime(struct ptp_clock_info *ptp,
mutex_lock(&idt82p33->reg_lock);
err = _idt82p33_settime(channel, ts);
+ if (err)
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
mutex_unlock(&idt82p33->reg_lock);
return err;
@@ -772,9 +852,6 @@ static int idt82p33_channel_init(struct idt82p33_channel *channel, int index)
return -EINVAL;
}
- INIT_DELAYED_WORK(&channel->sync_tod_work,
- idt82p33_sync_tod_work_handler);
- channel->sync_tod_on = false;
channel->current_freq_ppb = 0;
return 0;
@@ -784,11 +861,14 @@ static void idt82p33_caps_init(struct ptp_clock_info *caps)
{
caps->owner = THIS_MODULE;
caps->max_adj = 92000;
+ caps->n_per_out = 11;
+ caps->adjphase = idt82p33_adjwritephase;
caps->adjfine = idt82p33_adjfine;
caps->adjtime = idt82p33_adjtime;
caps->gettime64 = idt82p33_gettime;
caps->settime64 = idt82p33_settime;
caps->enable = idt82p33_enable;
+ caps->do_aux_work = idt82p33_sync_tod_work_handler;
}
static int idt82p33_enable_channel(struct idt82p33 *idt82p33, u32 index)
@@ -802,23 +882,18 @@ static int idt82p33_enable_channel(struct idt82p33 *idt82p33, u32 index)
channel = &idt82p33->channel[index];
err = idt82p33_channel_init(channel, index);
- if (err)
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Channel_init failed in %s with err %d!\n",
+ __func__, err);
return err;
+ }
channel->idt82p33 = idt82p33;
idt82p33_caps_init(&channel->caps);
snprintf(channel->caps.name, sizeof(channel->caps.name),
"IDT 82P33 PLL%u", index);
- channel->caps.n_per_out = hweight8(channel->output_mask);
-
- err = idt82p33_dpll_set_mode(channel, PLL_MODE_DCO);
- if (err)
- return err;
-
- err = idt82p33_enable_tod(channel);
- if (err)
- return err;
channel->ptp_clock = ptp_clock_register(&channel->caps, NULL);
@@ -831,6 +906,22 @@ static int idt82p33_enable_channel(struct idt82p33 *idt82p33, u32 index)
if (!channel->ptp_clock)
return -ENOTSUPP;
+ err = idt82p33_dpll_set_mode(channel, PLL_MODE_DCO);
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Dpll_set_mode failed in %s with err %d!\n",
+ __func__, err);
+ return err;
+ }
+
+ err = idt82p33_enable_tod(channel);
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Enable_tod failed in %s with err %d!\n",
+ __func__, err);
+ return err;
+ }
+
dev_info(&idt82p33->client->dev, "PLL%d registered as ptp%d\n",
index, channel->ptp_clock->index);
@@ -839,19 +930,22 @@ static int idt82p33_enable_channel(struct idt82p33 *idt82p33, u32 index)
static int idt82p33_load_firmware(struct idt82p33 *idt82p33)
{
+ char fname[128] = FW_FILENAME;
const struct firmware *fw;
struct idt82p33_fwrc *rec;
u8 loaddr, page, val;
int err;
s32 len;
- dev_dbg(&idt82p33->client->dev,
- "requesting firmware '%s'\n", FW_FILENAME);
+ dev_dbg(&idt82p33->client->dev, "requesting firmware '%s'\n", fname);
- err = request_firmware(&fw, FW_FILENAME, &idt82p33->client->dev);
+ err = request_firmware(&fw, fname, &idt82p33->client->dev);
- if (err)
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n", __func__, err);
return err;
+ }
dev_dbg(&idt82p33->client->dev, "firmware size %zu bytes\n", fw->size);
@@ -935,8 +1029,12 @@ static int idt82p33_probe(struct i2c_client *client,
for (i = 0; i < MAX_PHC_PLL; i++) {
if (idt82p33->pll_mask & (1 << i)) {
err = idt82p33_enable_channel(idt82p33, i);
- if (err)
+ if (err) {
+ dev_err(&idt82p33->client->dev,
+ "Failed in %s with err %d!\n",
+ __func__, err);
break;
+ }
}
}
} else {
diff --git a/drivers/ptp/ptp_idt82p33.h b/drivers/ptp/ptp_idt82p33.h
index 9d46966..3a0e001 100644
--- a/drivers/ptp/ptp_idt82p33.h
+++ b/drivers/ptp/ptp_idt82p33.h
@@ -56,6 +56,8 @@
#define PLL_MODE_SHIFT (0)
#define PLL_MODE_MASK (0x1F)
+#define PEROUT_ENABLE_OUTPUT_MASK (0xdeadbeef)
+
enum pll_mode {
PLL_MODE_MIN = 0,
PLL_MODE_AUTOMATIC = PLL_MODE_MIN,
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write
2020-11-04 16:01 [PATCH net-next 1/3] ptp: idt82p33: add adjphase support min.li.xe
@ 2020-11-04 16:01 ` min.li.xe
2020-11-04 16:43 ` Richard Cochran
2020-11-04 16:01 ` [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine min.li.xe
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: min.li.xe @ 2020-11-04 16:01 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Refactor idt82p33_xfer and use i2c_master_send for write operation.
Because some I2C controllers are only working with single-burst write
transaction.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_idt82p33.c | 52 +++++++++++++++++++++++++++++++++-------------
drivers/ptp/ptp_idt82p33.h | 1 +
2 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index 556cf6c..b1528a0 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -78,11 +78,10 @@ static void idt82p33_timespec_to_byte_array(struct timespec64 const *ts,
}
}
-static int idt82p33_xfer(struct idt82p33 *idt82p33,
- unsigned char regaddr,
- unsigned char *buf,
- unsigned int count,
- int write)
+static int idt82p33_xfer_read(struct idt82p33 *idt82p33,
+ unsigned char regaddr,
+ unsigned char *buf,
+ unsigned int count)
{
struct i2c_client *client = idt82p33->client;
struct i2c_msg msg[2];
@@ -94,7 +93,7 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
msg[0].buf = ®addr;
msg[1].addr = client->addr;
- msg[1].flags = write ? 0 : I2C_M_RD;
+ msg[1].flags = I2C_M_RD;
msg[1].len = count;
msg[1].buf = buf;
@@ -110,6 +109,31 @@ static int idt82p33_xfer(struct idt82p33 *idt82p33,
return 0;
}
+static int idt82p33_xfer_write(struct idt82p33 *idt82p33,
+ u8 regaddr,
+ u8 *buf,
+ u16 count)
+{
+ struct i2c_client *client = idt82p33->client;
+ /* we add 1 byte for device register */
+ u8 msg[IDT82P33_MAX_WRITE_COUNT + 1];
+ int err;
+
+ if (count > IDT82P33_MAX_WRITE_COUNT)
+ return -EINVAL;
+
+ msg[0] = regaddr;
+ memcpy(&msg[1], buf, count);
+
+ err = i2c_master_send(client, msg, count + 1);
+ if (err < 0) {
+ dev_err(&client->dev, "i2c_master_send returned %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
{
int err;
@@ -117,7 +141,7 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
if (idt82p33->page_offset == val)
return 0;
- err = _idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
+ err = idt82p33_xfer_write(idt82p33, PAGE_ADDR, &val, sizeof(val));
if (err)
dev_err(&idt82p33->client->dev,
"failed to set page offset %d\n", val);
@@ -130,20 +154,20 @@ static int idt82p33_page_offset(struct idt82p33 *idt82p33, unsigned char val)
static int idt82p33_rdwr(struct idt82p33 *idt82p33, unsigned int regaddr,
unsigned char *buf, unsigned int count, bool write)
{
+ u8 offset, page;
int err;
- u8 page;
- u8 offset;
page = _PAGE(regaddr);
- offset = _OFFSET(regaddr);
+ offset = _OFFSET(regaddr);
err = idt82p33_page_offset(idt82p33, page);
if (err)
- goto out;
+ return err;
- err = idt82p33_xfer(idt82p33, offset, buf, count, write);
-out:
- return err;
+ if (write)
+ return idt82p33_xfer_write(idt82p33, offset, buf, count);
+
+ return idt82p33_xfer_read(idt82p33, offset, buf, count);
}
static int idt82p33_read(struct idt82p33 *idt82p33, unsigned int regaddr,
diff --git a/drivers/ptp/ptp_idt82p33.h b/drivers/ptp/ptp_idt82p33.h
index 3a0e001..1c7a0f0 100644
--- a/drivers/ptp/ptp_idt82p33.h
+++ b/drivers/ptp/ptp_idt82p33.h
@@ -95,6 +95,7 @@ enum hw_tod_trig_sel {
#define MAX_MEASURMENT_COUNT (5)
#define SNAP_THRESHOLD_NS (150000)
#define SYNC_TOD_TIMEOUT_SEC (5)
+#define IDT82P33_MAX_WRITE_COUNT (512)
#define PLLMASK_ADDR_HI 0xFF
#define PLLMASK_ADDR_LO 0xA5
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
2020-11-04 16:01 [PATCH net-next 1/3] ptp: idt82p33: add adjphase support min.li.xe
2020-11-04 16:01 ` [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write min.li.xe
@ 2020-11-04 16:01 ` min.li.xe
2020-11-04 16:46 ` Richard Cochran
2020-11-04 16:42 ` [PATCH net-next 1/3] ptp: idt82p33: add adjphase support Richard Cochran
2020-11-04 21:56 ` Jakub Kicinski
3 siblings, 1 reply; 9+ messages in thread
From: min.li.xe @ 2020-11-04 16:01 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, linux-kernel, Min Li
From: Min Li <min.li.xe@renesas.com>
Use div_s64 so that the neg_adj is not needed.
Signed-off-by: Min Li <min.li.xe@renesas.com>
---
drivers/ptp/ptp_idt82p33.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index b1528a0..e970379d 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -320,7 +320,6 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
{
struct idt82p33 *idt82p33 = channel->idt82p33;
unsigned char buf[5] = {0};
- int neg_adj = 0;
int err, i;
s64 fcw;
@@ -340,16 +339,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
* FCW = -------------
* 168 * 2^4
*/
- if (scaled_ppm < 0) {
- neg_adj = 1;
- scaled_ppm = -scaled_ppm;
- }
fcw = scaled_ppm * 244140625ULL;
- fcw = div_u64(fcw, 2688);
-
- if (neg_adj)
- fcw = -fcw;
+ fcw = div_s64(fcw, 2688);
for (i = 0; i < 5; i++) {
buf[i] = fcw & 0xff;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] ptp: idt82p33: add adjphase support
2020-11-04 16:01 [PATCH net-next 1/3] ptp: idt82p33: add adjphase support min.li.xe
2020-11-04 16:01 ` [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write min.li.xe
2020-11-04 16:01 ` [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine min.li.xe
@ 2020-11-04 16:42 ` Richard Cochran
2020-11-04 21:56 ` Jakub Kicinski
3 siblings, 0 replies; 9+ messages in thread
From: Richard Cochran @ 2020-11-04 16:42 UTC (permalink / raw)
To: min.li.xe; +Cc: netdev, linux-kernel
On Wed, Nov 04, 2020 at 11:01:47AM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Add idt82p33_adjphase() to support PHC write phase mode.
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write
2020-11-04 16:01 ` [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write min.li.xe
@ 2020-11-04 16:43 ` Richard Cochran
0 siblings, 0 replies; 9+ messages in thread
From: Richard Cochran @ 2020-11-04 16:43 UTC (permalink / raw)
To: min.li.xe; +Cc: netdev, linux-kernel
On Wed, Nov 04, 2020 at 11:01:48AM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Refactor idt82p33_xfer and use i2c_master_send for write operation.
> Because some I2C controllers are only working with single-burst write
> transaction.
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
2020-11-04 16:01 ` [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine min.li.xe
@ 2020-11-04 16:46 ` Richard Cochran
2020-11-05 0:35 ` Vladimir Oltean
0 siblings, 1 reply; 9+ messages in thread
From: Richard Cochran @ 2020-11-04 16:46 UTC (permalink / raw)
To: min.li.xe; +Cc: netdev, linux-kernel
On Wed, Nov 04, 2020 at 11:01:49AM -0500, min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Use div_s64 so that the neg_adj is not needed.
Back in the day, I coded the neg_adj because there was some issue with
signed 64 bit division that I can't recall now. Either div_s64 didn't
exist or it was buggy on some archs... there was _some_ reason.
So unless you are sure that this works on all platforms, I would leave
it alone.
Thanks,
Richard
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---
> drivers/ptp/ptp_idt82p33.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index b1528a0..e970379d 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -320,7 +320,6 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
> {
> struct idt82p33 *idt82p33 = channel->idt82p33;
> unsigned char buf[5] = {0};
> - int neg_adj = 0;
> int err, i;
> s64 fcw;
>
> @@ -340,16 +339,9 @@ static int _idt82p33_adjfine(struct idt82p33_channel *channel, long scaled_ppm)
> * FCW = -------------
> * 168 * 2^4
> */
> - if (scaled_ppm < 0) {
> - neg_adj = 1;
> - scaled_ppm = -scaled_ppm;
> - }
>
> fcw = scaled_ppm * 244140625ULL;
> - fcw = div_u64(fcw, 2688);
> -
> - if (neg_adj)
> - fcw = -fcw;
> + fcw = div_s64(fcw, 2688);
>
> for (i = 0; i < 5; i++) {
> buf[i] = fcw & 0xff;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/3] ptp: idt82p33: add adjphase support
2020-11-04 16:01 [PATCH net-next 1/3] ptp: idt82p33: add adjphase support min.li.xe
` (2 preceding siblings ...)
2020-11-04 16:42 ` [PATCH net-next 1/3] ptp: idt82p33: add adjphase support Richard Cochran
@ 2020-11-04 21:56 ` Jakub Kicinski
3 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2020-11-04 21:56 UTC (permalink / raw)
To: min.li.xe; +Cc: richardcochran, netdev, linux-kernel
On Wed, 4 Nov 2020 11:01:47 -0500 min.li.xe@renesas.com wrote:
> From: Min Li <min.li.xe@renesas.com>
>
> Add idt82p33_adjphase() to support PHC write phase mode.
>
> Signed-off-by: Min Li <min.li.xe@renesas.com>
This appears to break the build. Each patch must build, otherwise we're
risking breaking builds when people bisect bugs with git bisect.
../drivers/ptp/ptp_idt82p33.c: In function ‘idt82p33_page_offset’:
../drivers/ptp/ptp_idt82p33.c:120:8: error: implicit declaration of function ‘_idt82p33_xfer’; did you mean ‘idt82p33_xfer’? [-Werror=implicit-function-declaration]
120 | err = _idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
| ^~~~~~~~~~~~~~
| idt82p33_xfer
cc1: some warnings being treated as errors
make[3]: *** [drivers/ptp/ptp_idt82p33.o] Error 1
make[2]: *** [drivers/ptp] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers] Error 2
make: *** [__sub-make] Error 2
../drivers/ptp/ptp_idt82p33.c: In function ‘idt82p33_page_offset’:
../drivers/ptp/ptp_idt82p33.c:120:8: error: implicit declaration of function ‘_idt82p33_xfer’; did you mean ‘idt82p33_xfer’? [-Werror=implicit-function-declaration]
120 | err = _idt82p33_xfer(idt82p33, PAGE_ADDR, &val, sizeof(val), 1);
| ^~~~~~~~~~~~~~
| idt82p33_xfer
cc1: some warnings being treated as errors
make[3]: *** [drivers/ptp/ptp_idt82p33.o] Error 1
make[2]: *** [drivers/ptp] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers] Error 2
make: *** [__sub-make] Error 2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
2020-11-04 16:46 ` Richard Cochran
@ 2020-11-05 0:35 ` Vladimir Oltean
2020-11-05 17:02 ` Richard Cochran
0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Oltean @ 2020-11-05 0:35 UTC (permalink / raw)
To: Richard Cochran; +Cc: min.li.xe, netdev, linux-kernel
On Wed, Nov 04, 2020 at 08:46:57AM -0800, Richard Cochran wrote:
> On Wed, Nov 04, 2020 at 11:01:49AM -0500, min.li.xe@renesas.com wrote:
> > From: Min Li <min.li.xe@renesas.com>
> >
> > Use div_s64 so that the neg_adj is not needed.
>
> Back in the day, I coded the neg_adj because there was some issue with
> signed 64 bit division that I can't recall now. Either div_s64 didn't
> exist or it was buggy on some archs... there was _some_ reason.
>
> So unless you are sure that this works on all platforms, I would leave
> it alone.
On the other hand and with all due respect, saying that it may have been
'buggy on some archs back in the day' and then not bringing any evidence
is a bit of a strange claim to make.
I am actively using div_s64 in drivers/net/dsa/sja1105/sja1105_ptp.c
successfully on arm and arm64.
We may keep the ptp_clock_info::adjfine procedure as is, and to be
copied by everyone, because we can't make sure that it works "on all
platforms" (aka "cargo cult"). Or we could waste a few hours from
somebody's time, until he figures out how to bisect the IDT 82P33 PTP
driver (a driver with 3 patches, and 3 more with Min's series) to find a
1-line change, and then we could find out what the problem you were
seeing was. I say waste that guy's time :)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine
2020-11-05 0:35 ` Vladimir Oltean
@ 2020-11-05 17:02 ` Richard Cochran
0 siblings, 0 replies; 9+ messages in thread
From: Richard Cochran @ 2020-11-05 17:02 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: min.li.xe, netdev, linux-kernel
On Thu, Nov 05, 2020 at 02:35:56AM +0200, Vladimir Oltean wrote:
> On the other hand and with all due respect, saying that it may have been
> 'buggy on some archs back in the day' and then not bringing any evidence
> is a bit of a strange claim to make.
You're right. I made the effort to look back into the days of v3.0,
and the only thing I could find is that the 32 bit implementation of
div_s64 does extra operations and invokes an additional function call.
But the difference in performance, if any, is probably not very large.
> I am actively using div_s64 in drivers/net/dsa/sja1105/sja1105_ptp.c
> successfully on arm and arm64.
Yeah, I see div_s64 has found its way into the ntp code, too, so it
must be fine.
Thanks,
Richard
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-05 17:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-04 16:01 [PATCH net-next 1/3] ptp: idt82p33: add adjphase support min.li.xe
2020-11-04 16:01 ` [PATCH net-next 2/3] ptp: idt82p33: use i2c_master_send for bus write min.li.xe
2020-11-04 16:43 ` Richard Cochran
2020-11-04 16:01 ` [PATCH net-next 3/3] ptp: idt82p33: optimize _idt82p33_adjfine min.li.xe
2020-11-04 16:46 ` Richard Cochran
2020-11-05 0:35 ` Vladimir Oltean
2020-11-05 17:02 ` Richard Cochran
2020-11-04 16:42 ` [PATCH net-next 1/3] ptp: idt82p33: add adjphase support Richard Cochran
2020-11-04 21:56 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).