| Message ID | 20240130085935.33722-2-bastien.curutchet@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add device tree binding support to TI's DP83640 | expand |
> + ti,led-config: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [1, 2, 3] > + description: | > + If present, configures the LED Mode (values defined in > + dt-bindings/net/ti-dp83640.h). > + LED configuration can also be strapped. If the strap pin is not set > + correctly or not set at all then this can be used to configure it. > + - 1 = Mode 1 > + LED_LINK = ON for Good Link, OFF for No Link > + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s > + LED_ACT = ON for Activity, OFF for No Activity > + - 2 = Mode 2 > + LED_LINK = ON for Good Link, BLINK for Activity > + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s > + LED_ACT = ON for Collision, OFF for No Collision > + - 3 = Mode 3 > + LED_LINK = ON for Good Link, BLINK for Activity > + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s > + LED_ACT = ON for Full Duplex, OFF for Half Duplex > + - unset = Configured by straps Please look at have the Marvell PHY driver supports LEDs via /sys/class/leds. Now we have a generic way to supports LEDs, DT properties like this will not be accepted. > + > + ti,phy-control-frames: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] > + description: | > + If present, enables or disables the PHY control frames. > + PHY Control Frames support can also be strapped. If the strap pin is not > + set correctly or not set at all then this can be used to configure it. > + - 0 = PHY Control Frames disabled > + - 1 = PHY Control Frames enabled > + - unset = Configured by straps What is a control frame? > + > + ti,energy-detect-en: > + $ref: /schemas/types.yaml#/definitions/flag > + description: | > + If present, Energy Detect Mode is enabled. If not present, Energy Detect > + Mode is disabled. This feature can not be strapped. Please use the phy tunable ETHTOOL_PHY_EDPD. There are a few examples you can copy. Andrew
Hey, On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote: > +description: | > + The DP83640 Precision PHYTER device delivers the highest level of precision This is not a marketing document. > + clock synchronization for real time industrial connectivity based on the > + IEEE 1588 standard. The DP83640 has deterministic, low latency and allows > + choice of microcontroller with no hardware customization required > + > + This device interfaces directly to the MAC layer through the > + IEEE 802.3 Standard Media Independent Interface (MII), or Reduced MII (RMII). > + > + Specifications about the Ethernet PHY can be found at: > + https://www.ti.com/lit/gpn/dp83640 > + > +properties: > + reg: > + maxItems: 1 > + > + ti,clk-output: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] > + description: | > + If present, enables or disables the CLK_OUT pin. > + CLK_OUT pin disabling can also be strapped. If the strap pin is not set > + correctly or not set at all then this can be used to configure it. > + - 0 = CLK_OUT pin disabled > + - 1 = CLK_OUT pin enabled > + - unset = Configured by straps If you are providing a clock, why is there no clock-controller property here? I don't think the 3-way nature of this property is needed, if you make this a "real" clock controller. > + ti,fiber-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] > + description: | > + If present, enables or disables the FX Fiber Mode. > + Fiber mode support can also be strapped. If the strap pin is not set > + correctly or not set at all then this can be used to configure it. > + - 0 = FX Fiber Mode disabled > + - 1 = FX Fiber Mode enabled > + - unset = Configured by straps I don't like these properties that map meanings onto numbers. We can have enums of strings in bindings that allow you to use something more meaningful than "0" or "1". Cheers, Conor.
On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote: > Hey, > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote: > > +description: | > > + The DP83640 Precision PHYTER device delivers the highest level of precision > > This is not a marketing document. > > > + clock synchronization for real time industrial connectivity based on the > > + IEEE 1588 standard. The DP83640 has deterministic, low latency and allows > > + choice of microcontroller with no hardware customization required > > + > > + This device interfaces directly to the MAC layer through the > > + IEEE 802.3 Standard Media Independent Interface (MII), or Reduced MII (RMII). > > + > > + Specifications about the Ethernet PHY can be found at: > > + https://www.ti.com/lit/gpn/dp83640 > > + > > +properties: > > + reg: > > + maxItems: 1 > > + > > + ti,clk-output: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1] > > + description: | > > + If present, enables or disables the CLK_OUT pin. > > + CLK_OUT pin disabling can also be strapped. If the strap pin is not set > > + correctly or not set at all then this can be used to configure it. > > + - 0 = CLK_OUT pin disabled > > + - 1 = CLK_OUT pin enabled > > + - unset = Configured by straps > > If you are providing a clock, why is there no clock-controller property > here? I don't think the 3-way nature of this property is needed, if you > make this a "real" clock controller. > > > + ti,fiber-mode: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1] > > + description: | > > + If present, enables or disables the FX Fiber Mode. > > + Fiber mode support can also be strapped. If the strap pin is not set > > + correctly or not set at all then this can be used to configure it. > > + - 0 = FX Fiber Mode disabled > > + - 1 = FX Fiber Mode enabled > > + - unset = Configured by straps > > I don't like these properties that map meanings onto numbers. We can > have enums of strings in bindings that allow you to use something more > meaningful than "0" or "1". Tristate properties are fairly common pattern where we need on/off/default. I've thought about making it a type. I don't think we need defines for it. Rob
On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote: > On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote: > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote: > > > + ti,fiber-mode: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1] > > > + description: | > > > + If present, enables or disables the FX Fiber Mode. > > > + Fiber mode support can also be strapped. If the strap pin is not set > > > + correctly or not set at all then this can be used to configure it. > > > + - 0 = FX Fiber Mode disabled > > > + - 1 = FX Fiber Mode enabled > > > + - unset = Configured by straps > > > > I don't like these properties that map meanings onto numbers. We can > > have enums of strings in bindings that allow you to use something more > > meaningful than "0" or "1". > > Tristate properties are fairly common pattern where we need > on/off/default. I've thought about making it a type. I don't think we > need defines for it. I think a type would be a good idea. I am not at all a fan of any of the properties people introduce along these lines.
On Wed, Jan 31, 2024 at 09:18:39PM +0000, Conor Dooley wrote: > On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote: > > On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote: > > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote: > > > > > + ti,fiber-mode: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + enum: [0, 1] > > > > + description: | > > > > + If present, enables or disables the FX Fiber Mode. > > > > + Fiber mode support can also be strapped. If the strap pin is not set > > > > + correctly or not set at all then this can be used to configure it. > > > > + - 0 = FX Fiber Mode disabled > > > > + - 1 = FX Fiber Mode enabled > > > > + - unset = Configured by straps > > > > > > I don't like these properties that map meanings onto numbers. We can > > > have enums of strings in bindings that allow you to use something more > > > meaningful than "0" or "1". > > > > Tristate properties are fairly common pattern where we need > > on/off/default. I've thought about making it a type. I don't think we > > need defines for it. > > I think a type would be a good idea. I am not at all a fan of any of the > properties people introduce along these lines. Before going too far with that, i'm not actually sure it is required here. I've not looked at the PHY driver itself, but i expect there is some indication somewhere that the network stack expects a fibre link is to be used. We probably can determine at runtime if fibre should be used. Andrew
Hi Andrew, Thank you for your feedback. On 1/30/24 14:34, Andrew Lunn wrote: >> + ti,led-config: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [1, 2, 3] >> + description: | >> + If present, configures the LED Mode (values defined in >> + dt-bindings/net/ti-dp83640.h). >> + LED configuration can also be strapped. If the strap pin is not set >> + correctly or not set at all then this can be used to configure it. >> + - 1 = Mode 1 >> + LED_LINK = ON for Good Link, OFF for No Link >> + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s >> + LED_ACT = ON for Activity, OFF for No Activity >> + - 2 = Mode 2 >> + LED_LINK = ON for Good Link, BLINK for Activity >> + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s >> + LED_ACT = ON for Collision, OFF for No Collision >> + - 3 = Mode 3 >> + LED_LINK = ON for Good Link, BLINK for Activity >> + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s >> + LED_ACT = ON for Full Duplex, OFF for Half Duplex >> + - unset = Configured by straps > Please look at have the Marvell PHY driver supports LEDs via > /sys/class/leds. Now we have a generic way to supports LEDs, DT > properties like this will not be accepted. Ok I'll use /sys/class/leds >> + >> + ti,phy-control-frames: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1] >> + description: | >> + If present, enables or disables the PHY control frames. >> + PHY Control Frames support can also be strapped. If the strap pin is not >> + set correctly or not set at all then this can be used to configure it. >> + - 0 = PHY Control Frames disabled >> + - 1 = PHY Control Frames enabled >> + - unset = Configured by straps > What is a control frame? I'm not an expert on this but it seems that if the PHY's Serial Management interface is not available, it is possible to build PCF (PHY Control Frame) packets that will be passed to PHY through the MAC Transmit Data interface. The PHY is then able to intercept and interpret these packets. Enabling it increases the MII Transmit packet latency. You'll find details in §5.4.6 of datasheet [https://www.ti.com/lit/gpn/dp83640] >> + >> + ti,energy-detect-en: >> + $ref: /schemas/types.yaml#/definitions/flag >> + description: | >> + If present, Energy Detect Mode is enabled. If not present, Energy Detect >> + Mode is disabled. This feature can not be strapped. > Please use the phy tunable ETHTOOL_PHY_EDPD. There are a few examples > you can copy. Ok I'll do that also, thank you. Best regards, Bastien
> > > + ti,phy-control-frames: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1] > > > + description: | > > > + If present, enables or disables the PHY control frames. > > > + PHY Control Frames support can also be strapped. If the strap pin is not > > > + set correctly or not set at all then this can be used to configure it. > > > + - 0 = PHY Control Frames disabled > > > + - 1 = PHY Control Frames enabled > > > + - unset = Configured by straps > > What is a control frame? > I'm not an expert on this but it seems that if the PHY's Serial Management > interface is not available, it is possible to build PCF (PHY Control Frame) > packets that will be passed to PHY through the MAC Transmit Data interface. > The > PHY is then able to intercept and interpret these packets. Enabling it > increases > the MII Transmit packet latency. > You'll find details in §5.4.6 of datasheet > [https://www.ti.com/lit/gpn/dp83640] Do you actually need this feature? [Looks at data sheet] Ah, so it allows you to access PHY registers by sending it commands in Ethernet frames. That should in theory be faster than MDIO. However, my experience with Ethernet switches which offer similar capabilities, it is often not faster, because of interrupt coalescing. Anyway, the serial management interface is the MDIO bus. You know this is available, because that is how the PHY driver it talking to the PHY! Also, i've not seen any code which implements sending commands to the PHY using Ethernet frames. So why not just hard code it in the driver to disable this feature? Andrew
Hi Andrew, Bastien, On Wed, 31 Jan 2024 23:40:45 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Jan 31, 2024 at 09:18:39PM +0000, Conor Dooley wrote: > > On Wed, Jan 31, 2024 at 03:05:21PM -0600, Rob Herring wrote: > > > On Tue, Jan 30, 2024 at 05:56:37PM +0000, Conor Dooley wrote: > > > > On Tue, Jan 30, 2024 at 09:59:34AM +0100, Bastien Curutchet wrote: > > > > > > > + ti,fiber-mode: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > + enum: [0, 1] > > > > > + description: | > > > > > + If present, enables or disables the FX Fiber Mode. > > > > > + Fiber mode support can also be strapped. If the strap pin is not set > > > > > + correctly or not set at all then this can be used to configure it. > > > > > + - 0 = FX Fiber Mode disabled > > > > > + - 1 = FX Fiber Mode enabled > > > > > + - unset = Configured by straps > > > > > > > > I don't like these properties that map meanings onto numbers. We can > > > > have enums of strings in bindings that allow you to use something more > > > > meaningful than "0" or "1". > > > > > > Tristate properties are fairly common pattern where we need > > > on/off/default. I've thought about making it a type. I don't think we > > > need defines for it. > > > > I think a type would be a good idea. I am not at all a fan of any of the > > properties people introduce along these lines. > > Before going too far with that, i'm not actually sure it is required > here. I've not looked at the PHY driver itself, but i expect there is > some indication somewhere that the network stack expects a fibre link > is to be used. We probably can determine at runtime if fibre should be > used. I've missed that thread initially. I guess that if Fiber is to be used, this would be done through sfp, then we have all the regular interfaces to configure the phy_interface_mode, in that case that would be 100BaseX. So, a sane behaviour could simply be to configure the PHY in copper mode by default, without relying on any DT property ? If anyone wants to use fiber mode, then they would have to implement the sfp_upstreamp_ops, which would take care of reconfiguring the MDI interface of the PHY in the correct mode. Maxime
> > I've missed that thread initially. I guess that if Fiber is to be used, > this would be done through sfp, then we have all the regular interfaces > to configure the phy_interface_mode, in that case that would be > 100BaseX. > > So, a sane behaviour could simply be to configure the PHY in copper > mode by default, without relying on any DT property ? If anyone wants to > use fiber mode, then they would have to implement the > sfp_upstreamp_ops, which would take care of reconfiguring the MDI > interface of the PHY in the correct mode. I missed the fact that this isn't a new driver... So this would indeed break existing setups who would have fiber-mode strapped-in but don't use SFP for some reason :( Maxime > Maxime >
diff --git a/Documentation/devicetree/bindings/net/ti,dp83640.yaml b/Documentation/devicetree/bindings/net/ti,dp83640.yaml new file mode 100644 index 000000000000..b0f389122934 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,dp83640.yaml @@ -0,0 +1,113 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright 2024 Nanometrics Inc +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,dp83640.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI DP83640 ethernet PHY + +allOf: + - $ref: ethernet-controller.yaml# + +maintainers: + - Bastien Curutchet <bastien.curutchet@bootlin.com> + +description: | + The DP83640 Precision PHYTER device delivers the highest level of precision + clock synchronization for real time industrial connectivity based on the + IEEE 1588 standard. The DP83640 has deterministic, low latency and allows + choice of microcontroller with no hardware customization required + + This device interfaces directly to the MAC layer through the + IEEE 802.3 Standard Media Independent Interface (MII), or Reduced MII (RMII). + + Specifications about the Ethernet PHY can be found at: + https://www.ti.com/lit/gpn/dp83640 + +properties: + reg: + maxItems: 1 + + ti,clk-output: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + description: | + If present, enables or disables the CLK_OUT pin. + CLK_OUT pin disabling can also be strapped. If the strap pin is not set + correctly or not set at all then this can be used to configure it. + - 0 = CLK_OUT pin disabled + - 1 = CLK_OUT pin enabled + - unset = Configured by straps + + ti,led-config: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 3] + description: | + If present, configures the LED Mode (values defined in + dt-bindings/net/ti-dp83640.h). + LED configuration can also be strapped. If the strap pin is not set + correctly or not set at all then this can be used to configure it. + - 1 = Mode 1 + LED_LINK = ON for Good Link, OFF for No Link + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s + LED_ACT = ON for Activity, OFF for No Activity + - 2 = Mode 2 + LED_LINK = ON for Good Link, BLINK for Activity + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s + LED_ACT = ON for Collision, OFF for No Collision + - 3 = Mode 3 + LED_LINK = ON for Good Link, BLINK for Activity + LED_SPEED = ON in 100 Mb/s, OFF in 10 Mb/s + LED_ACT = ON for Full Duplex, OFF for Half Duplex + - unset = Configured by straps + + ti,phy-control-frames: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + description: | + If present, enables or disables the PHY control frames. + PHY Control Frames support can also be strapped. If the strap pin is not + set correctly or not set at all then this can be used to configure it. + - 0 = PHY Control Frames disabled + - 1 = PHY Control Frames enabled + - unset = Configured by straps + + ti,fiber-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + description: | + If present, enables or disables the FX Fiber Mode. + Fiber mode support can also be strapped. If the strap pin is not set + correctly or not set at all then this can be used to configure it. + - 0 = FX Fiber Mode disabled + - 1 = FX Fiber Mode enabled + - unset = Configured by straps + + ti,energy-detect-en: + $ref: /schemas/types.yaml#/definitions/flag + description: | + If present, Energy Detect Mode is enabled. If not present, Energy Detect + Mode is disabled. This feature can not be strapped. + +required: + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/net/ti-dp83640.h> + + mdio0 { + #address-cells = <1>; + #size-cells = <0>; + ethphy0: ethernet-phy@0 { + reg = <0>; + ti,clk-output = <0>; + ti,energy-detect-en; + ti,led-config = <DP83640_PHYCR_LED_CNFG_MODE_3>; + ti,phy-control-frames = <1>; + ti,fiber-mode = <1>; + }; + }; diff --git a/include/dt-bindings/net/ti-dp83640.h b/include/dt-bindings/net/ti-dp83640.h new file mode 100644 index 000000000000..5f44f8eeb666 --- /dev/null +++ b/include/dt-bindings/net/ti-dp83640.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * Device Tree constants for the Texas Instruments DP83640 PHY + * + * Author: Bastien Curutchet bastien.curutchet@bootlin.com> + * + * Copyright: 2024 Nanometrics Inc. + */ + +#ifndef _DT_BINDINGS_TI_DP83640_H +#define _DT_BINDINGS_TI_DP83640_H + +/* PHY CTRL bits */ +#define DP83640_PHYCR_LED_CNFG_MODE_1 1 +#define DP83640_PHYCR_LED_CNFG_MODE_2 2 +#define DP83640_PHYCR_LED_CNFG_MODE_3 3 + +#endif
The TI DP83640 is a PTP PHY. Some of his features can be enabled by hardware straps or by registers configuration. Add a device tree binding for configuration through registers Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> --- .../devicetree/bindings/net/ti,dp83640.yaml | 113 ++++++++++++++++++ include/dt-bindings/net/ti-dp83640.h | 18 +++ 2 files changed, 131 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,dp83640.yaml create mode 100644 include/dt-bindings/net/ti-dp83640.h