| Message ID | 20200602164522.3276-5-dmurphy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | RGMII Internal delay common property | expand |
On 6/2/2020 9:45 AM, Dan Murphy wrote: > Add RGMII internal delay configuration for Rx and Tx. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- [snip] > + > enum { > DP83869_PORT_MIRRORING_KEEP, > DP83869_PORT_MIRRORING_EN, > @@ -108,6 +113,8 @@ enum { > struct dp83869_private { > int tx_fifo_depth; > int rx_fifo_depth; > + s32 rx_id_delay; > + s32 tx_id_delay; > int io_impedance; > int port_mirroring; > bool rxctrl_strap_quirk; > @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device *phydev) > &dp83869->tx_fifo_depth)) > dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; > > + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", > + &dp83869->rx_id_delay); > + if (ret) { > + dp83869->rx_id_delay = > + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; > + ret = 0; > + } > + > + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", > + &dp83869->tx_id_delay); > + if (ret) { > + dp83869->tx_id_delay = > + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; > + ret = 0; > + } It is still not clear to me why is not the parsing being done by the PHY library helper directly?
Florian On 6/2/20 5:33 PM, Florian Fainelli wrote: > > On 6/2/2020 9:45 AM, Dan Murphy wrote: >> Add RGMII internal delay configuration for Rx and Tx. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- > [snip] > >> + >> enum { >> DP83869_PORT_MIRRORING_KEEP, >> DP83869_PORT_MIRRORING_EN, >> @@ -108,6 +113,8 @@ enum { >> struct dp83869_private { >> int tx_fifo_depth; >> int rx_fifo_depth; >> + s32 rx_id_delay; >> + s32 tx_id_delay; >> int io_impedance; >> int port_mirroring; >> bool rxctrl_strap_quirk; >> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device *phydev) >> &dp83869->tx_fifo_depth)) >> dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; >> >> + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", >> + &dp83869->rx_id_delay); >> + if (ret) { >> + dp83869->rx_id_delay = >> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >> + ret = 0; >> + } >> + >> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >> + &dp83869->tx_id_delay); >> + if (ret) { >> + dp83869->tx_id_delay = >> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >> + ret = 0; >> + } > It is still not clear to me why is not the parsing being done by the PHY > library helper directly? Why would we do that for these properties and not any other? Unless there is a new precedence being set here by having the PHY framework do all the dt node parsing for common properties. Dan
On 6/2/2020 4:10 PM, Dan Murphy wrote: > Florian > > On 6/2/20 5:33 PM, Florian Fainelli wrote: >> >> On 6/2/2020 9:45 AM, Dan Murphy wrote: >>> Add RGMII internal delay configuration for Rx and Tx. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >> [snip] >> >>> + >>> enum { >>> DP83869_PORT_MIRRORING_KEEP, >>> DP83869_PORT_MIRRORING_EN, >>> @@ -108,6 +113,8 @@ enum { >>> struct dp83869_private { >>> int tx_fifo_depth; >>> int rx_fifo_depth; >>> + s32 rx_id_delay; >>> + s32 tx_id_delay; >>> int io_impedance; >>> int port_mirroring; >>> bool rxctrl_strap_quirk; >>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device >>> *phydev) >>> &dp83869->tx_fifo_depth)) >>> dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; >>> + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", >>> + &dp83869->rx_id_delay); >>> + if (ret) { >>> + dp83869->rx_id_delay = >>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>> + ret = 0; >>> + } >>> + >>> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >>> + &dp83869->tx_id_delay); >>> + if (ret) { >>> + dp83869->tx_id_delay = >>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>> + ret = 0; >>> + } >> It is still not clear to me why is not the parsing being done by the PHY >> library helper directly? > > Why would we do that for these properties and not any other? Those properties have a standard name, which makes them suitable for parsing by the core PHY library. > > Unless there is a new precedence being set here by having the PHY > framework do all the dt node parsing for common properties. You could parse the vendor properties through the driver, let the PHY library parse the standard properties, and resolve any ordering precedence within the driver. In general, I would favor standard properties over vendor properties. Does this help?
Florian On 6/2/20 6:13 PM, Florian Fainelli wrote: > > On 6/2/2020 4:10 PM, Dan Murphy wrote: >> Florian >> >> On 6/2/20 5:33 PM, Florian Fainelli wrote: >>> On 6/2/2020 9:45 AM, Dan Murphy wrote: >>>> Add RGMII internal delay configuration for Rx and Tx. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>> --- >>> [snip] >>> >>>> + >>>> enum { >>>> DP83869_PORT_MIRRORING_KEEP, >>>> DP83869_PORT_MIRRORING_EN, >>>> @@ -108,6 +113,8 @@ enum { >>>> struct dp83869_private { >>>> int tx_fifo_depth; >>>> int rx_fifo_depth; >>>> + s32 rx_id_delay; >>>> + s32 tx_id_delay; >>>> int io_impedance; >>>> int port_mirroring; >>>> bool rxctrl_strap_quirk; >>>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device >>>> *phydev) >>>> &dp83869->tx_fifo_depth)) >>>> dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; >>>> + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", >>>> + &dp83869->rx_id_delay); >>>> + if (ret) { >>>> + dp83869->rx_id_delay = >>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>>> + ret = 0; >>>> + } >>>> + >>>> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >>>> + &dp83869->tx_id_delay); >>>> + if (ret) { >>>> + dp83869->tx_id_delay = >>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>>> + ret = 0; >>>> + } >>> It is still not clear to me why is not the parsing being done by the PHY >>> library helper directly? >> Why would we do that for these properties and not any other? > Those properties have a standard name, which makes them suitable for > parsing by the core PHY library. >> Unless there is a new precedence being set here by having the PHY >> framework do all the dt node parsing for common properties. > You could parse the vendor properties through the driver, let the PHY > library parse the standard properties, and resolve any ordering > precedence within the driver. In general, I would favor standard > properties over vendor properties. > > Does this help? Ok so new precedence then. Because there are common properties like tx-fifo-depth, rx-fifo-depth, enet-phy-lane-swap and max_speed that the PHY framework should parse as well. Dan
Florian On 6/2/20 6:18 PM, Dan Murphy wrote: > Florian > > On 6/2/20 6:13 PM, Florian Fainelli wrote: >> >> On 6/2/2020 4:10 PM, Dan Murphy wrote: >>> Florian >>> >>> On 6/2/20 5:33 PM, Florian Fainelli wrote: >>>> On 6/2/2020 9:45 AM, Dan Murphy wrote: >>>>> Add RGMII internal delay configuration for Rx and Tx. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>> --- >>>> [snip] >>>> >>>>> + >>>>> enum { >>>>> DP83869_PORT_MIRRORING_KEEP, >>>>> DP83869_PORT_MIRRORING_EN, >>>>> @@ -108,6 +113,8 @@ enum { >>>>> struct dp83869_private { >>>>> int tx_fifo_depth; >>>>> int rx_fifo_depth; >>>>> + s32 rx_id_delay; >>>>> + s32 tx_id_delay; >>>>> int io_impedance; >>>>> int port_mirroring; >>>>> bool rxctrl_strap_quirk; >>>>> @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device >>>>> *phydev) >>>>> &dp83869->tx_fifo_depth)) >>>>> dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; >>>>> + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", >>>>> + &dp83869->rx_id_delay); >>>>> + if (ret) { >>>>> + dp83869->rx_id_delay = >>>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>>>> + ret = 0; >>>>> + } >>>>> + >>>>> + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", >>>>> + &dp83869->tx_id_delay); >>>>> + if (ret) { >>>>> + dp83869->tx_id_delay = >>>>> + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; >>>>> + ret = 0; >>>>> + } >>>> It is still not clear to me why is not the parsing being done by >>>> the PHY >>>> library helper directly? >>> Why would we do that for these properties and not any other? >> Those properties have a standard name, which makes them suitable for >> parsing by the core PHY library. >>> Unless there is a new precedence being set here by having the PHY >>> framework do all the dt node parsing for common properties. >> You could parse the vendor properties through the driver, let the PHY >> library parse the standard properties, and resolve any ordering >> precedence within the driver. In general, I would favor standard >> properties over vendor properties. >> >> Does this help? > > Ok so new precedence then. > > Because there are common properties like tx-fifo-depth, rx-fifo-depth, > enet-phy-lane-swap and max_speed that the PHY framework should parse > as well. > I am assuming that the retrieval of the property and getting the index should be 2 separate APIs? One API to get the property value and one API to find the index value. Dan > Dan >
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index cfb22a21a2e6..ba1e3d599888 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -64,6 +64,10 @@ #define DP83869_RGMII_TX_CLK_DELAY_EN BIT(1) #define DP83869_RGMII_RX_CLK_DELAY_EN BIT(0) +/* RGMIIDCTL */ +#define DP83869_RGMII_CLK_DELAY_SHIFT 4 +#define DP83869_CLK_DELAY_DEF 7 + /* STRAP_STS1 bits */ #define DP83869_STRAP_OP_MODE_MASK GENMASK(2, 0) #define DP83869_STRAP_STS1_RESERVED BIT(11) @@ -78,9 +82,6 @@ #define DP83869_PHYCR_FIFO_DEPTH_MASK GENMASK(15, 12) #define DP83869_PHYCR_RESERVED_MASK BIT(11) -/* RGMIIDCTL bits */ -#define DP83869_RGMII_TX_CLK_DELAY_SHIFT 4 - /* IO_MUX_CFG bits */ #define DP83869_IO_MUX_CFG_IO_IMPEDANCE_CTRL 0x1f @@ -99,6 +100,10 @@ #define DP83869_OP_MODE_MII BIT(5) #define DP83869_SGMII_RGMII_BRIDGE BIT(6) +static const int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, + 1750, 2000, 2250, 2500, 2750, 3000, + 3250, 3500, 3750, 4000}; + enum { DP83869_PORT_MIRRORING_KEEP, DP83869_PORT_MIRRORING_EN, @@ -108,6 +113,8 @@ enum { struct dp83869_private { int tx_fifo_depth; int rx_fifo_depth; + s32 rx_id_delay; + s32 tx_id_delay; int io_impedance; int port_mirroring; bool rxctrl_strap_quirk; @@ -232,6 +239,22 @@ static int dp83869_of_init(struct phy_device *phydev) &dp83869->tx_fifo_depth)) dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB; + ret = of_property_read_u32(of_node, "rx-internal-delay-ps", + &dp83869->rx_id_delay); + if (ret) { + dp83869->rx_id_delay = + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; + ret = 0; + } + + ret = of_property_read_u32(of_node, "tx-internal-delay-ps", + &dp83869->tx_id_delay); + if (ret) { + dp83869->tx_id_delay = + dp83869_internal_delay[DP83869_CLK_DELAY_DEF]; + ret = 0; + } + return ret; } #else @@ -367,10 +390,35 @@ static int dp83869_configure_mode(struct phy_device *phydev, return ret; } +static int dp83869_get_delay(struct phy_device *phydev) +{ + struct dp83869_private *dp83869 = phydev->priv; + int delay_size = ARRAY_SIZE(dp83869_internal_delay); + int tx_delay; + int rx_delay; + + tx_delay = phy_get_delay_index(phydev, &dp83869_internal_delay[0], + delay_size, dp83869->tx_id_delay); + if (tx_delay < 0) { + phydev_err(phydev, "Tx internal delay is invalid\n"); + return tx_delay; + } + + rx_delay = phy_get_delay_index(phydev, &dp83869_internal_delay[0], + delay_size, dp83869->rx_id_delay); + if (rx_delay < 0) { + phydev_err(phydev, "Rx internal delay is invalid\n"); + return rx_delay; + } + + return rx_delay | tx_delay << DP83869_RGMII_CLK_DELAY_SHIFT; +} + static int dp83869_config_init(struct phy_device *phydev) { struct dp83869_private *dp83869 = phydev->priv; int ret, val; + int delay; ret = dp83869_configure_mode(phydev, dp83869); if (ret) @@ -394,6 +442,34 @@ static int dp83869_config_init(struct phy_device *phydev) dp83869->clk_output_sel << DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT); + if (phy_interface_is_rgmii(phydev)) { + delay = dp83869_get_delay(phydev); + if (delay < 0) + return delay; + + ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL, + delay); + if (ret) + return ret; + + val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL); + val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | + DP83869_RGMII_RX_CLK_DELAY_EN); + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + val |= (DP83869_RGMII_TX_CLK_DELAY_EN | + DP83869_RGMII_RX_CLK_DELAY_EN); + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) + val |= DP83869_RGMII_TX_CLK_DELAY_EN; + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) + val |= DP83869_RGMII_RX_CLK_DELAY_EN; + + ret = phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, + val); + } + return ret; }
Add RGMII internal delay configuration for Rx and Tx. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- drivers/net/phy/dp83869.c | 82 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 3 deletions(-)