[PATCH v2] Add OF bindings to Micrel PHY

[PATCH v2] Add OF bindings to Micrel PHY

[Sean Cross]

Some boards require custom parameters be passed to the Micrel PHY.
Allow these boards to specify custom timing parameters in the device
tree node.

Changes since v1:
 - removed redefinition of registers and addresses 

Sean Cross (1):
  net/phy: micrel: Add OF configuration support

 .../devicetree/bindings/net/micrel-phy.txt         |   20 ++++++++
 drivers/net/phy/micrel.c                           |   50 ++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/micrel-phy.txt

-- 
1.7.9.5

[PATCH v2] net/phy: micrel: Add OF configuration support

[Sean Cross]

Some boards require custom PHY configuration, for example due to trace
length differences.  Add the ability to configure these registers in
order to get the PHY to function on boards that need it.

Because PHYs are auto-detected based on MDIO device IDs, allow PHY
configuration to be specified in the parent Ethernet device node if no
PHY device node is present.

Signed-off-by: Sean Cross <xobs@kosagi.com>
---
 .../devicetree/bindings/net/micrel-phy.txt         |   20 ++++++++
 drivers/net/phy/micrel.c                           |   50 ++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/micrel-phy.txt

diff --git a/Documentation/devicetree/bindings/net/micrel-phy.txt b/Documentation/devicetree/bindings/net/micrel-phy.txt
new file mode 100644
index 0000000..97c1ef2
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/micrel-phy.txt
@@ -0,0 +1,20 @@
+Micrel KS8737, KSZ8041, KSZ8001, KS8721, KSZ8081, KSZ8091, KSZ8061, KSZ9021,
+KSZ9031, Ethernet PHYs, and KSZ8873MLL and KSZ886X Ethernet switches.
+
+Some boards require special tuning values, particularly when it comes to
+clock delays.  You can specify clock delay values by adding
+micrel-specific properties to an Ethernet OF device node.
+
+Optional properties:
+- micrel,clk-control-pad-skew : Timing offset for the MII clock line
+- micrel,rx-data-pad-skew : Timing offset for the RX MII pad
+- micrel,tx-data-pad-skew : Timing offset for the TX MII pad
+
+Example:
+	&enet {
+		micrel,clk-control-pad-skew = <0xf0f0>;
+		micrel,rx-data-pad-skew = <0x0000>;
+		micrel,tx-data-pad-skew = <0xffff>;
+		status = "okay";
+	};
+
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2510435..e20aeee 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/of.h>
 
 /* Operation Mode Strap Override */
 #define MII_KSZPHY_OMSO				0x16
@@ -53,6 +54,18 @@
 #define KS8737_CTRL_INT_ACTIVE_HIGH		(1 << 14)
 #define KSZ8051_RMII_50MHZ_CLK			(1 << 7)
 
+/* Write/read to/from extended registers */
+#define MII_KSZPHY_EXTREG                       0x0b
+#define KSZPHY_EXTREG_WRITE                     0x8000
+
+#define MII_KSZPHY_EXTREG_WRITE                 0x0c
+#define MII_KSZPHY_EXTREG_READ                  0x0d
+
+/* Extended registers */
+#define MII_KSZPHY_CLK_CONTROL_PAD_SKEW         0x104
+#define MII_KSZPHY_RX_DATA_PAD_SKEW             0x105
+#define MII_KSZPHY_TX_DATA_PAD_SKEW             0x106
+
 static int ksz_config_flags(struct phy_device *phydev)
 {
 	int regval;
@@ -65,6 +78,13 @@ static int ksz_config_flags(struct phy_device *phydev)
 	return 0;
 }
 
+static int kszphy_extended_write(struct phy_device *phydev,
+                                 u32 regnum, u16 val)
+{
+	phy_write(phydev, MII_KSZPHY_EXTREG, KSZPHY_EXTREG_WRITE | regnum);
+	return phy_write(phydev, MII_KSZPHY_EXTREG_WRITE, val);
+}
+
 static int kszphy_ack_interrupt(struct phy_device *phydev)
 {
 	/* bit[7..0] int status, which is a read and clear register. */
@@ -121,6 +141,36 @@ static int ks8737_config_intr(struct phy_device *phydev)
 
 static int kszphy_config_init(struct phy_device *phydev)
 {
+	struct device *dev = &phydev->dev;
+	struct device_node *of_node = dev->of_node;
+
+	if (!of_node && dev->parent->of_node)
+		of_node = dev->parent->of_node;
+
+	if (of_node) {
+		u32 val;
+
+		if (!of_property_read_u32(of_node,
+					  "micrel,clk-control-pad-skew",
+					  &val))
+			kszphy_extended_write(phydev,
+					      MII_KSZPHY_CLK_CONTROL_PAD_SKEW,
+					      val);
+
+		if (!of_property_read_u32(of_node,
+					  "micrel,rx-data-pad-skew",
+					  &val))
+			kszphy_extended_write(phydev,
+					      MII_KSZPHY_RX_DATA_PAD_SKEW,
+					      val);
+
+		if (!of_property_read_u32(of_node,
+					  "micrel,tx-data-pad-skew",
+					  &val))
+			kszphy_extended_write(phydev,
+					      MII_KSZPHY_TX_DATA_PAD_SKEW,
+					      val);
+	}
 	return 0;
 }
 
-- 
1.7.9.5

RE: [PATCH v2] net/phy: micrel: Add OF configuration support

[Duan Fugang-B38611]

On Thursday, August 1, 2013 at 2:54 PM, Sean Cross wrote:

> Some boards require custom PHY configuration, for example due to trace length differences. 
> Add the ability to configure these registers in order to get the PHY to function on boards that need it.
>
> Because PHYs are auto-detected based on MDIO device IDs, allow PHY configuration to be specified in the parent Ethernet device node
> if no PHY device node is present.
>
> Signed-off-by: Sean Cross <xobs@kosagi.com>

Acked-by: Fugang Duan <B38611@freescale.com>

Re: [PATCH v2] net/phy: micrel: Add OF configuration support

[Sascha Hauer]

On Thu, Aug 01, 2013 at 06:53:54AM +0000, Sean Cross wrote:
> Some boards require custom PHY configuration, for example due to trace
> length differences.  Add the ability to configure these registers in
> order to get the PHY to function on boards that need it.
> 
> Because PHYs are auto-detected based on MDIO device IDs, allow PHY
> configuration to be specified in the parent Ethernet device node if no
> PHY device node is present.
> 
> Signed-off-by: Sean Cross <xobs@kosagi.com>
> ---
>  .../devicetree/bindings/net/micrel-phy.txt         |   20 ++++++++
>  drivers/net/phy/micrel.c                           |   50 ++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/micrel-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/micrel-phy.txt b/Documentation/devicetree/bindings/net/micrel-phy.txt
> new file mode 100644
> index 0000000..97c1ef2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/micrel-phy.txt
> @@ -0,0 +1,20 @@
> +Micrel KS8737, KSZ8041, KSZ8001, KS8721, KSZ8081, KSZ8091, KSZ8061, KSZ9021,
> +KSZ9031, Ethernet PHYs, and KSZ8873MLL and KSZ886X Ethernet switches.
> +
> +Some boards require special tuning values, particularly when it comes to
> +clock delays.  You can specify clock delay values by adding
> +micrel-specific properties to an Ethernet OF device node.
> +
> +Optional properties:
> +- micrel,clk-control-pad-skew : Timing offset for the MII clock line
> +- micrel,rx-data-pad-skew : Timing offset for the RX MII pad
> +- micrel,tx-data-pad-skew : Timing offset for the TX MII pad
> +
> +Example:
> +	&enet {
> +		micrel,clk-control-pad-skew = <0xf0f0>;
> +		micrel,rx-data-pad-skew = <0x0000>;
> +		micrel,tx-data-pad-skew = <0xffff>;
> +		status = "okay";
> +	};

Given that this patch adds a devicetree binding which I think we now
agree that this introduces an ABI I think this needs more thought. Some
questions:

- Does binding this also work for MII controllers external to the MAC?
  Several Marvell SoCs have this situation. MDIO is a bus. With the
  binding above you assume that all devices on the bus use the same
  settings
- You directly put the register contents into dt. This assumes that all
  micrel phys have a compatible register layout. This may be the case
  now, but what's with future phys?
- The pad skew settings are needed for other phys aswell. It might be
  worth introducing a binding which could work for say Artheros phys
  aswell.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] net/phy: micrel: Add OF configuration support

[Sean Cross]

-- 
Sean Cross


On Thursday, August 1, 2013 at 4:47 PM, Sascha Hauer wrote:

> On Thu, Aug 01, 2013 at 06:53:54AM +0000, Sean Cross wrote:
> > Some boards require custom PHY configuration, for example due to trace
> > length differences. Add the ability to configure these registers in
> > order to get the PHY to function on boards that need it.
> > 
> > Because PHYs are auto-detected based on MDIO device IDs, allow PHY
> > configuration to be specified in the parent Ethernet device node if no
> > PHY device node is present.
> > 
> > Signed-off-by: Sean Cross <xobs@kosagi.com (mailto:xobs@kosagi.com)>
> > ---
> > .../devicetree/bindings/net/micrel-phy.txt | 20 ++++++++
> > drivers/net/phy/micrel.c | 50 ++++++++++++++++++++
> > 2 files changed, 70 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/net/micrel-phy.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/micrel-phy.txt b/Documentation/devicetree/bindings/net/micrel-phy.txt
> > new file mode 100644
> > index 0000000..97c1ef2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/micrel-phy.txt
> > @@ -0,0 +1,20 @@
> > +Micrel KS8737, KSZ8041, KSZ8001, KS8721, KSZ8081, KSZ8091, KSZ8061, KSZ9021,
> > +KSZ9031, Ethernet PHYs, and KSZ8873MLL and KSZ886X Ethernet switches.
> > +
> > +Some boards require special tuning values, particularly when it comes to
> > +clock delays. You can specify clock delay values by adding
> > +micrel-specific properties to an Ethernet OF device node.
> > +
> > +Optional properties:
> > +- micrel,clk-control-pad-skew : Timing offset for the MII clock line
> > +- micrel,rx-data-pad-skew : Timing offset for the RX MII pad
> > +- micrel,tx-data-pad-skew : Timing offset for the TX MII pad
> > +
> > +Example:
> > + &enet {
> > + micrel,clk-control-pad-skew = <0xf0f0>;
> > + micrel,rx-data-pad-skew = <0x0000>;
> > + micrel,tx-data-pad-skew = <0xffff>;
> > + status = "okay";
> > + };
> 
> 
> 
> Given that this patch adds a devicetree binding which I think we now
> agree that this introduces an ABI I think this needs more thought. Some
> questions:
> 
> - Does binding this also work for MII controllers external to the MAC?
> Several Marvell SoCs have this situation. MDIO is a bus. With the
> binding above you assume that all devices on the bus use the same
> settings

I'm not quite sure what you mean here.  The board I'm testing on is based on an i.MX6q, which has a gigabit MAC and uses a Micrel PHY connected via MDIO.  I assumed (perhaps inaccurately) that phy_write() would pick the correct PHY.  The example binding is for a single Ethernet device, named "enet".  If a board had two Ethernet devices, enet1 and enet2, each could have its own micrel definitions.  For example:

&enet1 {
    micrel,clk-control-pad-skew = <0xf0f0>;
    micrel,rx-data-pad-skew = <0x0000>;
    micrel,tx-data-pad-skew = <0xffff>;
    status = "okay";
};

&enet2 {
    micrel,clk-control-pad-skew = <0x0000>;
    micrel,rx-data-pad-skew = <0x0000>;
    micrel,tx-data-pad-skew = <0x0000>;
    status = "okay";
};



> - You directly put the register contents into dt. This assumes that all
> micrel phys have a compatible register layout. This may be the case
> now, but what's with future phys?

This is a very valid point.  I assumed that most Micrel PHYs had similar register sets, but upon closer inspection it seems as though the KSZ9021RN is unique in this regard.  I should come up with a new function ksz9021_config_init that does this only for that one PHY, and rename the devicetree doc to reflect that. 
> - The pad skew settings are needed for other phys aswell. It might be
> worth introducing a binding which could work for say Artheros phys
> aswell.

I can't comment on other phys, as I'm not familiar with them.  How would such a system work?  This certainly seems like the most open-ended of the questions. 

I'll limit the scope of the patch to the KSZ9021 family of parts, and submit a v3 patch.

Re: [PATCH v2] net/phy: micrel: Add OF configuration support

[Sascha Hauer]

On Thu, Aug 01, 2013 at 05:06:13PM +0800, Sean Cross wrote:
> > Given that this patch adds a devicetree binding which I think we now
> > agree that this introduces an ABI I think this needs more thought. Some
> > questions:
> > 
> > - Does binding this also work for MII controllers external to the MAC?
> > Several Marvell SoCs have this situation. MDIO is a bus. With the
> > binding above you assume that all devices on the bus use the same
> > settings
> 
> I'm not quite sure what you mean here.  The board I'm testing on is
> based on an i.MX6q, which has a gigabit MAC and uses a Micrel PHY
> connected via MDIO.  I assumed (perhaps inaccurately) that phy_write()
> would pick the correct PHY.  The example binding is for a single
> Ethernet device, named "enet".  If a board had two Ethernet devices,
> enet1 and enet2, each could have its own micrel definitions.  For
> example:
> 
> &enet1 {
>     micrel,clk-control-pad-skew = <0xf0f0>;
>     micrel,rx-data-pad-skew = <0x0000>;
>     micrel,tx-data-pad-skew = <0xffff>;
>     status = "okay";
> };
> 
> &enet2 {
>     micrel,clk-control-pad-skew = <0x0000>;
>     micrel,rx-data-pad-skew = <0x0000>;
>     micrel,tx-data-pad-skew = <0x0000>;
>     status = "okay";
> };

Here each enet controller has its own mdio bus, but look for example at
Marvell armada-370-db.dts:

mdio {
	phy0: ethernet-phy@0 {
		reg = <0>;
	};

	phy1: ethernet-phy@1 {
		reg = <1>;
	};
};

ethernet@70000 {
	status = "okay";
	phy = <&phy0>;
	phy-mode = "rgmii-id";
};

ethernet@74000 {
	status = "okay";
	phy = <&phy1>;
	phy-mode = "rgmii-id";
};

Here two ethernet controllers share a single mdio bus. This at least
requires the binding in the phy node, not the ethernet node.

> > - You directly put the register contents into dt. This assumes that all
> > micrel phys have a compatible register layout. This may be the case
> > now, but what's with future phys?
> 
> This is a very valid point.  I assumed that most Micrel PHYs had
> similar register sets, but upon closer inspection it seems as though
> the KSZ9021RN is unique in this regard.  I should come up with a new
> function ksz9021_config_init that does this only for that one PHY, and
> rename the devicetree doc to reflect that.
> > - The pad skew settings are needed for other phys aswell. It might
> > be
> > worth introducing a binding which could work for say Artheros phys
> > aswell.
> 
> I can't comment on other phys, as I'm not familiar with them.  How
> would such a system work?  This certainly seems like the most
> open-ended of the questions.

I think we would need the delay for the different lines in ps, not in
hardware register specific values. This would have the additional
benefit that we don't have to hardcode the exact phy type.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] net/phy: micrel: Add OF configuration support

[Sean Cross]

On Thursday, August 1, 2013 at 6:24 PM, Sascha Hauer wrote:
> On Thu, Aug 01, 2013 at 05:06:13PM +0800, Sean Cross wrote:
> > > Given that this patch adds a devicetree binding which I think we now
> > > agree that this introduces an ABI I think this needs more thought. Some
> > > questions:
> > > 
> > > - Does binding this also work for MII controllers external to the MAC?
> > > Several Marvell SoCs have this situation. MDIO is a bus. With the
> > > binding above you assume that all devices on the bus use the same
> > > settings
> > 
> > 
> > 
> > I'm not quite sure what you mean here. The board I'm testing on is
> > based on an i.MX6q, which has a gigabit MAC and uses a Micrel PHY
> > connected via MDIO. I assumed (perhaps inaccurately) that phy_write()
> > would pick the correct PHY. The example binding is for a single
> > Ethernet device, named "enet". If a board had two Ethernet devices,
> > enet1 and enet2, each could have its own micrel definitions. For
> > example:
> > 
> > &enet1 {
> > micrel,clk-control-pad-skew = <0xf0f0>;
> > micrel,rx-data-pad-skew = <0x0000>;
> > micrel,tx-data-pad-skew = <0xffff>;
> > status = "okay";
> > };
> > 
> > &enet2 {
> > micrel,clk-control-pad-skew = <0x0000>;
> > micrel,rx-data-pad-skew = <0x0000>;
> > micrel,tx-data-pad-skew = <0x0000>;
> > status = "okay";
> > };
> 
> 
> 
> Here each enet controller has its own mdio bus, but look for example at
> Marvell armada-370-db.dts:
> 
> mdio {
> phy0: ethernet-phy@0 {
> reg = <0>;
> };
> 
> phy1: ethernet-phy@1 {
> reg = <1>;
> };
> };
> 
> ethernet@70000 {
> status = "okay";
> phy = <&phy0>;
> phy-mode = "rgmii-id";
> };
> 
> ethernet@74000 {
> status = "okay";
> phy = <&phy1>;
> phy-mode = "rgmii-id";
> };
> 
> Here two ethernet controllers share a single mdio bus. This at least
> requires the binding in the phy node, not the ethernet node.

The code handles this case.  It will only read from the ethernet node if there is no phy node present:

    struct device_node *of_node = dev->of_node;
    if (!of_node && dev->parent->of_node)

        of_node = dev->parent->of_node;

I can add an example in the Documentation to reflect the fact that if a phy has a device tree node, you should put the skew values there.
> > > - The pad skew settings are needed for other phys aswell. It might
> > > be
> > > worth introducing a binding which could work for say Artheros phys
> > > aswell.
> > 
> > 
> > 
> > I can't comment on other phys, as I'm not familiar with them. How
> > would such a system work? This certainly seems like the most
> > open-ended of the questions.
> 
> 
> 
> I think we would need the delay for the different lines in ps, not in
> hardware register specific values. This would have the additional
> benefit that we don't have to hardcode the exact phy type.

Ah, I see.  Your proposal would be to break micrel,clk-control-pad-skew into four different values, and rename them to something like: 
rxc-pad-skew
rx-ctl-pad-skew
txc-pad-skew
tx-ctl-pad-skew

Then the device tree entries would look something like:

rxc-pad-skew = <3000>; // picoseconds
rx-ctl-pad-skew <0>; // picoseconds
txc-pad-skew = <3000>; // picoseconds
tx-ctl-pad-skew = <0>; // picoseconds



How would this affect the Atheros driver?  Are there other names that should be preferred?