| Message ID | 1604505709-5483-3-git-send-email-min.li.xe@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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 >
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 :)
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
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;