diff mbox series

[net,2/3] net: dsa: mv88e6xxx: read cycle counter period from hardware

Message ID 20240929101949.723658-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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: pabeni@redhat.com kuba@kernel.org edumazet@google.com richardcochran@gmail.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 12 this patch: 12
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Shenghao Yang Sept. 29, 2024, 10:19 a.m. UTC 
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 with whose cycle counter frequencies
depend on board design.

Hardware with inaccessible clock period registers or unsupported periods
will fall back to the fixed mapping.

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 |  6 ++--
 drivers/net/dsa/mv88e6xxx/ptp.c  | 48 ++++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 15 deletions(-)

Comments

Andrew Lunn Sept. 30, 2024, 3:31 p.m. UTC | #1 
> +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_warn(chip->dev, "failed to read cycle counter period");
> +		return chip->info->ops->ptp_ops->default_cc_coeffs;
> +	}
> +
> +	switch (period_ps) {
> +	case 8000:
> +		return &mv88e6xxx_cc_8ns_coeffs;
> +	case 10000:
> +		return &mv88e6xxx_cc_10ns_coeffs;
> +	default:
> +		dev_warn(chip->dev, "unexpected cycle counter period of %u ps",
> +			 period_ps);
> +		return chip->info->ops->ptp_ops->default_cc_coeffs;

This chip mv88e6xxx_cc_coeffs vs ptp_ops mv88e6xxx_cc_coeffs all seems
a bit messy.

The mv88e6xxx_tai_read() MV88E6XXX_TAI_CLOCK_PERIOD is not going to
fail, except for the hardware is dead. There is nothing you can do
about that, so return the error code and let the probe fail.

What you are more worried about is if the value you get back is not
what you expect. It is not 8000 or 10000. I would do a dev_err() and
return -ENODEV, and let the probe fail. The datasheets suggests this
should not happen. But if it does, we should get reports from users
that PTP is issuing an error and the switch is not probing. We can
then fix the problem.

You can then drop mv88e6xxx_cc_coeffs from ptp_ops.

    Andrew

---
pw-bot: cr
kernel test robot Oct. 1, 2024, 4:17 a.m. UTC | #2 
Hi Shenghao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Shenghao-Yang/net-dsa-mv88e6xxx-group-cycle-counter-coefficients/20240929-182245
base:   net/main
patch link:    https://lore.kernel.org/r/20240929101949.723658-3-me%40shenghaoyang.info
patch subject: [PATCH net 2/3] net: dsa: mv88e6xxx: read cycle counter period from hardware
config: sparc64-randconfig-r133-20240930 (https://download.01.org/0day-ci/archive/20241001/202410011105.XXvEbI6k-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20241001/202410011105.XXvEbI6k-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410011105.XXvEbI6k-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/dsa/mv88e6xxx/ptp.c:35:34: sparse: sparse: symbol 'mv88e6xxx_cc_10ns_coeffs' was not declared. Should it be static?
>> drivers/net/dsa/mv88e6xxx/ptp.c:49:34: sparse: sparse: symbol 'mv88e6xxx_cc_8ns_coeffs' was not declared. Should it be static?

vim +/mv88e6xxx_cc_10ns_coeffs +35 drivers/net/dsa/mv88e6xxx/ptp.c

    27	
    28	/* Family MV88E6250:
    29	 * Raw timestamps are in units of 10-ns clock periods.
    30	 *
    31	 * clkadj = scaled_ppm * 10*2^28 / (10^6 * 2^16)
    32	 * simplifies to
    33	 * clkadj = scaled_ppm * 2^7 / 5^5
    34	 */
  > 35	const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_10ns_coeffs = {
    36		.cc_shift = 28,
    37		.cc_mult = 10 << 28,
    38		.cc_mult_num = 1 << 7,
    39		.cc_mult_dem = 3125ULL,
    40	};
    41	
    42	/* Other families:
    43	 * Raw timestamps are in units of 8-ns clock periods.
    44	 *
    45	 * clkadj = scaled_ppm * 8*2^28 / (10^6 * 2^16)
    46	 * simplifies to
    47	 * clkadj = scaled_ppm * 2^9 / 5^6
    48	 */
  > 49	const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_8ns_coeffs = {
    50		.cc_shift = 28,
    51		.cc_mult = 8 << 28,
    52		.cc_mult_num = 1 << 9,
    53		.cc_mult_dem = 15625ULL
    54	};
    55
Shenghao Yang Oct. 5, 2024, 11:17 a.m. UTC | #3 
On 30/9/24 23:31, Andrew Lunn wrote:
> The mv88e6xxx_tai_read() MV88E6XXX_TAI_CLOCK_PERIOD is not going to
> fail, except for the hardware is dead. There is nothing you can do
> about that, so return the error code and let the probe fail.

> What you are more worried about is if the value you get back is not
> what you expect. It is not 8000 or 10000. I would do a dev_err() and
> return -ENODEV, and let the probe fail.

Hi Andrew,

Thanks! I'll sort those and the naming in v2.

Shenghao
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index bd66189a593f..8ff3f15e0d01 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,7 @@  struct mv88e6xxx_ptp_ops {
 	int arr1_sts_reg;
 	int dep_sts_reg;
 	u32 rx_filters;
-	const struct mv88e6xxx_cc_coeffs *cc_coeffs;
+	const struct mv88e6xxx_cc_coeffs *default_cc_coeffs;
 };
 
 struct mv88e6xxx_pcs_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index 89040c9af9f3..be1fcbf75440 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -32,7 +32,7 @@  struct mv88e6xxx_cc_coeffs {
  * simplifies to
  * clkadj = scaled_ppm * 2^7 / 5^5
  */
-const struct mv88e6xxx_cc_coeffs mv88e6250_cc_coeffs = {
+const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_10ns_coeffs = {
 	.cc_shift = 28,
 	.cc_mult = 10 << 28,
 	.cc_mult_num = 1 << 7,
@@ -46,7 +46,7 @@  const struct mv88e6xxx_cc_coeffs mv88e6250_cc_coeffs = {
  * simplifies to
  * clkadj = scaled_ppm * 2^9 / 5^6
  */
-const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_coeffs = {
+const struct mv88e6xxx_cc_coeffs mv88e6xxx_cc_8ns_coeffs = {
 	.cc_shift = 28,
 	.cc_mult = 8 << 28,
 	.cc_mult_num = 1 << 9,
@@ -94,6 +94,30 @@  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_warn(chip->dev, "failed to read cycle counter period");
+		return chip->info->ops->ptp_ops->default_cc_coeffs;
+	}
+
+	switch (period_ps) {
+	case 8000:
+		return &mv88e6xxx_cc_8ns_coeffs;
+	case 10000:
+		return &mv88e6xxx_cc_10ns_coeffs;
+	default:
+		dev_warn(chip->dev, "unexpected cycle counter period of %u ps",
+			 period_ps);
+		return chip->info->ops->ptp_ops->default_cc_coeffs;
+	}
+}
+
 static u64 mv88e6352_ptp_clock_read(const struct cyclecounter *cc)
 {
 	struct mv88e6xxx_chip *chip = cc_to_chip(cc);
@@ -215,7 +239,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;
@@ -225,10 +248,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);
 
@@ -375,7 +398,7 @@  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
+	.default_cc_coeffs = &mv88e6xxx_cc_8ns_coeffs
 };
 
 const struct mv88e6xxx_ptp_ops mv88e6250_ptp_ops = {
@@ -399,7 +422,7 @@  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,
+	.default_cc_coeffs = &mv88e6xxx_cc_10ns_coeffs,
 };
 
 const struct mv88e6xxx_ptp_ops mv88e6352_ptp_ops = {
@@ -423,7 +446,7 @@  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,
+	.default_cc_coeffs = &mv88e6xxx_cc_8ns_coeffs,
 };
 
 const struct mv88e6xxx_ptp_ops mv88e6390_ptp_ops = {
@@ -448,7 +471,7 @@  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,
+	.default_cc_coeffs = &mv88e6xxx_cc_8ns_coeffs,
 };
 
 static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
@@ -483,11 +506,12 @@  int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
 	int i;
 
 	/* Set up the cycle counter */
+	chip->cc_coeffs = mv88e6xxx_cc_coeff_get(chip);
 	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()));