[PATCH net-next 0/5] net: stmmac: fix PPS input indexing

[PATCH net-next 0/5] net: stmmac: fix PPS input indexing

[Johannes Zink]

The stmmac can have 0 to 4 auxiliary snapshot in channels, which can be
used for capturing external triggers with respect to the eqos PTP timer.

Previously when enabling the auxiliary snapshot, an invalid request was
written to the hardware register, except for the Intel variant of this
driver, where the only snapshot available was hardcoded.

Patch 1 of this series cleans up the debug netdev_dbg message indicating
the auxiliary snapshot being {en,dis}abled. No functional changes here

Patch 2 of this series fixes the PPS input indexing

Patch 3 of this series removes a field member from plat_stmmacnet_data
that is no longer needed

Patch 4 of this series prepares Patch 5 by protecting the snapshot
enabled flag by the aux_ts_lock mutex

Patch 5 of this series adds a temporary workaround, since at the moment
the driver can handle only one single auxiliary snapshot at a time.
Previously the driver silently dropped the previous configuration and
enabled the new one. Now, if a snapshot is already enabled and userspace
tries to enable another without previously disabling the snapshot currently
enabled: issue a netdev_err and return an errorcode indicating the device is
busy.

Best Regards
Johannes

To: Alexandre Torgue <alexandre.torgue@foss.st.com>
To: Jose Abreu <joabreu@synopsys.com>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Maxime Coquelin <mcoquelin.stm32@gmail.com>
To: Richard Cochran <richardcochran@gmail.com>
To: Kurt Kanzenbach <kurt@linutronix.de>
Cc: patchwork-jzi@pengutronix.de
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel@pengutronix.de

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
Johannes Zink (5):
      net: stmmac: simplify debug message on stmmac_enable()
      net: stmmac: fix PPS capture input index
      net: stmmac: intel: remove unnecessary field struct plat_stmmacenet_data::ext_snapshot_num
      net: stmmac: ptp: stmmac_enable(): move change of plat->flags into mutex
      net: stmmac: do not silently change auxiliary snapshot capture channel

 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c |  1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 32 ++++++++++++++---------
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h  |  2 +-
 include/linux/stmmac.h                            |  1 -
 4 files changed, 21 insertions(+), 15 deletions(-)
---
base-commit: 21b2e2624d2ec69b831cd2edd202ca30ac6beae1
change-id: 20231010-stmmac_fix_auxiliary_event_capture-eaf21ea9c9fe

Best regards,
-- 
Johannes Zink <j.zink@pengutronix.de>

[PATCH net-next 1/5] net: stmmac: simplify debug message on stmmac_enable()

[Johannes Zink]

Simplify the netdev_dbg() call in stmmac_enable() in order to reduce code
duplication. No functional change.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 3d7825cb30bb..29569188b30c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -203,14 +203,10 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 			/* Enable External snapshot trigger */
 			acr_value |= priv->plat->ext_snapshot_num;
 			acr_value |= PTP_ACR_ATSFC;
-			netdev_dbg(priv->dev, "Auxiliary Snapshot %d enabled.\n",
-				   priv->plat->ext_snapshot_num >>
-				   PTP_ACR_ATSEN_SHIFT);
-		} else {
-			netdev_dbg(priv->dev, "Auxiliary Snapshot %d disabled.\n",
-				   priv->plat->ext_snapshot_num >>
-				   PTP_ACR_ATSEN_SHIFT);
 		}
+		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
+			   priv->plat->ext_snapshot_num >> PTP_ACR_ATSEN_SHIFT,
+			   on ? "enabled" : "disabled");
 		writel(acr_value, ptpaddr + PTP_ACR);
 		mutex_unlock(&priv->aux_ts_lock);
 		/* wait for auxts fifo clear to finish */

-- 
2.39.2

[PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Johannes Zink]

The stmmac supports up to 4 auxiliary snapshots that can be enabled by
setting the appropriate bits in the PTP_ACR bitfield.

Previously instead of setting the bits, a fixed value was written to
this bitfield instead of passing the appropriate bitmask.

Now the correct bit is set according to the ptp_clock_request.extts_index
passed as a parameter to stmmac_enable().

Fixes: f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 5 ++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 29569188b30c..60e3d3ff42f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -201,12 +201,11 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 		acr_value &= ~PTP_ACR_MASK;
 		if (on) {
 			/* Enable External snapshot trigger */
-			acr_value |= priv->plat->ext_snapshot_num;
+			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
 			acr_value |= PTP_ACR_ATSFC;
 		}
 		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
-			   priv->plat->ext_snapshot_num >> PTP_ACR_ATSEN_SHIFT,
-			   on ? "enabled" : "disabled");
+			   rq->extts.index, on ? "enabled" : "disabled");
 		writel(acr_value, ptpaddr + PTP_ACR);
 		mutex_unlock(&priv->aux_ts_lock);
 		/* wait for auxts fifo clear to finish */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
index d1fe4b46f162..fce3fba2ffd2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h
@@ -79,7 +79,7 @@
 #define	PTP_ACR_ATSEN1		BIT(5)	/* Auxiliary Snapshot 1 Enable */
 #define	PTP_ACR_ATSEN2		BIT(6)	/* Auxiliary Snapshot 2 Enable */
 #define	PTP_ACR_ATSEN3		BIT(7)	/* Auxiliary Snapshot 3 Enable */
-#define	PTP_ACR_ATSEN_SHIFT	5	/* Auxiliary Snapshot shift */
+#define	PTP_ACR_ATSEN(index)	(PTP_ACR_ATSEN0 << (index))
 #define	PTP_ACR_MASK		GENMASK(7, 4)	/* Aux Snapshot Mask */
 #define	PMC_ART_VALUE0		0x01	/* PMC_ART[15:0] timer value */
 #define	PMC_ART_VALUE1		0x02	/* PMC_ART[31:16] timer value */

-- 
2.39.2

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Simon Horman]

On Thu, Oct 12, 2023 at 11:02:13AM +0200, Johannes Zink wrote:
> The stmmac supports up to 4 auxiliary snapshots that can be enabled by
> setting the appropriate bits in the PTP_ACR bitfield.
> 
> Previously instead of setting the bits, a fixed value was written to
> this bitfield instead of passing the appropriate bitmask.
> 
> Now the correct bit is set according to the ptp_clock_request.extts_index
> passed as a parameter to stmmac_enable().
> 
> Fixes: f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>

Hi Johannes,

The fix language of the subject and presence of a fixes tag implies that
this is a bug fix. But it's not clear to me that this is resolving
bug that manifests as a problem.

If it is a bug fix then it should probably be targeted at 'net',
creating a dependency for the remainder of this series.

On the other hand, if it is not a bug fix then perhaps it is best to
update the subject and drop the Fixes tag.

I'm no expert on stmmac, but the rest of the series looks good to me.

...

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Johannes Zink]

Hi Simon,

On 10/14/23 16:44, Simon Horman wrote:
> On Thu, Oct 12, 2023 at 11:02:13AM +0200, Johannes Zink wrote:
>> The stmmac supports up to 4 auxiliary snapshots that can be enabled by
>> setting the appropriate bits in the PTP_ACR bitfield.
>>
>> Previously instead of setting the bits, a fixed value was written to
>> this bitfield instead of passing the appropriate bitmask.
>>
>> Now the correct bit is set according to the ptp_clock_request.extts_index
>> passed as a parameter to stmmac_enable().
>>
>> Fixes: f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
>> Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> 
> Hi Johannes,
> 
> The fix language of the subject and presence of a fixes tag implies that
> this is a bug fix. But it's not clear to me that this is resolving
> bug that manifests as a problem.

Thank you for taking your time to read through the series. This series is 
somewhere in the realm between "fixing some stuff added previously (and never 
worked)" and "filling the gaps/adding a new feature in some template code that 
never worked as intended". However, I do not have strong opinions about this.

If you prefer to have the commits reworded, I will just wait a bit more for any 
additional feedback and resend the series with the commit messages reworded+ 
fixes, should any be required.

> 
> If it is a bug fix then it should probably be targeted at 'net',
> creating a dependency for the remainder of this series.
> 
> On the other hand, if it is not a bug fix then perhaps it is best to
> update the subject and drop the Fixes tag.

I added the fixes-Tag in order to make code archeology easier, but as it may 
trigger picks to stable branches (which is not required imho), I have no 
objections to dropping it for a v2.

> 
> I'm no expert on stmmac, but the rest of the series looks good to me.
> 
> ...
> 

that's good news. thx for looking through the series.

Best regards
Johannes


-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Jakub Kicinski]

On Tue, 17 Oct 2023 11:12:53 +0200 Johannes Zink wrote:
> > If it is a bug fix then it should probably be targeted at 'net',
> > creating a dependency for the remainder of this series.
> > 
> > On the other hand, if it is not a bug fix then perhaps it is best to
> > update the subject and drop the Fixes tag.  
> 
> I added the fixes-Tag in order to make code archeology easier, but as it may 
> trigger picks to stable branches (which is not required imho), I have no 
> objections to dropping it for a v2.

Would be good to clarify what impact on device operation the problem
has. How would end user notice the problem?
Does it mean snapshots were always or never enabled, previously?

Note that if you submit this fix for net today it will still make it 
to -rc7 and net-next by tomorrow, so no major delay. We merge the trees
on Thursday, usually.

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On 17.10.2023 08:26:18, Jakub Kicinski wrote:
> On Tue, 17 Oct 2023 11:12:53 +0200 Johannes Zink wrote:
> > > If it is a bug fix then it should probably be targeted at 'net',
> > > creating a dependency for the remainder of this series.
> > > 
> > > On the other hand, if it is not a bug fix then perhaps it is best to
> > > update the subject and drop the Fixes tag.  
> > 
> > I added the fixes-Tag in order to make code archeology easier, but as it may 
> > trigger picks to stable branches (which is not required imho), I have no 
> > objections to dropping it for a v2.
> 
> Would be good to clarify what impact on device operation the problem
> has. How would end user notice the problem?
> Does it mean snapshots were always or never enabled, previously?

On all dwmac devices not covered by dwmac-intel.c (INTEL 10/100/1000
Ethernet PCI driver), PPS capture can be requested from user-space, but
is not enabled in HW. There is no error message or other feedback to the
user space. The user space will not get any PPS events.

As this change also affects the Intel driver, and we don't have any
hardware to test, I think it's better that this goes via net-next to
give it a bit more time of testing.

> Note that if you submit this fix for net today it will still make it 
> to -rc7 and net-next by tomorrow, so no major delay. We merge the trees
> on Thursday, usually.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Jakub Kicinski]

On Tue, 17 Oct 2023 22:27:41 +0200 Marc Kleine-Budde wrote:
> > Would be good to clarify what impact on device operation the problem
> > has. How would end user notice the problem?
> > Does it mean snapshots were always or never enabled, previously?  
> 
> On all dwmac devices not covered by dwmac-intel.c (INTEL 10/100/1000
> Ethernet PCI driver), PPS capture can be requested from user-space, but
> is not enabled in HW. There is no error message or other feedback to the
> user space. The user space will not get any PPS events.
> 
> As this change also affects the Intel driver, and we don't have any
> hardware to test, I think it's better that this goes via net-next to
> give it a bit more time of testing.

SGTM, we can chalk it up to "never worked, doesn't hurt anyone"
and put it in net-next. But then the Fixes tag must go.

Re: [PATCH net-next 2/5] net: stmmac: fix PPS capture input index

[Johannes Zink]

Hi Jakub, hi Marc,

On 10/18/23 01:50, Jakub Kicinski wrote:
> On Tue, 17 Oct 2023 22:27:41 +0200 Marc Kleine-Budde wrote:
>>> Would be good to clarify what impact on device operation the problem
>>> has. How would end user notice the problem?
>>> Does it mean snapshots were always or never enabled, previously?
>>
>> On all dwmac devices not covered by dwmac-intel.c (INTEL 10/100/1000
>> Ethernet PCI driver), PPS capture can be requested from user-space, but
>> is not enabled in HW. There is no error message or other feedback to the
>> user space. The user space will not get any PPS events.
>>
>> As this change also affects the Intel driver, and we don't have any
>> hardware to test, I think it's better that this goes via net-next to
>> give it a bit more time of testing.

I have also CC'ed Kurt in this series, as I know he has at least some hardware 
at hand, though I cannot tell whether he has any chance to test the PPS 
capture. Maybe he has a possibility to try it out. However, giving it a spin in 
net-next SGTM.

> 
> SGTM, we can chalk it up to "never worked, doesn't hurt anyone"
> and put it in net-next. But then the Fixes tag must go.
> 

sure, that's fine for me. I will reword the commit messages and send a v2.

Best regards,
Johannes

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

[PATCH net-next 3/5] net: stmmac: intel: remove unnecessary field struct plat_stmmacenet_data::ext_snapshot_num

[Johannes Zink]

Do not store bitmask for enabling AUX_SNAPSHOT0. The previous commit
("net: stmmac: fix PPS capture input index") takes care of calculating
the proper bit mask from the request data's extts.index field, which is
0 if not explicitly specified otherwise.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 -
 include/linux/stmmac.h                            | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index a3a249c63598..60283543ffc8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -605,7 +605,6 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
 	plat->mdio_bus_data->phy_mask |= 1 << INTEL_MGBE_XPCS_ADDR;
 
 	plat->int_snapshot_num = AUX_SNAPSHOT1;
-	plat->ext_snapshot_num = AUX_SNAPSHOT0;
 
 	plat->crosststamp = intel_crosststamp;
 	plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index c0079a7574ae..0b4658a7eceb 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -303,7 +303,6 @@ struct plat_stmmacenet_data {
 	unsigned int eee_usecs_rate;
 	struct pci_dev *pdev;
 	int int_snapshot_num;
-	int ext_snapshot_num;
 	int msi_mac_vec;
 	int msi_wol_vec;
 	int msi_lpi_vec;

-- 
2.39.2

[PATCH net-next 4/5] net: stmmac: ptp: stmmac_enable(): move change of plat->flags into mutex

[Johannes Zink]

This is a preparation patch. The next patch will check if an external TS
is active and return with an error. So we have to move the change of the
plat->flags that tracks if external timestamping is enabled after that
check.

Prepare for this change and move the plat->flags change into the mutex
and the if (on).

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 60e3d3ff42f3..2a141db70c2e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -192,17 +192,17 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 		write_unlock_irqrestore(&priv->ptp_lock, flags);
 		break;
 	case PTP_CLK_REQ_EXTTS:
-		if (on)
-			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
-		else
-			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
 		mutex_lock(&priv->aux_ts_lock);
 		acr_value = readl(ptpaddr + PTP_ACR);
 		acr_value &= ~PTP_ACR_MASK;
 		if (on) {
+			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
+
 			/* Enable External snapshot trigger */
 			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
 			acr_value |= PTP_ACR_ATSFC;
+		} else {
+			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
 		}
 		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
 			   rq->extts.index, on ? "enabled" : "disabled");

-- 
2.39.2

[PATCH net-next 5/5] net: stmmac: do not silently change auxiliary snapshot capture channel

[Johannes Zink]

Even though the hardware theoretically supports up to 4 simultaneous
auxiliary snapshot capture channels, the stmmac driver does support only
a single channel to be active at a time.

Previously in case of a PTP_CLK_REQ_EXTTS request, previously active
auxiliary snapshot capture channels were silently dropped and the new
channel was activated.

Instead of silently changing the state for all consumers, log an error
and return -EBUSY if a channel is already in use in order to signal to
userspace to disable the currently active channel before enabling another one.

Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 2a141db70c2e..56683afc650c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -191,11 +191,23 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 					     priv->systime_flags);
 		write_unlock_irqrestore(&priv->ptp_lock, flags);
 		break;
-	case PTP_CLK_REQ_EXTTS:
+	case PTP_CLK_REQ_EXTTS: {
+		u8 channel;
+
 		mutex_lock(&priv->aux_ts_lock);
 		acr_value = readl(ptpaddr + PTP_ACR);
+		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
 		acr_value &= ~PTP_ACR_MASK;
+
 		if (on) {
+			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
+				netdev_err(priv->dev,
+					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
+					rq->extts.index, channel);
+				mutex_unlock(&priv->aux_ts_lock);
+				return -EBUSY;
+			}
+
 			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
 
 			/* Enable External snapshot trigger */
@@ -213,6 +225,7 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
 					 !(acr_value & PTP_ACR_ATSFC),
 					 10, 10000);
 		break;
+	}
 
 	default:
 		break;

-- 
2.39.2