| Message ID | 20241005121411.583121-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it | expand |
On 05/10/2024 13:14, Niklas Söderlund wrote: > Recent work moving the reporting of Rx software timestamps to the core > [1] highlighted an issue where hardware time stamping where advertised > for the platforms where it is not supported. > > Fix this by covering advertising support for hardware timestamps only if > the hardware supports it. Due to the Tx implementation in RAVB software > Tx timestamping is also only considered if the hardware supports > hardware timestamps. This should be addressed in future, but this fix > only reflects what the driver currently implements. > > 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") > > Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> On RZ/G2UL, which doesn't support HW timestamping, before this patch: $ ethtool -T eth0 Time stamping parameters for eth0: Capabilities: hardware-transmit software-transmit hardware-receive software-receive software-system-clock hardware-raw-clock PTP Hardware Clock: 0 Hardware Transmit Timestamp Modes: off on Hardware Receive Filter Modes: none all ptpv2-l2-event After this patch: $ ethtool -T eth0 Time stamping parameters for eth0: Capabilities: software-receive software-system-clock PTP Hardware Clock: none Hardware Transmit Timestamp Modes: none Hardware Receive Filter Modes: none Thanks Niklas! Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com> Tested-by: Paul Barker <paul.barker.ct@bp.renesas.com>
On 10/5/24 15:14, Niklas Söderlund wrote: > Recent work moving the reporting of Rx software timestamps to the core > [1] highlighted an issue where hardware time stamping where advertised > for the platforms where it is not supported. > > Fix this by covering advertising support for hardware timestamps only if > the hardware supports it. Due to the Tx implementation in RAVB software > Tx timestamping is also only considered if the hardware supports > hardware timestamps. This should be addressed in future, but this fix > only reflects what the driver currently implements. > > 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") > > Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> [...] Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index d2a6518532f3..907af4651c55 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev, > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *hw_info = priv->info; > > - info->so_timestamping = > - SOF_TIMESTAMPING_TX_SOFTWARE | > - SOF_TIMESTAMPING_TX_HARDWARE | > - SOF_TIMESTAMPING_RX_HARDWARE | > - SOF_TIMESTAMPING_RAW_HARDWARE; > - info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); > - info->rx_filters = > - (1 << HWTSTAMP_FILTER_NONE) | > - (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > - (1 << HWTSTAMP_FILTER_ALL); > - if (hw_info->gptp || hw_info->ccc_gac) > + if (hw_info->gptp || hw_info->ccc_gac) { > + info->so_timestamping = > + SOF_TIMESTAMPING_TX_SOFTWARE | > + SOF_TIMESTAMPING_TX_HARDWARE | > + SOF_TIMESTAMPING_RX_HARDWARE | > + SOF_TIMESTAMPING_RAW_HARDWARE; > + info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); > + info->rx_filters = > + (1 << HWTSTAMP_FILTER_NONE) | > + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > + (1 << HWTSTAMP_FILTER_ALL); > info->phc_index = ptp_clock_index(priv->ptp.clock); > - else > - info->phc_index = 0; Is it OK to remove this line? > + } [...] MBR, Sergey
On 10/7/24 10:05 PM, Sergey Shtylyov wrote: [...] >> Recent work moving the reporting of Rx software timestamps to the core >> [1] highlighted an issue where hardware time stamping where advertised >> for the platforms where it is not supported. >> >> Fix this by covering advertising support for hardware timestamps only if >> the hardware supports it. Due to the Tx implementation in RAVB software >> Tx timestamping is also only considered if the hardware supports >> hardware timestamps. This should be addressed in future, but this fix >> only reflects what the driver currently implements. >> >> 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") >> >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > [...] > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index d2a6518532f3..907af4651c55 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev, >> struct ravb_private *priv = netdev_priv(ndev); >> const struct ravb_hw_info *hw_info = priv->info; >> >> - info->so_timestamping = >> - SOF_TIMESTAMPING_TX_SOFTWARE | >> - SOF_TIMESTAMPING_TX_HARDWARE | >> - SOF_TIMESTAMPING_RX_HARDWARE | >> - SOF_TIMESTAMPING_RAW_HARDWARE; >> - info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); >> - info->rx_filters = >> - (1 << HWTSTAMP_FILTER_NONE) | >> - (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | >> - (1 << HWTSTAMP_FILTER_ALL); >> - if (hw_info->gptp || hw_info->ccc_gac) >> + if (hw_info->gptp || hw_info->ccc_gac) { >> + info->so_timestamping = >> + SOF_TIMESTAMPING_TX_SOFTWARE | >> + SOF_TIMESTAMPING_TX_HARDWARE | >> + SOF_TIMESTAMPING_RX_HARDWARE | >> + SOF_TIMESTAMPING_RAW_HARDWARE; >> + info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); >> + info->rx_filters = >> + (1 << HWTSTAMP_FILTER_NONE) | >> + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | >> + (1 << HWTSTAMP_FILTER_ALL); >> info->phc_index = ptp_clock_index(priv->ptp.clock); >> - else >> - info->phc_index = 0; > > Is it OK to remove this line? Also, how about inverting the *if* condition above (and doing an early *return*) and avoiding reindenting the code below it? [...] MBR, Sergey
Hi Sergey, Sorry for missing your comment earlier. On 2024-10-08 20:26:21 +0300, Sergey Shtylyov wrote: > On 10/7/24 10:05 PM, Sergey Shtylyov wrote: > [...] > > >> Recent work moving the reporting of Rx software timestamps to the core > >> [1] highlighted an issue where hardware time stamping where advertised > >> for the platforms where it is not supported. > >> > >> Fix this by covering advertising support for hardware timestamps only if > >> the hardware supports it. Due to the Tx implementation in RAVB software > >> Tx timestamping is also only considered if the hardware supports > >> hardware timestamps. This should be addressed in future, but this fix > >> only reflects what the driver currently implements. > >> > >> 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") > >> > >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") > >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > [...] > > > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > >> index d2a6518532f3..907af4651c55 100644 > >> --- a/drivers/net/ethernet/renesas/ravb_main.c > >> +++ b/drivers/net/ethernet/renesas/ravb_main.c > >> @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev, > >> struct ravb_private *priv = netdev_priv(ndev); > >> const struct ravb_hw_info *hw_info = priv->info; > >> > >> - info->so_timestamping = > >> - SOF_TIMESTAMPING_TX_SOFTWARE | > >> - SOF_TIMESTAMPING_TX_HARDWARE | > >> - SOF_TIMESTAMPING_RX_HARDWARE | > >> - SOF_TIMESTAMPING_RAW_HARDWARE; > >> - info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); > >> - info->rx_filters = > >> - (1 << HWTSTAMP_FILTER_NONE) | > >> - (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > >> - (1 << HWTSTAMP_FILTER_ALL); > >> - if (hw_info->gptp || hw_info->ccc_gac) > >> + if (hw_info->gptp || hw_info->ccc_gac) { > >> + info->so_timestamping = > >> + SOF_TIMESTAMPING_TX_SOFTWARE | > >> + SOF_TIMESTAMPING_TX_HARDWARE | > >> + SOF_TIMESTAMPING_RX_HARDWARE | > >> + SOF_TIMESTAMPING_RAW_HARDWARE; > >> + info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); > >> + info->rx_filters = > >> + (1 << HWTSTAMP_FILTER_NONE) | > >> + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > >> + (1 << HWTSTAMP_FILTER_ALL); > >> info->phc_index = ptp_clock_index(priv->ptp.clock); > >> - else > >> - info->phc_index = 0; > > > > Is it OK to remove this line? Yes it is OK, see the discussion that sparked this patch. https://lore.kernel.org/netdev/20240829204429.GA3708622@ragnatech.se/ > > Also, how about inverting the *if* condition above (and doing an early > *return*) and avoiding reindenting the code below it? I thought about that but opted not to do so. The same check is used all over the code and I think it's value in keeping it similar. I will go over all this code again as Gen4 will need more work to fully enable gPTP. My hope is to abstract the check into something bore descriptive instead of sprinkling yet more conditions on to this one. Is it OK for you to keep them aligned for now? > > [...] > > MBR, Sergey >
On 10/8/24 8:45 PM, Niklas Söderlund wrote: [...] > Sorry for missing your comment earlier. I prolly shouldn't have stamped my R-b tag so easily before asking a question. :-) [...] >>>> Recent work moving the reporting of Rx software timestamps to the core >>>> [1] highlighted an issue where hardware time stamping where advertised s/where/was/. >>>> for the platforms where it is not supported. >>>> >>>> Fix this by covering advertising support for hardware timestamps only if >>>> the hardware supports it. Due to the Tx implementation in RAVB software >>>> Tx timestamping is also only considered if the hardware supports >>>> hardware timestamps. This should be addressed in future, but this fix >>>> only reflects what the driver currently implements. >>>> >>>> 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") >>>> >>>> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> [...] >>> >>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>>> index d2a6518532f3..907af4651c55 100644 >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>>> @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev, >>>> struct ravb_private *priv = netdev_priv(ndev); >>>> const struct ravb_hw_info *hw_info = priv->info; >>>> >>>> - info->so_timestamping = >>>> - SOF_TIMESTAMPING_TX_SOFTWARE | >>>> - SOF_TIMESTAMPING_TX_HARDWARE | >>>> - SOF_TIMESTAMPING_RX_HARDWARE | >>>> - SOF_TIMESTAMPING_RAW_HARDWARE; >>>> - info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); >>>> - info->rx_filters = >>>> - (1 << HWTSTAMP_FILTER_NONE) | >>>> - (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | >>>> - (1 << HWTSTAMP_FILTER_ALL); >>>> - if (hw_info->gptp || hw_info->ccc_gac) >>>> + if (hw_info->gptp || hw_info->ccc_gac) { >>>> + info->so_timestamping = >>>> + SOF_TIMESTAMPING_TX_SOFTWARE | >>>> + SOF_TIMESTAMPING_TX_HARDWARE | >>>> + SOF_TIMESTAMPING_RX_HARDWARE | >>>> + SOF_TIMESTAMPING_RAW_HARDWARE; >>>> + info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); >>>> + info->rx_filters = >>>> + (1 << HWTSTAMP_FILTER_NONE) | >>>> + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | >>>> + (1 << HWTSTAMP_FILTER_ALL); >>>> info->phc_index = ptp_clock_index(priv->ptp.clock); >>>> - else >>>> - info->phc_index = 0; >>> >>> Is it OK to remove this line? > > Yes it is OK, see the discussion that sparked this patch. > > https://lore.kernel.org/netdev/20240829204429.GA3708622@ragnatech.se/ Ah, now I'm seeing where that 0 came from... :-) >> Also, how about inverting the *if* condition above (and doing an early >> *return*) and avoiding reindenting the code below it? > > I thought about that but opted not to do so. The same check is used all > over the code and I think it's value in keeping it similar. I will go > over all this code again as Gen4 will need more work to fully enable > gPTP. My hope is to abstract the check into something bore descriptive s/bore/more/? :-) > instead of sprinkling yet more conditions on to this one. Is it OK for > you to keep them aligned for now? Fine by me. >> [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index d2a6518532f3..907af4651c55 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1750,20 +1750,19 @@ static int ravb_get_ts_info(struct net_device *ndev, struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *hw_info = priv->info; - info->so_timestamping = - SOF_TIMESTAMPING_TX_SOFTWARE | - SOF_TIMESTAMPING_TX_HARDWARE | - SOF_TIMESTAMPING_RX_HARDWARE | - SOF_TIMESTAMPING_RAW_HARDWARE; - info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); - info->rx_filters = - (1 << HWTSTAMP_FILTER_NONE) | - (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | - (1 << HWTSTAMP_FILTER_ALL); - if (hw_info->gptp || hw_info->ccc_gac) + if (hw_info->gptp || hw_info->ccc_gac) { + info->so_timestamping = + SOF_TIMESTAMPING_TX_SOFTWARE | + SOF_TIMESTAMPING_TX_HARDWARE | + SOF_TIMESTAMPING_RX_HARDWARE | + SOF_TIMESTAMPING_RAW_HARDWARE; + info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON); + info->rx_filters = + (1 << HWTSTAMP_FILTER_NONE) | + (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | + (1 << HWTSTAMP_FILTER_ALL); info->phc_index = ptp_clock_index(priv->ptp.clock); - else - info->phc_index = 0; + } return 0; }
Recent work moving the reporting of Rx software timestamps to the core [1] highlighted an issue where hardware time stamping where advertised for the platforms where it is not supported. Fix this by covering advertising support for hardware timestamps only if the hardware supports it. Due to the Tx implementation in RAVB software Tx timestamping is also only considered if the hardware supports hardware timestamps. This should be addressed in future, but this fix only reflects what the driver currently implements. 1. Commit 277901ee3a26 ("ravb: Remove setting of RX software timestamp") Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L") Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-)