| Message ID | 20241006145951.719162-3-me@shenghaoyang.info (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: fix MV88E6393X PHC frequency on internal clock | expand |
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > index bd66189a593f..a54682240839 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.h > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -206,6 +206,7 @@ struct mv88e6xxx_gpio_ops; > struct mv88e6xxx_avb_ops; > struct mv88e6xxx_ptp_ops; > struct mv88e6xxx_pcs_ops; > +struct mv88e6xxx_cc_coeffs; > > struct mv88e6xxx_irq { > u16 masked; > @@ -408,6 +409,7 @@ struct mv88e6xxx_chip { > struct cyclecounter tstamp_cc; > struct timecounter tstamp_tc; > struct delayed_work overflow_work; > + const struct mv88e6xxx_cc_coeffs *cc_coeffs; > > struct ptp_clock *ptp_clock; > struct ptp_clock_info ptp_clock_info; > @@ -714,8 +716,6 @@ struct mv88e6xxx_avb_ops { > int (*tai_write)(struct mv88e6xxx_chip *chip, int addr, u16 data); > }; > > -struct mv88e6xxx_cc_coeffs; > - It is better to put it in the correct place with the first patch, rather than move it in the second patch. > memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc)); > chip->tstamp_cc.read = mv88e6xxx_ptp_clock_read; > chip->tstamp_cc.mask = CYCLECOUNTER_MASK(32); > - chip->tstamp_cc.mult = ptp_ops->cc_coeffs->cc_mult; > - chip->tstamp_cc.shift = ptp_ops->cc_coeffs->cc_shift; > + chip->tstamp_cc.mult = chip->cc_coeffs->cc_mult; > + chip->tstamp_cc.shift = chip->cc_coeffs->cc_shift; Once these patches are merged, it would be nice to remove chip->tstamp_cc.mult and chip->tstamp_cc.shift and use chip->cc_coeffs->cc_mult and chip->cc_coeffs->cc_shift. We don't need the same values in two places. Andrew
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h >> struct mv88e6xxx_ptp_ops; >> struct mv88e6xxx_pcs_ops; >> +struct mv88e6xxx_cc_coeffs; >> >> -struct mv88e6xxx_cc_coeffs; >> - > > It is better to put it in the correct place with the first patch, > rather than move it in the second patch. Hi Andrew, Thanks! Happy to spin a v3 if preferred. >> memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc)); >> chip->tstamp_cc.read = mv88e6xxx_ptp_clock_read; >> chip->tstamp_cc.mask = CYCLECOUNTER_MASK(32); >> - chip->tstamp_cc.mult = ptp_ops->cc_coeffs->cc_mult; >> - chip->tstamp_cc.shift = ptp_ops->cc_coeffs->cc_shift; >> + chip->tstamp_cc.mult = chip->cc_coeffs->cc_mult; >> + chip->tstamp_cc.shift = chip->cc_coeffs->cc_shift; > > Once these patches are merged, it would be nice to remove > chip->tstamp_cc.mult and chip->tstamp_cc.shift and use > chip->cc_coeffs->cc_mult and chip->cc_coeffs->cc_shift. We don't need > the same values in two places. I've looked around a bit and this doesn't seem possible - the common timecounter code in time/timecounter.c and linux/timecounter.h has a dependency on the cc_mult and cc_shift fields within the cyclecounter tstamp_cc. We can't remove that dependency without specializing it for mv88e6xxx. Shenghao
On Sun, Oct 13, 2024 at 01:26:53PM +0800, Shenghao Yang wrote: > > >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > >> struct mv88e6xxx_ptp_ops; > >> struct mv88e6xxx_pcs_ops; > >> +struct mv88e6xxx_cc_coeffs; > >> > >> -struct mv88e6xxx_cc_coeffs; > >> - > > > > It is better to put it in the correct place with the first patch, > > rather than move it in the second patch. > > Hi Andrew, > > Thanks! Happy to spin a v3 if preferred. > > >> memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc)); > >> chip->tstamp_cc.read = mv88e6xxx_ptp_clock_read; > >> chip->tstamp_cc.mask = CYCLECOUNTER_MASK(32); > >> - chip->tstamp_cc.mult = ptp_ops->cc_coeffs->cc_mult; > >> - chip->tstamp_cc.shift = ptp_ops->cc_coeffs->cc_shift; > >> + chip->tstamp_cc.mult = chip->cc_coeffs->cc_mult; > >> + chip->tstamp_cc.shift = chip->cc_coeffs->cc_shift; > > > > Once these patches are merged, it would be nice to remove > > chip->tstamp_cc.mult and chip->tstamp_cc.shift and use > > chip->cc_coeffs->cc_mult and chip->cc_coeffs->cc_shift. We don't need > > the same values in two places. > > I've looked around a bit and this doesn't seem possible - the common > timecounter code in time/timecounter.c and linux/timecounter.h has a > dependency on the cc_mult and cc_shift fields within the cyclecounter > tstamp_cc. Ah, sorry. I did not see that tstamp_cc was a timecounter structure. Maybe have 3 const struct cyclecounter similar to you having 3 const mv88e6xxx_cc_coeffs. Assign the appropriate one to chip. mult and shift can then be dropped from mv88ex6xxx_cc_coeffs? Andrew
On 14/10/24 01:01, Andrew Lunn wrote: >> I've looked around a bit and this doesn't seem possible - the common >> timecounter code in time/timecounter.c and linux/timecounter.h has a >> dependency on the cc_mult and cc_shift fields within the cyclecounter >> tstamp_cc. > > Ah, sorry. I did not see that tstamp_cc was a timecounter structure. > > Maybe have 3 const struct cyclecounter similar to you having 3 const > mv88e6xxx_cc_coeffs. Assign the appropriate one to chip. mult and > shift can then be dropped from mv88ex6xxx_cc_coeffs? > > Andrew Hi Andrew, That might be a bit hairy too - the cyclecounters need way to reference the chip so they can access the hardware counter in tstamp_cc->read. That's currently done via a container_of() on tstamp_cc itself in mv88e6xxx_ptp_clock_read. With only that single pointer available sharing doesn't seem too possible :/. Thanks, Shenghao
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index bd66189a593f..a54682240839 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -206,6 +206,7 @@ struct mv88e6xxx_gpio_ops; struct mv88e6xxx_avb_ops; struct mv88e6xxx_ptp_ops; struct mv88e6xxx_pcs_ops; +struct mv88e6xxx_cc_coeffs; struct mv88e6xxx_irq { u16 masked; @@ -408,6 +409,7 @@ struct mv88e6xxx_chip { struct cyclecounter tstamp_cc; struct timecounter tstamp_tc; struct delayed_work overflow_work; + const struct mv88e6xxx_cc_coeffs *cc_coeffs; struct ptp_clock *ptp_clock; struct ptp_clock_info ptp_clock_info; @@ -714,8 +716,6 @@ struct mv88e6xxx_avb_ops { int (*tai_write)(struct mv88e6xxx_chip *chip, int addr, u16 data); }; -struct mv88e6xxx_cc_coeffs; - struct mv88e6xxx_ptp_ops { u64 (*clock_read)(const struct cyclecounter *cc); int (*ptp_enable)(struct ptp_clock_info *ptp, @@ -733,7 +733,6 @@ struct mv88e6xxx_ptp_ops { int arr1_sts_reg; int dep_sts_reg; u32 rx_filters; - const struct mv88e6xxx_cc_coeffs *cc_coeffs; }; struct mv88e6xxx_pcs_ops { diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index 641af44e00af..a409b8661fad 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.c +++ b/drivers/net/dsa/mv88e6xxx/ptp.c @@ -32,10 +32,10 @@ struct mv88e6xxx_cc_coeffs { * simplifies to * clkadj = scaled_ppm * 2^7 / 5^5 */ -#define MV88E6250_CC_SHIFT 28 -static const struct mv88e6xxx_cc_coeffs mv88e6250_cc_coeffs = { - .cc_shift = MV88E6250_CC_SHIFT, - .cc_mult = 10 << MV88E6250_CC_SHIFT, +#define MV88E6XXX_CC_10NS_SHIFT 28 +static const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_10ns_coeffs = { + .cc_shift = MV88E6XXX_CC_10NS_SHIFT, + .cc_mult = 10 << MV88E6XXX_CC_10NS_SHIFT, .cc_mult_num = 1 << 7, .cc_mult_dem = 3125ULL, }; @@ -47,10 +47,10 @@ static const struct mv88e6xxx_cc_coeffs mv88e6250_cc_coeffs = { * simplifies to * clkadj = scaled_ppm * 2^9 / 5^6 */ -#define MV88E6XXX_CC_SHIFT 28 -static const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_coeffs = { - .cc_shift = MV88E6XXX_CC_SHIFT, - .cc_mult = 8 << MV88E6XXX_CC_SHIFT, +#define MV88E6XXX_CC_8NS_SHIFT 28 +static const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_8ns_coeffs = { + .cc_shift = MV88E6XXX_CC_8NS_SHIFT, + .cc_mult = 8 << MV88E6XXX_CC_8NS_SHIFT, .cc_mult_num = 1 << 9, .cc_mult_dem = 15625ULL }; @@ -96,6 +96,31 @@ static int mv88e6352_set_gpio_func(struct mv88e6xxx_chip *chip, int pin, return chip->info->ops->gpio_ops->set_pctl(chip, pin, func); } +static const struct mv88e6xxx_cc_coeffs * +mv88e6xxx_cc_coeff_get(struct mv88e6xxx_chip *chip) +{ + u16 period_ps; + int err; + + err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_CLOCK_PERIOD, &period_ps, 1); + if (err) { + dev_err(chip->dev, "failed to read cycle counter period: %d\n", + err); + return ERR_PTR(err); + } + + switch (period_ps) { + case 8000: + return &mv88e6xxx_cc_8ns_coeffs; + case 10000: + return &mv88e6xxx_cc_10ns_coeffs; + default: + dev_err(chip->dev, "unexpected cycle counter period of %u ps\n", + period_ps); + return ERR_PTR(-ENODEV); + } +} + static u64 mv88e6352_ptp_clock_read(const struct cyclecounter *cc) { struct mv88e6xxx_chip *chip = cc_to_chip(cc); @@ -217,7 +242,6 @@ 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); - const struct mv88e6xxx_ptp_ops *ptp_ops = chip->info->ops->ptp_ops; int neg_adj = 0; u32 diff, mult; u64 adj; @@ -227,10 +251,10 @@ static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) scaled_ppm = -scaled_ppm; } - mult = ptp_ops->cc_coeffs->cc_mult; - adj = ptp_ops->cc_coeffs->cc_mult_num; + mult = chip->cc_coeffs->cc_mult; + adj = chip->cc_coeffs->cc_mult_num; adj *= scaled_ppm; - diff = div_u64(adj, ptp_ops->cc_coeffs->cc_mult_dem); + diff = div_u64(adj, chip->cc_coeffs->cc_mult_dem); mv88e6xxx_reg_lock(chip); @@ -377,7 +401,6 @@ const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = { (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), - .cc_coeffs = &mv88e6xxx_cc_coeffs }; const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = { @@ -401,7 +424,6 @@ const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = { (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), - .cc_coeffs = &mv88e6250_cc_coeffs, }; const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { @@ -425,7 +447,6 @@ const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = { (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), - .cc_coeffs = &mv88e6xxx_cc_coeffs, }; const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = { @@ -450,7 +471,6 @@ const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = { (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), - .cc_coeffs = &mv88e6xxx_cc_coeffs, }; static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc) @@ -485,11 +505,15 @@ int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip) int i; /* Set up the cycle counter */ + chip->cc_coeffs = mv88e6xxx_cc_coeff_get(chip); + if (IS_ERR(chip->cc_coeffs)) + return PTR_ERR(chip->cc_coeffs); + memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc)); chip->tstamp_cc.read = mv88e6xxx_ptp_clock_read; chip->tstamp_cc.mask = CYCLECOUNTER_MASK(32); - chip->tstamp_cc.mult = ptp_ops->cc_coeffs->cc_mult; - chip->tstamp_cc.shift = ptp_ops->cc_coeffs->cc_shift; + chip->tstamp_cc.mult = chip->cc_coeffs->cc_mult; + chip->tstamp_cc.shift = chip->cc_coeffs->cc_shift; timecounter_init(&chip->tstamp_tc, &chip->tstamp_cc, ktime_to_ns(ktime_get_real()));
Instead of relying on a fixed mapping of hardware family to cycle counter frequency, pull this information from the MV88E6XXX_TAI_CLOCK_PERIOD register. This lets us support switches whose cycle counter frequencies depend on board design. Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family") Suggested-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Shenghao Yang <me@shenghaoyang.info> --- drivers/net/dsa/mv88e6xxx/chip.h | 5 ++- drivers/net/dsa/mv88e6xxx/ptp.c | 60 ++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 21 deletions(-)