| Message ID | 20231010-stmmac_fix_auxiliary_event_capture-v1-2-3eeca9e844fa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: fix PPS input indexing | expand |
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. ...
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
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
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
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 */
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(-)