Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
On Tue, Jul 30, 2019 at 11:46:51PM +0300, Vladimir Oltean wrote: > Technically it is not "not true". [Sigh] The statement was: The adjfine API clamps ppb between [-32,768,000, 32,768,000] The adjfine API does NOT clamp to that range. That statement is simply false. > And what is the reason for the neg_adj thing? Can you give an example > of when does the "normal way" of doing signed arithmetics not work? The detail from years ago escape me ATM, but I needed to use div_u64. Maybe div_s64 was broken. But that is not the point. Changing the adjfine() logic for this driver is out of scope for this series. If someone thinks the logic needs changing, then that must carefully explained and justified in the changelog of a patch implementing that _one_ change. Thanks, Richard
Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
Hi Hubert, Richard, On Tue, 30 Jul 2019 at 19:44, Richard Cochran wrote: > > On Tue, Jul 30, 2019 at 12:04:29PM +0200, Hubert Feurstein wrote: > > diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c > > b/drivers/net/dsa/mv88e6xxx/ptp.c > > index 768d256f7c9f..51cdf4712517 100644 > > --- a/drivers/net/dsa/mv88e6xxx/ptp.c > > +++ b/drivers/net/dsa/mv88e6xxx/ptp.c > > @@ -15,11 +15,38 @@ > > #include "hwtstamp.h" > > #include "ptp.h" > > > > -/* Raw timestamps are in units of 8-ns clock periods. */ > > -#define CC_SHIFT 28 > > -#define CC_MULT (8 << CC_SHIFT) > > -#define CC_MULT_NUM (1 << 9) > > -#define CC_MULT_DEM 15625ULL > > +/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and > > That is not true. > I was referring to this: https://github.com/richardcochran/linuxptp/blob/master/phc.c#L38 /* * On 32 bit platforms, the PHC driver's maximum adjustment (type * 'int' in units of ppb) can overflow the timex.freq field (type * 'long'). So in this case we clamp the maximum to the largest * possible adjustment that fits into a 32 bit long. */ #define BITS_PER_LONG (sizeof(long)*8) #define MAX_PPB_32 32767999 /* 2^31 - 1 / 65.536 */ Technically it is not "not true". [snip] On Tue, 30 Jul 2019 at 21:09, Richard Cochran wrote: > > On Tue, Jul 30, 2019 at 06:20:00PM +0200, Hubert Feurstein wrote: > > > Please don't re-write this logic. It is written like that for a reason. > > I used the sja1105_ptp.c as a reference. So it is also wrong there. > > I'll let that driver's author worry about that. > > Thanks, > Richard > And what is the reason for the neg_adj thing? Can you give an example of when does the "normal way" of doing signed arithmetics not work? Thanks, -Vladimir
Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
On Tue, Jul 30, 2019 at 06:20:00PM +0200, Hubert Feurstein wrote: > > Please don't re-write this logic. It is written like that for a reason. > I used the sja1105_ptp.c as a reference. So it is also wrong there. I'll let that driver's author worry about that. Thanks, Richard
Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
Hi Richard,
thank you for your comments.
Am Di., 30. Juli 2019 um 18:00 Uhr schrieb Richard Cochran
:
[...]
> > -/* Raw timestamps are in units of 8-ns clock periods. */
> > -#define CC_SHIFT 28
> > -#define CC_MULT (8 << CC_SHIFT)
> > -#define CC_MULT_NUM (1 << 9)
> > -#define CC_MULT_DEM 15625ULL
> > +/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and
>
> That is not true.
>
> > + * therefore scaled_ppm between [-2,147,483,648, 2,147,483,647].
> > + * Set the maximum supported ppb to a round value smaller than the maximum.
> > + *
> > + * Percentually speaking, this is a +/- 0.032x adjustment of the
> > + * free-running counter (0.968x to 1.032x).
> > + */
> > +#define MV88E6XXX_MAX_ADJ_PPB3200
>
> I had set an arbitrary limit of 1000 ppm. I can't really see any
> point in raising the limit.
>
> > +/* Family MV88E6250:
> > + * Raw timestamps are in units of 10-ns clock periods.
> > + *
> > + * clkadj = scaled_ppm * 10*2^28 / (10^6 * 2^16)
> > + * simplifies to
> > + * clkadj = scaled_ppm * 2^7 / 5^5
> > + */
> > +#define MV88E6250_CC_SHIFT 28
> > +#define MV88E6250_CC_MULT(10 << MV88E6250_CC_SHIFT)
> > +#define MV88E6250_CC_MULT_NUM(1 << 7)
> > +#define MV88E6250_CC_MULT_DEM3125ULL
> > +
> > +/* Other families:
> > + * Raw timestamps are in units of 8-ns clock periods.
> > + *
> > + * clkadj = scaled_ppm * 8*2^28 / (10^6 * 2^16)
> > + * simplifies to
> > + * clkadj = scaled_ppm * 2^9 / 5^6
> > + */
> > +#define MV88E6XXX_CC_SHIFT 28
> > +#define MV88E6XXX_CC_MULT(8 << MV88E6XXX_CC_SHIFT)
> > +#define MV88E6XXX_CC_MULT_NUM(1 << 9)
> > +#define MV88E6XXX_CC_MULT_DEM15625ULL
> >
> > #define TAI_EVENT_WORK_INTERVAL msecs_to_jiffies(100)
> >
> > @@ -179,24 +206,14 @@ static void mv88e6352_tai_event_work(struct
> > work_struct *ugly)
> > static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long
> > scaled_ppm)
> > {
> > struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
> > - int neg_adj = 0;
> > - u32 diff, mult;
> > - u64 adj;
> > + s64 adj;
> >
> > - if (scaled_ppm < 0) {
> > - neg_adj = 1;
> > - scaled_ppm = -scaled_ppm;
> > - }
>
> Please don't re-write this logic. It is written like that for a reason.
I used the sja1105_ptp.c as a reference. So it is also wrong there.
Hubert
Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
On Tue, Jul 30, 2019 at 12:04:29PM +0200, Hubert Feurstein wrote:
> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
> index 768d256f7c9f..51cdf4712517 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.c
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.c
> @@ -15,11 +15,38 @@
> #include "hwtstamp.h"
> #include "ptp.h"
>
> -/* Raw timestamps are in units of 8-ns clock periods. */
> -#define CC_SHIFT 28
> -#define CC_MULT (8 << CC_SHIFT)
> -#define CC_MULT_NUM (1 << 9)
> -#define CC_MULT_DEM 15625ULL
> +/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and
That is not true.
> + * therefore scaled_ppm between [-2,147,483,648, 2,147,483,647].
> + * Set the maximum supported ppb to a round value smaller than the maximum.
> + *
> + * Percentually speaking, this is a +/- 0.032x adjustment of the
> + * free-running counter (0.968x to 1.032x).
> + */
> +#define MV88E6XXX_MAX_ADJ_PPB3200
I had set an arbitrary limit of 1000 ppm. I can't really see any
point in raising the limit.
> +/* Family MV88E6250:
> + * Raw timestamps are in units of 10-ns clock periods.
> + *
> + * clkadj = scaled_ppm * 10*2^28 / (10^6 * 2^16)
> + * simplifies to
> + * clkadj = scaled_ppm * 2^7 / 5^5
> + */
> +#define MV88E6250_CC_SHIFT 28
> +#define MV88E6250_CC_MULT(10 << MV88E6250_CC_SHIFT)
> +#define MV88E6250_CC_MULT_NUM(1 << 7)
> +#define MV88E6250_CC_MULT_DEM3125ULL
> +
> +/* Other families:
> + * Raw timestamps are in units of 8-ns clock periods.
> + *
> + * clkadj = scaled_ppm * 8*2^28 / (10^6 * 2^16)
> + * simplifies to
> + * clkadj = scaled_ppm * 2^9 / 5^6
> + */
> +#define MV88E6XXX_CC_SHIFT 28
> +#define MV88E6XXX_CC_MULT(8 << MV88E6XXX_CC_SHIFT)
> +#define MV88E6XXX_CC_MULT_NUM(1 << 9)
> +#define MV88E6XXX_CC_MULT_DEM15625ULL
>
> #define TAI_EVENT_WORK_INTERVAL msecs_to_jiffies(100)
>
> @@ -179,24 +206,14 @@ static void mv88e6352_tai_event_work(struct work_struct
> *ugly)
> static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> {
> struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
> - int neg_adj = 0;
> - u32 diff, mult;
> - u64 adj;
> + s64 adj;
>
> - if (scaled_ppm < 0) {
> - neg_adj = 1;
> - scaled_ppm = -scaled_ppm;
> - }
Please don't re-write this logic. It is written like that for a reason.
> - mult = CC_MULT;
> - adj = CC_MULT_NUM;
> - adj *= scaled_ppm;
> - diff = div_u64(adj, CC_MULT_DEM);
Just substitute CC_MULT* with your new chip->ptp_cc_mult*
and leave the rest alone.
Thanks,
Richard
Re: [PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h
> b/drivers/net/dsa/mv88e6xxx/chip.h
> index 720cace3db4e..64872251e479 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -273,6 +273,10 @@ struct mv88e6xxx_chip {
> u16 trig_config;
> u16 evcap_config;
> u16 enable_count;
> + u32 ptp_cc_shift;
> + u32 ptp_cc_mult;
> + u32 ptp_cc_mult_num;
> + u32 ptp_cc_mult_dem;
Hi Hubert
Please add these to mv88e6xxx_ptp_ops. You can create a new one of
6250 which has the different values.
>
> /* Per-port timestamping resources. */
> struct mv88e6xxx_port_hwtstamp port_hwtstamp[DSA_MAX_PORTS];
> diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
> index 768d256f7c9f..51cdf4712517 100644
> --- a/drivers/net/dsa/mv88e6xxx/ptp.c
> +++ b/drivers/net/dsa/mv88e6xxx/ptp.c
> @@ -15,11 +15,38 @@
> #include "hwtstamp.h"
> #include "ptp.h"
>
> -/* Raw timestamps are in units of 8-ns clock periods. */
> -#define CC_SHIFT 28
> -#define CC_MULT (8 << CC_SHIFT)
> -#define CC_MULT_NUM (1 << 9)
> -#define CC_MULT_DEM 15625ULL
> +/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and
> + * therefore scaled_ppm between [-2,147,483,648, 2,147,483,647].
> + * Set the maximum supported ppb to a round value smaller than the maximum.
> + *
> + * Percentually speaking, this is a +/- 0.032x adjustment of the
> + * free-running counter (0.968x to 1.032x).
> + */
> +#define MV88E6XXX_MAX_ADJ_PPB3200
> +
> +/* Family MV88E6250:
> + * Raw timestamps are in units of 10-ns clock periods.
> + *
> + * clkadj = scaled_ppm * 10*2^28 / (10^6 * 2^16)
> + * simplifies to
> + * clkadj = scaled_ppm * 2^7 / 5^5
> + */
> +#define MV88E6250_CC_SHIFT 28
> +#define MV88E6250_CC_MULT(10 << MV88E6250_CC_SHIFT)
> +#define MV88E6250_CC_MULT_NUM(1 << 7)
> +#define MV88E6250_CC_MULT_DEM3125ULL
> +
> +/* Other families:
> + * Raw timestamps are in units of 8-ns clock periods.
> + *
> + * clkadj = scaled_ppm * 8*2^28 / (10^6 * 2^16)
> + * simplifies to
> + * clkadj = scaled_ppm * 2^9 / 5^6
> + */
> +#define MV88E6XXX_CC_SHIFT 28
> +#define MV88E6XXX_CC_MULT(8 << MV88E6XXX_CC_SHIFT)
> +#define MV88E6XXX_CC_MULT_NUM(1 << 9)
> +#define MV88E6XXX_CC_MULT_DEM15625ULL
Nice comments :-)
Andrew
[PATCH 4/4] net: dsa: mv88e6xxx: add PTP support for MV88E6250 family
This adds PTP support for the MV88E6250 family.
Signed-off-by: Hubert Feurstein
---
drivers/net/dsa/mv88e6xxx/chip.c | 4 ++
drivers/net/dsa/mv88e6xxx/chip.h | 4 ++
drivers/net/dsa/mv88e6xxx/ptp.c | 73 ++--
3 files changed, 59 insertions(+), 22 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c2fb4ea66434..58e298cc90e0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3514,6 +3514,8 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
.reset = mv88e6250_g1_reset,
.vtu_getnext = mv88e6250_g1_vtu_getnext,
.vtu_loadpurge = mv88e6250_g1_vtu_loadpurge,
+ .avb_ops = _avb_ops,
+ .ptp_ops = _ptp_ops,
.phylink_validate = mv88e6065_phylink_validate,
};
@@ -4333,6 +4335,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.atu_move_port_mask = 0xf,
.dual_chip = true,
.tag_protocol = DSA_TAG_PROTO_DSA,
+ .ptp_support = true,
.ops = _ops,
},
@@ -4354,6 +4357,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.atu_move_port_mask = 0xf,
.dual_chip = true,
.tag_protocol = DSA_TAG_PROTO_DSA,
+ .ptp_support = true,
.ops = _ops,
},
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 720cace3db4e..64872251e479 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -273,6 +273,10 @@ struct mv88e6xxx_chip {
u16 trig_config;
u16 evcap_config;
u16 enable_count;
+ u32 ptp_cc_shift;
+ u32 ptp_cc_mult;
+ u32 ptp_cc_mult_num;
+ u32 ptp_cc_mult_dem;
/* Per-port timestamping resources. */
struct mv88e6xxx_port_hwtstamp port_hwtstamp[DSA_MAX_PORTS];
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index 768d256f7c9f..51cdf4712517 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -15,11 +15,38 @@
#include "hwtstamp.h"
#include "ptp.h"
-/* Raw timestamps are in units of 8-ns clock periods. */
-#define CC_SHIFT 28
-#define CC_MULT(8 << CC_SHIFT)
-#define CC_MULT_NUM(1 << 9)
-#define CC_MULT_DEM15625ULL
+/* The adjfine API clamps ppb between [-32,768,000, 32,768,000], and
+ * therefore scaled_ppm between [-2,147,483,648, 2,147,483,647].
+ * Set the maximum supported ppb to a round value smaller than the maximum.
+ *
+ * Percentually speaking, this is a +/- 0.032x adjustment of the
+ * free-running counter (0.968x to 1.032x).
+ */
+#define MV88E6XXX_MAX_ADJ_PPB 3200
+
+/* Family MV88E6250:
+ * Raw timestamps are in units of 10-ns clock periods.
+ *
+ * clkadj = scaled_ppm * 10*2^28 / (10^6 * 2^16)
+ * simplifies to
+ * clkadj = scaled_ppm * 2^7 / 5^5
+ */
+#define MV88E6250_CC_SHIFT 28
+#define MV88E6250_CC_MULT (10 << MV88E6250_CC_SHIFT)
+#define MV88E6250_CC_MULT_NUM (1 << 7)
+#define MV88E6250_CC_MULT_DEM 3125ULL
+
+/* Other families:
+ * Raw timestamps are in units of 8-ns clock periods.
+ *
+ * clkadj = scaled_ppm * 8*2^28 / (10^6 * 2^16)
+ * simplifies to
+ * clkadj = scaled_ppm * 2^9 / 5^6
+ */
+#define MV88E6XXX_CC_SHIFT 28
+#define MV88E6XXX_CC_MULT (8 << MV88E6XXX_CC_SHIFT)
+#define MV88E6XXX_CC_MULT_NUM (1 << 9)
+#define MV88E6XXX_CC_MULT_DEM 15625ULL
#define TAI_EVENT_WORK_INTERVAL msecs_to_jiffies(100)
@@ -179,24 +206,14 @@ static void mv88e6352_tai_event_work(struct work_struct
*ugly)
static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
{
struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
- int neg_adj = 0;
- u32 diff, mult;
- u64 adj;
+ s64 adj;
- if (scaled_ppm < 0) {
- neg_adj = 1;
- scaled_ppm = -scaled_ppm;
- }
- mult = CC_MULT;
- adj = CC_MULT_NUM;
- adj *= scaled_ppm;
- diff = div_u64(adj, CC_MULT_DEM);
+ adj = (s64)scaled_ppm * chip->ptp_cc_mult_num;
+ adj = div_s64(adj, chip->ptp_cc_mult_dem);
mv88e6xxx_reg_lock(chip);
-
timecounter_read(>tstamp_tc);
- chip->tstamp_cc.mult = neg_adj ? mult - diff : mult + diff;
-
+ chip->tstamp_cc.mult = chip->ptp_cc_mult + adj;
mv88e6xxx_reg_unlock(chip);
return 0;
@@ -380,12 +397,24 @@ int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops;
int i;
+ if (chip->info->family == MV88E6XXX_FAMILY_6250) {
+ chip->ptp_cc_shift = MV88E6250_CC_SHIFT;
+ chip->ptp_cc_mult = MV88E6250_CC_MULT;
+ chip->ptp_cc_mult_num = MV88E6250_CC_MULT_NUM;
+ chip->ptp_cc_mult_dem = MV88E6250_CC_MULT_DEM;
+ } else {
+
