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 <[email protected]>
To: Jose Abreu <[email protected]>
To: "David S. Miller" <[email protected]>
To: Eric Dumazet <[email protected]>
To: Jakub Kicinski <[email protected]>
To: Paolo Abeni <[email protected]>
To: Maxime Coquelin <[email protected]>
To: Richard Cochran <[email protected]>
To: Kurt Kanzenbach <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Johannes Zink <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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.
...
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 <[email protected]>
>
> 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 |
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.
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 |
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.
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 |