[PATCH net-next 00/10] net: dsa: microchip: add periodic output support for the KSZ8463

[PATCH net-next 00/10] net: dsa: microchip: add periodic output support for the KSZ8463

[Bastien Curutchet (Schneider Electric)]

This series aims to add periodic output support for the KSZ8463. The
way the KSZ8463 handles periodic output differs from that of the other
KSZ switches. It therefore needs its own set of perout callbacks
(configure, start, reset, enable, restart).

Patches 1 to 3 prepare the driver to accept different kinds of periodic
output settings.

Patches 4 to 9 extract a set of functions from the existing periodic
output support that can be reused by the KSZ8463, avoiding code
duplication as much as possible.

Patch 10 adds periodic output support for the KSZ8463.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
Bastien Curutchet (Schneider Electric) (10):
      net: dsa: microchip: add the number of pins to chip infos
      net: dsa: microchip: add the number of periodic signals to chip infos
      net: dsa: microchip: use dynamic mask to check pulse width validity
      net: dsa: microchip: extract PTP callbacks configuration from PTP registration
      net: dsa: microchip: extract ptp_get_pin
      net: dsa: microchip: extract compute_width
      net: dsa: microchip: extract prepare reset
      net: dsa: microchip: extract time update
      net: dsa: microchip: extract time adjustment
      net: dsa: microchip: add periodic output support for the KSZ8463

 drivers/net/dsa/microchip/ksz8.c         |   2 +
 drivers/net/dsa/microchip/ksz9477.c      |   1 +
 drivers/net/dsa/microchip/ksz_common.c   |   4 +
 drivers/net/dsa/microchip/ksz_common.h   |   2 +
 drivers/net/dsa/microchip/ksz_ptp.c      | 473 ++++++++++++++++++++++++++-----
 drivers/net/dsa/microchip/ksz_ptp.h      |   5 +-
 drivers/net/dsa/microchip/ksz_ptp_reg.h  |  16 ++
 drivers/net/dsa/microchip/lan937x_main.c |   1 +
 8 files changed, 433 insertions(+), 71 deletions(-)
---
base-commit: 1bb784eb6e38fd73143f021608e4ef3095d0c0d7
change-id: 20260825-ksz-perout-af9813e12158

Best regards,
-- 
Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

[PATCH net-next 01/10] net: dsa: microchip: add the number of pins to chip infos

[Bastien Curutchet (Schneider Electric)]

The number of periodic output pins available is hardcoded to 2 while
KSZ8463 has 12 pins that can be used as periodic outputs.

Add an n_pin attribute to the struct ksz_chip_data to make this setting
configurable.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 1 +
 drivers/net/dsa/microchip/ksz_common.h | 1 +
 drivers/net/dsa/microchip/ksz_ptp.c    | 4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 1a9d6f83a023..55d1e3e3c529 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1194,6 +1194,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.ptp_capable = true,
 		.wr_table = &ksz8563_register_set,
 		.rd_table = &ksz8563_register_set,
+		.n_pins = 2,
 	},
 
 	[KSZ8795] = {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cbe98494578c..c678fc6597dc 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -139,6 +139,7 @@ struct ksz_chip_data {
 	u8 sgmii_port;
 	const struct regmap_access_table *wr_table;
 	const struct regmap_access_table *rd_table;
+	const u8 n_pins;
 };
 
 struct ksz_irq {
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 39cc70d65900..c92936bf78d3 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1050,14 +1050,14 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
 	ptp_data->caps.enable		= ksz_ptp_enable;
 	ptp_data->caps.verify		= ksz_ptp_verify_pin;
-	ptp_data->caps.n_pins		= KSZ_PTP_N_GPIO;
+	ptp_data->caps.n_pins		= dev->info->n_pins;
 	ptp_data->caps.n_per_out	= 3;
 
 	ret = ksz_ptp_start_clock(dev);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < KSZ_PTP_N_GPIO; i++) {
+	for (i = 0; i < dev->info->n_pins; i++) {
 		struct ptp_pin_desc *ptp_pin = &ptp_data->pin_config[i];
 
 		snprintf(ptp_pin->name,

-- 
2.55.0

Re: [net-next,01/10] net: dsa: microchip: add the number of pins to chip infos

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete conversion of the PTP pin count to per-chip data
  silently disables PTP pin/periodic-output support on every PTP-capable…

--

net: dsa: microchip: add the number of pins to chip infos

This adds an n_pins field to struct ksz_chip_data and makes
ksz_ptp_clock_register() take the PTP pin count from the per-chip info
instead of the KSZ_PTP_N_GPIO constant.  Only the KSZ8563 entry in
ksz_switch_chips[] is given an .n_pins value.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 1a9d6f83a0232..55d1e3e3c5299 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1194,6 +1194,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.ptp_capable = true,
>  		.wr_table = &ksz8563_register_set,
>  		.rd_table = &ksz8563_register_set,
> +		.n_pins = 2,
>  	},
>  
>  	[KSZ8795] = {

[Severity: High]
Is KSZ8563 the only entry that needs .n_pins?  Grepping ksz_switch_chips[]
at this commit, KSZ8563 is the only entry carrying the new field, while
these other entries also have .ptp_capable = true and no .n_pins:

  KSZ9477, KSZ9563, KSZ8567, KSZ9567, LAN9370, LAN9371, LAN9372,
  LAN9373, LAN9374

For those entries the designated initializers leave n_pins at 0, so
ksz_ptp_clock_register() now advertises caps.n_pins = 0 where it
previously advertised KSZ_PTP_N_GPIO (2) and filled in 2 pin
descriptors.  Does this drop PTP pin and periodic output support on
those switches?

The KSZ8463 entry also has no .n_pins here, and ksz8463_setup() in
ksz8.c reaches ksz_ptp_clock_register() when dev->irq > 0:

drivers/net/dsa/microchip/ksz8.c:ksz8463_setup() {
	...
		ret = ksz8463_ptp_irq_setup(ds);
		if (ret)
			goto free_girq;

		ret = ksz_ptp_clock_register(ds);
	...
}

The commit message says KSZ8463 "has 12 pins that can be used as
periodic outputs", but this commit gives that entry n_pins == 0 rather
than 12.  Was the KSZ8463 initializer meant to be part of this patch?

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 39cc70d65900b..c92936bf78d3d 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -1050,14 +1050,14 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
>  	ptp_data->caps.enable		= ksz_ptp_enable;
>  	ptp_data->caps.verify		= ksz_ptp_verify_pin;
> -	ptp_data->caps.n_pins		= KSZ_PTP_N_GPIO;
> +	ptp_data->caps.n_pins		= dev->info->n_pins;
>  	ptp_data->caps.n_per_out	= 3;
>  
>  	ret = ksz_ptp_start_clock(dev);
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < KSZ_PTP_N_GPIO; i++) {
> +	for (i = 0; i < dev->info->n_pins; i++) {
>  		struct ptp_pin_desc *ptp_pin = &ptp_data->pin_config[i];
>  
>  		snprintf(ptp_pin->name,

[Severity: High]
When dev->info->n_pins is 0, does this silently register a clock with no
usable pins?  The loop body never runs, so pin_config[] stays empty, and
the PTP core takes an early return instead of reporting an error:

drivers/ptp/ptp_sysfs.c:ptp_populate_pin_groups() {
	...
	if (!n_pins)
		return 0;
	...
}

That removes the pinN sysfs files and PTP_PIN_GETFUNC/PTP_PIN_SETFUNC
support, and ptp_find_pin() iterates over info->n_pins so it can only
return -1:

drivers/ptp/ptp_clock.c:ptp_find_pin() {
	...
	for (i = 0; i < ptp->info->n_pins; i++) {
	...
}

which makes every periodic output request fail here:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_enable_perout() {
	...
	pin = ptp_find_pin(ptp_data->clock, PTP_PF_PEROUT, request->index);
	if (pin < 0)
		return -EINVAL;
	...
}

so REG_PTP_UNIT_INDEX__4 is never programmed on those chips.  Meanwhile
caps.n_per_out is still hardcoded to 3, advertising three periodic
outputs with zero pins.

Checking the end of the series (c35b10618e5929c5a72f7e06b0bad4a4027caf11),
only KSZ8463 (.n_pins = 12) and KSZ8563 (.n_pins = 2) are initialized;
KSZ9477, KSZ9563, KSZ8567, KSZ9567 and LAN9370..LAN9374 still leave
n_pins at 0, and after "net: dsa: microchip: add the number of periodic
signals to chip infos" they get n_per_out == 0 as well, so this is not
picked up later in the series.

Would it work to initialize .n_pins for every ptp_capable entry, or to
fall back to 2 in ksz_ptp_clock_register() when the field is 0?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com

[PATCH net-next 02/10] net: dsa: microchip: add the number of periodic signals to chip infos

[Bastien Curutchet (Schneider Electric)]

The number of periodic signals available is hardcoded to 3 while
KSZ8463 can produce 12 different periodic signals.

Add an n_per_out attribute to the struct ksz_chip_data to make this
setting configurable.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 1 +
 drivers/net/dsa/microchip/ksz_common.h | 1 +
 drivers/net/dsa/microchip/ksz_ptp.c    | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 55d1e3e3c529..194c88cdd00b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1195,6 +1195,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.wr_table = &ksz8563_register_set,
 		.rd_table = &ksz8563_register_set,
 		.n_pins = 2,
+		.n_per_out = 3,
 	},
 
 	[KSZ8795] = {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index c678fc6597dc..1155b39350c2 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -140,6 +140,7 @@ struct ksz_chip_data {
 	const struct regmap_access_table *wr_table;
 	const struct regmap_access_table *rd_table;
 	const u8 n_pins;
+	const u8 n_per_out;
 };
 
 struct ksz_irq {
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index c92936bf78d3..f80015250e5a 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1051,7 +1051,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_data->caps.enable		= ksz_ptp_enable;
 	ptp_data->caps.verify		= ksz_ptp_verify_pin;
 	ptp_data->caps.n_pins		= dev->info->n_pins;
-	ptp_data->caps.n_per_out	= 3;
+	ptp_data->caps.n_per_out	= dev->info->n_per_out;
 
 	ret = ksz_ptp_start_clock(dev);
 	if (ret)

-- 
2.55.0

Re: [net-next,02/10] net: dsa: microchip: add the number of periodic signals to chip infos

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Functional regression: `ksz_ptp_clock_register()` now advertises
  `caps.n_per_out = dev->info->n_per_out`…

--

net: dsa: microchip: add the number of periodic signals to chip infos

This adds an n_per_out field to struct ksz_chip_data and makes
ksz_ptp_clock_register() take the number of periodic outputs from the
chip info instead of the hardcoded value 3.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 55d1e3e3c5299..194c88cdd00bc 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1195,6 +1195,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.wr_table = &ksz8563_register_set,
>  		.rd_table = &ksz8563_register_set,
>  		.n_pins = 2,
> +		.n_per_out = 3,
>  	},
>  
>  	[KSZ8795] = {
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index c678fc6597dc4..1155b39350c2b 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -140,6 +140,7 @@ struct ksz_chip_data {
>  	const struct regmap_access_table *wr_table;
>  	const struct regmap_access_table *rd_table;
>  	const u8 n_pins;
> +	const u8 n_per_out;
>  };
>  
>  struct ksz_irq {
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index c92936bf78d3d..f80015250e5ab 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -1051,7 +1051,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.enable		= ksz_ptp_enable;
>  	ptp_data->caps.verify		= ksz_ptp_verify_pin;
>  	ptp_data->caps.n_pins		= dev->info->n_pins;
> -	ptp_data->caps.n_per_out	= 3;
> +	ptp_data->caps.n_per_out	= dev->info->n_per_out;

[Severity: High]
Does this drop periodic output support on the other PTP capable chips?

Only the KSZ8563 entry of ksz_switch_chips[] gets .n_per_out in this patch.
A grep of ksz_common.c at this commit shows one n_per_out initializer, while
there are ten entries with .ptp_capable = true (KSZ8563, KSZ9477, KSZ9563,
KSZ8567, KSZ9567 and LAN9370/9371/9372/9373/9374).

Since the remaining entries use designated initializers, the new member is
zero-filled for them, so ksz_ptp_clock_register() registers those clocks with
caps.n_per_out = 0 where it previously advertised 3.

The PTP core then refuses the feature outright, for example in
drivers/ptp/ptp_chardev.c:

	ptp_ioctl():
		if (perout->index >= ops->n_per_out)
			return -EINVAL;

	ptp_set_pinfunc():
	case PTP_PF_PEROUT:
		if (chan >= info->n_per_out)
			return -EINVAL;

so every PTP_PEROUT_REQUEST and every PTP_PF_PEROUT pin assignment on those
switches would now fail with -EINVAL, and PTP_CLOCK_GETCAPS would report zero
periodic channels. The generic back end (ksz_ptp_enable_perout() and
ksz_ptp_tou_start()) is still present for these parts, so this looks like a
user visible behaviour change rather than a pure configurability refactor.

Would it make sense to add .n_per_out to every ptp_capable entry, or to fall
back to the previous value when the field is zero?

The same question applies to the preceding commit "net: dsa: microchip: add
the number of pins to chip infos": .n_pins is only set for KSZ8563 there as
well, which additionally turns the

	for (i = 0; i < dev->info->n_pins; i++)

pin descriptor loop in ksz_ptp_clock_register() into a no-op for those chips.

This also still holds at the end of the series (c35b1061), where
ksz_ptp_set_caps() copies the same zero valued fields and ksz_common.c has
only the KSZ8463 and KSZ8563 n_per_out initializers.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com

[PATCH net-next 03/10] net: dsa: microchip: use dynamic mask to check pulse width validity

[Bastien Curutchet (Schneider Electric)]

The pulse width validity is checked against an hardcoded mask. KSZ8463
uses a smaller register than other switches so this check won't work for
it.

Move the mask as a ksz_ptp_tou_pulse_verify()'s input to allow changing
it for KSZ8463 case.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index f80015250e5a..7bf83e0b5918 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -86,7 +86,7 @@ static int ksz_ptp_tou_reset(struct ksz_device *dev, u8 unit)
 			 0);
 }
 
-static int ksz_ptp_tou_pulse_verify(u64 pulse_ns)
+static int ksz_ptp_tou_pulse_verify(u64 pulse_ns, u32 mask)
 {
 	u32 data;
 
@@ -94,7 +94,7 @@ static int ksz_ptp_tou_pulse_verify(u64 pulse_ns)
 		return -EINVAL;
 
 	data = (pulse_ns / 8);
-	if (!FIELD_FIT(TRIG_PULSE_WIDTH_M, data))
+	if ((mask & data) != data)
 		return -ERANGE;
 
 	return 0;
@@ -245,7 +245,7 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
 				       KSZ_MAX_PULSE_WIDTH);
 	}
 
-	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns);
+	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns, TRIG_PULSE_WIDTH_M);
 	if (ret)
 		return ret;
 

-- 
2.55.0

[PATCH net-next 04/10] net: dsa: microchip: extract PTP callbacks configuration from PTP registration

[Bastien Curutchet (Schneider Electric)]

KSZ8463 also has periodic output capabilities but the way its outputs
are driven differs from the other switches. It will need its own set of
PTP callbacks to implements this behaviour.

Extract the PTP callbacks configuration in a dedicated function to be
called before the PTP registration to ease the use of others callbacks
when needed.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c         |  2 ++
 drivers/net/dsa/microchip/ksz9477.c      |  1 +
 drivers/net/dsa/microchip/ksz_ptp.c      | 20 ++++++++++++++------
 drivers/net/dsa/microchip/ksz_ptp.h      |  2 ++
 drivers/net/dsa/microchip/lan937x_main.c |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index 941ae9f66f70..f2a23e2e474a 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -2478,6 +2478,7 @@ static int ksz8463_setup(struct dsa_switch *ds)
 		if (ret)
 			goto free_girq;
 
+		ksz_ptp_set_caps(ds);
 		ret = ksz_ptp_clock_register(ds);
 		if (ret) {
 			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
@@ -2775,6 +2776,7 @@ static int ksz8_setup(struct dsa_switch *ds)
 	}
 
 	if (dev->info->ptp_capable) {
+		ksz_ptp_set_caps(ds);
 		ret = ksz_ptp_clock_register(ds);
 		if (ret) {
 			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3ee995545c57..72528a53b67d 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1781,6 +1781,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	}
 
 	if (dev->info->ptp_capable) {
+		ksz_ptp_set_caps(ds);
 		ret = ksz_ptp_clock_register(ds);
 		if (ret) {
 			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 7bf83e0b5918..ea22601f6b26 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -1028,17 +1028,12 @@ static int ksz_ptp_start_clock(struct ksz_device *dev)
 	return 0;
 }
 
-int ksz_ptp_clock_register(struct dsa_switch *ds)
+void ksz_ptp_set_caps(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
-	const u16 *regs = dev->info->regs;
 	struct ksz_ptp_data *ptp_data;
-	int ret;
-	u8 i;
 
 	ptp_data = &dev->ptp_data;
-	mutex_init(&ptp_data->lock);
-	spin_lock_init(&ptp_data->clock_lock);
 
 	ptp_data->caps.owner		= THIS_MODULE;
 	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
@@ -1052,6 +1047,19 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_data->caps.verify		= ksz_ptp_verify_pin;
 	ptp_data->caps.n_pins		= dev->info->n_pins;
 	ptp_data->caps.n_per_out	= dev->info->n_per_out;
+}
+
+int ksz_ptp_clock_register(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	const u16 *regs = dev->info->regs;
+	struct ksz_ptp_data *ptp_data;
+	int ret;
+	u8 i;
+
+	ptp_data = &dev->ptp_data;
+	mutex_init(&ptp_data->lock);
+	spin_lock_init(&ptp_data->clock_lock);
 
 	ret = ksz_ptp_start_clock(dev);
 	if (ret)
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
index 7067ec9bd1e6..89716b31596b 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -33,6 +33,7 @@ struct ksz_ptp_data {
 	struct timespec64 perout_period;
 };
 
+void ksz_ptp_set_caps(struct dsa_switch *ds);
 int ksz_ptp_clock_register(struct dsa_switch *ds);
 
 void ksz_ptp_clock_unregister(struct dsa_switch *ds);
@@ -65,6 +66,7 @@ struct ksz_ptp_data {
 	struct mutex lock;
 };
 
+static inline void ksz_ptp_set_caps(struct dsa_switch *ds) { }
 static inline int ksz_ptp_clock_register(struct dsa_switch *ds)
 {
 	return 0;
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index 86ce3a86705f..3a209122fc7d 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -867,6 +867,7 @@ static int lan937x_setup(struct dsa_switch *ds)
 		}
 	}
 
+	ksz_ptp_set_caps(ds);
 	ret = ksz_ptp_clock_register(ds);
 	if (ret) {
 		dev_err(dev->dev, "Failed to register PTP clock: %d\n",

-- 
2.55.0

Re: [net-next,04/10] net: dsa: microchip: extract PTP callbacks configuration from PTP registration

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Per-chip PTP capability fields `n_pins`/`n_per_out` are only
  populated for KSZ8563 (and, at the end of the series, KSZ8463), so…

--

net: dsa: microchip: extract PTP callbacks configuration from PTP registration

This moves the ptp_clock_info fields assignment out of
ksz_ptp_clock_register() into a new ksz_ptp_set_caps(), and calls the new
helper from each of the ksz8463/ksz8/ksz9477/lan937x setup paths before
registering the PTP clock.

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 7bf83e0b59185..ea22601f6b26b 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -1028,17 +1028,12 @@ static int ksz_ptp_start_clock(struct ksz_device *dev)
>  	return 0;
>  }
>  
> -int ksz_ptp_clock_register(struct dsa_switch *ds)
> +void ksz_ptp_set_caps(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> -	const u16 *regs = dev->info->regs;
>  	struct ksz_ptp_data *ptp_data;
> -	int ret;
> -	u8 i;
>  
>  	ptp_data = &dev->ptp_data;
> -	mutex_init(&ptp_data->lock);
> -	spin_lock_init(&ptp_data->clock_lock);
>  
>  	ptp_data->caps.owner		= THIS_MODULE;
>  	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
> @@ -1052,6 +1047,19 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.verify		= ksz_ptp_verify_pin;
>  	ptp_data->caps.n_pins		= dev->info->n_pins;
>  	ptp_data->caps.n_per_out	= dev->info->n_per_out;

[Severity: High]
Are the per-chip n_pins and n_per_out values actually populated for all
PTP capable switches?  ksz_ptp_set_caps() publishes dev->info->n_pins and
dev->info->n_per_out directly, but in ksz_common.c only the [KSZ8563]
entry initializes them:

	[KSZ8563] = {
		...
		.ptp_capable = true,
		...
		.n_pins = 2,
		.n_per_out = 3,

The other .ptp_capable = true entries ([KSZ9477], [KSZ9563], [KSZ8567],
[KSZ9567], [LAN9370], [LAN9371], [LAN9372], [LAN9373], [LAN9374]) leave
both fields at 0.

Before this rework the code hardcoded caps.n_pins = KSZ_PTP_N_GPIO (2) and
caps.n_per_out = 3 for every one of these chips, so does this leave them
advertising zero pins and zero periodic output channels?

With n_per_out == 0, ptp_chardev.c rejects every request:

	if (perout->index >= ops->n_per_out)
		return -EINVAL;

and with n_pins == 0 both PTP_PIN_SETFUNC and PTP_PIN_GETFUNC take the
same path:

	if (pd.index >= ops->n_pins)
		return -EINVAL;

> +}
> +
> +int ksz_ptp_clock_register(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	const u16 *regs = dev->info->regs;
> +	struct ksz_ptp_data *ptp_data;
> +	int ret;
> +	u8 i;
> +
> +	ptp_data = &dev->ptp_data;
> +	mutex_init(&ptp_data->lock);
> +	spin_lock_init(&ptp_data->clock_lock);
>  
>  	ret = ksz_ptp_start_clock(dev);
>  	if (ret)

Related to the same question: the pin descriptor loop later in
ksz_ptp_clock_register() is bounded by dev->info->n_pins, so for the chips
above no ptp_pin_desc entry gets a name, index or func at all:

	for (i = 0; i < dev->info->n_pins; i++) {
		struct ptp_pin_desc *ptp_pin = &ptp_data->pin_config[i];

		snprintf(ptp_pin->name,
			 sizeof(ptp_pin->name), "ksz_ptp_pin_%02d", i);
		ptp_pin->index = i;
		ptp_pin->func = PTP_PF_NONE;
	}

> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index 86ce3a86705f9..3a209122fc7dc 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -867,6 +867,7 @@ static int lan937x_setup(struct dsa_switch *ds)
>  		}
>  	}
>  
> +	ksz_ptp_set_caps(ds);
>  	ret = ksz_ptp_clock_register(ds);
>  	if (ret) {
>  		dev_err(dev->dev, "Failed to register PTP clock: %d\n",

[Severity: High]
lan937x_setup() registers the PTP clock unconditionally, so does every
LAN9370-LAN9374 device end up registering with n_pins = 0 and
n_per_out = 0 here?

Reading ksz_common.c at the end of the series (c35b1061) the initializers
are still only present for [KSZ8463] and [KSZ8563], so nothing later
appears to restore the values for the remaining chips.  Should the
.n_pins / .n_per_out initializers be added to the other .ptp_capable
entries?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com

[PATCH net-next 05/10] net: dsa: microchip: extract ptp_get_pin

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 supports periodic outputs but doesn't handle them in the same
way as the other KSZ switches. To add proper support for the KSZ8463, a
dedicated ksz8463_ptp_enable_perout() function needs to be created. This
function will use the same algorithm to select the output pin as the
common ksz_ptp_enable_perout() function.

Extract the pin selection algorithm into a dedicated function so it can
be used later by the KSZ8463 support.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index ea22601f6b26..f97ea9d69ebc 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -183,17 +183,11 @@ static int ksz_ptp_configure_perout(struct ksz_device *dev,
 	return 0;
 }
 
-static int ksz_ptp_enable_perout(struct ksz_device *dev,
-				 struct ptp_perout_request const *request,
-				 int on)
+static int ksz_ptp_get_pin(struct ksz_device *dev,
+			   struct ptp_perout_request const *request)
 {
 	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
-	u64 req_pulse_width_ns;
-	u64 cycle_width_ns;
-	u64 pulse_width_ns;
-	int pin = 0;
-	u32 data32;
-	int ret;
+	int pin;
 
 	if (request->flags & ~PTP_PEROUT_DUTY_CYCLE)
 		return -EOPNOTSUPP;
@@ -206,6 +200,25 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
 	if (pin < 0)
 		return -EINVAL;
 
+	return pin;
+}
+
+static int ksz_ptp_enable_perout(struct ksz_device *dev,
+				 struct ptp_perout_request const *request,
+				 int on)
+{
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+	u64 req_pulse_width_ns;
+	u64 cycle_width_ns;
+	u64 pulse_width_ns;
+	int pin = 0;
+	u32 data32;
+	int ret;
+
+	pin = ksz_ptp_get_pin(dev, request);
+	if (pin < 0)
+		return pin;
+
 	data32 = FIELD_PREP(PTP_GPIO_INDEX, pin) |
 		 FIELD_PREP(PTP_TOU_INDEX, request->index);
 	ret = ksz_rmw32(dev, REG_PTP_UNIT_INDEX__4,

-- 
2.55.0

Re: [net-next,05/10] net: dsa: microchip: extract ptp_get_pin

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] PTP periodic-output/pin capabilities are silently zeroed for
  most PTP-capable KSZ/LAN switches.

--

net: dsa: microchip: extract ptp_get_pin

This patch splits the periodic output pin selection out of
ksz_ptp_enable_perout() into a new ksz_ptp_get_pin() helper, so that an
upcoming ksz8463_ptp_enable_perout() can reuse the same algorithm. No
functional change is intended by this patch itself.

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index ea22601f6b26b..f97ea9d69ebc5 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -183,17 +183,11 @@ static int ksz_ptp_configure_perout(struct ksz_device *dev,
>  	return 0;
>  }
>  
> -static int ksz_ptp_enable_perout(struct ksz_device *dev,
> -				 struct ptp_perout_request const *request,
> -				 int on)
> +static int ksz_ptp_get_pin(struct ksz_device *dev,
> +			   struct ptp_perout_request const *request)
>  {
>  	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> -	u64 req_pulse_width_ns;
> -	u64 cycle_width_ns;
> -	u64 pulse_width_ns;
> -	int pin = 0;
> -	u32 data32;
> -	int ret;
> +	int pin;
>  
>  	if (request->flags & ~PTP_PEROUT_DUTY_CYCLE)
>  		return -EOPNOTSUPP;
> @@ -206,6 +200,25 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
>  	if (pin < 0)
>  		return -EINVAL;
>  
> +	return pin;
> +}

[Severity: High]
The pin selection moved here still relies on ptp_find_pin():

drivers/net/dsa/microchip/ksz_ptp.c:ksz_ptp_get_pin() {
	pin = ptp_find_pin(ptp_data->clock, PTP_PF_PEROUT, request->index);
	if (pin < 0)
		return -EINVAL;
}

Earlier patches in this series ("add the number of pins to chip infos" and
"add the number of periodic signals to chip infos") replaced the previously
unconditional values in ksz_ptp_set_caps() with per-chip descriptor fields:

	ptp_data->caps.n_pins		= dev->info->n_pins;
	ptp_data->caps.n_per_out	= dev->info->n_per_out;

Before the series these were caps.n_pins = KSZ_PTP_N_GPIO (2) and
caps.n_per_out = 3 for every ptp_capable switch.

At this commit, only the KSZ8563 entry in ksz_switch_chips[] sets the new
members (.n_pins = 2, .n_per_out = 3). The other entries with
.ptp_capable = true - KSZ9477, KSZ9563, KSZ8567, KSZ9567, LAN9370, LAN9371,
LAN9372, LAN9373 and LAN9374 - leave both fields zero-initialized.

Can periodic output still work on those switches? With n_per_out == 0,
ptp_perout_request() in drivers/ptp/ptp_chardev.c rejects every request:

	if (perout->index >= ops->n_per_out)
		return -EINVAL;

and with n_pins == 0 the loop in ksz_ptp_clock_register() initializes no pin
descriptor at all:

	for (i = 0; i < dev->info->n_pins; i++) {
		struct ptp_pin_desc *ptp_pin = &ptp_data->pin_config[i];
		...
	}

so PTP_PIN_SETFUNC cannot address any pin and the ptp_find_pin() call above
can never succeed on those chips.

Should every .ptp_capable descriptor carry its previous .n_pins = 2 and
.n_per_out = 3 values? The later patch adding KSZ8463 periodic output
support only fills in the KSZ8463 entry, so the other families appear to
remain without pins and periodic outputs after the whole series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com

[PATCH net-next 06/10] net: dsa: microchip: extract compute_width

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 supports periodic outputs but doesn't handle them in the same
way as the other KSZ switches. To add proper support for the KSZ8463, a
dedicated ksz8463_ptp_enable_perout() function needs to be created. This
function will use the same algorithm to compute the periodic cycles as
the common ksz_ptp_enable_perout() function.

Extract these algorithms into dedicated functions so they can be used
later by the KSZ8463 support.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 67 ++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index f97ea9d69ebc..2f141df65eb0 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -203,12 +203,49 @@ static int ksz_ptp_get_pin(struct ksz_device *dev,
 	return pin;
 }
 
+static int ksz_ptp_compute_perout_cycle(struct ksz_device *dev,
+					struct ptp_perout_request const *request,
+					u64 *cycle_width_ns)
+{
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+
+	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
+	ptp_data->perout_target_time_first.tv_nsec = request->start.nsec;
+
+	ptp_data->perout_period.tv_sec = request->period.sec;
+	ptp_data->perout_period.tv_nsec = request->period.nsec;
+
+	*cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
+	if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {
+		*cycle_width_ns = 0;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
+					struct ptp_perout_request const *request,
+					u64 max_pulse_width)
+{
+	u64 req_pulse_width_ns;
+
+	if (request->flags & PTP_PEROUT_DUTY_CYCLE)
+		return request->on.sec * NSEC_PER_SEC +	request->on.nsec;
+
+	/* Use a duty cycle of 50%. Maximum pulse width supported by the
+	 * hardware is a little bit more than 125 ms.
+	 */
+	req_pulse_width_ns = (request->period.sec * NSEC_PER_SEC +
+			      request->period.nsec) / 2;
+	return min_t(u64, req_pulse_width_ns, max_pulse_width);
+}
+
 static int ksz_ptp_enable_perout(struct ksz_device *dev,
 				 struct ptp_perout_request const *request,
 				 int on)
 {
 	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
-	u64 req_pulse_width_ns;
 	u64 cycle_width_ns;
 	u64 pulse_width_ns;
 	int pin = 0;
@@ -234,29 +271,11 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
 		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
 		return 0;
 	}
-
-	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
-	ptp_data->perout_target_time_first.tv_nsec = request->start.nsec;
-
-	ptp_data->perout_period.tv_sec = request->period.sec;
-	ptp_data->perout_period.tv_nsec = request->period.nsec;
-
-	cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
-	if ((cycle_width_ns & TRIG_CYCLE_WIDTH_M) != cycle_width_ns)
-		return -EINVAL;
-
-	if (request->flags & PTP_PEROUT_DUTY_CYCLE) {
-		pulse_width_ns = request->on.sec * NSEC_PER_SEC +
-			request->on.nsec;
-	} else {
-		/* Use a duty cycle of 50%. Maximum pulse width supported by the
-		 * hardware is a little bit more than 125 ms.
-		 */
-		req_pulse_width_ns = (request->period.sec * NSEC_PER_SEC +
-				      request->period.nsec) / 2;
-		pulse_width_ns = min_t(u64, req_pulse_width_ns,
-				       KSZ_MAX_PULSE_WIDTH);
-	}
+	ret = ksz_ptp_compute_perout_cycle(dev, request, &cycle_width_ns);
+	if (ret)
+		return ret;
+	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
+						      KSZ_MAX_PULSE_WIDTH);
 
 	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns, TRIG_PULSE_WIDTH_M);
 	if (ret)

-- 
2.55.0

Re: [net-next,06/10] net: dsa: microchip: extract compute_width

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 (2) · Medium: 0 · Low: 2

- [Low] The new helper `ksz_ptp_compute_perout_pulse()`
  (drivers/net/dsa/microchip/ksz_ptp.c:227) declares a `struct…
- [Low] The comment moved into the generic helper
  `ksz_ptp_compute_perout_pulse()`…

Pre-existing issues:
- [High] A denormalized periodic-output period from userspace is accepted
  as a zero-nanosecond period and later causes a kernel divide-by-zero…
- [High] A rejected periodic-output reconfiguration leaves hardware and
  driver bookkeeping inconsistent, and the poisoned cache can later…

--

net: dsa: microchip: extract compute_width

This moves the periodic-output cycle width and pulse width computations out
of ksz_ptp_enable_perout() into two new helpers,
ksz_ptp_compute_perout_cycle() and ksz_ptp_compute_perout_pulse(), so a
future ksz8463_ptp_enable_perout() can reuse them. The pulse helper takes
the maximum pulse width as a parameter instead of using KSZ_MAX_PULSE_WIDTH
directly.

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index f97ea9d69ebc5..2f141df65eb01 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -203,12 +203,49 @@ static int ksz_ptp_get_pin(struct ksz_device *dev,
>  	return pin;
>  }
>  
> +static int ksz_ptp_compute_perout_cycle(struct ksz_device *dev,
> +					struct ptp_perout_request const *request,
> +					u64 *cycle_width_ns)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +
> +	ptp_data->perout_target_time_first.tv_sec  = request->start.sec;
> +	ptp_data->perout_target_time_first.tv_nsec = request->start.nsec;
> +
> +	ptp_data->perout_period.tv_sec = request->period.sec;
> +	ptp_data->perout_period.tv_nsec = request->period.nsec;
> +
> +	*cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
> +	if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {
> +		*cycle_width_ns = 0;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
This isn't a bug introduced by this patch, the baseline inline code did the
same mask check on the same cached values, but since the code is now being
moved into a helper that a second caller will reuse, can a zero period slip
through here?

ptp_perout_request() in drivers/ptp/ptp_chardev.c decides enable/disable
with:

	return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);

It never normalizes the timespec, so period.sec = -1, period.nsec =
1000000000 is treated as "enable". timespec64_to_ns() then yields
-1e9 + 1e9 = 0, and the check above passes because (0 & 0xffffffff) == 0.
ksz_ptp_compute_perout_pulse() returns 0 as well, which
ksz_ptp_tou_pulse_verify() accepts.

The zero period stays cached in ptp_data->perout_period while tou_mode is
KSZ_PTP_TOU_PEROUT, and the next PTP_CLOCK_SETTIME or ADJTIME reaches
ksz_ptp_restart_perout():

	period_ns = timespec64_to_ns(&ptp_data->perout_period);

	if (first_ns < now_ns) {
		count = div_u64(now_ns - first_ns, period_ns);

div_u64() takes a u32 divisor, so with period_ns == 0 this is a division by
zero in ioctl context. If instead the cached start time falls inside the
next 100 ms, this loop never terminates while ptp_data->lock is held:

	while (next_ns < now_ns + 100000000)
		next_ns += period_ns;

Would it make sense to reject a zero or negative period in
ksz_ptp_compute_perout_cycle() while it is being factored out?

[Severity: High]
This is a pre-existing issue as well, the same ordering and the same
unconditional cache write existed before the move, but the same helper is
now shared with the KSZ8463 path, so it seems worth asking about.

The software cache is committed before the range check, so when the check
fails ptp_data->perout_period and perout_target_time_first describe a
configuration the hardware refused. Combined with the truncating divisor in
ksz_ptp_restart_perout():

	count = div_u64(now_ns - first_ns, period_ns);

a cached period of exactly 2^32 ns (period.sec = 4, period.nsec =
294967296) is precisely the kind of value that the TRIG_CYCLE_WIDTH_M check
rejects, and its low 32 bits are zero, so div_u64() divides by zero on the
next clock step. Should the period be validated before ptp_data is updated?

> +
> +static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
> +					struct ptp_perout_request const *request,
> +					u64 max_pulse_width)
> +{
> +	u64 req_pulse_width_ns;
> +
> +	if (request->flags & PTP_PEROUT_DUTY_CYCLE)
> +		return request->on.sec * NSEC_PER_SEC +	request->on.nsec;
                                                     ^^^

[Severity: Low]
This isn't a bug, but two small things in ksz_ptp_compute_perout_pulse():
the dev parameter is never dereferenced in the body (only request->flags,
request->on, request->period and max_pulse_width are used), and a stray tab
ended up in the middle of the return statement where the original code had
a line break.

> +
> +	/* Use a duty cycle of 50%. Maximum pulse width supported by the
> +	 * hardware is a little bit more than 125 ms.
> +	 */
> +	req_pulse_width_ns = (request->period.sec * NSEC_PER_SEC +
> +			      request->period.nsec) / 2;
> +	return min_t(u64, req_pulse_width_ns, max_pulse_width);
> +}

[Severity: Low]
This isn't a bug either, but the comment now states a fixed device property
inside a helper that clamps to the caller-supplied max_pulse_width. The
KSZ8463 support that follows passes KSZ8463_MAX_PULSE_WIDTH (500000, i.e.
0.5 ms), which contradicts the "a little bit more than 125 ms" wording
sitting in the shared helper.

Would it be clearer to move that sentence next to the KSZ_MAX_PULSE_WIDTH
definition, or to reword it in terms of max_pulse_width?

>  
>  static int ksz_ptp_enable_perout(struct ksz_device *dev,
>  				 struct ptp_perout_request const *request,
>  				 int on)
>  {
>  	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> -	u64 req_pulse_width_ns;
>  	u64 cycle_width_ns;
>  	u64 pulse_width_ns;
>  	int pin = 0;
> @@ -234,29 +271,11 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
>  		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
>  		return 0;
>  	}
> -
[ ... ]
> +	ret = ksz_ptp_compute_perout_cycle(dev, request, &cycle_width_ns);
> +	if (ret)
> +		return ret;

[Severity: High]
Related to the pre-existing ordering noted above, and again not changed by
this patch: ksz_ptp_tou_reset() earlier in ksz_ptp_enable_perout() has
already cleared TRIG_RESET/TRIG_ENABLE and stopped a running output by the
time this validation runs. On the -EINVAL return here, tou_mode stays at
KSZ_PTP_TOU_PEROUT (ksz_ptp_get_pin() explicitly allows re-entry in PEROUT
mode), so the driver reports an armed periodic output while the trigger unit
is disabled. Should tou_mode be set back to KSZ_PTP_TOU_IDLE on these error
paths?

> +	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
> +						      KSZ_MAX_PULSE_WIDTH);
>  
>  	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns, TRIG_PULSE_WIDTH_M);
>  	if (ret)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com

[PATCH net-next 07/10] net: dsa: microchip: extract prepare reset

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 supports periodic outputs but doesn't handle them in the same
way as the other KSZ switches. To add proper support for the KSZ8463, a
dedicated ksz8463_ptp_restart_perout() function needs to be created. This
function will use the same request initialization as the common
ksz_ptp_restart_perout() function.

Extract the inialization into a dedicated function so it can be used
later by the KSZ8463 support.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 2f141df65eb0..e262bc7448f2 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -788,11 +788,11 @@ static int ksz_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return ret;
 }
 
-static int ksz_ptp_restart_perout(struct ksz_device *dev)
+static int ksz_ptp_prepare_restart_perout(struct ksz_device *dev,
+					  struct ptp_perout_request *request)
 {
 	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
 	s64 now_ns, first_ns, period_ns, next_ns;
-	struct ptp_perout_request request;
 	struct timespec64 next;
 	struct timespec64 now;
 	unsigned int count;
@@ -823,12 +823,24 @@ static int ksz_ptp_restart_perout(struct ksz_device *dev)
 
 	/* Restart periodic output signal */
 	next = ns_to_timespec64(next_ns);
-	request.start.sec  = next.tv_sec;
-	request.start.nsec = next.tv_nsec;
-	request.period.sec  = ptp_data->perout_period.tv_sec;
-	request.period.nsec = ptp_data->perout_period.tv_nsec;
-	request.index = 0;
-	request.flags = 0;
+	request->start.sec  = next.tv_sec;
+	request->start.nsec = next.tv_nsec;
+	request->period.sec  = ptp_data->perout_period.tv_sec;
+	request->period.nsec = ptp_data->perout_period.tv_nsec;
+	request->index = 0;
+	request->flags = 0;
+
+	return 0;
+}
+
+static int ksz_ptp_restart_perout(struct ksz_device *dev)
+{
+	struct ptp_perout_request request;
+	int ret;
+
+	ret = ksz_ptp_prepare_restart_perout(dev, &request);
+	if (ret)
+		return ret;
 
 	return ksz_ptp_enable_perout(dev, &request, 1);
 }

-- 
2.55.0

[PATCH net-next 08/10] net: dsa: microchip: extract time update

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 supports periodic outputs but doesn't handle them in the same
way as the other KSZ switches. To add proper support for the KSZ8463, a
dedicated ksz8463_ptp_settime() function needs to be created. This
function will access the same registers than the common
ksz_ptp_settime().

Extract the register accesses into a dedicated function so it can be
used later by the KSZ8463 support.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index e262bc7448f2..aa73ad1f05d8 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -845,30 +845,42 @@ static int ksz_ptp_restart_perout(struct ksz_device *dev)
 	return ksz_ptp_enable_perout(dev, &request, 1);
 }
 
-static int ksz_ptp_settime(struct ptp_clock_info *ptp,
-			   const struct timespec64 *ts)
+static int __ksz_ptp_settime(struct ksz_device *dev,
+			     const struct timespec64 *ts)
 {
-	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
-	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
 	const u16 *regs = dev->info->regs;
 	int ret;
 
-	mutex_lock(&ptp_data->lock);
-
 	/* Write to shadow registers and Load PTP clock */
 	ret = ksz_write16(dev, regs[PTP_RTC_SUB_NANOSEC], PTP_RTC_0NS);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = ksz_write32(dev, regs[PTP_RTC_NANOSEC], ts->tv_nsec);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = ksz_write32(dev, regs[PTP_RTC_SEC], ts->tv_sec);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = ksz_rmw16(dev, regs[PTP_CLK_CTRL], PTP_LOAD_TIME, PTP_LOAD_TIME);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz_ptp_settime(struct ptp_clock_info *ptp,
+			   const struct timespec64 *ts)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	int ret;
+
+	mutex_lock(&ptp_data->lock);
+
+	ret = __ksz_ptp_settime(dev, ts);
 	if (ret)
 		goto unlock;
 

-- 
2.55.0

[PATCH net-next 09/10] net: dsa: microchip: extract time adjustment

[Bastien Curutchet (Schneider Electric)]

The KSZ8463 supports periodic outputs but doesn't handle them in the same
way as the other KSZ switches. To add proper support for the KSZ8463, a
dedicated ksz8463_ptp_adjtime() function needs to be created. This
function will access the same registers as the common ksz_ptp_adjtime()
function.

Extract the register accesses into a dedicated function so it can be
used later by the KSZ8463 support.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_ptp.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index aa73ad1f05d8..0a77b46cfad2 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -946,18 +946,13 @@ static int ksz_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	return ret;
 }
 
-static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+static int __ksz_ptp_adjtime(struct ksz_device *dev, s64 delta)
 {
-	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
-	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
-	struct timespec64 delta64 = ns_to_timespec64(delta);
 	const u16 *regs = dev->info->regs;
 	s32 sec, nsec;
 	u16 data16;
 	int ret;
 
-	mutex_lock(&ptp_data->lock);
-
 	/* do not use ns_to_timespec64(),
 	 * both sec and nsec are subtracted by hw
 	 */
@@ -965,15 +960,15 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 
 	ret = ksz_write32(dev, regs[PTP_RTC_NANOSEC], abs(nsec));
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = ksz_write32(dev, regs[PTP_RTC_SEC], abs(sec));
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = ksz_read16(dev, regs[PTP_CLK_CTRL], &data16);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	data16 |= PTP_STEP_ADJ;
 
@@ -984,6 +979,22 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 		data16 |= PTP_STEP_DIR;
 
 	ret = ksz_write16(dev, regs[PTP_CLK_CTRL], data16);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	struct timespec64 delta64 = ns_to_timespec64(delta);
+	int ret;
+
+	mutex_lock(&ptp_data->lock);
+
+	ret = __ksz_ptp_adjtime(dev, delta);
 	if (ret)
 		goto unlock;
 

-- 
2.55.0

[PATCH net-next 10/10] net: dsa: microchip: add periodic output support for the KSZ8463

[Bastien Curutchet (Schneider Electric)]

KSZ8463 has several periodic outputs. The way it handles them differ
from the other KSZ switches:
 - KSZ8463 has one set of registers for each of its 12 units while others
   switches share on set of configuration registers for all their units.
 - Maximum size of the KSZ8463 pulse width is smaller
 - KSZ8463 has 12 outputs while others only have 2

Add support for the KSZ8463 periodics outputs through a set of KSZ8463
specific functions.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz8.c        |   2 +-
 drivers/net/dsa/microchip/ksz_common.c  |   2 +
 drivers/net/dsa/microchip/ksz_ptp.c     | 258 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_ptp.h     |   3 +-
 drivers/net/dsa/microchip/ksz_ptp_reg.h |  16 ++
 5 files changed, 279 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
index f2a23e2e474a..6b08bb8fed5b 100644
--- a/drivers/net/dsa/microchip/ksz8.c
+++ b/drivers/net/dsa/microchip/ksz8.c
@@ -2478,7 +2478,7 @@ static int ksz8463_setup(struct dsa_switch *ds)
 		if (ret)
 			goto free_girq;
 
-		ksz_ptp_set_caps(ds);
+		ksz8463_ptp_set_caps(ds);
 		ret = ksz_ptp_clock_register(ds);
 		if (ret) {
 			dev_err(dev->dev, "Failed to register PTP clock: %d\n",
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 194c88cdd00b..111f3014e5a9 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1161,6 +1161,8 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.supports_mii = {false, false, true},
 		.supports_rmii = {false, false, true},
 		.internal_phy = {true, true, false},
+		.n_pins = 12,
+		.n_per_out = 12,
 	},
 
 	[KSZ8563] = {
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 0a77b46cfad2..d33034892a8f 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -26,6 +26,7 @@
  */
 #define KSZ_MAX_DRIFT_CORR 6249999
 #define KSZ_MAX_PULSE_WIDTH 125000000LL
+#define KSZ8463_MAX_PULSE_WIDTH 500000LL
 
 #define KSZ_PTP_INC_NS 40ULL  /* HW clock is incremented every 40 ns (by 40) */
 #define KSZ_PTP_SUBNS_BITS 32
@@ -63,6 +64,17 @@ static int ksz_ptp_tou_gpio(struct ksz_device *dev)
 			 LED_SRC_PTP_GPIO_1 | LED_SRC_PTP_GPIO_2);
 }
 
+static int ksz8463_ptp_tou_reset(struct ksz_device *dev, u8 unit)
+{
+	int ret;
+
+	ret = ksz_rmw16(dev, KSZ8463_TOU_SW_RST, BIT(unit), BIT(unit));
+	if (ret)
+		return ret;
+
+	return ksz_rmw16(dev, KSZ8463_TOU_SW_RST, BIT(unit), 0);
+}
+
 static int ksz_ptp_tou_reset(struct ksz_device *dev, u8 unit)
 {
 	u32 data;
@@ -120,6 +132,28 @@ static int ksz_ptp_tou_target_time_set(struct ksz_device *dev,
 	return 0;
 }
 
+static int ksz8463_ptp_tou_start(struct ksz_device *dev, u8 unit)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_rmw16(dev, KSZ8463_TOU_EN, BIT(unit), BIT(unit));
+	if (ret)
+		return ret;
+
+	ret = ksz_read16(dev, KSZ8463_TOU_ACTIVE, &data);
+	if (ret)
+		return ret;
+
+	if (!(data & BIT(unit))) {
+		dev_err(dev->dev, "%s: Trigger unit%d error!\n", __func__,
+			unit);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
 {
 	u32 data;
@@ -147,6 +181,56 @@ static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
 	return 0;
 }
 
+static int ksz8463_ptp_configure_perout(struct ksz_device *dev,
+					u32 cycle_width_ns, u32 pulse_width_ns,
+					struct timespec64 const *target_time,
+					u8 index)
+{
+	struct ptp_pin_desc *pin = &dev->ptp_data.pin_config[index];
+	u16 cfg_base = KSZ8463_TRIG1_CFG + KSZ8463_TRIGN_CFG_SIZE * pin->chan;
+	u16 data;
+	int ret;
+
+	/* Hardware has only 32 bit */
+	if ((target_time->tv_sec & 0xffffffff) != target_time->tv_sec)
+		return -EINVAL;
+
+	data = KSZ8463_NOTIFY_BIT |
+	       FIELD_PREP(KSZ8463_PATTERN_M, TRIG_POS_PERIOD) |
+	       pin->index;
+	ret = ksz_write16(dev, cfg_base + KSZ8463_PATTERN_OFF, data);
+	if (ret)
+		return ret;
+
+	ret = ksz_write32(dev, cfg_base + KSZ8463_CYCLE_WIDTH_OFF,
+			  cycle_width_ns);
+	if (ret)
+		return ret;
+
+	/* Set cycle count 0 - Infinite */
+	ret = ksz_write16(dev, cfg_base + KSZ8463_CYCLE_CNT_OFF, 0);
+	if (ret)
+		return ret;
+
+	/* KSZ8463 uses a 8 ns unit value to compute the pulse width */
+	data = (pulse_width_ns / 8);
+	ret = ksz_write16(dev, cfg_base + KSZ8463_PULSE_WIDTH_OFF, data);
+	if (ret)
+		return ret;
+
+	ret = ksz_write32(dev, cfg_base + KSZ8463_TARGET_NSEC,
+			  target_time->tv_nsec);
+	if (ret)
+		return ret;
+
+	ret = ksz_write32(dev, cfg_base + KSZ8463_TARGET_SEC,
+			  target_time->tv_sec);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ksz_ptp_configure_perout(struct ksz_device *dev,
 				    u32 cycle_width_ns, u32 pulse_width_ns,
 				    struct timespec64 const *target_time,
@@ -241,6 +325,58 @@ static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
 	return min_t(u64, req_pulse_width_ns, max_pulse_width);
 }
 
+static int ksz8463_ptp_enable_perout(struct ksz_device *dev,
+				     struct ptp_perout_request const *request,
+				     int on)
+{
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+	u64 cycle_width_ns;
+	u64 pulse_width_ns;
+	int pin;
+	int ret;
+
+	pin = ksz_ptp_get_pin(dev, request);
+	if (pin < 0)
+		return pin;
+
+	ret = ksz8463_ptp_tou_reset(dev, request->index);
+	if (ret)
+		return ret;
+
+	if (!on) {
+		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
+		return 0;
+	}
+	ret = ksz_ptp_compute_perout_cycle(dev, request, &cycle_width_ns);
+	if (ret)
+		return ret;
+	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
+						      KSZ8463_MAX_PULSE_WIDTH);
+
+	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns,
+				       KSZ8463_TRIG_PULSE_WIDTH_M);
+	if (ret)
+		return ret;
+
+	ret = ksz8463_ptp_configure_perout(dev, cycle_width_ns, pulse_width_ns,
+					   &ptp_data->perout_target_time_first,
+					   pin);
+	if (ret)
+		return ret;
+
+	ret = ksz_ptp_tou_gpio(dev);
+	if (ret)
+		return ret;
+
+	ret = ksz8463_ptp_tou_start(dev, request->index);
+	if (ret)
+		return ret;
+
+	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
+
+	return 0;
+}
+
 static int ksz_ptp_enable_perout(struct ksz_device *dev,
 				 struct ptp_perout_request const *request,
 				 int on)
@@ -845,6 +981,18 @@ static int ksz_ptp_restart_perout(struct ksz_device *dev)
 	return ksz_ptp_enable_perout(dev, &request, 1);
 }
 
+static int ksz8463_ptp_restart_perout(struct ksz_device *dev)
+{
+	struct ptp_perout_request request;
+	int ret;
+
+	ret = ksz_ptp_prepare_restart_perout(dev, &request);
+	if (ret)
+		return ret;
+
+	return ksz8463_ptp_enable_perout(dev, &request, 1);
+}
+
 static int __ksz_ptp_settime(struct ksz_device *dev,
 			     const struct timespec64 *ts)
 {
@@ -871,6 +1019,41 @@ static int __ksz_ptp_settime(struct ksz_device *dev,
 	return 0;
 }
 
+static int ksz8463_ptp_settime(struct ptp_clock_info *ptp,
+			       const struct timespec64 *ts)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	int ret;
+
+	mutex_lock(&ptp_data->lock);
+
+	ret = __ksz_ptp_settime(dev, ts);
+	if (ret)
+		goto unlock;
+
+	switch (ptp_data->tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PEROUT:
+		ret = ksz8463_ptp_restart_perout(dev);
+		if (ret)
+			goto unlock;
+
+		break;
+	}
+
+	spin_lock_bh(&ptp_data->clock_lock);
+	ptp_data->clock_time = *ts;
+	spin_unlock_bh(&ptp_data->clock_lock);
+
+unlock:
+	mutex_unlock(&ptp_data->lock);
+
+	return ret;
+}
+
 static int ksz_ptp_settime(struct ptp_clock_info *ptp,
 			   const struct timespec64 *ts)
 {
@@ -985,6 +1168,40 @@ static int __ksz_ptp_adjtime(struct ksz_device *dev, s64 delta)
 	return 0;
 }
 
+static int ksz8463_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	struct timespec64 delta64 = ns_to_timespec64(delta);
+	int ret;
+
+	mutex_lock(&ptp_data->lock);
+
+	ret = __ksz_ptp_adjtime(dev, delta);
+	if (ret)
+		goto unlock;
+
+	switch (ptp_data->tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PEROUT:
+		ret = ksz8463_ptp_restart_perout(dev);
+		if (ret)
+			goto unlock;
+
+		break;
+	}
+
+	spin_lock_bh(&ptp_data->clock_lock);
+	ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64);
+	spin_unlock_bh(&ptp_data->clock_lock);
+
+unlock:
+	mutex_unlock(&ptp_data->lock);
+	return ret;
+}
+
 static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
@@ -1019,6 +1236,26 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	return ret;
 }
 
+static int ksz8463_ptp_enable(struct ptp_clock_info *ptp,
+			      struct ptp_clock_request *req, int on)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	int ret;
+
+	switch (req->type) {
+	case PTP_CLK_REQ_PEROUT:
+		mutex_lock(&ptp_data->lock);
+		ret = ksz8463_ptp_enable_perout(dev, &req->perout, on);
+		mutex_unlock(&ptp_data->lock);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
 static int ksz_ptp_enable(struct ptp_clock_info *ptp,
 			  struct ptp_clock_request *req, int on)
 {
@@ -1095,6 +1332,27 @@ static int ksz_ptp_start_clock(struct ksz_device *dev)
 	return 0;
 }
 
+void ksz8463_ptp_set_caps(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_ptp_data *ptp_data;
+
+	ptp_data = &dev->ptp_data;
+
+	ptp_data->caps.owner		= THIS_MODULE;
+	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
+	ptp_data->caps.max_adj		= KSZ_MAX_DRIFT_CORR;
+	ptp_data->caps.gettime64	= ksz_ptp_gettime;
+	ptp_data->caps.settime64	= ksz8463_ptp_settime;
+	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
+	ptp_data->caps.adjtime		= ksz8463_ptp_adjtime;
+	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
+	ptp_data->caps.enable		= ksz8463_ptp_enable;
+	ptp_data->caps.verify		= ksz_ptp_verify_pin;
+	ptp_data->caps.n_pins		= dev->info->n_pins;
+	ptp_data->caps.n_per_out	= dev->info->n_per_out;
+}
+
 void ksz_ptp_set_caps(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
index 89716b31596b..92589cf373ac 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -12,7 +12,7 @@
 
 #include <linux/ptp_clock_kernel.h>
 
-#define KSZ_PTP_N_GPIO		2
+#define KSZ_PTP_N_GPIO		12
 
 enum ksz_ptp_tou_mode {
 	KSZ_PTP_TOU_IDLE,
@@ -34,6 +34,7 @@ struct ksz_ptp_data {
 };
 
 void ksz_ptp_set_caps(struct dsa_switch *ds);
+void ksz8463_ptp_set_caps(struct dsa_switch *ds);
 int ksz_ptp_clock_register(struct dsa_switch *ds);
 
 void ksz_ptp_clock_unregister(struct dsa_switch *ds);
diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h b/drivers/net/dsa/microchip/ksz_ptp_reg.h
index 65ea8577af75..16f117fa5d49 100644
--- a/drivers/net/dsa/microchip/ksz_ptp_reg.h
+++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
@@ -51,6 +51,22 @@
 
 #define REG_PTP_UNIT_INDEX__4		0x0520
 
+#define KSZ8463_TOU_ACTIVE		0x202
+#define KSZ8463_TOU_EN			0x206
+#define KSZ8463_TOU_SW_RST		0x208
+
+#define KSZ8463_TRIGN_CFG_SIZE		0x20
+#define KSZ8463_TRIG1_CFG		0x220
+#define KSZ8463_TARGET_NSEC		0x000
+#define KSZ8463_TARGET_SEC		0x004
+#define KSZ8463_PATTERN_OFF		0x008
+#define KSZ8463_NOTIFY_BIT		BIT(8)
+#define KSZ8463_PATTERN_M		GENMASK(6, 4)
+#define KSZ8463_PULSE_WIDTH_OFF		0x00A
+#define KSZ8463_TRIG_PULSE_WIDTH_M	GENMASK(15, 0)
+#define KSZ8463_CYCLE_WIDTH_OFF		0x00C
+#define KSZ8463_CYCLE_CNT_OFF		0x010
+
 #define PTP_GPIO_INDEX			GENMASK(19, 16)
 #define PTP_TSI_INDEX			BIT(8)
 #define PTP_TOU_INDEX			GENMASK(1, 0)

-- 
2.55.0

Re: [net-next,10/10] net: dsa: microchip: add periodic output support for the KSZ8463

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 4 (1) · Low: 2

- [High] KSZ8463 advertises 12 periodic-output channels (.n_pins = 12 /
  .n_per_out = 12) but the driver keeps only one set of software state…
- [Medium] Nothing in the new KSZ8463 path ever clears the per-unit
  enable bit in KSZ8463_TOU_EN (0x206).
- [Medium] Build break with CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=n:
  ksz8463_setup() (drivers/net/dsa/microchip/ksz8.c:2481) now calls…
- [Medium] ksz8463_ptp_enable_perout() performs destructive actions
  before validating the request and never reconciles state on failure.
- [Medium] ksz8463_ptp_configure_perout() (ksz_ptp.c:184) directly
  dereferences dev->ptp_data.pin_config[index], reading pin->chan to…
- [Low] Stale comment in the shared helper
  ksz_ptp_compute_perout_pulse(): 'Use a duty cycle of 50%.
- [Low] ksz8463_ptp_set_caps() (ksz_ptp.c:1335) does not set
  caps.supported_perout_flags, yet the KSZ8463 perout implementation…

Pre-existing issues:
- [High] A non-normalized periodic-output period is accepted and stored
  as zero nanoseconds, and the restart path then divides by it.
- [Medium] ksz_ptp_prepare_restart_perout() stores the 64-bit quotient
  div_u64(now_ns - first_ns, period_ns) in an 'unsigned int count'…

--

net: dsa: microchip: add periodic output support for the KSZ8463

This adds KSZ8463-specific periodic output support to the microchip DSA
driver, with per-unit trigger configuration register blocks, a smaller
maximum pulse width, and 12 advertised outputs instead of 2.

A few questions on the new code below.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index f2a23e2e474ac..6b08bb8fed5b7 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -2478,7 +2478,7 @@ static int ksz8463_setup(struct dsa_switch *ds)
>  		if (ret)
>  			goto free_girq;
>  
> -		ksz_ptp_set_caps(ds);
> +		ksz8463_ptp_set_caps(ds);

[Severity: Medium]
Does this build with CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=n?

ksz8.o is built unconditionally, while the Makefile only adds ksz_ptp.o
inside "ifdef CONFIG_NET_DSA_MICROCHIP_KSZ_PTP".  In ksz_ptp.h, the new
declaration lives in the enabled branch:

  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
  ...
  void ksz8463_ptp_set_caps(struct dsa_switch *ds);

and the #else branch only provides the old stub:

  static inline void ksz_ptp_set_caps(struct dsa_switch *ds) { }

Every other KSZ8463-specific PTP entry point (ksz8463_ptp_irq_setup,
ksz8463_ptp_irq_free, ksz8463_get_ts_info, ksz8463_hwtstamp_set) does have
a stub or #define in the #else branch.  Should ksz8463_ptp_set_caps() get
one too, otherwise ksz8463_setup() is compiled with no declaration?

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 194c88cdd00bc..111f3014e5a97 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1161,6 +1161,8 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.supports_mii = {false, false, true},
>  		.supports_rmii = {false, false, true},
>  		.internal_phy = {true, true, false},
> +		.n_pins = 12,
> +		.n_per_out = 12,
>  	},

> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 0a77b46cfad25..d33034892a8f6 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c

[ ... ]

> @@ -63,6 +64,17 @@ static int ksz_ptp_tou_gpio(struct ksz_device *dev)
>  			 LED_SRC_PTP_GPIO_1 | LED_SRC_PTP_GPIO_2);
>  }
>  
> +static int ksz8463_ptp_tou_reset(struct ksz_device *dev, u8 unit)
> +{
> +	int ret;
> +
> +	ret = ksz_rmw16(dev, KSZ8463_TOU_SW_RST, BIT(unit), BIT(unit));
> +	if (ret)
> +		return ret;
> +
> +	return ksz_rmw16(dev, KSZ8463_TOU_SW_RST, BIT(unit), 0);
> +}
> +

[Severity: Medium]
Should this also clear BIT(unit) in KSZ8463_TOU_EN?

The generic counterpart just below deliberately ends by clearing
TRIG_ENABLE (and write-1-clears the per-unit status bits):

  /* Clear reset and set GPIO direction */
  return ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, (TRIG_RESET | TRIG_ENABLE),
                   0);

Here only KSZ8463_TOU_SW_RST is pulsed, and the disable path in
ksz8463_ptp_enable_perout() returns right after it:

  ret = ksz8463_ptp_tou_reset(dev, request->index);
  if (ret)
          return ret;

  if (!on) {
          ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
          return 0;
  }

If asserting the software reset bit does not itself clear TOU_EN, does the
unit keep driving the pin after userspace disabled it while the driver
reports KSZ_PTP_TOU_IDLE?

There is a second consequence on the re-arm path, see the note on
ksz8463_ptp_tou_start() below.

>  static int ksz_ptp_tou_reset(struct ksz_device *dev, u8 unit)
>  {
>  	u32 data;

[ ... ]

> @@ -120,6 +132,28 @@ static int ksz_ptp_tou_target_time_set(struct ksz_device *dev,
>  	return 0;
>  }
>  
> +static int ksz8463_ptp_tou_start(struct ksz_device *dev, u8 unit)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = ksz_rmw16(dev, KSZ8463_TOU_EN, BIT(unit), BIT(unit));
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_read16(dev, KSZ8463_TOU_ACTIVE, &data);
> +	if (ret)
> +		return ret;
> +
> +	if (!(data & BIT(unit))) {
> +		dev_err(dev->dev, "%s: Trigger unit%d error!\n", __func__,
> +			unit);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

[Severity: Medium]
Continuing the note above: if BIT(unit) is still set in KSZ8463_TOU_EN when
the restart path reaches here, does ksz_rmw16() actually emit a write?

The KSZ8463 regmap config uses .cache_type = REGCACHE_NONE with no
reg_update_bits, and _regmap_update_bits() only writes when the value
changes:

  drivers/base/regmap/regmap.c:_regmap_update_bits() {
          ...
          if (force_write || (tmp != orig) || map->force_write_field) {
                  ret = _regmap_write(map, reg, tmp);
          ...
  }

So no 0->1 edge would be generated to latch the freshly written
TARGET_SEC/TARGET_NSEC, and the KSZ8463_TOU_ACTIVE check right after would
pass on the stale ACTIVE bit, making
ksz8463_ptp_settime()/ksz8463_ptp_adjtime() return success while the unit
still runs against the pre-step target time.

Also on the -EIO exit here, BIT(unit) is left set in KSZ8463_TOU_EN while
tou_mode is never advanced to KSZ_PTP_TOU_PEROUT.  Should the enable bit be
cleared before returning the error?

>  static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
>  {
>  	u32 data;
> @@ -147,6 +181,56 @@ static int ksz_ptp_tou_start(struct ksz_device *dev, u8 unit)
>  	return 0;
>  }
>  
> +static int ksz8463_ptp_configure_perout(struct ksz_device *dev,
> +					u32 cycle_width_ns, u32 pulse_width_ns,
> +					struct timespec64 const *target_time,
> +					u8 index)
> +{
> +	struct ptp_pin_desc *pin = &dev->ptp_data.pin_config[index];
> +	u16 cfg_base = KSZ8463_TRIG1_CFG + KSZ8463_TRIGN_CFG_SIZE * pin->chan;

[Severity: Medium]
Is pin_config[] safe to dereference here on the settime/adjtime path?

pin_config[] is owned by the PTP core and protected by
ptp_clock::pincfg_mux, which the core holds only around ->enable():

  drivers/ptp/ptp_chardev.c:ptp_perout_request() {
          ...
          scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &ptp->pincfg_mux)
                  return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
  }

The new path ksz8463_ptp_settime()/ksz8463_ptp_adjtime() ->
ksz8463_ptp_restart_perout() -> ksz8463_ptp_enable_perout() ->
ksz8463_ptp_configure_perout() only holds the driver-private
ptp_data->lock.  Can a concurrent PTP_PIN_SETFUNC change pin->chan
underneath, so that cfg_base points at a different trigger unit than
BIT(request->index) which ksz8463_ptp_tou_start() enables?

Note that simply taking pincfg_mux here would invert the order used by the
->enable() path (pincfg_mux then ptp_data->lock); caching pin/chan under
pincfg_mux at enable() time would avoid that.

> +	u16 data;
> +	int ret;
> +
> +	/* Hardware has only 32 bit */
> +	if ((target_time->tv_sec & 0xffffffff) != target_time->tv_sec)
> +		return -EINVAL;
> +
> +	data = KSZ8463_NOTIFY_BIT |
> +	       FIELD_PREP(KSZ8463_PATTERN_M, TRIG_POS_PERIOD) |
> +	       pin->index;
> +	ret = ksz_write16(dev, cfg_base + KSZ8463_PATTERN_OFF, data);
> +	if (ret)
> +		return ret;

[ ... ]

> +	return 0;
> +}
> +
>  static int ksz_ptp_configure_perout(struct ksz_device *dev,
>  				    u32 cycle_width_ns, u32 pulse_width_ns,
>  				    struct timespec64 const *target_time,

[ ... ]

> @@ -241,6 +325,58 @@ static u64 ksz_ptp_compute_perout_pulse(struct ksz_device *dev,
>  	return min_t(u64, req_pulse_width_ns, max_pulse_width);
>  }
>  
> +static int ksz8463_ptp_enable_perout(struct ksz_device *dev,
> +				     struct ptp_perout_request const *request,
> +				     int on)
> +{
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	u64 cycle_width_ns;
> +	u64 pulse_width_ns;
> +	int pin;
> +	int ret;
> +
> +	pin = ksz_ptp_get_pin(dev, request);
> +	if (pin < 0)
> +		return pin;
> +
> +	ret = ksz8463_ptp_tou_reset(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	if (!on) {
> +		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
> +		return 0;
> +	}
> +	ret = ksz_ptp_compute_perout_cycle(dev, request, &cycle_width_ns);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Can a rejected request leave the driver and hardware out of sync here?

The unit is reset before any of the timing values are validated, and
ksz_ptp_compute_perout_cycle() stores the requested period before deciding
it is out of range:

  ptp_data->perout_period.tv_sec = request->period.sec;
  ptp_data->perout_period.tv_nsec = request->period.nsec;

  *cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
  if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {
          *cycle_width_ns = 0;
          return -EINVAL;
  }

So with a perout already active (tou_mode == KSZ_PTP_TOU_PEROUT), a
PTP_PEROUT_REQUEST with an out-of-range period stops the running unit,
overwrites perout_period with the rejected value, and returns while
tou_mode still says PEROUT.  Every later
ksz8463_ptp_settime()/ksz8463_ptp_adjtime() then rebuilds the request from
that stored period and fails again, after the RTC has already been
written.  An SPI error in one of the six writes in
ksz8463_ptp_configure_perout() gives the same half-programmed state.
Should validation happen before the unit reset and before the state is
committed?

> +	pulse_width_ns = ksz_ptp_compute_perout_pulse(dev, request,
> +						      KSZ8463_MAX_PULSE_WIDTH);

[Severity: Low]
This isn't a bug, but the comment in ksz_ptp_compute_perout_pulse() is now
stale for this caller:

  /* Use a duty cycle of 50%. Maximum pulse width supported by the
   * hardware is a little bit more than 125 ms.
   */

The maximum is a parameter now, and this call passes
KSZ8463_MAX_PULSE_WIDTH = 500000LL, i.e. 0.5 ms.  Could the comment be
made generic or moved to the callers?

> +
> +	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns,
> +				       KSZ8463_TRIG_PULSE_WIDTH_M);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz8463_ptp_configure_perout(dev, cycle_width_ns, pulse_width_ns,
> +					   &ptp_data->perout_target_time_first,
> +					   pin);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz_ptp_tou_gpio(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz8463_ptp_tou_start(dev, request->index);
> +	if (ret)
> +		return ret;
> +
> +	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
> +
> +	return 0;
> +}
> +
>  static int ksz_ptp_enable_perout(struct ksz_device *dev,
>  				 struct ptp_perout_request const *request,
>  				 int on)

[ ... ]

> @@ -845,6 +981,18 @@ static int ksz_ptp_restart_perout(struct ksz_device *dev)
>  	return ksz_ptp_enable_perout(dev, &request, 1);
>  }
>  
> +static int ksz8463_ptp_restart_perout(struct ksz_device *dev)
> +{
> +	struct ptp_perout_request request;
> +	int ret;
> +
> +	ret = ksz_ptp_prepare_restart_perout(dev, &request);
> +	if (ret)
> +		return ret;
> +
> +	return ksz8463_ptp_enable_perout(dev, &request, 1);
> +}
> +

[Severity: High]
Does this work for channels other than 0?

ksz_ptp_prepare_restart_perout() hardcodes the channel and drops the flags:

  request->index = 0;
  request->flags = 0;

but this chip now advertises .n_pins = 12 and .n_per_out = 12, and
struct ksz_ptp_data keeps a single tou_mode / perout_target_time_first /
perout_period for all of them.

With a perout armed on, say, channel 3 (PTP_PIN_SETFUNC accepts it,
ksz_ptp_verify_pin() only filters on func and the core only checks
chan < n_per_out), the restart path becomes:

  ksz8463_ptp_adjtime()
    ksz8463_ptp_restart_perout()
      ksz_ptp_prepare_restart_perout()   /* index = 0 */
        ksz8463_ptp_enable_perout()
          ksz_ptp_get_pin()
            ptp_find_pin(clock, PTP_PF_PEROUT, 0)   /* no such pin */

so -EINVAL is returned, and this happens after __ksz_ptp_adjtime() has
already stepped the RTC:

  ret = __ksz_ptp_adjtime(dev, delta);
  if (ret)
          goto unlock;
  ...
  case KSZ_PTP_TOU_PEROUT:
          ret = ksz8463_ptp_restart_perout(dev);
          if (ret)
                  goto unlock;

The "goto unlock" also skips the ptp_data->clock_time update.  Since
ptp4l/phc2sys call adjtime/adjfine continuously, would every PHC adjust
fail for 11 of the 12 channels the commit advertises?  And if some pin does
happen to be mapped to channel 0, unit 0 gets reprogrammed with whatever
timing is stored in the single set of fields while the genuinely active
unit keeps its pre-step target.

Related to the single set of state fields: units 1..11 are never re-armed,
"request->flags = 0" discards PTP_PEROUT_DUTY_CYCLE on every restart,
enabling a second channel overwrites the first channel's stored timing, and
disabling any one channel sets the global tou_mode to KSZ_PTP_TOU_IDLE
while the other units stay enabled in hardware.  Should the per-unit state
be tracked per channel, and should the restart carry the real index?

[Severity: High]
This isn't a bug introduced by this patch, but the new caller of
ksz_ptp_prepare_restart_perout() adds a second path to it, so it is worth
raising: can a stored period of zero nanoseconds divide by zero here?

struct ptp_clock_time has a signed sec and an unsigned nsec, and
ptp_perout_request() validates flags, reserved fields, duty
cycle/phase and "index < n_per_out", but never normalization or
positivity; it decides "on" from "period.sec || period.nsec".  A period of
{sec = -1, nsec = 1000000000} is therefore treated as an enable, while
timespec64_to_ns() evaluates it to 0, which passes both checks:

  *cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
  if ((*cycle_width_ns & TRIG_CYCLE_WIDTH_M) != *cycle_width_ns) {

and ksz_ptp_tou_pulse_verify(0) passes too.  A later settime/adjtime then
reaches:

  period_ns = timespec64_to_ns(&ptp_data->perout_period);

  if (first_ns < now_ns) {
          count = div_u64(now_ns - first_ns, period_ns);
  ...
  while (next_ns < now_ns + 100000000)
          next_ns += period_ns;

which is either a divide-error with a zero divisor, or a loop that never
advances, both with ptp_data->lock held.  Should non-normalized times be
rejected and the converted period required to be strictly positive before
it is stored?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but this new
function adds another caller: ksz_ptp_prepare_restart_perout() stores the
64-bit quotient in an unsigned int:

  unsigned int count;
  ...
  if (first_ns < now_ns) {
          count = div_u64(now_ns - first_ns, period_ns);
          next_ns = first_ns + count * period_ns;
  ...
  /* Ensure 100 ms guard time prior next event */
  while (next_ns < now_ns + 100000000)
          next_ns += period_ns;

Once more than UINT_MAX periods have elapsed (about 72 minutes with a 1 us
period, about 50 days with 1 ms), count truncates and the guard-time loop
has to catch up one period at a time, up to roughly 4.29e9 iterations with
ptp_data->lock held.  Would a u64 count avoid that stall?

>  static int __ksz_ptp_settime(struct ksz_device *dev,
>  			     const struct timespec64 *ts)
>  {

[ ... ]

> @@ -1095,6 +1332,27 @@ static int ksz_ptp_start_clock(struct ksz_device *dev)
>  	return 0;
>  }
>  
> +void ksz8463_ptp_set_caps(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_ptp_data *ptp_data;
> +
> +	ptp_data = &dev->ptp_data;
> +
> +	ptp_data->caps.owner		= THIS_MODULE;
> +	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
> +	ptp_data->caps.max_adj		= KSZ_MAX_DRIFT_CORR;
> +	ptp_data->caps.gettime64	= ksz_ptp_gettime;
> +	ptp_data->caps.settime64	= ksz8463_ptp_settime;
> +	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
> +	ptp_data->caps.adjtime		= ksz8463_ptp_adjtime;
> +	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
> +	ptp_data->caps.enable		= ksz8463_ptp_enable;
> +	ptp_data->caps.verify		= ksz_ptp_verify_pin;
> +	ptp_data->caps.n_pins		= dev->info->n_pins;
> +	ptp_data->caps.n_per_out	= dev->info->n_per_out;
> +}
> +

[Severity: Low]
Should caps.supported_perout_flags be set to PTP_PEROUT_DUTY_CYCLE here?

The implementation accepts that flag:

  ksz_ptp_get_pin():
          if (request->flags & ~PTP_PEROUT_DUTY_CYCLE)
                  return -EOPNOTSUPP;

and ksz_ptp_compute_perout_pulse() consumes request->on when it is set, but
the core rejects it first:

  drivers/ptp/ptp_chardev.c:ptp_perout_request() {
          ...
          if (perout->flags & ~ops->supported_perout_flags)
                  return -EOPNOTSUPP;
  }

so with supported_perout_flags left at 0 the duty-cycle handling is
unreachable.  The same omission exists in ksz_ptp_set_caps().

>  void ksz_ptp_set_caps(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
> index 89716b31596b5..92589cf373ac2 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.h
> +++ b/drivers/net/dsa/microchip/ksz_ptp.h
> @@ -12,7 +12,7 @@
>  
>  #include <linux/ptp_clock_kernel.h>
>  
> -#define KSZ_PTP_N_GPIO		2
> +#define KSZ_PTP_N_GPIO		12
>  
>  enum ksz_ptp_tou_mode {
>  	KSZ_PTP_TOU_IDLE,
> @@ -34,6 +34,7 @@ struct ksz_ptp_data {
>  };
>  
>  void ksz_ptp_set_caps(struct dsa_switch *ds);
> +void ksz8463_ptp_set_caps(struct dsa_switch *ds);
>  int ksz_ptp_clock_register(struct dsa_switch *ds);
>  
>  void ksz_ptp_clock_unregister(struct dsa_switch *ds);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz-perout-v1-0-14202db763b3%40bootlin.com