| Message ID | 20230406173308.401924-4-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Make MAC/PHY time stamping selectable | expand |
On Thu, Apr 06, 2023 at 07:33:06PM +0200, Köry Maincent wrote: > From: Kory Maincent <kory.maincent@bootlin.com> > > Add property to select the preferred hardware timestamp layer. > The choice of using devicetree binding has been made as the PTP precision > and quality depends of external things, like adjustable clock, or the lack > of a temperature compensated crystal or specific features. Even if the > preferred timestamp is a configuration it is hardly related to the design > of the board. > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > Documentation/devicetree/bindings/net/ethernet-phy.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > index ac04f8efa35c..32d7ef7520e6 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > @@ -144,6 +144,13 @@ properties: > Mark the corresponding energy efficient ethernet mode as > broken and request the ethernet to stop advertising it. > > + preferred-timestamp: > + enum: > + - phy > + - mac > + description: > + Specifies the preferred hardware timestamp layer. > + > pses: > $ref: /schemas/types.yaml#/definitions/phandle-array > maxItems: 1 > -- > 2.25.1 > Do we need this device tree functionality?
On Wed, 12 Apr 2023 16:14:21 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > + preferred-timestamp: > > + enum: > > + - phy > > + - mac > > + description: > > + Specifies the preferred hardware timestamp layer. > > + > > pses: > > $ref: /schemas/types.yaml#/definitions/phandle-array > > maxItems: 1 > > -- > > 2.25.1 > > > > Do we need this device tree functionality? I would say so. Expected as I wrote the patch. ;) My point was that the new behavior to MAC as default timestamping does not fit all the case, especially when a board is designed with PHY like the TI PHYTER which is a far better timestamping choice (according to Richard). The user doesn't need to know this, he wants to have the better time stamp selected by default without any investigation. That's why having devicetree property for that could be useful.
On 06/04/2023 19:33, Köry Maincent wrote: > From: Kory Maincent <kory.maincent@bootlin.com> > > Add property to select the preferred hardware timestamp layer. > The choice of using devicetree binding has been made as the PTP precision > and quality depends of external things, like adjustable clock, or the lack > of a temperature compensated crystal or specific features. Even if the > preferred timestamp is a configuration it is hardly related to the design > of the board. Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Since you skipped some entries, no tests will be executed here. Resend following Linux kernel process. Best regards, Krzysztof
On Wed, Apr 12, 2023 at 03:44:46PM +0200, Köry Maincent wrote: > On Wed, 12 Apr 2023 16:14:21 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Do we need this device tree functionality? > > I would say so. Expected as I wrote the patch. ;) > > My point was that the new behavior to MAC as default timestamping does not fit > all the case, especially when a board is designed with PHY like the TI PHYTER > which is a far better timestamping choice (according to Richard). The user > doesn't need to know this, he wants to have the better time stamp selected by > default without any investigation. That's why having devicetree property for > that could be useful. The TI PHYTER is the "NatSemi DP83640" entry in the whitelist for PHYs that still use their timestamping by default. Can you please come up with an example which is actually useful?
On Sat, 29 Apr 2023 20:42:17 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Wed, Apr 12, 2023 at 03:44:46PM +0200, Köry Maincent wrote: > > On Wed, 12 Apr 2023 16:14:21 +0300 Vladimir Oltean > > <vladimir.oltean@nxp.com> wrote: > > > Do we need this device tree functionality? > > > > I would say so. Expected as I wrote the patch. ;) > > > > My point was that the new behavior to MAC as default timestamping does not > > fit all the case, especially when a board is designed with PHY like the TI > > PHYTER which is a far better timestamping choice (according to Richard). > > The user doesn't need to know this, he wants to have the better time stamp > > selected by default without any investigation. That's why having devicetree > > property for that could be useful. > > The TI PHYTER is the "NatSemi DP83640" entry in the whitelist for PHYs > that still use their timestamping by default. Can you please come up > with an example which is actually useful? If a future PHY, featured like this TI PHYTER, is supported in the future the default timestamp will be the MAC and we won't be able to select the PHY by default. Another example is my case with the 88E151x PHY, on the Russell side with the Macchiatobin board, the MAC is more precise, and in my side with a custom board with macb MAC, the PHY is more precise. Be able to select the prefer one from devicetree is convenient.
On Tue, May 02, 2023 at 11:10:43AM +0200, Köry Maincent wrote: > If a future PHY, featured like this TI PHYTER, is supported in the future the > default timestamp will be the MAC and we won't be able to select the PHY by > default. > Another example is my case with the 88E151x PHY, on the Russell side with the > Macchiatobin board, the MAC is more precise, and in my side with a custom board > with macb MAC, the PHY is more precise. Be able to select the prefer one from > devicetree is convenient. If convenience is all that there is, I guess that's not a very strong argument for putting something in the device tree which couldn't have been handled by user space through an init script, and nothing would have been broken as a result of that.
On Thu, 11 May 2023 16:10:08 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Tue, May 02, 2023 at 11:10:43AM +0200, Köry Maincent wrote: > > If a future PHY, featured like this TI PHYTER, is supported in the future > > the default timestamp will be the MAC and we won't be able to select the > > PHY by default. > > Another example is my case with the 88E151x PHY, on the Russell side with > > the Macchiatobin board, the MAC is more precise, and in my side with a > > custom board with macb MAC, the PHY is more precise. Be able to select the > > prefer one from devicetree is convenient. > > If convenience is all that there is, I guess that's not a very strong > argument for putting something in the device tree which couldn't have > been handled by user space through an init script, and nothing would > have been broken as a result of that. The user may not and don't need to know which hardware timestamping is better. He just want to use the best one by default without investigation and benchmarking. It is more related to the hardware design of the board which should be described in the devicetree, don't you think? Of course it should not break anything and if it does, well then let the user select it in userspace. But if you really think my point is irrelevant then I will drop this feature.
On Thu, May 11, 2023 at 03:25:50PM +0200, Köry Maincent wrote: > The user may not and don't need to know which hardware timestamping is better. > He just want to use the best one by default without investigation and > benchmarking. > It is more related to the hardware design of the board which should be > described in the devicetree, don't you think? Of course it should not break > anything and if it does, well then let the user select it in userspace. > But if you really think my point is irrelevant then I will drop this feature. You are putting an equality sign between user space and the end-user of a system. A user space distribution has a lot of configuration files where the end user isn't expected to know how to configure them all, and that's not an argument for putting them in the kernel/device tree, is it? I can relate to 2 examples which are closer to what I know slightly better (Documentation/devicetree/bindings/net/dsa/dsa-port.yaml). One is "label" (the netdev name of a switch port). It has been argued that we should deprecate this, because udev permits selecting specific netdev named based on hardware properties already. I agree with this, and this is why on NXP LS1028A, we don't use "label" in the device tree, but advise people to ship this in the rootfs: cat /etc/udev/rules.d/10-network.rules # ENETC rules ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.0", DRIVERS=="fsl_enetc", NAME:="eno0" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.1", DRIVERS=="fsl_enetc", NAME:="eno1" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.2", DRIVERS=="fsl_enetc", NAME:="eno2" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.6", DRIVERS=="fsl_enetc", NAME:="eno3" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.0", DRIVERS=="fsl_enetc_vf", NAME:="eno0vf0" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.1", DRIVERS=="fsl_enetc_vf", NAME:="eno0vf1" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.2", DRIVERS=="fsl_enetc_vf", NAME:="eno1vf0" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:01.3", DRIVERS=="fsl_enetc_vf", NAME:="eno1vf1" # LS1028 switch rules ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p0", NAME="swp0" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p1", NAME="swp1" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p2", NAME="swp2" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p3", NAME="swp3" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p4", NAME="swp4" ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5", DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p5", NAME="swp5" The other example is "dsa-tag-protocol", which you'd normally expect that user space would select through /sys/class/net/ethN/dsa/tagging and that would be the end of the story. I was only convinced to let it live in the device tree because a tagging protocol change might be necessary for traffic on the port to work at all, and if traffic doesn't work, then the kernel can't load userspace through e.g. NFS, and thus, user space can't change this setting. In your case, I don't think this argument applies. I guess the general rule of thumb is - if a functionality can live outside the kernel or of the device tree, it's probably better that it does.
On Thu, 11 May 2023 16:56:31 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Thu, May 11, 2023 at 03:25:50PM +0200, Köry Maincent wrote: > > I guess the general rule of thumb is - if a functionality can live outside > the kernel or of the device tree, it's probably better that it does. Ok it is clearer to me. Thanks for your explanation! Then I will drop the device tree parameter.
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index ac04f8efa35c..32d7ef7520e6 100644 --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml @@ -144,6 +144,13 @@ properties: Mark the corresponding energy efficient ethernet mode as broken and request the ethernet to stop advertising it. + preferred-timestamp: + enum: + - phy + - mac + description: + Specifies the preferred hardware timestamp layer. + pses: $ref: /schemas/types.yaml#/definitions/phandle-array maxItems: 1