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

[Sean Cross <xobs@kosagi.com>]

-- 
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.