| Message ID | 20210709164216.18561-1-ms@dev.tdt.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v3] net: phy: intel-xway: Add RGMII internal delay configuration | expand |
Hi Martin, overall this is looking good. A few comments below - I think none of them is a "must change" in my opinion. On Fri, Jul 9, 2021 at 6:42 PM Martin Schiller <ms@dev.tdt.de> wrote: > > This adds the posibility to configure the RGMII RX/TX clock skew via typo: posibility -> possibility [...] > +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) > +#define XWAY_MDIO_MIICTRL_RXSKEW_SHIFT 12 if you use - FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, rxskew); (as for example [0] does) - and FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val); below then you can drop the _SHIFT #define this is purely cosmetic though, so nothing which blocks this from being merged > +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) > +#define XWAY_MDIO_MIICTRL_TXSKEW_SHIFT 8 same as above [...] > +#if IS_ENABLED(CONFIG_OF_MDIO) is there any particular reason why we need to guard this with CONFIG_OF_MDIO? The dp83822 driver does not use this #if either (as far as I understand at least) [...] > +static int xway_gphy_of_reg_init(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + int delay_size = ARRAY_SIZE(xway_internal_delay); Some people in the kernel community are working on automatically detecting and fixing signedness issues. I am not sure if they would find this at some point suggesting that it can be an "unsigned int". > + s32 rx_int_delay; > + s32 tx_int_delay; xway_gphy14_config_aneg() below defines two variables in one line, so to be consistent this would be: s32 rx_int_delay, tx_int_delay; another option is to just re-use one "int_delay" variable (as it seems that they're both used in different code-paths). > + u16 mask = 0; I think this should be dropped and the phy_modify() call below should read: return phy_modify(phydev, XWAY_MDIO_MIICTRL, XWAY_MDIO_MIICTRL_RXSKEW_MASK | XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); For rgmii-txid the RX delay might be provided by the MAC or PCB trace length so the PHY should not add any RX delay. Similarly for rgmii-rxid the TX delay might be provided by the MAC or PCB trace length so the PHY should not add any TX delay. That means we always need to mask the RX and TX skew bits, regardless of what we're setting later on (as phy_modify is only called for one of: rgmii-id, rgmii-txid, rgmii-rxid). [...] > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > + rx_int_delay = phy_get_internal_delay(phydev, dev, > + &xway_internal_delay[0], I think above line can be simplified as: xway_internal_delay, [...] > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + tx_int_delay = phy_get_internal_delay(phydev, dev, > + &xway_internal_delay[0], same as above Best regards, Martin [0] https://elixir.bootlin.com/linux/v5.13/source/drivers/net/phy/dp83867.c#L438
On 2021-07-09 20:31, Martin Blumenstingl wrote: > Hi Martin, > > overall this is looking good. > A few comments below - I think none of them is a "must change" in my > opinion. > > On Fri, Jul 9, 2021 at 6:42 PM Martin Schiller <ms@dev.tdt.de> wrote: >> >> This adds the posibility to configure the RGMII RX/TX clock skew via > typo: posibility -> possibility > Thanks. I'll fix that. > [...] >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) >> +#define XWAY_MDIO_MIICTRL_RXSKEW_SHIFT 12 > if you use > - FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, rxskew); (as for example > [0] does) > - and FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val); > below then you can drop the _SHIFT #define > this is purely cosmetic though, so nothing which blocks this from being > merged > >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) >> +#define XWAY_MDIO_MIICTRL_TXSKEW_SHIFT 8 > same as above > Thanks for the hint. I'll switch to the FIELD_... macros. > [...] >> +#if IS_ENABLED(CONFIG_OF_MDIO) > is there any particular reason why we need to guard this with > CONFIG_OF_MDIO? > The dp83822 driver does not use this #if either (as far as I > understand at least) > It makes no sense to retrieve properties from the device tree if we are compiling for a target that does not support a device tree. At least that is my understanding of this condition. > [...] >> +static int xway_gphy_of_reg_init(struct phy_device *phydev) >> +{ >> + struct device *dev = &phydev->mdio.dev; >> + int delay_size = ARRAY_SIZE(xway_internal_delay); > Some people in the kernel community are working on automatically > detecting and fixing signedness issues. > I am not sure if they would find this at some point suggesting that it > can be an "unsigned int". > OK, I'll change that. >> + s32 rx_int_delay; >> + s32 tx_int_delay; > xway_gphy14_config_aneg() below defines two variables in one line, so > to be consistent this would be: > s32 rx_int_delay, tx_int_delay; > another option is to just re-use one "int_delay" variable (as it seems > that they're both used in different code-paths). > I'll switch to use one "int_delay". >> + u16 mask = 0; > I think this should be dropped and the phy_modify() call below should > read: > return phy_modify(phydev, XWAY_MDIO_MIICTRL, > XWAY_MDIO_MIICTRL_RXSKEW_MASK | > XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); > For rgmii-txid the RX delay might be provided by the MAC or PCB trace > length so the PHY should not add any RX delay. > Similarly for rgmii-rxid the TX delay might be provided by the MAC or > PCB trace length so the PHY should not add any TX delay. > That means we always need to mask the RX and TX skew bits, regardless > of what we're setting later on (as phy_modify is only called for one > of: rgmii-id, rgmii-txid, rgmii-rxid). > Yes, you are right. I'll change that as suggested. > [...] >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { >> + rx_int_delay = phy_get_internal_delay(phydev, dev, >> + >> &xway_internal_delay[0], > I think above line can be simplified as: > xway_internal_delay, I've copied that from dp83869.c, but yes, I can change it. > > [...] >> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || >> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { >> + tx_int_delay = phy_get_internal_delay(phydev, dev, >> + >> &xway_internal_delay[0], > same as above > dito. > > Best regards, > Martin > > > [0] > https://elixir.bootlin.com/linux/v5.13/source/drivers/net/phy/dp83867.c#L438
> > [...] > > > +#if IS_ENABLED(CONFIG_OF_MDIO) > > is there any particular reason why we need to guard this with > > CONFIG_OF_MDIO? > > The dp83822 driver does not use this #if either (as far as I > > understand at least) > > > > It makes no sense to retrieve properties from the device tree if we are > compiling for a target that does not support a device tree. > At least that is my understanding of this condition. There should be stubs for all these functions, so if OF is not part of the configured kernel, stub functions take their place. That has the advantage of at least compiling the code, so checking parameter types etc. We try to avoid #ifdef where possible, so we get better compiler build test coverage. The more #ifef there are, the more different configurations that need compiling in order to get build coverage. Andrew
On 7/13/21 3:06 PM, Andrew Lunn wrote: >>> [...] >>>> +#if IS_ENABLED(CONFIG_OF_MDIO) >>> is there any particular reason why we need to guard this with >>> CONFIG_OF_MDIO? >>> The dp83822 driver does not use this #if either (as far as I >>> understand at least) >>> >> >> It makes no sense to retrieve properties from the device tree if we are >> compiling for a target that does not support a device tree. >> At least that is my understanding of this condition. > > There should be stubs for all these functions, so if OF is not part of > the configured kernel, stub functions take their place. That has the > advantage of at least compiling the code, so checking parameter types > etc. We try to avoid #ifdef where possible, so we get better compiler > build test coverage. The more #ifef there are, the more different > configurations that need compiling in order to get build coverage. > > Andrew > The phy_get_internal_delay() function does not have a stub function directly, but it calls phy_get_int_delay_property() which has a stub, if CONFIG_OF_MDIO is not set, see: https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/net/phy/phy_device.c#L2797 The extra ifdef in the PHY driver only saves some code in the HY driver, but it should still work as before on systems without CONFIG_OF_MDIO. I would also prefer to remove the ifdef from the intel-xway phy driver. Hauke
On 2021-07-17 18:38, Hauke Mehrtens wrote: > On 7/13/21 3:06 PM, Andrew Lunn wrote: >>>> [...] >>>>> +#if IS_ENABLED(CONFIG_OF_MDIO) >>>> is there any particular reason why we need to guard this with >>>> CONFIG_OF_MDIO? >>>> The dp83822 driver does not use this #if either (as far as I >>>> understand at least) >>>> >>> >>> It makes no sense to retrieve properties from the device tree if we >>> are >>> compiling for a target that does not support a device tree. >>> At least that is my understanding of this condition. >> >> There should be stubs for all these functions, so if OF is not part of >> the configured kernel, stub functions take their place. That has the >> advantage of at least compiling the code, so checking parameter types >> etc. We try to avoid #ifdef where possible, so we get better compiler >> build test coverage. The more #ifef there are, the more different >> configurations that need compiling in order to get build coverage. >> >> Andrew >> > > The phy_get_internal_delay() function does not have a stub function > directly, but it calls phy_get_int_delay_property() which has a stub, > if CONFIG_OF_MDIO is not set, see: > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/net/phy/phy_device.c#L2797 > > The extra ifdef in the PHY driver only saves some code in the HY > driver, but it should still work as before on systems without > CONFIG_OF_MDIO. > > I would also prefer to remove the ifdef from the intel-xway phy driver. > > Hauke OK, so I'll remove the ifdef from the driver.
diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c index d453ec016168..796e6f2eb2d5 100644 --- a/drivers/net/phy/intel-xway.c +++ b/drivers/net/phy/intel-xway.c @@ -9,10 +9,16 @@ #include <linux/phy.h> #include <linux/of.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_RXSKEW_SHIFT 12 +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) +#define XWAY_MDIO_MIICTRL_TXSKEW_SHIFT 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 +163,86 @@ #define PHY_ID_PHY11G_VR9_1_2 0xD565A409 #define PHY_ID_PHY22F_VR9_1_2 0xD565A419 +#if IS_ENABLED(CONFIG_OF_MDIO) +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 2500, + 3000, 3500}; + +static int xway_gphy_of_reg_init(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + int delay_size = ARRAY_SIZE(xway_internal_delay); + s32 rx_int_delay; + s32 tx_int_delay; + u16 mask = 0; + 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 = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >> + XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; + rxskew = (val & XWAY_MDIO_MIICTRL_RXSKEW_MASK) >> + XWAY_MDIO_MIICTRL_RXSKEW_SHIFT; + + 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) { + rx_int_delay = phy_get_internal_delay(phydev, dev, + &xway_internal_delay[0], + delay_size, true); + + if (rx_int_delay < 0) { + phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n"); + rx_int_delay = 4; /* 2000 ps */ + } + + mask |= XWAY_MDIO_MIICTRL_RXSKEW_MASK; + val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT; + } + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + tx_int_delay = phy_get_internal_delay(phydev, dev, + &xway_internal_delay[0], + delay_size, false); + + if (tx_int_delay < 0) { + phydev_warn(phydev, "tx-internal-delay-ps is missing, use default of 2.0 ns\n"); + tx_int_delay = 4; /* 2000 ps */ + } + + mask |= XWAY_MDIO_MIICTRL_TXSKEW_MASK; + val |= tx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; + } + + return phy_modify(phydev, XWAY_MDIO_MIICTRL, mask, val); +} +#else +static int xway_gphy_of_reg_init(struct phy_device *phydev) +{ + return 0; +} +#endif /* CONFIG_OF_MDIO */ + static int xway_gphy_config_init(struct phy_device *phydev) { int err; @@ -204,6 +290,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_of_reg_init(phydev); + if (err) + return err; + return 0; }
This adds the posibility 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 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 | 90 ++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)