| Message ID | b940ddf9-745f-4f2a-a29e-d6efe64de9a8@shenghaoyang.info (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net: dsa: mv88e6xxx: correct CC scale factor for 88E6393X | expand |
On Sun, Sep 15, 2024 at 07:57:27PM +0800, Shenghao Yang wrote: > Sending this as an RFC: no datasheet access - this > scaling factor may not be correct for all boards if this > 4ns vs 8ns discrepancy is down to physical design. > > If the counter is truly spec'd to always count at > 250MHz other chips in the same family may need > correction too. Hi Shenghao This sort of text should be placed below the --- marker so it is not part of the commit message which actually get merged. What we want above the --- is a description of the problem you see, and how your patch fixes this problem. It should include an explanation of what you think the real problem is. My understanding is that the clock can either be 250MHz or 125MHz like older generations. If an external clock is used, it should be 125MHz. The internally generated clock is however 250Mhz. There is a register MV88E6XXX_TAI_CLOCK_PERIOD which indicates the period of one clock tick. It probably defaults to 0x0FA0, or 4000 decimal which should be correct for the internal clock. But if an external clock is being used, it needs to be set to 0x1F40, or 8000 decimal. It would be good if you could read it out and see if it is correct by default. This should apply to the 6393X family, so 6191X 6193X 6361 6393X. It does not appear to apply to older devices. Andrew
On 16/9/24 03:14, Andrew Lunn wrote: > On Sun, Sep 15, 2024 at 07:57:27PM +0800, Shenghao Yang wrote: >> Sending this as an RFC: no datasheet access - this >> scaling factor may not be correct for all boards if this >> 4ns vs 8ns discrepancy is down to physical design. >> >> If the counter is truly spec'd to always count at >> 250MHz other chips in the same family may need >> correction too. > > This sort of text should be placed below the --- marker so it is not > part of the commit message which actually get merged. Hi Andrew, Gotcha - I'll move things around in the future. > There is a register MV88E6XXX_TAI_CLOCK_PERIOD which indicates the > period of one clock tick. It probably defaults to 0x0FA0, or 4000 > decimal which should be correct for the internal clock. But if an > external clock is being used, it needs to be set to 0x1F40, or 8000 > decimal. It would be good if you could read it out and see if it is > correct by default. Thanks! The register appears to contain the correct value on this device - 4000ps using the 250MHz internal clock. Would you happen to know if that register is valid for all the families currently supported? I'm preparing a few patches to read off that register in mv88e6xxx_ptp_setup() and choose a correct set of cycle counter coefficients to avoid introducing more device-specific handling. If that sounds reasonable I'll send them for net - would you also be okay with a Suggested-By? Shenghao
> Would you happen to know if that register is valid for all the > families currently supported? 6352 has it, 6320 also has it. So it seems to be common to all implementations. > If that sounds reasonable I'll send them for net - would you also be > okay with a Suggested-By? Yes, that is fine. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 5b4e2ce5470d..480fd93a336a 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -5617,7 +5617,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = { /* TODO: serdes stats */ .gpio_ops = &mv88e6352_gpio_ops, .avb_ops = &mv88e6390_avb_ops, - .ptp_ops = &mv88e6352_ptp_ops, + .ptp_ops = &mv88e6393x_ptp_ops, .phylink_get_caps = mv88e6393x_phylink_get_caps, .pcs_ops = &mv88e6393x_pcs_ops, }; diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c index 56391e09b325..02bfff368be2 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.c +++ b/drivers/net/dsa/mv88e6xxx/ptp.c @@ -30,6 +30,18 @@ #define MV88E6250_CC_MULT_NUM (1 << 7) #define MV88E6250_CC_MULT_DEM 3125ULL +/* Family MV88E6393X: + * Raw timestamps appear to be in units of 4-ns clock periods. + * + * clkadj = scaled_ppm * 4*2^28 / (10^6 * 2^16) + * simplifies to + * clkadj = scaled_ppm * 2^8 / 5^6 + */ +#define MV88E6393X_CC_SHIFT 28 +#define MV88E6393X_CC_MULT (4 << MV88E6393X_CC_SHIFT) +#define MV88E6393X_CC_MULT_NUM (1 << 8) +#define MV88E6393X_CC_MULT_DEM 15625ULL + /* Other families: * Raw timestamps are in units of 8-ns clock periods. * @@ -452,6 +464,33 @@ const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = { .cc_mult_dem = MV88E6XXX_CC_MULT_DEM, }; +const struct mv88e6xxx_ptp_ops mv88e6393x_ptp_ops = { + .clock_read = mv88e6352_ptp_clock_read, + .ptp_enable = mv88e6352_ptp_enable, + .ptp_verify = mv88e6352_ptp_verify, + .event_work = mv88e6352_tai_event_work, + .port_enable = mv88e6352_hwtstamp_port_enable, + .port_disable = mv88e6352_hwtstamp_port_disable, + .n_ext_ts = 1, + .arr0_sts_reg = MV88E6XXX_PORT_PTP_ARR0_STS, + .arr1_sts_reg = MV88E6XXX_PORT_PTP_ARR1_STS, + .dep_sts_reg = MV88E6XXX_PORT_PTP_DEP_STS, + .rx_filters = (1 << HWTSTAMP_FILTER_NONE) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) | + (1 << HWTSTAMP_FILTER_PTP_V2_EVENT) | + (1 << HWTSTAMP_FILTER_PTP_V2_SYNC) | + (1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ), + .cc_shift = MV88E6393X_CC_SHIFT, + .cc_mult = MV88E6393X_CC_MULT, + .cc_mult_num = MV88E6393X_CC_MULT_NUM, + .cc_mult_dem = MV88E6393X_CC_MULT_DEM, +}; + static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc) { struct mv88e6xxx_chip *chip = cc_to_chip(cc); @@ -462,10 +501,10 @@ static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc) return 0; } -/* With a 125MHz input clock, the 32-bit timestamp counter overflows in ~34.3 +/* With a 125MHz input clock, the 32-bit timestamp counter overflows in ~17.2 * seconds; this task forces periodic reads so that we don't miss any. */ -#define MV88E6XXX_TAI_OVERFLOW_PERIOD (HZ * 16) +#define MV88E6XXX_TAI_OVERFLOW_PERIOD (HZ * 8) static void mv88e6xxx_ptp_overflow_check(struct work_struct *work) { struct delayed_work *dw = to_delayed_work(work); diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h index 6c4d09adc93c..b236e11c6d78 100644 --- a/drivers/net/dsa/mv88e6xxx/ptp.h +++ b/drivers/net/dsa/mv88e6xxx/ptp.h @@ -152,6 +152,7 @@ extern const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops; extern const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops; extern const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops; extern const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops; +extern const struct mv88e6xxx_ptp_ops mv88e6393x_ptp_ops; #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */ @@ -173,6 +174,7 @@ static const struct mv88e6xxx_ptp_ops mv88e6165_ptp_ops = {}; static const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {}; static const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {}; static const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = {}; +static const struct mv88e6xxx_ptp_ops mv88e6393x_ptp_ops = {}; #endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */
Sending this as an RFC: no datasheet access - this scaling factor may not be correct for all boards if this 4ns vs 8ns discrepancy is down to physical design. If the counter is truly spec'd to always count at 250MHz other chips in the same family may need correction too. Tested on a MikroTik RB5009. Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family") Signed-off-by: Shenghao Yang <me@shenghaoyang.info> --- drivers/net/dsa/mv88e6xxx/chip.c | 2 +- drivers/net/dsa/mv88e6xxx/ptp.c | 43 ++++++++++++++++++++++++++++++-- drivers/net/dsa/mv88e6xxx/ptp.h | 2 ++ 3 files changed, 44 insertions(+), 3 deletions(-)