| Message ID | 1290204798-28569-1-git-send-email-ddaney@caviumnetworks.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/19/2010 03:02 PM, Andy Fleming wrote: > 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]>; I considered doing that, but rejected the idea because it is so complex. There are literally probably a hundred different register fields with widths varying between 1 and 16 bits. The number of settings that the driver might want to deal with are astronomical. For any given PHY model there could be 30 different settings that you might want to tweak. In the example above I configure 5 LEDs with two parameters each, and about 4 or 5 more blink settings, that is something like 15 register fields. Take the marvell.c file. It currently supports nine different PHY models, each would have a bunch of model specific settings, the complexity of decoding all that is formidable (and would be a lot more code) also there would invariably be settings that were not implemented in the first version, so people would have to repeatedly extend it. My patch adds 550 bytes (not including the string storage) of code on MIPS64, and can handle many cases without any changes. > > 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. We can always add more properties as you suggest, if the existing code does not prove to be sufficiently flexible. > 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. > I certainly understand your desire to make the dts readable, but I think it has to be balanced against the complexity and chance for bugs in the drivers as well as churn in the bindings specifications. David Daney -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 19, 2010 at 03:38:15PM -0800, David Daney wrote: > On 11/19/2010 03:02 PM, Andy Fleming wrote: > >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]>; I'm certainly a strong advocate of describing the desired behaviour instead of trying to list a bunch of register values or similar. However, I think the use-case here is sufficiently straight forward (initial conditions for a set of registers) on things that will be set once and then forgotten about that I think that the simple approach is appropriate. > I considered doing that, but rejected the idea because it is so > complex. There are literally probably a hundred different register > fields with widths varying between 1 and 16 bits. The number of > settings that the driver might want to deal with are astronomical. > For any given PHY model there could be 30 different settings that > you might want to tweak. In the example above I configure 5 LEDs > with two parameters each, and about 4 or 5 more blink settings, that > is something like 15 register fields. > > Take the marvell.c file. It currently supports nine different PHY > models, each would have a bunch of model specific settings, the > complexity of decoding all that is formidable (and would be a lot > more code) also there would invariably be settings that were not > implemented in the first version, so people would have to repeatedly > extend it. > > My patch adds 550 bytes (not including the string storage) of code > on MIPS64, and can handle many cases without any changes. Right; the permutations are huge in this case, and the payoff quite low. > >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. > > We can always add more properties as you suggest, if the existing > code does not prove to be sufficiently flexible. That is a very different situation however. I am *strongly* against anything that begins to look like a scripting or interpreted commands. Initial conditions for a set of registers are one thing, but trying to encode rmw cycles or delay cycles enters into the realm where the driver really needs to understand the behaviour of the device and be responsible for talking to it appropriately. So, to summarize my opinion: - list of initial register settings? okay given the alternatives - method to perform arbitrary initialization sequences, including delays and rmw? right out. > > > 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. Probably unlikely considering the driver still needs to handle the device in the non-devicetree use case. However, it may be appropriate for the driver to filter which registers it will initialize from dt data. > > > >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. > > > > I certainly understand your desire to make the dts readable, but I > think it has to be balanced against the complexity and chance for > bugs in the drivers as well as churn in the bindings specifications. > > David Daney -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 19, 2010 at 02:13:18PM -0800, David Daney 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 */ > }; > > phy6: ethernet-phy@6 { > reg = <6>; > compatible = "marvell,88e1118"; > marvell,reg-init = > /* Fix rx and tx clock transition timing */ > <2 0x15 0xffcf 0>, /* Reg 2,21 Clear bits 4, 5 */ > /* Adjust LED drive. */ > <3 0x11 0 0x442a>, /* Reg 3,17 <- 0442a */ > /* irq, blink-activity, blink-link */ > <3 0x10 0 0x0242>; /* Reg 3,16 <- 0x0242 */ > }; > > The Marvell PHYs have a page select register at register 22 (0x16), we > can specify any register by its page and register number. These are > the first and second word. The third word contains a mask to be ANDed > with the existing register value, and the fourth word is ORed with the > result to yield the new register value. The new marvell_of_reg_init > function leaves the page select register unchanged, so a call to it > can be dropped into the .config_init functions without unduly > affecting the state of the PHY. > > If CONFIG_OF_MDIO is not set, there is no of_node, or no > "marvell,reg-init" property, the PHY initialization is unchanged. > > Signed-off-by: David Daney <ddaney@caviumnetworks.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Cyril Chemparathy <cyril@ti.com> > Cc: David Daney <ddaney@caviumnetworks.com> > Cc: Arnaud Patard <arnaud.patard@rtp-net.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Untested/compiled, but looks good to me. Reviewed-by: Grant Likely <grant.likely@secretlab.ca> > --- > > Note: this patch now depends on the recently sent 88E1149R support > patch, which was seperated into its own patch set. > > I think I have incororated all feedback from Grant Likely and Cyril > Chemparathy, and Milton Miller. > > David Daney > > drivers/net/phy/marvell.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 97 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index def19d7..e8b9c53 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -30,6 +30,7 @@ > #include <linux/ethtool.h> > #include <linux/phy.h> > #include <linux/marvell_phy.h> > +#include <linux/of.h> > > #include <asm/io.h> > #include <asm/irq.h> > @@ -187,6 +188,87 @@ static int marvell_config_aneg(struct phy_device *phydev) > return 0; > } > > +#ifdef CONFIG_OF_MDIO > +/* > + * Set and/or override some configuration registers based on the > + * marvell,reg-init property stored in the of_node for the phydev. > + * > + * marvell,reg-init = <reg-page reg mask value>,...; > + * > + * There may be one or more sets of <reg-page reg mask value>: > + * > + * reg-page: which register bank to use. > + * reg: the register. > + * mask: if non-zero, ANDed with existing register value. > + * value: ORed with the masked value and written to the regiser. > + * > + */ > +static int marvell_of_reg_init(struct phy_device *phydev) > +{ > + const __be32 *paddr; > + int len, i, saved_page, current_page, page_changed, ret; > + > + if (!phydev->dev.of_node) > + return 0; > + > + paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len); > + if (!paddr || len < (4 * sizeof(*paddr))) > + return 0; > + > + saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE); > + if (saved_page < 0) > + return saved_page; > + page_changed = 0; > + current_page = saved_page; > + > + ret = 0; > + len /= sizeof(*paddr); > + for (i = 0; i < len - 3; i += 4) { > + u16 reg_page = be32_to_cpup(paddr + i); > + u16 reg = be32_to_cpup(paddr + i + 1); > + u16 mask = be32_to_cpup(paddr + i + 2); > + u16 val_bits = be32_to_cpup(paddr + i + 3); > + int val; > + > + if (reg_page != current_page) { > + current_page = reg_page; > + page_changed = 1; > + ret = phy_write(phydev, MII_MARVELL_PHY_PAGE, reg_page); > + if (ret < 0) > + goto err; > + } > + > + val = 0; > + if (mask) { > + val = phy_read(phydev, reg); > + if (val < 0) { > + ret = val; > + goto err; > + } > + val &= mask; > + } > + val |= val_bits; > + > + ret = phy_write(phydev, reg, val); > + if (ret < 0) > + goto err; > + > + } > +err: > + if (page_changed) { > + i = phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page); > + if (ret == 0) > + ret = i; > + } > + return ret; > +} > +#else > +static int marvell_of_reg_init(struct phy_device *phydev) > +{ > + return 0; > +} > +#endif /* CONFIG_OF_MDIO */ > + > static int m88e1121_config_aneg(struct phy_device *phydev) > { > int err, oldpage, mscr; > @@ -369,6 +451,9 @@ static int m88e1111_config_init(struct phy_device *phydev) > return err; > } > > + err = marvell_of_reg_init(phydev); > + if (err < 0) > + return err; > > err = phy_write(phydev, MII_BMCR, BMCR_RESET); > if (err < 0) > @@ -421,6 +506,10 @@ static int m88e1118_config_init(struct phy_device *phydev) > if (err < 0) > return err; > > + err = marvell_of_reg_init(phydev); > + if (err < 0) > + return err; > + > /* Reset address */ > err = phy_write(phydev, MII_MARVELL_PHY_PAGE, 0x0); > if (err < 0) > @@ -447,6 +536,10 @@ static int m88e1149_config_init(struct phy_device *phydev) > if (err < 0) > return err; > > + err = marvell_of_reg_init(phydev); > + if (err < 0) > + return err; > + > /* Reset address */ > err = phy_write(phydev, MII_MARVELL_PHY_PAGE, 0x0); > if (err < 0) > @@ -518,6 +611,10 @@ static int m88e1145_config_init(struct phy_device *phydev) > } > } > > + err = marvell_of_reg_init(phydev); > + if (err < 0) > + return err; > + > return 0; > } > > -- > 1.7.2.3 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Grant Likely <grant.likely@secretlab.ca> Date: Fri, 19 Nov 2010 21:28:49 -0700 > On Fri, Nov 19, 2010 at 02:13:18PM -0800, David Daney 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 */ >> }; >> >> phy6: ethernet-phy@6 { >> reg = <6>; >> compatible = "marvell,88e1118"; >> marvell,reg-init = >> /* Fix rx and tx clock transition timing */ >> <2 0x15 0xffcf 0>, /* Reg 2,21 Clear bits 4, 5 */ >> /* Adjust LED drive. */ >> <3 0x11 0 0x442a>, /* Reg 3,17 <- 0442a */ >> /* irq, blink-activity, blink-link */ >> <3 0x10 0 0x0242>; /* Reg 3,16 <- 0x0242 */ >> }; >> >> The Marvell PHYs have a page select register at register 22 (0x16), we >> can specify any register by its page and register number. These are >> the first and second word. The third word contains a mask to be ANDed >> with the existing register value, and the fourth word is ORed with the >> result to yield the new register value. The new marvell_of_reg_init >> function leaves the page select register unchanged, so a call to it >> can be dropped into the .config_init functions without unduly >> affecting the state of the PHY. >> >> If CONFIG_OF_MDIO is not set, there is no of_node, or no >> "marvell,reg-init" property, the PHY initialization is unchanged. >> >> Signed-off-by: David Daney <ddaney@caviumnetworks.com> >> Cc: Grant Likely <grant.likely@secretlab.ca> >> Cc: Cyril Chemparathy <cyril@ti.com> >> Cc: David Daney <ddaney@caviumnetworks.com> >> Cc: Arnaud Patard <arnaud.patard@rtp-net.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Untested/compiled, but looks good to me. > > Reviewed-by: Grant Likely <grant.likely@secretlab.ca> Also applied, thanks everyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index def19d7..e8b9c53 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -30,6 +30,7 @@ #include <linux/ethtool.h> #include <linux/phy.h> #include <linux/marvell_phy.h> +#include <linux/of.h> #include <asm/io.h> #include <asm/irq.h> @@ -187,6 +188,87 @@ static int marvell_config_aneg(struct phy_device *phydev) return 0; } +#ifdef CONFIG_OF_MDIO +/* + * Set and/or override some configuration registers based on the + * marvell,reg-init property stored in the of_node for the phydev. + * + * marvell,reg-init = <reg-page reg mask value>,...; + * + * There may be one or more sets of <reg-page reg mask value>: + * + * reg-page: which register bank to use. + * reg: the register. + * mask: if non-zero, ANDed with existing register value. + * value: ORed with the masked value and written to the regiser. + * + */ +static int marvell_of_reg_init(struct phy_device *phydev) +{ + const __be32 *paddr; + int len, i, saved_page, current_page, page_changed, ret; + + if (!phydev->dev.of_node) + return 0; + + paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len); + if (!paddr || len < (4 * sizeof(*paddr))) + return 0; + + saved_page = phy_read(phydev, MII_MARVELL_PHY_PAGE); + if (saved_page < 0) + return saved_page; + page_changed = 0; + current_page = saved_page; + + ret = 0; + len /= sizeof(*paddr); + for (i = 0; i < len - 3; i += 4) { + u16 reg_page = be32_to_cpup(paddr + i); + u16 reg = be32_to_cpup(paddr + i + 1); + u16 mask = be32_to_cpup(paddr + i + 2); + u16 val_bits = be32_to_cpup(paddr + i + 3); + int val; + + if (reg_page != current_page) { + current_page = reg_page; + page_changed = 1; + ret = phy_write(phydev, MII_MARVELL_PHY_PAGE, reg_page); + if (ret < 0) + goto err; + } + + val = 0; + if (mask) { + val = phy_read(phydev, reg); + if (val < 0) { + ret = val; + goto err; + } + val &= mask; + } + val |= val_bits; + + ret = phy_write(phydev, reg, val); + if (ret < 0) + goto err; + + } +err: + if (page_changed) { + i = phy_write(phydev, MII_MARVELL_PHY_PAGE, saved_page); + if (ret == 0) + ret = i; + } + return ret; +} +#else +static int marvell_of_reg_init(struct phy_device *phydev) +{ + return 0; +} +#endif /* CONFIG_OF_MDIO */ + static int m88e1121_config_aneg(struct phy_device *phydev) { int err, oldpage, mscr; @@ -369,6 +451,9 @@ static int m88e1111_config_init(struct phy_device *phydev) return err; } + err = marvell_of_reg_init(phydev); + if (err < 0) + return err; err = phy_write(phydev, MII_BMCR, BMCR_RESET); if (err < 0) @@ -421,6 +506,10 @@ static int m88e1118_config_init(struct phy_device *phydev) if (err < 0) return err; + err = marvell_of_reg_init(phydev); + if (err < 0) + return err; + /* Reset address */ err = phy_write(phydev, MII_MARVELL_PHY_PAGE, 0x0); if (err < 0) @@ -447,6 +536,10 @@ static int m88e1149_config_init(struct phy_device *phydev) if (err < 0) return err; + err = marvell_of_reg_init(phydev); + if (err < 0) + return err; + /* Reset address */ err = phy_write(phydev, MII_MARVELL_PHY_PAGE, 0x0); if (err < 0) @@ -518,6 +611,10 @@ static int m88e1145_config_init(struct phy_device *phydev) } } + err = marvell_of_reg_init(phydev); + if (err < 0) + return err; + return 0; }
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 */ }; phy6: ethernet-phy@6 { reg = <6>; compatible = "marvell,88e1118"; marvell,reg-init = /* Fix rx and tx clock transition timing */ <2 0x15 0xffcf 0>, /* Reg 2,21 Clear bits 4, 5 */ /* Adjust LED drive. */ <3 0x11 0 0x442a>, /* Reg 3,17 <- 0442a */ /* irq, blink-activity, blink-link */ <3 0x10 0 0x0242>; /* Reg 3,16 <- 0x0242 */ }; The Marvell PHYs have a page select register at register 22 (0x16), we can specify any register by its page and register number. These are the first and second word. The third word contains a mask to be ANDed with the existing register value, and the fourth word is ORed with the result to yield the new register value. The new marvell_of_reg_init function leaves the page select register unchanged, so a call to it can be dropped into the .config_init functions without unduly affecting the state of the PHY. If CONFIG_OF_MDIO is not set, there is no of_node, or no "marvell,reg-init" property, the PHY initialization is unchanged. Signed-off-by: David Daney <ddaney@caviumnetworks.com> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: Cyril Chemparathy <cyril@ti.com> Cc: David Daney <ddaney@caviumnetworks.com> Cc: Arnaud Patard <arnaud.patard@rtp-net.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- Note: this patch now depends on the recently sent 88E1149R support patch, which was seperated into its own patch set. I think I have incororated all feedback from Grant Likely and Cyril Chemparathy, and Milton Miller. David Daney drivers/net/phy/marvell.c | 97 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 97 insertions(+), 0 deletions(-)