* [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
@ 2024-10-05 12:14 Niklas Söderlund
2024-10-07 9:23 ` Paul Barker
2024-10-07 19:05 ` Sergey Shtylyov
0 siblings, 2 replies; 6+ messages in thread
From: Niklas Söderlund @ 2024-10-05 12:14 UTC (permalink / raw)
To: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Claudiu Beznea, Paul Barker, Biju Das, Lad Prabhakar,
netdev
Cc: linux-renesas-soc, Niklas Söderlund
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(-)
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;
}
--
2.46.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
2024-10-05 12:14 [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it Niklas Söderlund
@ 2024-10-07 9:23 ` Paul Barker
2024-10-07 19:05 ` Sergey Shtylyov
1 sibling, 0 replies; 6+ messages in thread
From: Paul Barker @ 2024-10-07 9:23 UTC (permalink / raw)
To: Niklas Söderlund, Sergey Shtylyov, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Beznea,
Biju Das, Lad Prabhakar, netdev
Cc: linux-renesas-soc
[-- Attachment #1.1.1: Type: text/plain, Size: 1810 bytes --]
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>
--
Paul Barker
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
2024-10-05 12:14 [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it Niklas Söderlund
2024-10-07 9:23 ` Paul Barker
@ 2024-10-07 19:05 ` Sergey Shtylyov
2024-10-08 17:26 ` Sergey Shtylyov
1 sibling, 1 reply; 6+ messages in thread
From: Sergey Shtylyov @ 2024-10-07 19:05 UTC (permalink / raw)
To: Niklas Söderlund, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Claudiu Beznea, Paul Barker,
Biju Das, Lad Prabhakar, netdev
Cc: linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
2024-10-07 19:05 ` Sergey Shtylyov
@ 2024-10-08 17:26 ` Sergey Shtylyov
2024-10-08 17:45 ` Niklas Söderlund
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Shtylyov @ 2024-10-08 17:26 UTC (permalink / raw)
To: Niklas Söderlund, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Claudiu Beznea, Paul Barker,
Biju Das, Lad Prabhakar, netdev
Cc: linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
2024-10-08 17:26 ` Sergey Shtylyov
@ 2024-10-08 17:45 ` Niklas Söderlund
2024-10-08 20:35 ` Sergey Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Niklas Söderlund @ 2024-10-08 17:45 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Claudiu Beznea, Paul Barker, Biju Das, Lad Prabhakar, netdev,
linux-renesas-soc
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
>
--
Kind Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it
2024-10-08 17:45 ` Niklas Söderlund
@ 2024-10-08 20:35 ` Sergey Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2024-10-08 20:35 UTC (permalink / raw)
To: Niklas Söderlund
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Claudiu Beznea, Paul Barker, Biju Das, Lad Prabhakar, netdev,
linux-renesas-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-08 20:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-05 12:14 [net-next] net: ravb: Only advertise Rx/Tx timestamps if hardware supports it Niklas Söderlund
2024-10-07 9:23 ` Paul Barker
2024-10-07 19:05 ` Sergey Shtylyov
2024-10-08 17:26 ` Sergey Shtylyov
2024-10-08 17:45 ` Niklas Söderlund
2024-10-08 20:35 ` Sergey Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).