| Message ID | 20240227093945.21525-7-bastien.curutchet@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Add TI's DP83640 device tree binding | expand |
On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote: > @@ -1141,6 +1147,17 @@ static int dp83640_config_init(struct phy_device *phydev) > val = phy_read(phydev, PCFCR) & ~PCF_EN; > phy_write(phydev, PCFCR, val); > > + if (dp83640->fiber != FIBER_MODE_DEFAULT) { > + val = phy_read(phydev, PCSR) & ~FX_EN; > + if (dp83640->fiber == FIBER_MODE_ENABLE) > + val |= FX_EN; > + phy_write(phydev, PCSR, val); val = 0; if (dp83640->fiber == FIBER_MODE_ENABLE) val = FX_EN; phy_modify(phydev, PCSR, FX_EN, val); > + > + /* Write SOFT_RESET bit to ensure configuration */ > + val = phy_read(phydev, PHYCR2) | SOFT_RESET; > + phy_write(phydev, PHYCR2, val); phy_set_bits(phydev, PHYCR2, SOFT_RESET); ... > +#else > +static int dp83640_of_init(struct phy_device *phydev) > +{ > + dp83640->fiber = FIBER_MODE_DEFAULT; This hasn't been build tested - dp83640 won't exist here.
On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote: > The PHY is able to use copper or fiber. The fiber mode can be enabled or > disabled by hardware strap. If hardware strap is incorrect, PHY can't > establish link. > > Add a DT attribute 'ti,fiber-mode' that can be use to override the > hardware strap configuration. If the property is not present, hardware > strap configuration is left as is. How have you tested this? Do you have a RDK with it connected to an SFP cage? Andrew
Hi Andrew, On 2/27/24 17:18, Andrew Lunn wrote: > On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote: >> The PHY is able to use copper or fiber. The fiber mode can be enabled or >> disabled by hardware strap. If hardware strap is incorrect, PHY can't >> establish link. >> >> Add a DT attribute 'ti,fiber-mode' that can be use to override the >> hardware strap configuration. If the property is not present, hardware >> strap configuration is left as is. > How have you tested this? Do you have a RDK with it connected to an > SFP cage? I did not test fiber mode as my board uses copper. My use case is that I need to explicitly disable the fiber mode because the strap hardware is misconfigured and could possibly enable fiber mode from time to time. Best regards, Bastien
On Thu, Feb 29, 2024 at 08:31:55AM +0100, Bastien Curutchet wrote: > Hi Andrew, > > On 2/27/24 17:18, Andrew Lunn wrote: > > On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote: > > > The PHY is able to use copper or fiber. The fiber mode can be enabled or > > > disabled by hardware strap. If hardware strap is incorrect, PHY can't > > > establish link. > > > > > > Add a DT attribute 'ti,fiber-mode' that can be use to override the > > > hardware strap configuration. If the property is not present, hardware > > > strap configuration is left as is. > > How have you tested this? Do you have a RDK with it connected to an > > SFP cage? > > I did not test fiber mode as my board uses copper. > > My use case is that I need to explicitly disable the fiber mode because the > strap hardware is > misconfigured and could possibly enable fiber mode from time to time. O.K. So lets refocus this is little. Rather than support fibre mode, just support disabling fibre mode. But leave a clear path for somebody to add fibre support sometime in the future. Looking at the current code, do you think fibre mode actually works today? If you think it cannot actually work today in fibre mode, one option would be to hard code it to copper mode. Leave the configuration between fibre and copper mode to the future when somebody actually implements fibre mode. Andrew
Hi Bastien, Andrew, On Thu, 29 Feb 2024 16:23:59 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Feb 29, 2024 at 08:31:55AM +0100, Bastien Curutchet wrote: > > Hi Andrew, > > > > On 2/27/24 17:18, Andrew Lunn wrote: > > > On Tue, Feb 27, 2024 at 10:39:45AM +0100, Bastien Curutchet wrote: > > > > The PHY is able to use copper or fiber. The fiber mode can be enabled or > > > > disabled by hardware strap. If hardware strap is incorrect, PHY can't > > > > establish link. > > > > > > > > Add a DT attribute 'ti,fiber-mode' that can be use to override the > > > > hardware strap configuration. If the property is not present, hardware > > > > strap configuration is left as is. > > > How have you tested this? Do you have a RDK with it connected to an > > > SFP cage? > > > > I did not test fiber mode as my board uses copper. > > > > My use case is that I need to explicitly disable the fiber mode because the > > strap hardware is > > misconfigured and could possibly enable fiber mode from time to time. > > O.K. So lets refocus this is little. Rather than support fibre mode, > just support disabling fibre mode. But leave a clear path for somebody > to add fibre support sometime in the future. > > Looking at the current code, do you think fibre mode actually works > today? If you think it cannot actually work today in fibre mode, one > option would be to hard code it to copper mode. Leave the > configuration between fibre and copper mode to the future when > somebody actually implements fibre mode. Looking at the driver and the datasheet, it's hard to say that the fiber mode can't work in the current state. It's configured either through straps or an overriding register, and it's enough to get the scrambler/descrambler automatically setup according to that single strap. So it's hard to say that defaulting to copper won't break anything :( OTOH there's no SFP support in this PHY, in terms of register config, some aneg modes won't work in 100BaseFX, which the driver doesn't account for, So nothing would indicate that the fiber mode was ever used. There's the DP83822 driver that can accept the "ti,fiber-mode" property, so adding that would at least be coherent with other DP83xxx PHYs but it has the opposite logic we want, so doesn't prevent any possible regression for existing fiber users. All in all, do you think that defaulting to copper and leaving users an option to implement "ti,fiber-mode" is an acceptable risk to take ? Maxime
> All in all, do you think that defaulting to copper and leaving users an > option to implement "ti,fiber-mode" is an acceptable risk to take ? Yes, lets do that. But try to make it easy to revert the change if anybody complains about a regression. Andrew
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c index b371dea23937..886f2bc3710d 100644 --- a/drivers/net/phy/dp83640.c +++ b/drivers/net/phy/dp83640.c @@ -16,6 +16,7 @@ #include <linux/net_tstamp.h> #include <linux/netdevice.h> #include <linux/if_vlan.h> +#include <linux/of.h> #include <linux/phy.h> #include <linux/ptp_classify.h> #include <linux/ptp_clock_kernel.h> @@ -141,6 +142,11 @@ struct dp83640_private { /* queues of incoming and outgoing packets */ struct sk_buff_head rx_queue; struct sk_buff_head tx_queue; + +#define FIBER_MODE_DEFAULT 0 +#define FIBER_MODE_ENABLE 1 +#define FIBER_MODE_DISABLE 2 + int fiber; }; struct dp83640_clock { @@ -1141,6 +1147,17 @@ static int dp83640_config_init(struct phy_device *phydev) val = phy_read(phydev, PCFCR) & ~PCF_EN; phy_write(phydev, PCFCR, val); + if (dp83640->fiber != FIBER_MODE_DEFAULT) { + val = phy_read(phydev, PCSR) & ~FX_EN; + if (dp83640->fiber == FIBER_MODE_ENABLE) + val |= FX_EN; + phy_write(phydev, PCSR, val); + + /* Write SOFT_RESET bit to ensure configuration */ + val = phy_read(phydev, PHYCR2) | SOFT_RESET; + phy_write(phydev, PHYCR2, val); + } + return 0; } @@ -1440,6 +1457,39 @@ static int dp83640_ts_info(struct mii_timestamper *mii_ts, return 0; } +#ifdef CONFIG_OF_MDIO +static int dp83640_of_init(struct phy_device *phydev) +{ + struct dp83640_private *dp83640 = phydev->priv; + struct device *dev = &phydev->mdio.dev; + struct device_node *of_node = dev->of_node; + const char *fiber; + int ret; + + if (of_property_present(of_node, "ti,fiber-mode")) { + ret = of_property_read_string(of_node, "ti,fiber-mode", &fiber); + if (ret) + return ret; + + dp83640->fiber = FIBER_MODE_DEFAULT; + if (!strncmp(fiber, "enable", 6)) + dp83640->fiber = FIBER_MODE_ENABLE; + else if (!strncmp(fiber, "disable", 7)) + dp83640->fiber = FIBER_MODE_DISABLE; + else + return -EINVAL; + } + + return 0; +} +#else +static int dp83640_of_init(struct phy_device *phydev) +{ + dp83640->fiber = FIBER_MODE_DEFAULT; + return 0; +} +#endif /* CONFIG_OF_MDIO */ + static int dp83640_probe(struct phy_device *phydev) { struct dp83640_clock *clock; @@ -1472,6 +1522,10 @@ static int dp83640_probe(struct phy_device *phydev) phydev->mii_ts = &dp83640->mii_ts; phydev->priv = dp83640; + err = dp83640_of_init(phydev); + if (err < 0) + goto of_failed; + spin_lock_init(&dp83640->rx_lock); skb_queue_head_init(&dp83640->rx_queue); skb_queue_head_init(&dp83640->tx_queue); @@ -1494,6 +1548,7 @@ static int dp83640_probe(struct phy_device *phydev) no_register: clock->chosen = NULL; +of_failed: kfree(dp83640); no_memory: dp83640_clock_put(clock); diff --git a/drivers/net/phy/dp83640_reg.h b/drivers/net/phy/dp83640_reg.h index b5adb8958c08..cbecf04da5a5 100644 --- a/drivers/net/phy/dp83640_reg.h +++ b/drivers/net/phy/dp83640_reg.h @@ -6,6 +6,7 @@ #define HAVE_DP83640_REGISTERS /* #define PAGE0 0x0000 */ +#define PCSR 0x0016 /* PCS Configuration and Status Register */ #define LEDCR 0x0018 /* PHY Control Register */ #define PHYCR 0x0019 /* PHY Control Register */ #define PHYCR2 0x001c /* PHY Control Register 2 */ @@ -54,6 +55,9 @@ #define PTP_GPIOMON 0x001e /* PTP GPIO Monitor Register */ #define PTP_RXHASH 0x001f /* PTP Receive Hash Register */ +/* Bit definitions for the PCSR register */ +#define FX_EN BIT(6) /* Enable FX Fiber Mode */ + /* Bit definitions for the LEDCR register */ #define DP83640_LED_DIS(x) BIT((x) + 9) /* Disable LED */ #define DP83640_LED_DRV(x) BIT((x) + 3) /* Force LED val to output */ @@ -64,6 +68,7 @@ #define LED_CNFG_1 BIT(6) /* LED configuration, bit 1 */ /* Bit definitions for the PHYCR2 register */ +#define SOFT_RESET BIT(9) /* Soft Reset */ #define BC_WRITE (1<<11) /* Broadcast Write Enable */ /* Bit definitions for the EDCR register */
The PHY is able to use copper or fiber. The fiber mode can be enabled or disabled by hardware strap. If hardware strap is incorrect, PHY can't establish link. Add a DT attribute 'ti,fiber-mode' that can be use to override the hardware strap configuration. If the property is not present, hardware strap configuration is left as is. Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> --- drivers/net/phy/dp83640.c | 55 +++++++++++++++++++++++++++++++++++ drivers/net/phy/dp83640_reg.h | 5 ++++ 2 files changed, 60 insertions(+)