| Message ID | 20230426104313.28950-4-harini.katakam@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for VSC8531_02 PHY and DT RGMII tuning | expand |
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote: > From: Harini Katakam <harini.katakam@xilinx.com> > > Add support for VSC8531_02 (Rev 2) device. > Add support for optional RGMII RX and TX delay tuning via devicetree. > The hierarchy is: > - Retain the defaul 0.2ns delay when RGMII tuning is not set. > - Retain the default 2ns delay when RGMII tuning is set and DT delay > property is NOT specified. > - Use the DT delay value when RGMII tuning is set and a DT delay > property is specified. > > Signed-off-by: Harini Katakam <harini.katakam@amd.com> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > v2: > - Switch both VSC8531 and VSC8531-02 to use exact phy id match as they > share the same model number > - Ensure RCT > - Improve optional property read > > drivers/net/phy/mscc/mscc.h | 3 +++ > drivers/net/phy/mscc/mscc_main.c | 40 ++++++++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > index a50235fdf7d9..5a26eba0ace0 100644 > --- a/drivers/net/phy/mscc/mscc.h > +++ b/drivers/net/phy/mscc/mscc.h > @@ -281,6 +281,7 @@ enum rgmii_clock_delay { > #define PHY_ID_VSC8514 0x00070670 > #define PHY_ID_VSC8530 0x00070560 > #define PHY_ID_VSC8531 0x00070570 > +#define PHY_ID_VSC8531_02 0x00070572 > #define PHY_ID_VSC8540 0x00070760 > #define PHY_ID_VSC8541 0x00070770 > #define PHY_ID_VSC8552 0x000704e0 > @@ -373,6 +374,8 @@ struct vsc8531_private { > * package. > */ > unsigned int base_addr; > + u32 rx_delay; > + u32 tx_delay; > > #if IS_ENABLED(CONFIG_MACSEC) > /* MACsec fields: > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > index 75d9582e5784..80cc90a23d57 100644 > --- a/drivers/net/phy/mscc/mscc_main.c > +++ b/drivers/net/phy/mscc/mscc_main.c > @@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl, > { > u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1; > u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1; > + struct vsc8531_private *vsc8531 = phydev->priv; > u16 reg_val = 0; > int rc; > > @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl, > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos; > + reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos; > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos; > + reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos; > > rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, > rgmii_cntl, > @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev) > { > int rc, i, phy_id; > struct vsc8531_private *vsc8531 = phydev->priv; > + struct device_node *of_node = phydev->mdio.dev.of_node; > + > + vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS; > + rc = of_property_read_u32(of_node, "mscc,rx-delay", > + &vsc8531->rx_delay); > + > + vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS; > + rc = of_property_read_u32(of_node, "mscc,tx-delay", > + &vsc8531->tx_delay); Since the dt-bindings document says "If this property is present then the PHY applies the RX|TX delay", then I guess the precedence as applied by vsc85xx_rgmii_set_skews() should be different. The RX delays should be applied based on rx-internal-delay-ps (if present) regardless of phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid or rgmii-id modes. Similar for tx. Also, please split the VSC8531-02 addition into a separate patch, since the configurable RGMII delays also affect existing PHYs (like VSC8502).
Hi Vladimir, > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Wednesday, April 26, 2023 4:48 PM > To: Katakam, Harini <harini.katakam@amd.com> > Cc: robh+dt@kernel.org; andrew@lunn.ch; hkallweit1@gmail.com; > linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org; > edumazet@google.com; pabeni@redhat.com; wsa+renesas@sang- > engineering.com; krzysztof.kozlowski+dt@linaro.org; > simon.horman@corigine.com; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 > with RGMII tuning > > On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote: > > From: Harini Katakam <harini.katakam@xilinx.com> > > > > Add support for VSC8531_02 (Rev 2) device. > > Add support for optional RGMII RX and TX delay tuning via devicetree. > > The hierarchy is: > > - Retain the defaul 0.2ns delay when RGMII tuning is not set. > > - Retain the default 2ns delay when RGMII tuning is set and DT delay > > property is NOT specified. > > - Use the DT delay value when RGMII tuning is set and a DT delay > > property is specified. > > > > Signed-off-by: Harini Katakam <harini.katakam@amd.com> > > Signed-off-by: Radhey Shyam Pandey > <radhey.shyam.pandey@xilinx.com> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > --- <snip> > > @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct > phy_device *phydev, u32 rgmii_cntl, > > > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > > - reg_val |= RGMII_CLK_DELAY_2_0_NS << > rgmii_rx_delay_pos; > > + reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos; > > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || > > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > > - reg_val |= RGMII_CLK_DELAY_2_0_NS << > rgmii_tx_delay_pos; > > + reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos; > > > > rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, > > rgmii_cntl, > > @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device > *phydev) > > { > > int rc, i, phy_id; > > struct vsc8531_private *vsc8531 = phydev->priv; > > + struct device_node *of_node = phydev->mdio.dev.of_node; > > + > > + vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS; > > + rc = of_property_read_u32(of_node, "mscc,rx-delay", > > + &vsc8531->rx_delay); > > + > > + vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS; > > + rc = of_property_read_u32(of_node, "mscc,tx-delay", > > + &vsc8531->tx_delay); > > Since the dt-bindings document says "If this property is present then > the PHY applies the RX|TX delay", then I guess the precedence as applied > by vsc85xx_rgmii_set_skews() should be different. The RX delays should > be applied based on rx-internal-delay-ps (if present) regardless of > phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid phy_get_internal_delay > or rgmii-id modes. Similar for tx. Thanks for the review. The intention is to have the following precedence (I'll rephrase the commit if required) -> If phy-mode is rgmii, current behavior persists for all devices -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices (i.e. delay of RGMII_CLK_DELAY_2_0_NS) -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode) I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like the above is allowed. Please do let me know otherwise. I will re-spin the series using phy_get_internal_delay. Regards, Harini
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote: > From: Harini Katakam <harini.katakam@xilinx.com> > > Add support for VSC8531_02 (Rev 2) device. > Add support for optional RGMII RX and TX delay tuning via devicetree. > The hierarchy is: > - Retain the defaul 0.2ns delay when RGMII tuning is not set. > - Retain the default 2ns delay when RGMII tuning is set and DT delay > property is NOT specified. tuning is probably the wrong word here. I normally consider tuning as small changes from 0ns/2ns. The course setting of 0ns or 2ns is not tuning. I normally use RGMII internal delays to refer to that. However, i'm not sure there is consistency among drivers. Andrew
On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote: > Thanks for the review. > The intention is to have the following precedence (I'll rephrase the commit if required) > -> If phy-mode is rgmii, current behavior persists for all devices > -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices > (i.e. delay of RGMII_CLK_DELAY_2_0_NS) > -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps > is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode) > > I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode > in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like > the above is allowed. Please do let me know otherwise. I understood what your intention was. What I meant was: phy-mode rgmii rgmii-rxid/rgmii-id -------------------------------------------------------------------------------------------- rx-internal-delay-ps absent 0.2 ns 2 ns rx-internal-delay-ps present follow rx-internal-delay-ps follow rx-internal-delay-ps I agree with Andrew that probably there isn't consistency among PHY drivers for this interpretation - see dp83822 vs intel-xway for example. My interpretation was based on the wording from the dt-bindings document, which seems to suggest that rx-internal-delay-ps takes precedence.
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote: > From: Harini Katakam <harini.katakam@xilinx.com> > > Add support for VSC8531_02 (Rev 2) device. > Add support for optional RGMII RX and TX delay tuning via devicetree. > The hierarchy is: > - Retain the defaul 0.2ns delay when RGMII tuning is not set. nit: s/defaul/default/ ...
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, April 26, 2023 7:18 PM > To: Katakam, Harini <harini.katakam@amd.com> > Cc: robh+dt@kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk; > davem@davemloft.net; kuba@kernel.org; edumazet@google.com; > pabeni@redhat.com; vladimir.oltean@nxp.com; wsa+renesas@sang- > engineering.com; krzysztof.kozlowski+dt@linaro.org; > simon.horman@corigine.com; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 > with RGMII tuning > > On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote: > > From: Harini Katakam <harini.katakam@xilinx.com> > > > > Add support for VSC8531_02 (Rev 2) device. > > Add support for optional RGMII RX and TX delay tuning via devicetree. > > The hierarchy is: > > - Retain the defaul 0.2ns delay when RGMII tuning is not set. > > - Retain the default 2ns delay when RGMII tuning is set and DT delay > > property is NOT specified. > > tuning is probably the wrong word here. I normally consider tuning as small > changes from 0ns/2ns. The course setting of 0ns or 2ns is not tuning. I > normally use RGMII internal delays to refer to that. > > However, i'm not sure there is consistency among drivers. Thanks for the input, I understand. I'm planning on re-phrasing this with phy-mode and property values to describe rather than say tuning. Regards, Harini
Hi Vladimir, > -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Wednesday, April 26, 2023 7:49 PM > To: Katakam, Harini <harini.katakam@amd.com> > Cc: robh+dt@kernel.org; andrew@lunn.ch; hkallweit1@gmail.com; > linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org; > edumazet@google.com; pabeni@redhat.com; wsa+renesas@sang- > engineering.com; krzysztof.kozlowski+dt@linaro.org; > simon.horman@corigine.com; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>; > Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com> > Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 > with RGMII tuning > > On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote: > > Thanks for the review. > > The intention is to have the following precedence (I'll rephrase the > > commit if required) > > -> If phy-mode is rgmii, current behavior persists for all devices If > > -> phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior > > -> persists for all devices > > (i.e. delay of RGMII_CLK_DELAY_2_0_NS) > > -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND > > -> rx-internal-delay-ps/tx-internal-delay-ps > > is defined, then the value from DT is considered instead of 2ns. (NOT > > irrespective of phy-mode) > > > > I'm checking the phy drivers that use phy_get_internal_delay and the > > description phy-mode in ethernet-controller.yaml and > > rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like the above is > allowed. Please do let me know otherwise. > > I understood what your intention was. What I meant was: > > phy-mode rgmii rgmii-rxid/rgmii-id > -------------------------------------------------------------------------------------------- > rx-internal-delay-ps absent 0.2 ns 2 ns > rx-internal-delay-ps present follow rx-internal-delay-ps follow rx-internal- > delay-ps > > I agree with Andrew that probably there isn't consistency among PHY drivers > for this interpretation - see dp83822 vs intel-xway for example. Thanks, yes I noticed the difference here and also in older PHY drivers that used custom properties (see dp83867 which is what I originally aligned it to). But the table you mentioned above makes sense; I'll re-spin accordingly. > My interpretation was based on the wording from the dt-bindings document, > which seems to suggest that rx-internal-delay-ps takes precedence. OK I understand. Regards, Harini
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h index a50235fdf7d9..5a26eba0ace0 100644 --- a/drivers/net/phy/mscc/mscc.h +++ b/drivers/net/phy/mscc/mscc.h @@ -281,6 +281,7 @@ enum rgmii_clock_delay { #define PHY_ID_VSC8514 0x00070670 #define PHY_ID_VSC8530 0x00070560 #define PHY_ID_VSC8531 0x00070570 +#define PHY_ID_VSC8531_02 0x00070572 #define PHY_ID_VSC8540 0x00070760 #define PHY_ID_VSC8541 0x00070770 #define PHY_ID_VSC8552 0x000704e0 @@ -373,6 +374,8 @@ struct vsc8531_private { * package. */ unsigned int base_addr; + u32 rx_delay; + u32 tx_delay; #if IS_ENABLED(CONFIG_MACSEC) /* MACsec fields: diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c index 75d9582e5784..80cc90a23d57 100644 --- a/drivers/net/phy/mscc/mscc_main.c +++ b/drivers/net/phy/mscc/mscc_main.c @@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl, { u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1; u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1; + struct vsc8531_private *vsc8531 = phydev->priv; u16 reg_val = 0; int rc; @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl, if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos; + reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos; if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos; + reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos; rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, rgmii_cntl, @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev) { int rc, i, phy_id; struct vsc8531_private *vsc8531 = phydev->priv; + struct device_node *of_node = phydev->mdio.dev.of_node; + + vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS; + rc = of_property_read_u32(of_node, "mscc,rx-delay", + &vsc8531->rx_delay); + + vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS; + rc = of_property_read_u32(of_node, "mscc,tx-delay", + &vsc8531->tx_delay); rc = vsc85xx_default_config(phydev); if (rc) @@ -2413,9 +2423,8 @@ static struct phy_driver vsc85xx_driver[] = { .get_stats = &vsc85xx_get_stats, }, { - .phy_id = PHY_ID_VSC8531, + PHY_ID_MATCH_EXACT(PHY_ID_VSC8531), .name = "Microsemi VSC8531", - .phy_id_mask = 0xfffffff0, /* PHY_GBIT_FEATURES */ .soft_reset = &genphy_soft_reset, .config_init = &vsc85xx_config_init, @@ -2436,6 +2445,29 @@ static struct phy_driver vsc85xx_driver[] = { .get_strings = &vsc85xx_get_strings, .get_stats = &vsc85xx_get_stats, }, +{ + PHY_ID_MATCH_EXACT(PHY_ID_VSC8531_02), + .name = "Microsemi VSC8531-02", + /* PHY_GBIT_FEATURES */ + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &vsc85xx_config_aneg, + .read_status = &vsc85xx_read_status, + .handle_interrupt = vsc85xx_handle_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, + .probe = &vsc85xx_probe, + .set_wol = &vsc85xx_wol_set, + .get_wol = &vsc85xx_wol_get, + .get_tunable = &vsc85xx_get_tunable, + .set_tunable = &vsc85xx_set_tunable, + .read_page = &vsc85xx_phy_read_page, + .write_page = &vsc85xx_phy_write_page, + .get_sset_count = &vsc85xx_get_sset_count, + .get_strings = &vsc85xx_get_strings, + .get_stats = &vsc85xx_get_stats, +}, { .phy_id = PHY_ID_VSC8540, .name = "Microsemi FE VSC8540 SyncE",