| Message ID | 20210719082756.15733-1-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v6] net: phy: intel-xway: Add RGMII internal delay configuration | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 101 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > + int_delay = phy_get_internal_delay(phydev, dev, > + xway_internal_delay, > + delay_size, true); > + > + if (int_delay < 0) { > + phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n"); > + int_delay = 4; /* 2000 ps */ The binding say: rx-internal-delay-ps: description: | RGMII Receive PHY Clock Delay defined in pico seconds. This is used for PHY's that have configurable RX internal delays. If this property is present then the PHY applies the RX delay. So the property is optional. It being missing should not generate a warning. Please just use the default of 2ns. This makes the usage the same as the other drivers using phy_get_internal_delay(). Andrew
On 2021-07-19 22:56, Andrew Lunn wrote: >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { >> + int_delay = phy_get_internal_delay(phydev, dev, >> + xway_internal_delay, >> + delay_size, true); >> + >> + if (int_delay < 0) { >> + phydev_warn(phydev, "rx-internal-delay-ps is missing, use default >> of 2.0 ns\n"); >> + int_delay = 4; /* 2000 ps */ > > The binding say: > > rx-internal-delay-ps: > description: | > RGMII Receive PHY Clock Delay defined in pico seconds. This is > used for > PHY's that have configurable RX internal delays. If this > property is > present then the PHY applies the RX delay. > > So the property is optional. It being missing should not generate a > warning. Please just use the default of 2ns. This makes the usage the > same as the other drivers using phy_get_internal_delay(). > > Andrew OK, I'll remove the warnings. Thanks.
On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote: > > This adds the possibility to configure the RGMII RX/TX clock skew via > devicetree. > > Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and add > the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the > devicetree. > > Furthermore, a warning is now issued if the phy mode is configured to > "rgmii" and an internal delay is set in the phy (e.g. by pin-strapping), > as in the dp83867 driver. > > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > --- > > Changes to v5: > o remove #if IS_ENABLED(CONFIG_OF_MDIO) check > o rename new function to xway_gphy_rgmii_init() > > Changes to v4: > o Fix Alignment to match open parenthesis > > Changes to v3: > o Fix typo in commit message > o use FIELD_PREP() and FIELD_GET() macros > o further code cleanups > o always mask rxskew AND txskew value in the register value > > Changes to v2: > o Fix missing whitespace in warning. > > Changes to v1: > o code cleanup and use phy_modify(). > o use default of 2.0ns if delay property is absent instead of returning > an error. > > --- > drivers/net/phy/intel-xway.c | 78 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c > index d453ec016168..fd7da2eeb963 100644 > --- a/drivers/net/phy/intel-xway.c > +++ b/drivers/net/phy/intel-xway.c > @@ -8,11 +8,16 @@ > #include <linux/module.h> > #include <linux/phy.h> > #include <linux/of.h> > +#include <linux/bitfield.h> > > +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */ > #define XWAY_MDIO_IMASK 0x19 /* interrupt mask */ > #define XWAY_MDIO_ISTAT 0x1A /* interrupt status */ > #define XWAY_MDIO_LED 0x1B /* led control */ > > +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) > +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) > + > /* bit 15:12 are reserved */ > #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the integrated function of LED3 */ > #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the integrated function of LED2 */ > @@ -157,6 +162,75 @@ > #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 > #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 > > +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 2500, > + 3000, 3500}; > + > +static int xway_gphy_rgmii_init(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay); > + s32 int_delay; > + int val = 0; > + > + if (!phy_interface_is_rgmii(phydev)) > + return 0; > + > + /* Existing behavior was to use default pin strapping delay in rgmii > + * mode, but rgmii should have meant no delay. Warn existing users, > + * but do not change anything at the moment. > + */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { > + u16 txskew, rxskew; > + > + val = phy_read(phydev, XWAY_MDIO_MIICTRL); > + if (val < 0) > + return val; > + > + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); > + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val); > + > + if (txskew > 0 || rxskew > 0) > + phydev_warn(phydev, > + "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n" > + "Should be 'rgmii-id' to use internal delays txskew:%d ps rxskew:%d ps\n", > + xway_internal_delay[txskew], > + xway_internal_delay[rxskew]); > + return 0; > + } > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > + int_delay = phy_get_internal_delay(phydev, dev, > + xway_internal_delay, > + delay_size, true); > + > + if (int_delay < 0) { > + phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n"); > + int_delay = 4; /* 2000 ps */ > + } > + > + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, int_delay); > + } > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + int_delay = phy_get_internal_delay(phydev, dev, > + xway_internal_delay, > + delay_size, false); > + > + if (int_delay < 0) { > + phydev_warn(phydev, "tx-internal-delay-ps is missing, use default of 2.0 ns\n"); > + int_delay = 4; /* 2000 ps */ > + } > + > + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, int_delay); > + } > + > + return phy_modify(phydev, XWAY_MDIO_MIICTRL, > + XWAY_MDIO_MIICTRL_RXSKEW_MASK | > + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); > +} > + > static int xway_gphy_config_init(struct phy_device *phydev) > { > int err; > @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct phy_device *phydev) > phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh); > phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl); > > + err = xway_gphy_rgmii_init(phydev); > + if (err) > + return err; > + > return 0; > } > > -- > 2.20.1 > Martin, I've got some boards with the GPY111 phy on them and I'm finding that modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I do a soft reset (BCMR_RESET) first. I don't see anything in the datasheet which specifies this to be the case so I'm interested it what you have found. Are you sure adjusting the skews like this without a soft (or hard pin based) reset actually works? Best regards, Tim
On 2022-01-11 00:12, Tim Harvey wrote: > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> This adds the possibility to configure the RGMII RX/TX clock skew via >> devicetree. >> >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and >> add >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the >> devicetree. >> >> Furthermore, a warning is now issued if the phy mode is configured to >> "rgmii" and an internal delay is set in the phy (e.g. by >> pin-strapping), >> as in the dp83867 driver. >> >> Signed-off-by: Martin Schiller <ms@dev.tdt.de> >> --- >> >> Changes to v5: >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check >> o rename new function to xway_gphy_rgmii_init() >> >> Changes to v4: >> o Fix Alignment to match open parenthesis >> >> Changes to v3: >> o Fix typo in commit message >> o use FIELD_PREP() and FIELD_GET() macros >> o further code cleanups >> o always mask rxskew AND txskew value in the register value >> >> Changes to v2: >> o Fix missing whitespace in warning. >> >> Changes to v1: >> o code cleanup and use phy_modify(). >> o use default of 2.0ns if delay property is absent instead of >> returning >> an error. >> >> --- >> drivers/net/phy/intel-xway.c | 78 >> ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 78 insertions(+) >> >> diff --git a/drivers/net/phy/intel-xway.c >> b/drivers/net/phy/intel-xway.c >> index d453ec016168..fd7da2eeb963 100644 >> --- a/drivers/net/phy/intel-xway.c >> +++ b/drivers/net/phy/intel-xway.c >> @@ -8,11 +8,16 @@ >> #include <linux/module.h> >> #include <linux/phy.h> >> #include <linux/of.h> >> +#include <linux/bitfield.h> >> >> +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */ >> #define XWAY_MDIO_IMASK 0x19 /* interrupt >> mask */ >> #define XWAY_MDIO_ISTAT 0x1A /* interrupt >> status */ >> #define XWAY_MDIO_LED 0x1B /* led control */ >> >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) >> + >> /* bit 15:12 are reserved */ >> #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the >> integrated function of LED3 */ >> #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the >> integrated function of LED2 */ >> @@ -157,6 +162,75 @@ >> #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 >> #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 >> >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, >> 2500, >> + 3000, 3500}; >> + >> +static int xway_gphy_rgmii_init(struct phy_device *phydev) >> +{ >> + struct device *dev = &phydev->mdio.dev; >> + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay); >> + s32 int_delay; >> + int val = 0; >> + >> + if (!phy_interface_is_rgmii(phydev)) >> + return 0; >> + >> + /* Existing behavior was to use default pin strapping delay in >> rgmii >> + * mode, but rgmii should have meant no delay. Warn existing >> users, >> + * but do not change anything at the moment. >> + */ >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { >> + u16 txskew, rxskew; >> + >> + val = phy_read(phydev, XWAY_MDIO_MIICTRL); >> + if (val < 0) >> + return val; >> + >> + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, >> val); >> + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, >> val); >> + >> + if (txskew > 0 || rxskew > 0) >> + phydev_warn(phydev, >> + "PHY has delays (e.g. via pin >> strapping), but phy-mode = 'rgmii'\n" >> + "Should be 'rgmii-id' to use >> internal delays txskew:%d ps rxskew:%d ps\n", >> + xway_internal_delay[txskew], >> + xway_internal_delay[rxskew]); >> + return 0; >> + } >> + >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { >> + int_delay = phy_get_internal_delay(phydev, dev, >> + >> xway_internal_delay, >> + delay_size, true); >> + >> + if (int_delay < 0) { >> + phydev_warn(phydev, "rx-internal-delay-ps is >> missing, use default of 2.0 ns\n"); >> + int_delay = 4; /* 2000 ps */ >> + } >> + >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, >> int_delay); >> + } >> + >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { >> + int_delay = phy_get_internal_delay(phydev, dev, >> + >> xway_internal_delay, >> + delay_size, false); >> + >> + if (int_delay < 0) { >> + phydev_warn(phydev, "tx-internal-delay-ps is >> missing, use default of 2.0 ns\n"); >> + int_delay = 4; /* 2000 ps */ >> + } >> + >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, >> int_delay); >> + } >> + >> + return phy_modify(phydev, XWAY_MDIO_MIICTRL, >> + XWAY_MDIO_MIICTRL_RXSKEW_MASK | >> + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); >> +} >> + >> static int xway_gphy_config_init(struct phy_device *phydev) >> { >> int err; >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct >> phy_device *phydev) >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh); >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl); >> >> + err = xway_gphy_rgmii_init(phydev); >> + if (err) >> + return err; >> + >> return 0; >> } >> >> -- >> 2.20.1 >> > > Martin, > > I've got some boards with the GPY111 phy on them and I'm finding that > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I > do a soft reset (BCMR_RESET) first. I don't see anything in the > datasheet which specifies this to be the case so I'm interested it > what you have found. Are you sure adjusting the skews like this > without a soft (or hard pin based) reset actually works? > > Best regards, > > Tim Hello Tim, yes, you are right. It is not applied immediately. The link needs to be toggled to get this settings active. But my experience shows that this would be done in the further boot process anyway e.g. by restarting the autonegotiation etc. Regards, Martin
> > Martin, > > > > I've got some boards with the GPY111 phy on them and I'm finding that > > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I > > do a soft reset (BCMR_RESET) first. I don't see anything in the > > datasheet which specifies this to be the case so I'm interested it > > what you have found. Are you sure adjusting the skews like this > > without a soft (or hard pin based) reset actually works? > > > > Best regards, > > > > Tim > > Hello Tim, > > yes, you are right. It is not applied immediately. The link needs to be > toggled to get this settings active. But my experience shows that this > would be done in the further boot process anyway e.g. by restarting the > autonegotiation etc. Hi Martin Have you verified this? Maybe try NFS root, so the kernel is the one up'ing the interface, before user space exists. Andrew
On Mon, Jan 10, 2022 at 11:44 PM Martin Schiller <ms@dev.tdt.de> wrote: > > On 2022-01-11 00:12, Tim Harvey wrote: > > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote: > >> > >> This adds the possibility to configure the RGMII RX/TX clock skew via > >> devicetree. > >> > >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and > >> add > >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the > >> devicetree. > >> > >> Furthermore, a warning is now issued if the phy mode is configured to > >> "rgmii" and an internal delay is set in the phy (e.g. by > >> pin-strapping), > >> as in the dp83867 driver. > >> > >> Signed-off-by: Martin Schiller <ms@dev.tdt.de> > >> --- > >> > >> Changes to v5: > >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check > >> o rename new function to xway_gphy_rgmii_init() > >> > >> Changes to v4: > >> o Fix Alignment to match open parenthesis > >> > >> Changes to v3: > >> o Fix typo in commit message > >> o use FIELD_PREP() and FIELD_GET() macros > >> o further code cleanups > >> o always mask rxskew AND txskew value in the register value > >> > >> Changes to v2: > >> o Fix missing whitespace in warning. > >> > >> Changes to v1: > >> o code cleanup and use phy_modify(). > >> o use default of 2.0ns if delay property is absent instead of > >> returning > >> an error. > >> > >> --- > >> drivers/net/phy/intel-xway.c | 78 > >> ++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 78 insertions(+) > >> > >> diff --git a/drivers/net/phy/intel-xway.c > >> b/drivers/net/phy/intel-xway.c > >> index d453ec016168..fd7da2eeb963 100644 > >> --- a/drivers/net/phy/intel-xway.c > >> +++ b/drivers/net/phy/intel-xway.c > >> @@ -8,11 +8,16 @@ > >> #include <linux/module.h> > >> #include <linux/phy.h> > >> #include <linux/of.h> > >> +#include <linux/bitfield.h> > >> > >> +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */ > >> #define XWAY_MDIO_IMASK 0x19 /* interrupt > >> mask */ > >> #define XWAY_MDIO_ISTAT 0x1A /* interrupt > >> status */ > >> #define XWAY_MDIO_LED 0x1B /* led control */ > >> > >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) > >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) > >> + > >> /* bit 15:12 are reserved */ > >> #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the > >> integrated function of LED3 */ > >> #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the > >> integrated function of LED2 */ > >> @@ -157,6 +162,75 @@ > >> #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 > >> #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 > >> > >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, > >> 2500, > >> + 3000, 3500}; > >> + > >> +static int xway_gphy_rgmii_init(struct phy_device *phydev) > >> +{ > >> + struct device *dev = &phydev->mdio.dev; > >> + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay); > >> + s32 int_delay; > >> + int val = 0; > >> + > >> + if (!phy_interface_is_rgmii(phydev)) > >> + return 0; > >> + > >> + /* Existing behavior was to use default pin strapping delay in > >> rgmii > >> + * mode, but rgmii should have meant no delay. Warn existing > >> users, > >> + * but do not change anything at the moment. > >> + */ > >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { > >> + u16 txskew, rxskew; > >> + > >> + val = phy_read(phydev, XWAY_MDIO_MIICTRL); > >> + if (val < 0) > >> + return val; > >> + > >> + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, > >> val); > >> + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, > >> val); > >> + > >> + if (txskew > 0 || rxskew > 0) > >> + phydev_warn(phydev, > >> + "PHY has delays (e.g. via pin > >> strapping), but phy-mode = 'rgmii'\n" > >> + "Should be 'rgmii-id' to use > >> internal delays txskew:%d ps rxskew:%d ps\n", > >> + xway_internal_delay[txskew], > >> + xway_internal_delay[rxskew]); > >> + return 0; > >> + } > >> + > >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > >> + int_delay = phy_get_internal_delay(phydev, dev, > >> + > >> xway_internal_delay, > >> + delay_size, true); > >> + > >> + if (int_delay < 0) { > >> + phydev_warn(phydev, "rx-internal-delay-ps is > >> missing, use default of 2.0 ns\n"); > >> + int_delay = 4; /* 2000 ps */ > >> + } > >> + > >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, > >> int_delay); > >> + } > >> + > >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > >> + int_delay = phy_get_internal_delay(phydev, dev, > >> + > >> xway_internal_delay, > >> + delay_size, false); > >> + > >> + if (int_delay < 0) { > >> + phydev_warn(phydev, "tx-internal-delay-ps is > >> missing, use default of 2.0 ns\n"); > >> + int_delay = 4; /* 2000 ps */ > >> + } > >> + > >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, > >> int_delay); > >> + } > >> + > >> + return phy_modify(phydev, XWAY_MDIO_MIICTRL, > >> + XWAY_MDIO_MIICTRL_RXSKEW_MASK | > >> + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); > >> +} > >> + > >> static int xway_gphy_config_init(struct phy_device *phydev) > >> { > >> int err; > >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct > >> phy_device *phydev) > >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh); > >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl); > >> > >> + err = xway_gphy_rgmii_init(phydev); > >> + if (err) > >> + return err; > >> + > >> return 0; > >> } > >> > >> -- > >> 2.20.1 > >> > > > > Martin, > > > > I've got some boards with the GPY111 phy on them and I'm finding that > > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I > > do a soft reset (BCMR_RESET) first. I don't see anything in the > > datasheet which specifies this to be the case so I'm interested it > > what you have found. Are you sure adjusting the skews like this > > without a soft (or hard pin based) reset actually works? > > > > Best regards, > > > > Tim > > Hello Tim, > > yes, you are right. It is not applied immediately. The link needs to be > toggled to get this settings active. But my experience shows that this > would be done in the further boot process anyway e.g. by restarting the > autonegotiation etc. > Martin, I added a debug statement in xway_gphy_rgmii_init and here you can see it gets called 'before' the link comes up from the NIC on a board that has a cable plugged in at power-on. I can tell from testing that the rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take effect unless I then bring the link down and up again manually as you indicate. # dmesg | egrep "xway|nicvf" [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 rx_delay=1500 tx_delay=500 [ 6.999651] nicvf, ver 1.0 [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex If I add a 'genphy_soft_reset(phydev);' at the top of xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values do take effect so perhaps that's the proper fix. I'm not fond of even using this phy driver either as it blatantly forces LED configuration which may not agree with what boot firmware does. I've noticed phy drivers starting to configure LED behavior more and more but it seems like there should be dt bindings for that or maybe an option to preserve the configuration that is set from boot firmware. Best regards, Tim
On 2022-01-11 20:12, Tim Harvey wrote: > On Mon, Jan 10, 2022 at 11:44 PM Martin Schiller <ms@dev.tdt.de> wrote: >> >> On 2022-01-11 00:12, Tim Harvey wrote: >> > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> >> >> This adds the possibility to configure the RGMII RX/TX clock skew via >> >> devicetree. >> >> >> >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and >> >> add >> >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the >> >> devicetree. >> >> >> >> Furthermore, a warning is now issued if the phy mode is configured to >> >> "rgmii" and an internal delay is set in the phy (e.g. by >> >> pin-strapping), >> >> as in the dp83867 driver. >> >> >> >> Signed-off-by: Martin Schiller <ms@dev.tdt.de> >> >> --- >> >> >> >> Changes to v5: >> >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check >> >> o rename new function to xway_gphy_rgmii_init() >> >> >> >> Changes to v4: >> >> o Fix Alignment to match open parenthesis >> >> >> >> Changes to v3: >> >> o Fix typo in commit message >> >> o use FIELD_PREP() and FIELD_GET() macros >> >> o further code cleanups >> >> o always mask rxskew AND txskew value in the register value >> >> >> >> Changes to v2: >> >> o Fix missing whitespace in warning. >> >> >> >> Changes to v1: >> >> o code cleanup and use phy_modify(). >> >> o use default of 2.0ns if delay property is absent instead of >> >> returning >> >> an error. >> >> >> >> --- >> >> drivers/net/phy/intel-xway.c | 78 >> >> ++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 78 insertions(+) >> >> >> >> diff --git a/drivers/net/phy/intel-xway.c >> >> b/drivers/net/phy/intel-xway.c >> >> index d453ec016168..fd7da2eeb963 100644 >> >> --- a/drivers/net/phy/intel-xway.c >> >> +++ b/drivers/net/phy/intel-xway.c >> >> @@ -8,11 +8,16 @@ >> >> #include <linux/module.h> >> >> #include <linux/phy.h> >> >> #include <linux/of.h> >> >> +#include <linux/bitfield.h> >> >> >> >> +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */ >> >> #define XWAY_MDIO_IMASK 0x19 /* interrupt >> >> mask */ >> >> #define XWAY_MDIO_ISTAT 0x1A /* interrupt >> >> status */ >> >> #define XWAY_MDIO_LED 0x1B /* led control */ >> >> >> >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) >> >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) >> >> + >> >> /* bit 15:12 are reserved */ >> >> #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the >> >> integrated function of LED3 */ >> >> #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the >> >> integrated function of LED2 */ >> >> @@ -157,6 +162,75 @@ >> >> #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 >> >> #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 >> >> >> >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, >> >> 2500, >> >> + 3000, 3500}; >> >> + >> >> +static int xway_gphy_rgmii_init(struct phy_device *phydev) >> >> +{ >> >> + struct device *dev = &phydev->mdio.dev; >> >> + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay); >> >> + s32 int_delay; >> >> + int val = 0; >> >> + >> >> + if (!phy_interface_is_rgmii(phydev)) >> >> + return 0; >> >> + >> >> + /* Existing behavior was to use default pin strapping delay in >> >> rgmii >> >> + * mode, but rgmii should have meant no delay. Warn existing >> >> users, >> >> + * but do not change anything at the moment. >> >> + */ >> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { >> >> + u16 txskew, rxskew; >> >> + >> >> + val = phy_read(phydev, XWAY_MDIO_MIICTRL); >> >> + if (val < 0) >> >> + return val; >> >> + >> >> + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, >> >> val); >> >> + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, >> >> val); >> >> + >> >> + if (txskew > 0 || rxskew > 0) >> >> + phydev_warn(phydev, >> >> + "PHY has delays (e.g. via pin >> >> strapping), but phy-mode = 'rgmii'\n" >> >> + "Should be 'rgmii-id' to use >> >> internal delays txskew:%d ps rxskew:%d ps\n", >> >> + xway_internal_delay[txskew], >> >> + xway_internal_delay[rxskew]); >> >> + return 0; >> >> + } >> >> + >> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { >> >> + int_delay = phy_get_internal_delay(phydev, dev, >> >> + >> >> xway_internal_delay, >> >> + delay_size, true); >> >> + >> >> + if (int_delay < 0) { >> >> + phydev_warn(phydev, "rx-internal-delay-ps is >> >> missing, use default of 2.0 ns\n"); >> >> + int_delay = 4; /* 2000 ps */ >> >> + } >> >> + >> >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, >> >> int_delay); >> >> + } >> >> + >> >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { >> >> + int_delay = phy_get_internal_delay(phydev, dev, >> >> + >> >> xway_internal_delay, >> >> + delay_size, false); >> >> + >> >> + if (int_delay < 0) { >> >> + phydev_warn(phydev, "tx-internal-delay-ps is >> >> missing, use default of 2.0 ns\n"); >> >> + int_delay = 4; /* 2000 ps */ >> >> + } >> >> + >> >> + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, >> >> int_delay); >> >> + } >> >> + >> >> + return phy_modify(phydev, XWAY_MDIO_MIICTRL, >> >> + XWAY_MDIO_MIICTRL_RXSKEW_MASK | >> >> + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); >> >> +} >> >> + >> >> static int xway_gphy_config_init(struct phy_device *phydev) >> >> { >> >> int err; >> >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct >> >> phy_device *phydev) >> >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh); >> >> phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl); >> >> >> >> + err = xway_gphy_rgmii_init(phydev); >> >> + if (err) >> >> + return err; >> >> + >> >> return 0; >> >> } >> >> >> >> -- >> >> 2.20.1 >> >> >> > >> > Martin, >> > >> > I've got some boards with the GPY111 phy on them and I'm finding that >> > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I >> > do a soft reset (BCMR_RESET) first. I don't see anything in the >> > datasheet which specifies this to be the case so I'm interested it >> > what you have found. Are you sure adjusting the skews like this >> > without a soft (or hard pin based) reset actually works? >> > >> > Best regards, >> > >> > Tim >> >> Hello Tim, >> >> yes, you are right. It is not applied immediately. The link needs to >> be >> toggled to get this settings active. But my experience shows that this >> would be done in the further boot process anyway e.g. by restarting >> the >> autonegotiation etc. >> > > Martin, > > I added a debug statement in xway_gphy_rgmii_init and here you can see > it gets called 'before' the link comes up from the NIC on a board that > has a cable plugged in at power-on. I can tell from testing that the > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take > effect unless I then bring the link down and up again manually as you > indicate. > > # dmesg | egrep "xway|nicvf" > [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 > rx_delay=1500 tx_delay=500 > [ 6.999651] nicvf, ver 1.0 > [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 > [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) > [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 > [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) > [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full > duplex > > If I add a 'genphy_soft_reset(phydev);' at the top of > xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values > do take effect so perhaps that's the proper fix. OK, I see that we have to change something here. But I would like to avoid a complete reset (BMCR_RESET) if possible. How about this: Before configuring the skews set BMCR_PDOWN using genphy_suspend(phydev) and when we are done let's call genphy_resume(phydev). > > I'm not fond of even using this phy driver either as it blatantly > forces LED configuration which may not agree with what boot firmware > does. I've noticed phy drivers starting to configure LED behavior more > and more but it seems like there should be dt bindings for that or > maybe an option to preserve the configuration that is set from boot > firmware. There is already an approach which is used in openwrt [1] and which Hauke tried to get into the kernel [2]. Interesting would be also a solution like this approach here [3]. But this is rather off-topic now. [1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/patches-5.10/0023-NET-PHY-add-led-support-for-intel-xway.patch;h=fb8d97511066bd7e20f8cd298401e81f74e02ae9;hb=d337731f85c880acc96e8a6b99b62aeb57b8253f [2] https://www.spinics.net/lists/devicetree/msg129574.html [3] https://www.spinics.net/lists/linux-leds/msg17241.html
> > If I add a 'genphy_soft_reset(phydev);' at the top of > > xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values > > do take effect so perhaps that's the proper fix. > > OK, I see that we have to change something here. > But I would like to avoid a complete reset (BMCR_RESET) if possible. What does the datasheet say about BMCR_RESET? Some PHYs, like Marvell, it only resets the internal state machines. Register values are not changed back to defaults or anything like that. Also for many register writes in Marvell PHYs the write does not take effect until the next reset. So a BMCR_RESET can be the correct thing to do. Andrew
On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote: > I added a debug statement in xway_gphy_rgmii_init and here you can see > it gets called 'before' the link comes up from the NIC on a board that > has a cable plugged in at power-on. I can tell from testing that the > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take > effect unless I then bring the link down and up again manually as you > indicate. > > # dmesg | egrep "xway|nicvf" > [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 > rx_delay=1500 tx_delay=500 > [ 6.999651] nicvf, ver 1.0 > [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 > [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) > [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 > [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) > [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex Does the kernel message about the link coming up reflect what is going on physically with the link though? If a network interface is down, it's entirely possible that the link is already established at the hardware level, buit the "Link is Up" message gets reported when the network interface is later brought up. So, debugging this by looking at the kernel messages is unreliable.
On Wed, Jan 12, 2022 at 5:14 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > If I add a 'genphy_soft_reset(phydev);' at the top of > > > xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values > > > do take effect so perhaps that's the proper fix. > > > > OK, I see that we have to change something here. > > But I would like to avoid a complete reset (BMCR_RESET) if possible. > > What does the datasheet say about BMCR_RESET? Some PHYs, like Marvell, > it only resets the internal state machines. Register values are not > changed back to defaults or anything like that. Also for many register > writes in Marvell PHYs the write does not take effect until the next > reset. > > So a BMCR_RESET can be the correct thing to do. > Andrew, Datasheet [1] says "Resets the PHY to its default state. Active links are terminated. Note that this is a self-clearing bit which is set to zero by the hardware after reset has been done. See also IEEE 802.3-2008 22.2.4.1.1." Experimentally I can change the delays and read them back as such, then issue a BMCR_RESET and read them and they revert to strapped values so I know BMCR_RESET resets at least some of the registers. I suppose something to force auto-negotiation to occur again (BMCR_ANEN?) would suffice. I'm not sure what the best course of action is. Tim [1] https://assets.maxlinear.com/web/documents/618152_gpy111_pef7071vv16_hd_rev1.5.pdf
On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote: > > I added a debug statement in xway_gphy_rgmii_init and here you can see > > it gets called 'before' the link comes up from the NIC on a board that > > has a cable plugged in at power-on. I can tell from testing that the > > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take > > effect unless I then bring the link down and up again manually as you > > indicate. > > > > # dmesg | egrep "xway|nicvf" > > [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 > > rx_delay=1500 tx_delay=500 > > [ 6.999651] nicvf, ver 1.0 > > [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 > > [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) > > [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 > > [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) > > [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex > > Does the kernel message about the link coming up reflect what is going > on physically with the link though? > > If a network interface is down, it's entirely possible that the link is > already established at the hardware level, buit the "Link is Up" message > gets reported when the network interface is later brought up. So, > debugging this by looking at the kernel messages is unreliable. > Russell, You are correct... the link doesn't come up at that point its already linked. So we need to force a reset or an auto negotiation reset after modifying the delays. Tim
On 2022-01-12 19:25, Tim Harvey wrote: > On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: >> >> On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote: >> > I added a debug statement in xway_gphy_rgmii_init and here you can see >> > it gets called 'before' the link comes up from the NIC on a board that >> > has a cable plugged in at power-on. I can tell from testing that the >> > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take >> > effect unless I then bring the link down and up again manually as you >> > indicate. >> > >> > # dmesg | egrep "xway|nicvf" >> > [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 >> > rx_delay=1500 tx_delay=500 >> > [ 6.999651] nicvf, ver 1.0 >> > [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 >> > [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) >> > [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 >> > [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) >> > [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex >> >> Does the kernel message about the link coming up reflect what is going >> on physically with the link though? >> >> If a network interface is down, it's entirely possible that the link >> is >> already established at the hardware level, buit the "Link is Up" >> message >> gets reported when the network interface is later brought up. So, >> debugging this by looking at the kernel messages is unreliable. >> > > Russell, > > You are correct... the link doesn't come up at that point its already > linked. So we need to force a reset or an auto negotiation reset after > modifying the delays. > > Tim Setting BMCR_ANRESTART would work, but only if BMCR_ANENABLE is also or already set. Otherwise BMCR_ANRESTART has no effect (see the note in the datasheet). This is the reason why I came up with the idea of BMCR_PDOWN. Personally I would have no problem with setting BMCR_ANRESTART and BMCR_ANENABLE, but it would possibly change the existing configuration if (e.g. by the bootloader) aneg should be disabled. Martin
On Wed, Jan 12, 2022 at 10:32 PM Martin Schiller <ms@dev.tdt.de> wrote: > > On 2022-01-12 19:25, Tim Harvey wrote: > > On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > >> > >> On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote: > >> > I added a debug statement in xway_gphy_rgmii_init and here you can see > >> > it gets called 'before' the link comes up from the NIC on a board that > >> > has a cable plugged in at power-on. I can tell from testing that the > >> > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take > >> > effect unless I then bring the link down and up again manually as you > >> > indicate. > >> > > >> > # dmesg | egrep "xway|nicvf" > >> > [ 6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100 > >> > rx_delay=1500 tx_delay=500 > >> > [ 6.999651] nicvf, ver 1.0 > >> > [ 7.002478] nicvf 0000:05:00.1: Adding to iommu group 7 > >> > [ 7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006) > >> > [ 7.053189] nicvf 0000:05:00.2: Adding to iommu group 8 > >> > [ 7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006) > >> > [ 11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex > >> > >> Does the kernel message about the link coming up reflect what is going > >> on physically with the link though? > >> > >> If a network interface is down, it's entirely possible that the link > >> is > >> already established at the hardware level, buit the "Link is Up" > >> message > >> gets reported when the network interface is later brought up. So, > >> debugging this by looking at the kernel messages is unreliable. > >> > > > > Russell, > > > > You are correct... the link doesn't come up at that point its already > > linked. So we need to force a reset or an auto negotiation reset after > > modifying the delays. > > > > Tim > > Setting BMCR_ANRESTART would work, but only if BMCR_ANENABLE is also or > already set. Otherwise BMCR_ANRESTART has no effect (see the note in the > datasheet). > > This is the reason why I came up with the idea of BMCR_PDOWN. > > Personally I would have no problem with setting BMCR_ANRESTART and > BMCR_ANENABLE, but it would possibly change the existing configuration > if (e.g. by the bootloader) aneg should be disabled. > Martin, Sorry for the silence - I've been trying to figure out if and how I can deal with some very nasty errata on this particular PHY that can cause the link to not be stable and/or excessive errors in packets sent to the MAC. I do like the idea of BMCR_PDOWN. With my board its pretty obvious if the pin-strapped rx/tx delays are being used rather than the ones configured in the phy driver, so I'll have to do some testing when I find some time. However, I don't at all like the fact that this particular patch defaults the delays to 2ns if 'rx-internal-delay-ps' and 'tx-internal-delay-ps' is missing from the dt. These properties were added via Dan Murphy's series 'RGMII Internal delay common property' which was merged into v5.9: 8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay") 736b25afe284 ("net: dp83869: Add RGMII internal delay configuration") 2fb305c37d5b ("dt-bindings: net: Add RGMII internal delay for DP83869") 92252eec913b ("net: phy: Add a helper to return the index for of the internal delay") 9150069bf5fc dt-bindings: net: Add tx and rx internal delays The issue I have here is that if dt's have not been updated to add the common delay properties this code will override what the boot firmware may have configured them to. I feel that if these common delay properties are not found in the device tree, then no changes to the delays should be made at all. Best Regards, Tim
> However, I don't at all like the fact that this > particular patch defaults the delays to 2ns if 'rx-internal-delay-ps' > and 'tx-internal-delay-ps' is missing from the dt. How does this work in combination with phy-mode 'rgmii', 'rgmii-id' etc? Using 2ns as a default for rgmii-id is sensible, however i would say it is wrong for rgmii. > The issue I have here is that if dt's have not been updated to add the > common delay properties this code will override what the boot firmware > may have configured them to. I feel that if these common delay > properties are not found in the device tree, then no changes to the > delays should be made at all. If you don't want the PHY driver to touch the delays at all because you know something else has set it up, you can use phy-mode="", which should be interpreted as PHY_INTERFACE_MODE_NA. Andrew
Hi Tim, Hi Martin, > I've got some boards with the GPY111 phy on them and I'm finding that > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I > do a soft reset (BCMR_RESET) first. I don't see anything in the > datasheet which specifies this to be the case so I'm interested it > what you have found. Are you sure adjusting the skews like this > without a soft (or hard pin based) reset actually works? I do have the same PHY and I'm puzzled with the delay settings. Do you have an EEPROM attached to the PHY? According to my datasheet, that seems to make a difference. Apparently, only if there is an EEPROM, you can change the value (the value is then also written to the EEPROM according the datasheet). If you don't have one, the values will get overwritten by the external strappings on a soft reset. Therefore, it seems they cannot be set. (FWIW there is also a sticky bit, but that doesn't seem to help in this case). -michael
On 2023-02-22 17:04, Michael Walle wrote: > Hi Tim, Hi Martin, > >> I've got some boards with the GPY111 phy on them and I'm finding that >> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I >> do a soft reset (BCMR_RESET) first. I don't see anything in the >> datasheet which specifies this to be the case so I'm interested it >> what you have found. Are you sure adjusting the skews like this >> without a soft (or hard pin based) reset actually works? > > I do have the same PHY and I'm puzzled with the delay settings. Do > you have an EEPROM attached to the PHY? According to my datasheet, > that seems to make a difference. Apparently, only if there is an > EEPROM, you can change the value (the value is then also written to > the EEPROM according the datasheet). > If you don't have one, the values will get overwritten by the > external strappings on a soft reset. Therefore, it seems they cannot > be set. (FWIW there is also a sticky bit, but that doesn't seem to > help in this case). > > -michael Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to this register has no impact on operation of the device". But changing this settings without an EEPROM indeed has an impact. We don't use an EEPROM and without tuning this values some boards are unable to communicate on the ethernet port(s). I varied these values during operation in the uboot and was able to test the limits very nicely. I wouldn't have introduced this feature if it hasn't got any impact. Regards, Martin
Hi Martin, Am 2023-02-24 07:25, schrieb Martin Schiller: > On 2023-02-22 17:04, Michael Walle wrote: >> Hi Tim, Hi Martin, >> >>> I've got some boards with the GPY111 phy on them and I'm finding that >>> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I >>> do a soft reset (BCMR_RESET) first. I don't see anything in the >>> datasheet which specifies this to be the case so I'm interested it >>> what you have found. Are you sure adjusting the skews like this >>> without a soft (or hard pin based) reset actually works? >> >> I do have the same PHY and I'm puzzled with the delay settings. Do >> you have an EEPROM attached to the PHY? According to my datasheet, >> that seems to make a difference. Apparently, only if there is an >> EEPROM, you can change the value (the value is then also written to >> the EEPROM according the datasheet). >> If you don't have one, the values will get overwritten by the >> external strappings on a soft reset. Therefore, it seems they cannot >> be set. (FWIW there is also a sticky bit, but that doesn't seem to >> help in this case). >> >> -michael > > Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to > this register has no impact on operation of the device". > > But changing this settings without an EEPROM indeed has an impact. > > We don't use an EEPROM and without tuning this values some boards are > unable to communicate on the ethernet port(s). Thanks for confirming! Could you share your PHYID1/PHYID2 register and firmware version (FWV, 0x1E) contents? In our case, any changes in MIICTRL are lost after a soft reset. > I varied these values during operation in the uboot and was able to > test > the limits very nicely. So I guess, the value you write into MIICTRL are retained on a soft reset. I.e. mii write <phyad> 0x17 0xffff mii write <phyad> 0x00 0x8000 mii read <phyad> 0x17 will still return 0xffff? > > I wouldn't have introduced this feature if it hasn't got any impact. Sure, I'm just trying to figure out the differences ;) Thanks, Michael
On 2023-02-24 09:04, Michael Walle wrote: > Hi Martin, > > Am 2023-02-24 07:25, schrieb Martin Schiller: >> On 2023-02-22 17:04, Michael Walle wrote: >>> Hi Tim, Hi Martin, >>> >>>> I've got some boards with the GPY111 phy on them and I'm finding >>>> that >>>> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless >>>> I >>>> do a soft reset (BCMR_RESET) first. I don't see anything in the >>>> datasheet which specifies this to be the case so I'm interested it >>>> what you have found. Are you sure adjusting the skews like this >>>> without a soft (or hard pin based) reset actually works? >>> >>> I do have the same PHY and I'm puzzled with the delay settings. Do >>> you have an EEPROM attached to the PHY? According to my datasheet, >>> that seems to make a difference. Apparently, only if there is an >>> EEPROM, you can change the value (the value is then also written to >>> the EEPROM according the datasheet). >>> If you don't have one, the values will get overwritten by the >>> external strappings on a soft reset. Therefore, it seems they cannot >>> be set. (FWIW there is also a sticky bit, but that doesn't seem to >>> help in this case). >>> >>> -michael >> >> Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to >> this register has no impact on operation of the device". >> >> But changing this settings without an EEPROM indeed has an impact. >> >> We don't use an EEPROM and without tuning this values some boards are >> unable to communicate on the ethernet port(s). > > Thanks for confirming! Could you share your PHYID1/PHYID2 register and > firmware version (FWV, 0x1E) contents? I've 2 PHYs integrated into the VR268 SoC which shows this values: STD_PHYID1(reg 0x02): 0xd565 STD_PHYID2(reg 0x03): 0xa409 PHY_FWV (reg 0x1E): 0x8435 And then there are 2 external GPY111 with this values: STD_PHYID1(reg 0x02): 0xd565 STD_PHYID2(reg 0x03): 0xa401 PHY_FWV (reg 0x1E): 0x8435 And one external GPY112 with this values: STD_PHYID1(reg 0x02): 0xd565 STD_PHYID2(reg 0x03): 0xa401 PHY_FWV (reg 0x1E): 0x8435 > > In our case, any changes in MIICTRL are lost after a soft reset. > >> I varied these values during operation in the uboot and was able to >> test >> the limits very nicely. > > So I guess, the value you write into MIICTRL are retained on a soft > reset. No, the value I write into MIICTRL are not retained on a soft reset. > I.e. > > mii write <phyad> 0x17 0xffff > mii write <phyad> 0x00 0x8000 > mii read <phyad> 0x17 > > will still return 0xffff? In my tests I always set the skew values in register 0x17 first and then triggered a restart of the ANEG via register 0x0. This then led to the new values being adopted. > >> >> I wouldn't have introduced this feature if it hasn't got any impact. > > Sure, I'm just trying to figure out the differences ;) > > Thanks, > Michael - Martin
Hi Martin, > In my tests I always set the skew values in register 0x17 first and > then triggered a restart of the ANEG via register 0x0. This then led to > the new values being adopted. Right. I missed that. And I can confirm this is working perfectly although mentioned otherwise in the datasheet. The answer FAE anser is still outstanding. Thank you! -michael
diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c index d453ec016168..fd7da2eeb963 100644 --- a/drivers/net/phy/intel-xway.c +++ b/drivers/net/phy/intel-xway.c @@ -8,11 +8,16 @@ #include <linux/module.h> #include <linux/phy.h> #include <linux/of.h> +#include <linux/bitfield.h> +#define XWAY_MDIO_MIICTRL 0x17 /* mii control */ #define XWAY_MDIO_IMASK 0x19 /* interrupt mask */ #define XWAY_MDIO_ISTAT 0x1A /* interrupt status */ #define XWAY_MDIO_LED 0x1B /* led control */ +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) + /* bit 15:12 are reserved */ #define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the integrated function of LED3 */ #define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the integrated function of LED2 */ @@ -157,6 +162,75 @@ #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 2500, + 3000, 3500}; + +static int xway_gphy_rgmii_init(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + unsigned int delay_size = ARRAY_SIZE(xway_internal_delay); + s32 int_delay; + int val = 0; + + if (!phy_interface_is_rgmii(phydev)) + return 0; + + /* Existing behavior was to use default pin strapping delay in rgmii + * mode, but rgmii should have meant no delay. Warn existing users, + * but do not change anything at the moment. + */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { + u16 txskew, rxskew; + + val = phy_read(phydev, XWAY_MDIO_MIICTRL); + if (val < 0) + return val; + + txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); + rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val); + + if (txskew > 0 || rxskew > 0) + phydev_warn(phydev, + "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n" + "Should be 'rgmii-id' to use internal delays txskew:%d ps rxskew:%d ps\n", + xway_internal_delay[txskew], + xway_internal_delay[rxskew]); + return 0; + } + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { + int_delay = phy_get_internal_delay(phydev, dev, + xway_internal_delay, + delay_size, true); + + if (int_delay < 0) { + phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n"); + int_delay = 4; /* 2000 ps */ + } + + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, int_delay); + } + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + int_delay = phy_get_internal_delay(phydev, dev, + xway_internal_delay, + delay_size, false); + + if (int_delay < 0) { + phydev_warn(phydev, "tx-internal-delay-ps is missing, use default of 2.0 ns\n"); + int_delay = 4; /* 2000 ps */ + } + + val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, int_delay); + } + + return phy_modify(phydev, XWAY_MDIO_MIICTRL, + XWAY_MDIO_MIICTRL_RXSKEW_MASK | + XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); +} + static int xway_gphy_config_init(struct phy_device *phydev) { int err; @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct phy_device *phydev) phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh); phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl); + err = xway_gphy_rgmii_init(phydev); + if (err) + return err; + return 0; }
This adds the possibility to configure the RGMII RX/TX clock skew via devicetree. Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and add the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the devicetree. Furthermore, a warning is now issued if the phy mode is configured to "rgmii" and an internal delay is set in the phy (e.g. by pin-strapping), as in the dp83867 driver. Signed-off-by: Martin Schiller <ms@dev.tdt.de> --- Changes to v5: o remove #if IS_ENABLED(CONFIG_OF_MDIO) check o rename new function to xway_gphy_rgmii_init() Changes to v4: o Fix Alignment to match open parenthesis Changes to v3: o Fix typo in commit message o use FIELD_PREP() and FIELD_GET() macros o further code cleanups o always mask rxskew AND txskew value in the register value Changes to v2: o Fix missing whitespace in warning. Changes to v1: o code cleanup and use phy_modify(). o use default of 2.0ns if delay property is absent instead of returning an error. --- drivers/net/phy/intel-xway.c | 78 ++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)