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 <[email protected]>
---
drivers/net/phy/intel-xway.c | 91 ++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c
index d453ec016168..6fc8f54247c2 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,87 @@
#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;
+ int err = 0;
+ int val;
+
+ if (phy_interface_is_rgmii(phydev)) {
+ val = phy_read(phydev, XWAY_MDIO_MIICTRL);
+ if (val < 0)
+ return val;
+ }
+
+ /* Existing behavior was to use default pin strapping delay in rgmii
+ * mode, but rgmii should have meant no delay. Warn existing users.
+ */
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
+ const u16 txskew = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >>
+ XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
+ const u16 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:%x rxskew:%x\n",
+ txskew, rxskew);
+ }
+
+ /* RX delay *must* be specified if internal delay of RX is used. */
+ 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_err(phydev, "rx-internal-delay-ps must be specified\n");
+ return rx_int_delay;
+ }
+
+ val &= ~XWAY_MDIO_MIICTRL_RXSKEW_MASK;
+ val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT;
+ }
+
+ /* TX delay *must* be specified if internal delay of TX is used. */
+ 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_err(phydev, "tx-internal-delay-ps must be specified\n");
+ return tx_int_delay;
+ }
+
+ val &= ~XWAY_MDIO_MIICTRL_TXSKEW_MASK;
+ val |= tx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
+ }
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+ err = phy_write(phydev, XWAY_MDIO_MIICTRL, val);
+
+ return err;
+}
+#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 +291,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;
}
--
2.20.1
On Fri, Jul 09, 2021 at 01:57:26PM +0200, Martin Schiller wrote:
> +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;
> + int err = 0;
> + int val;
> +
> + if (phy_interface_is_rgmii(phydev)) {
> + val = phy_read(phydev, XWAY_MDIO_MIICTRL);
> + if (val < 0)
> + return val;
> + }
> +
> + /* Existing behavior was to use default pin strapping delay in rgmii
> + * mode, but rgmii should have meant no delay. Warn existing users.
> + */
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> + const u16 txskew = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >>
> + XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
> + const u16 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:%x rxskew:%x\n",
> + txskew, rxskew);
> + }
> +
> + /* RX delay *must* be specified if internal delay of RX is used. */
> + 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_err(phydev, "rx-internal-delay-ps must be specified\n");
> + return rx_int_delay;
> + }
> +
> + val &= ~XWAY_MDIO_MIICTRL_RXSKEW_MASK;
> + val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT;
> + }
> +
> + /* TX delay *must* be specified if internal delay of TX is used. */
> + 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_err(phydev, "tx-internal-delay-ps must be specified\n");
> + return tx_int_delay;
> + }
> +
> + val &= ~XWAY_MDIO_MIICTRL_TXSKEW_MASK;
> + val |= tx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
> + }
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> + err = phy_write(phydev, XWAY_MDIO_MIICTRL, val);
> +
> + return err;
> +}
Please reconsider the above. Maybe something like the following would
be better:
u16 mask = 0;
int val = 0;
if (!phy_interface_is_rgmii(phydev))
return;
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:%x rxskew:%x\n",
txskew, rxskew);
return;
}
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
...
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) {
...
mask |= XWAY_MDIO_MIICTRL_TXSKEW_MASK;
val |= rx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
}
return phy_modify(phydev, XWAY_MDIO_MIICTRL, mask, val);
Using phy_modify() has the advantage that the read-modify-write is
done as a locked transaction on the bus, meaning that it is atomic.
There isn't a high cost to writing functions in a way that makes use
of that as can be seen from the above.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 2021-07-09 14:26, Russell King (Oracle) wrote:
> On Fri, Jul 09, 2021 at 01:57:26PM +0200, Martin Schiller wrote:
>> +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;
>> + int err = 0;
>> + int val;
>> +
>> + if (phy_interface_is_rgmii(phydev)) {
>> + val = phy_read(phydev, XWAY_MDIO_MIICTRL);
>> + if (val < 0)
>> + return val;
>> + }
>> +
>> + /* Existing behavior was to use default pin strapping delay in rgmii
>> + * mode, but rgmii should have meant no delay. Warn existing users.
>> + */
>> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
>> + const u16 txskew = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >>
>> + XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
>> + const u16 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:%x
>> rxskew:%x\n",
>> + txskew, rxskew);
>> + }
>> +
>> + /* RX delay *must* be specified if internal delay of RX is used. */
>> + 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_err(phydev, "rx-internal-delay-ps must be specified\n");
>> + return rx_int_delay;
>> + }
>> +
>> + val &= ~XWAY_MDIO_MIICTRL_RXSKEW_MASK;
>> + val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT;
>> + }
>> +
>> + /* TX delay *must* be specified if internal delay of TX is used. */
>> + 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_err(phydev, "tx-internal-delay-ps must be specified\n");
>> + return tx_int_delay;
>> + }
>> +
>> + val &= ~XWAY_MDIO_MIICTRL_TXSKEW_MASK;
>> + val |= tx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
>> + }
>> +
>> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>> + err = phy_write(phydev, XWAY_MDIO_MIICTRL, val);
>> +
>> + return err;
>> +}
>
> Please reconsider the above. Maybe something like the following would
> be better:
>
> u16 mask = 0;
> int val = 0;
>
> if (!phy_interface_is_rgmii(phydev))
> return;
>
> 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:%x
> rxskew:%x\n",
> txskew, rxskew);
> return;
> }
>
> if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> ...
> 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) {
> ...
> mask |= XWAY_MDIO_MIICTRL_TXSKEW_MASK;
> val |= rx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT;
> }
>
> return phy_modify(phydev, XWAY_MDIO_MIICTRL, mask, val);
>
> Using phy_modify() has the advantage that the read-modify-write is
> done as a locked transaction on the bus, meaning that it is atomic.
> There isn't a high cost to writing functions in a way that makes use
> of that as can be seen from the above.
>
Thanks for the hint. I'll update my patch.
> + /* RX delay *must* be specified if internal delay of RX is used. */
> + 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_err(phydev, "rx-internal-delay-ps must be specified\n");
> + return rx_int_delay;
> + }
> +
> + val &= ~XWAY_MDIO_MIICTRL_RXSKEW_MASK;
> + val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT;
> + }
Please don't force the delay property to be present, use the default
of 2ns if it is missing.
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> + err = phy_write(phydev, XWAY_MDIO_MIICTRL, val);
> +
This is the tricky bit. Do we want to act on PHY_INTERFACE_MODE_RGMII?
At the moment, i would say no, lets see how many patches we get
because of the warning you add. But i think it is worth adding a
comment here, briefly explaining why it is missing.
Andrew