Re: [PATCH v2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Andy Fleming <afleming@gmail.com>]

On Fri, Nov 19, 2010 at 4:13 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
>
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
>
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
>
>      phy5: ethernet-phy@5 {
>        reg = <5>;
>        compatible = "marvell,88e1149r";
>        marvell,reg-init =
>                /* led[0]:1000, led[1]:100, led[2]:10, led[3]:tx */
>                <3 0x10 0 0x5777>, /* Reg 3,16 <- 0x5777 */
>                /* mix %:0, led[0123]:drive low off hiZ */
>                <3 0x11 0 0x00aa>, /* Reg 3,17 <- 0x00aa */
>                /* default blink periods. */
>                <3 0x12 0 0x4105>, /* Reg 3,18 <- 0x4105 */
>                /* led[4]:rx, led[5]:dplx, led[45]:drive low off hiZ */
>                <3 0x13 0 0x0a60>; /* Reg 3,19 <- 0x0a60 */


My inclination is to shy away from such hard-coded values.  I agreed
with Grant's comment about separating into separate cells, but I think
specification of hard-coded register writes is too rigid.  I think a
better approach would be to specify configuration attributes, like:

marvell,blink-periods = <blah>;
marvell,led-config = <[drive type] [indicates]>;

For one, I always advocate making the DTS human-readable.  For
another, I think that there are a number of configuration sequences
that require read-modify-write, or write-wait-write.  In other words,
I think that there are enough cases where actual software will be
needed, that an attempt to generically specify a register
initialization sequence will be impossible, and leave us with the same
problems to solve later on.  For third...ly... allowing
device-tree-specified register initializations might encourage
developers to put all of their register initializations in the device
tree.  Especially when they realize that the LED initialization for
*their* PHY has to come between two standard initialization steps in
the driver.  Or before.  Or after.

By specifying actual functionality, the driver can work around those
problems, while being aware of the functional goal.  However, I'm
aware that such a path is more difficult, and perhaps just as futile,
as PHY vendors frequently don't want to document what their magic
sequences mean.

Andy