Message ID | 1463587403-26809-1-git-send-email-alexander.stein@systec-electronic.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
CC'ing Andrew, John, On 05/18/2016 09:03 AM, Alexander Stein wrote: > This currently only supports PEF7071 and allows to specify max-speed and > is able to read the LED configuration from device-tree. > > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> > --- > The main purpose for now is to set a LED configuration from device tree and > to limit the maximum speed. The latter one in my case hardware limited. > As MAC and it's link partner support 1000MBit/s they would try to use that > but will eventually fail due to magnetics only supporting 100MBit/s. So > limit the maximum link speed supported directly from the start. The 'max-speed' parsing that you do in the driver should not be needed, PHYLIB takes care of that already see drivers/net/phy/phy_device.c::of_set_phy_supported For LEDs, we had a patch series floating around adding LED triggers [1], and it seems to me like the LEDs class subsystem would be a good fit for controlling PHY LEDs, possibly with the help of PHYLIB when it comes to doing the low-level work of registering LEDs and their names with the LEDS subsystem. [1]: http://lists.openwall.net/netdev/2016/03/23/61 > > As this is a RFC I skipped the device tree binding doc. Too bad, that's probably what needs to be discussed here, because the driver looks pretty reasonable otherwise. > > drivers/net/phy/Kconfig | 5 ++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/lantiq.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 173 insertions(+) > create mode 100644 drivers/net/phy/lantiq.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 3e28f7a..c004885 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -119,6 +119,11 @@ config STE10XP > ---help--- > This is the driver for the STe100p and STe101p PHYs. > > +config LANTIQ_PHY > + tristate "Driver for Lantiq PHYs" > + ---help--- > + Supports the PEF7071 PHYs. > + > config LSI_ET1011C_PHY > tristate "Driver for LSI ET1011C PHY" > ---help--- > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 8ad4ac6..e886549 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -38,3 +38,4 @@ obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o > obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o > obj-$(CONFIG_AMD_XGBE_PHY) += amd-xgbe-phy.o > obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o > +obj-$(CONFIG_LANTIQ_PHY) += lantiq.o > diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c > new file mode 100644 > index 0000000..876a7d1 > --- /dev/null > +++ b/drivers/net/phy/lantiq.c > @@ -0,0 +1,167 @@ > +/* > + * Driver for Lantiq PHYs > + * > + * Author: Alexander Stein <alexander.stein@systec-electronic.com> > + * > + * Copyright (c) 2015-2016 SYS TEC electronic GmbH > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/phy.h> > +#include <linux/of.h> > + > +#define PHY_ID_PEF7071 0xd565a401 > + > +#define MII_LANTIQ_MMD_CTRL_REG 0x0d > +#define MII_LANTIQ_MMD_REGDATA_REG 0x0e > +#define OP_DATA 1 > + > +struct lantiqphy_led_ctrl { > + const char *property; > + u32 regnum; > +}; > + > +static int lantiq_extended_write(struct phy_device *phydev, > + u8 mode, u32 dev_addr, u32 regnum, u16 val) > +{ > + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, dev_addr); > + phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, regnum); > + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, (mode << 14) | dev_addr); > + return phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, val); > +} > + > +static int lantiq_of_load_led_config(struct phy_device *phydev, > + struct device_node *of_node, > + const struct lantiqphy_led_ctrl *leds, > + u8 entries) > +{ > + u16 val; > + int i; > + int ret = 0; > + > + for (i = 0; i < entries; i++) { > + if (!of_property_read_u16(of_node, leds[i].property, &val)) { > + ret = lantiq_extended_write(phydev, OP_DATA, 0x1f, > + leds[i].regnum, val); > + if (ret) { > + dev_err(&phydev->dev, "Error writing register 0x1f.%04x (%d)\n", > + leds[i].regnum, ret); > + break; > + } > + } > + } > + > + return ret; > +} > + > +static const struct lantiqphy_led_ctrl leds[] = { > + { > + .property = "led0h", > + .regnum = 0x01e2, > + }, > + { > + .property = "led0l", > + .regnum = 0x01e3, > + }, > + { > + .property = "led1h", > + .regnum = 0x01e4, > + }, > + { > + .property = "led1l", > + .regnum = 0x01e5, > + }, > + { > + .property = "led2h", > + .regnum = 0x01e6, > + }, > + { > + .property = "led2l", > + .regnum = 0x01e7, > + }, > +}; > + > +static int lantiqphy_config_init(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->dev; > + struct device_node *of_node = dev->of_node; > + u32 max_speed; > + > + if (!of_node && dev->parent->of_node) > + of_node = dev->parent->of_node; > + > + if (of_node) { > + lantiq_of_load_led_config(phydev, of_node, leds, > + ARRAY_SIZE(leds)); > + > + if (!of_property_read_u32(of_node, "max-speed", > + &max_speed)) { > + /* The default values for phydev->supported are > + * provided by the PHY driver "features" member, > + * we want to reset to sane defaults fist before > + * supporting higher speeds. > + */ > + phydev->supported &= PHY_DEFAULT_FEATURES; > + > + switch (max_speed) { > + default: > + break; > + > + case SPEED_1000: > + phydev->supported |= PHY_1000BT_FEATURES; > + case SPEED_100: > + phydev->supported |= PHY_100BT_FEATURES; > + case SPEED_10: > + phydev->supported |= PHY_10BT_FEATURES; > + } > + } > + } > + return 0; > +} > + > +static struct phy_driver lantiqphy_driver[] = { > +{ > + .phy_id = PHY_ID_PEF7071, > + .phy_id_mask = 0x00fffffe, > + .name = "Lantiq PEF7071", > + .features = (PHY_GBIT_FEATURES | SUPPORTED_Pause), > + .flags = PHY_HAS_MAGICANEG, > + .config_init = lantiqphy_config_init, > + .config_aneg = genphy_config_aneg, > + .read_status = genphy_read_status, > + .suspend = genphy_suspend, > + .resume = genphy_resume, > + .driver = { .owner = THIS_MODULE,}, > +} }; > + > +static int __init lantiqphy_init(void) > +{ > + return phy_drivers_register(lantiqphy_driver, > + ARRAY_SIZE(lantiqphy_driver)); > +} > + > +static void __exit lantiqphy_exit(void) > +{ > + phy_drivers_unregister(lantiqphy_driver, > + ARRAY_SIZE(lantiqphy_driver)); > +} > + > +module_init(lantiqphy_init); > +module_exit(lantiqphy_exit); > + > +MODULE_DESCRIPTION("Lantiq PHY driver"); > +MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>"); > +MODULE_LICENSE("GPL"); > + > +static struct mdio_device_id __maybe_unused lantiq_tbl[] = { > + { PHY_ID_PEF7071, 0x00fffffe }, > + { } > +}; > +MODULE_DEVICE_TABLE(mdio, lantiq_tbl); >
> For LEDs, we had a patch series floating around adding LED triggers [1], > and it seems to me like the LEDs class subsystem would be a good fit for > controlling PHY LEDs, possibly with the help of PHYLIB when it comes to > doing the low-level work of registering LEDs and their names with the > LEDS subsystem. > > [1]: http://lists.openwall.net/netdev/2016/03/23/61 That patch fizzled out. I got the feeling it was pushing the capabilities of the coder. I do however think it is a reasonable path to follow for PHY LEDs. I took a quick look at the datasheet and the controlling of the LEDs is very flexible. It should not be a problem to expose some of that functionality via LED triggers. Andrew
Hi, On 18/05/2016 18:24, Florian Fainelli wrote: > CC'ing Andrew, John, > also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for several years that seems a little more complete than this one. https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patches-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f398afb8fdc879c1dd973f997;hb=HEAD John > On 05/18/2016 09:03 AM, Alexander Stein wrote: >> This currently only supports PEF7071 and allows to specify max-speed and >> is able to read the LED configuration from device-tree. >> >> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> >> --- >> The main purpose for now is to set a LED configuration from device tree and >> to limit the maximum speed. The latter one in my case hardware limited. >> As MAC and it's link partner support 1000MBit/s they would try to use that >> but will eventually fail due to magnetics only supporting 100MBit/s. So >> limit the maximum link speed supported directly from the start. > > The 'max-speed' parsing that you do in the driver should not be needed, > PHYLIB takes care of that already see > drivers/net/phy/phy_device.c::of_set_phy_supported > > For LEDs, we had a patch series floating around adding LED triggers [1], > and it seems to me like the LEDs class subsystem would be a good fit for > controlling PHY LEDs, possibly with the help of PHYLIB when it comes to > doing the low-level work of registering LEDs and their names with the > LEDS subsystem. > > [1]: http://lists.openwall.net/netdev/2016/03/23/61 > >> >> As this is a RFC I skipped the device tree binding doc. > > Too bad, that's probably what needs to be discussed here, because the > driver looks pretty reasonable otherwise. > >> >> drivers/net/phy/Kconfig | 5 ++ >> drivers/net/phy/Makefile | 1 + >> drivers/net/phy/lantiq.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 173 insertions(+) >> create mode 100644 drivers/net/phy/lantiq.c >> >> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >> index 3e28f7a..c004885 100644 >> --- a/drivers/net/phy/Kconfig >> +++ b/drivers/net/phy/Kconfig >> @@ -119,6 +119,11 @@ config STE10XP >> ---help--- >> This is the driver for the STe100p and STe101p PHYs. >> >> +config LANTIQ_PHY >> + tristate "Driver for Lantiq PHYs" >> + ---help--- >> + Supports the PEF7071 PHYs. >> + >> config LSI_ET1011C_PHY >> tristate "Driver for LSI ET1011C PHY" >> ---help--- >> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile >> index 8ad4ac6..e886549 100644 >> --- a/drivers/net/phy/Makefile >> +++ b/drivers/net/phy/Makefile >> @@ -38,3 +38,4 @@ obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o >> obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o >> obj-$(CONFIG_AMD_XGBE_PHY) += amd-xgbe-phy.o >> obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o >> +obj-$(CONFIG_LANTIQ_PHY) += lantiq.o >> diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c >> new file mode 100644 >> index 0000000..876a7d1 >> --- /dev/null >> +++ b/drivers/net/phy/lantiq.c >> @@ -0,0 +1,167 @@ >> +/* >> + * Driver for Lantiq PHYs >> + * >> + * Author: Alexander Stein <alexander.stein@systec-electronic.com> >> + * >> + * Copyright (c) 2015-2016 SYS TEC electronic GmbH >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or (at your >> + * option) any later version. >> + * >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/phy.h> >> +#include <linux/of.h> >> + >> +#define PHY_ID_PEF7071 0xd565a401 >> + >> +#define MII_LANTIQ_MMD_CTRL_REG 0x0d >> +#define MII_LANTIQ_MMD_REGDATA_REG 0x0e >> +#define OP_DATA 1 >> + >> +struct lantiqphy_led_ctrl { >> + const char *property; >> + u32 regnum; >> +}; >> + >> +static int lantiq_extended_write(struct phy_device *phydev, >> + u8 mode, u32 dev_addr, u32 regnum, u16 val) >> +{ >> + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, dev_addr); >> + phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, regnum); >> + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, (mode << 14) | dev_addr); >> + return phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, val); >> +} >> + >> +static int lantiq_of_load_led_config(struct phy_device *phydev, >> + struct device_node *of_node, >> + const struct lantiqphy_led_ctrl *leds, >> + u8 entries) >> +{ >> + u16 val; >> + int i; >> + int ret = 0; >> + >> + for (i = 0; i < entries; i++) { >> + if (!of_property_read_u16(of_node, leds[i].property, &val)) { >> + ret = lantiq_extended_write(phydev, OP_DATA, 0x1f, >> + leds[i].regnum, val); >> + if (ret) { >> + dev_err(&phydev->dev, "Error writing register 0x1f.%04x (%d)\n", >> + leds[i].regnum, ret); >> + break; >> + } >> + } >> + } >> + >> + return ret; >> +} >> + >> +static const struct lantiqphy_led_ctrl leds[] = { >> + { >> + .property = "led0h", >> + .regnum = 0x01e2, >> + }, >> + { >> + .property = "led0l", >> + .regnum = 0x01e3, >> + }, >> + { >> + .property = "led1h", >> + .regnum = 0x01e4, >> + }, >> + { >> + .property = "led1l", >> + .regnum = 0x01e5, >> + }, >> + { >> + .property = "led2h", >> + .regnum = 0x01e6, >> + }, >> + { >> + .property = "led2l", >> + .regnum = 0x01e7, >> + }, >> +}; >> + >> +static int lantiqphy_config_init(struct phy_device *phydev) >> +{ >> + struct device *dev = &phydev->dev; >> + struct device_node *of_node = dev->of_node; >> + u32 max_speed; >> + >> + if (!of_node && dev->parent->of_node) >> + of_node = dev->parent->of_node; >> + >> + if (of_node) { >> + lantiq_of_load_led_config(phydev, of_node, leds, >> + ARRAY_SIZE(leds)); >> + >> + if (!of_property_read_u32(of_node, "max-speed", >> + &max_speed)) { >> + /* The default values for phydev->supported are >> + * provided by the PHY driver "features" member, >> + * we want to reset to sane defaults fist before >> + * supporting higher speeds. >> + */ >> + phydev->supported &= PHY_DEFAULT_FEATURES; >> + >> + switch (max_speed) { >> + default: >> + break; >> + >> + case SPEED_1000: >> + phydev->supported |= PHY_1000BT_FEATURES; >> + case SPEED_100: >> + phydev->supported |= PHY_100BT_FEATURES; >> + case SPEED_10: >> + phydev->supported |= PHY_10BT_FEATURES; >> + } >> + } >> + } >> + return 0; >> +} >> + >> +static struct phy_driver lantiqphy_driver[] = { >> +{ >> + .phy_id = PHY_ID_PEF7071, >> + .phy_id_mask = 0x00fffffe, >> + .name = "Lantiq PEF7071", >> + .features = (PHY_GBIT_FEATURES | SUPPORTED_Pause), >> + .flags = PHY_HAS_MAGICANEG, >> + .config_init = lantiqphy_config_init, >> + .config_aneg = genphy_config_aneg, >> + .read_status = genphy_read_status, >> + .suspend = genphy_suspend, >> + .resume = genphy_resume, >> + .driver = { .owner = THIS_MODULE,}, >> +} }; >> + >> +static int __init lantiqphy_init(void) >> +{ >> + return phy_drivers_register(lantiqphy_driver, >> + ARRAY_SIZE(lantiqphy_driver)); >> +} >> + >> +static void __exit lantiqphy_exit(void) >> +{ >> + phy_drivers_unregister(lantiqphy_driver, >> + ARRAY_SIZE(lantiqphy_driver)); >> +} >> + >> +module_init(lantiqphy_init); >> +module_exit(lantiqphy_exit); >> + >> +MODULE_DESCRIPTION("Lantiq PHY driver"); >> +MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>"); >> +MODULE_LICENSE("GPL"); >> + >> +static struct mdio_device_id __maybe_unused lantiq_tbl[] = { >> + { PHY_ID_PEF7071, 0x00fffffe }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(mdio, lantiq_tbl); >> > >
Hi John, On Thursday 19 May 2016 06:50:56, John Crispin wrote: > On 18/05/2016 18:24, Florian Fainelli wrote: > > CC'ing Andrew, John, > > also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for > several years that seems a little more complete than this one. > > https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patc > hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f3 > 98afb8fdc879c1dd973f997;hb=HEAD Thanks for the link, I wasn't aware of that patch. I like it in general, but there are some things I'd like to get addressed first: * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on PEf7071 which does not have this register at all * Why is PHY_HAS_INTERRUPT commented out everywhere? * ltq_phy_init and ltq_phy_exit can be simplified using phy_drivers_register and phy_drivers_unregister * A mdio_device_id table is missing Best regards, Alexander
[ changing haukes mail addr to the intel one ] On 19/05/2016 08:57, Alexander Stein wrote: > Hi John, > > On Thursday 19 May 2016 06:50:56, John Crispin wrote: >> On 18/05/2016 18:24, Florian Fainelli wrote: >>> CC'ing Andrew, John, >> >> also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for >> several years that seems a little more complete than this one. >> >> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/patc >> hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d261f3 >> 98afb8fdc879c1dd973f997;hb=HEAD > > Thanks for the link, I wasn't aware of that patch. I like it in general, but > there are some things I'd like to get addressed first: > * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on > PEf7071 which does not have this register at all we use this driver mainly on the 11g and 22f version. mathias recently added the led3 handling. @Mathias, can you have a look at this and fix it inside the lede tree ? > * Why is PHY_HAS_INTERRUPT commented out everywhere? legacy code, the old mips silicon had a bug and the internal phys irq lines worked unreliably so we used polling instead. rather than remove the code i just disabled that part. code is not cleaned up yet for upstream submission as you can tell :-) > * ltq_phy_init and ltq_phy_exit can be simplified using phy_drivers_register > and phy_drivers_unregister yes, this driver is based on a version made by daniel from spharion about 5 years ago. that api probably did not exist at the time > * A mdio_device_id table is missing same as previous answer, old api version John
On Wednesday 18 May 2016 19:01:09, Andrew Lunn wrote: > > For LEDs, we had a patch series floating around adding LED triggers [1], > > and it seems to me like the LEDs class subsystem would be a good fit for > > controlling PHY LEDs, possibly with the help of PHYLIB when it comes to > > doing the low-level work of registering LEDs and their names with the > > LEDS subsystem. > > > > [1]: http://lists.openwall.net/netdev/2016/03/23/61 > > That patch fizzled out. I got the feeling it was pushing the > capabilities of the coder. I do however think it is a reasonable path > to follow for PHY LEDs. > > I took a quick look at the datasheet and the controlling of the LEDs > is very flexible. It should not be a problem to expose some of that > functionality via LED triggers. To be honest I don't know how the PHY LEDs could be set by LED triggers. Wouldn't that require to create triggers for each value which can be written to LEDxH and LEDxL? In my case the hardware requires some specific setting due to LED connections. Of course making the LED configuration changeable at runtime would be awesome. Best regards, Alexander
On Thursday 19 May 2016 09:03:26, John Crispin wrote: > [ changing haukes mail addr to the intel one ] > > On 19/05/2016 08:57, Alexander Stein wrote: > > Hi John, > > > > On Thursday 19 May 2016 06:50:56, John Crispin wrote: > >> On 18/05/2016 18:24, Florian Fainelli wrote: > >>> CC'ing Andrew, John, > >> > >> also CC'ing Matthias and Hauke. we have had a driver in OpenWrt/LEDE for > >> several years that seems a little more complete than this one. > >> > >> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/lantiq/p > >> atc > >> hes-4.4/0023-NET-PHY-adds-driver-for-lantiq-PHY11G.patch;h=93bb4275ec1d2 > >> 61f3 98afb8fdc879c1dd973f997;hb=HEAD > > > > Thanks for the link, I wasn't aware of that patch. I like it in general, > > but there are some things I'd like to get addressed first: > > * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on > > > > PEf7071 which does not have this register at all > > we use this driver mainly on the 11g and 22f version. mathias recently > added the led3 handling. > > @Mathias, can you have a look at this and fix it inside the lede tree ? > > > * Why is PHY_HAS_INTERRUPT commented out everywhere? > > legacy code, the old mips silicon had a bug and the internal phys irq > lines worked unreliably so we used polling instead. rather than remove > the code i just disabled that part. code is not cleaned up yet for > upstream submission as you can tell :-) Would you or Mathias mind dropping a cleaned up patch to netdev ml, cc'ing me? I can try it on our hardware using the 11g. Maybe I can even test the IRQ feature. Regards, Alexander
2016-05-19 9:03 GMT+02:00 John Crispin <john@phrozen.org>: > On 19/05/2016 08:57, Alexander Stein wrote: >> Thanks for the link, I wasn't aware of that patch. I like it in general, but >> there are some things I'd like to get addressed first: >> * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on >> PEf7071 which does not have this register at all > > we use this driver mainly on the 11g and 22f version. mathias recently > added the led3 handling. > > @Mathias, can you have a look at this and fix it inside the lede tree ? Well, I haven't added the led3 handling, I've only changed the initial value (function) of led3. Maybe it's cleaner to not use a default value for the led function and completely rely on the device tree bindings. But by adjusting the initial values, I had to change only the led function of one board in the openwrt xrx200 subtarget instead of touching all dts files. I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions the led3 control register albeit there is no pin for a forth led. So I guess it's safe to write to the led3 register even for the PEF 7071. Mathias
On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote: > 2016-05-19 9:03 GMT+02:00 John Crispin <john@phrozen.org>: > > On 19/05/2016 08:57, Alexander Stein wrote: > >> Thanks for the link, I wasn't aware of that patch. I like it in general, > >> but there are some things I'd like to get addressed first: > >> * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l even on > >> > >> PEf7071 which does not have this register at all > > > > we use this driver mainly on the 11g and 22f version. mathias recently > > added the led3 handling. > > > > @Mathias, can you have a look at this and fix it inside the lede tree ? > > Well, I haven't added the led3 handling, I've only changed the initial > value (function) of led3. > > Maybe it's cleaner to not use a default value for the led function and > completely rely on the device tree bindings. But by adjusting the > initial values, I had to change only the led function of one board in > the openwrt xrx200 subtarget instead of touching all dts files. I think setting default values is good. > I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions > the led3 control register albeit there is no pin for a forth led. So I > guess it's safe to write to the led3 register even for the PEF 7071. Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention LED3x registers. There is LED3DA and LED3EN in PHY_LED but was removed in 1.6 manual. I think, some flag if the PHY supports LED3 and depend on that is just fine. Best regards, Alexander
> To be honest I don't know how the PHY LEDs could be set by LED triggers. > Wouldn't that require to create triggers for each value which can be written > to LEDxH and LEDxL? In my case the hardware requires some specific setting due > to LED connections. Supporting all possibilities is not possible. However you could picking out a subset which other PHYs could also implement. Think about the typical patterns you see if you have two LEDs, which seems to be the most common setup. We would have generic PHY trigger code, and a driver specific part which configures the PHY to that configuration, if the trigger is activated. Andrew
Hi Alexander, > -----Original Message----- > From: Alexander Stein [mailto:alexander.stein@systec-electronic.com] > Sent: Thursday, May 19, 2016 12:22 PM > To: Mathias Kresin <openwrt@kresin.me> > Cc: John Crispin <john@phrozen.org>; Florian Fainelli <f.fainelli@gmail.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; andrew@lunn.ch; > Mehrtens, Hauke <hauke.mehrtens@intel.com> > Subject: Re: [PATCH 1/1 RFC] net/phy: Add Lantiq PHY driver > > On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote: > > 2016-05-19 9:03 GMT+02:00 John Crispin <john@phrozen.org>: > > > On 19/05/2016 08:57, Alexander Stein wrote: > > >> Thanks for the link, I wasn't aware of that patch. I like it in > > >> general, but there are some things I'd like to get addressed first: > > >> * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l > > >> even on > > >> > > >> PEf7071 which does not have this register at all > > > > > > we use this driver mainly on the 11g and 22f version. mathias > > > recently added the led3 handling. > > > > > > @Mathias, can you have a look at this and fix it inside the lede tree ? > > > > Well, I haven't added the led3 handling, I've only changed the initial > > value (function) of led3. > > > > Maybe it's cleaner to not use a default value for the led function and > > completely rely on the device tree bindings. But by adjusting the > > initial values, I had to change only the led function of one board in > > the openwrt xrx200 subtarget instead of touching all dts files. > > I think setting default values is good. The registers are set to some reset values after the chip is coming out of reset, but we should set them all to the same value, Mathias said that all except for one board he knows are using only one LED per port, but they are often using different LED pins, I will change my patch. > > I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions > > the led3 control register albeit there is no pin for a forth led. So I > > guess it's safe to write to the led3 register even for the PEF 7071. > > Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention > LED3x registers. There is LED3DA and LED3EN in PHY_LED but was removed in > 1.6 manual. LED3x is only available in PEF 7072 which is a different package with more pins for the LED3 and some other interfaces. > I think, some flag if the PHY supports LED3 and depend on that is just fine. I do not know how to distinguish between PEF 7071 and PEF 7072. Hauke
Hi Hauke, On Monday 23 May 2016 09:12:54, Mehrtens, Hauke wrote: > > On Thursday 19 May 2016 12:03:10, Mathias Kresin wrote: > > > 2016-05-19 9:03 GMT+02:00 John Crispin <john@phrozen.org>: > > > > On 19/05/2016 08:57, Alexander Stein wrote: > > > >> Thanks for the link, I wasn't aware of that patch. I like it in > > > >> general, but there are some things I'd like to get addressed first: > > > >> * vr9_gphy_of_reg_init() writes uncoditionally to led3h and led3l > > > >> even on > > > >> > > > >> PEf7071 which does not have this register at all > > > > > > > > we use this driver mainly on the 11g and 22f version. mathias > > > > recently added the led3 handling. > > > > > > > > @Mathias, can you have a look at this and fix it inside the lede tree > > > > ? > > > > > > Well, I haven't added the led3 handling, I've only changed the initial > > > value (function) of led3. > > > > > > Maybe it's cleaner to not use a default value for the led function and > > > completely rely on the device tree bindings. But by adjusting the > > > initial values, I had to change only the led function of one board in > > > the openwrt xrx200 subtarget instead of touching all dts files. > > > > I think setting default values is good. > > The registers are set to some reset values after the chip is coming out of > reset, but we should set them all to the same value, Mathias said that all > except for one board he knows are using only one LED per port, but they are > often using different LED pins, I will change my patch. One LED per port? I would think of using one RJ45 socket per port which usually have 2 LEDs. > > > I know that the LTQ Datasheet for the PEF 7071 Version 1.5 mentions > > > the led3 control register albeit there is no pin for a forth led. So I > > > guess it's safe to write to the led3 register even for the PEF 7071. > > > > Mh, my PEF 7071 User Manual (Version 2.0, 2012-10-17) doesn't mention > > LED3x registers. There is LED3DA and LED3EN in PHY_LED but was removed in > > 1.6 manual. > > LED3x is only available in PEF 7072 which is a different package with more > pins for the LED3 and some other interfaces. > > I think, some flag if the PHY supports LED3 and depend on that is just > > fine. > I do not know how to distinguish between PEF 7071 and PEF 7072. I expected that PEF 7072 would have a different PHY ID, but apparently this is not the case, though I don't have a datasheet for 7072. Is there really no way to distinguish those two? Alexander
2016-05-23 11:49 GMT+02:00 Alexander Stein <alexander.stein@systec-electronic.com>: >> The registers are set to some reset values after the chip is coming out of >> reset, but we should set them all to the same value, Mathias said that all >> except for one board he knows are using only one LED per port, but they are >> often using different LED pins, I will change my patch. > > One LED per port? I would think of using one RJ45 socket per port which > usually have 2 LEDs. The majority of the CPEs I'm talking about, do not have any leds at the back side RJ45 socket. They are using a single led at the front for status indication. I'm only aware of one CPE that has two LEDs at the RJ45 socket. Mathias
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 3e28f7a..c004885 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -119,6 +119,11 @@ config STE10XP ---help--- This is the driver for the STe100p and STe101p PHYs. +config LANTIQ_PHY + tristate "Driver for Lantiq PHYs" + ---help--- + Supports the PEF7071 PHYs. + config LSI_ET1011C_PHY tristate "Driver for LSI ET1011C PHY" ---help--- diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 8ad4ac6..e886549 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -38,3 +38,4 @@ obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o obj-$(CONFIG_AMD_XGBE_PHY) += amd-xgbe-phy.o obj-$(CONFIG_MDIO_BCM_UNIMAC) += mdio-bcm-unimac.o +obj-$(CONFIG_LANTIQ_PHY) += lantiq.o diff --git a/drivers/net/phy/lantiq.c b/drivers/net/phy/lantiq.c new file mode 100644 index 0000000..876a7d1 --- /dev/null +++ b/drivers/net/phy/lantiq.c @@ -0,0 +1,167 @@ +/* + * Driver for Lantiq PHYs + * + * Author: Alexander Stein <alexander.stein@systec-electronic.com> + * + * Copyright (c) 2015-2016 SYS TEC electronic GmbH + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/phy.h> +#include <linux/of.h> + +#define PHY_ID_PEF7071 0xd565a401 + +#define MII_LANTIQ_MMD_CTRL_REG 0x0d +#define MII_LANTIQ_MMD_REGDATA_REG 0x0e +#define OP_DATA 1 + +struct lantiqphy_led_ctrl { + const char *property; + u32 regnum; +}; + +static int lantiq_extended_write(struct phy_device *phydev, + u8 mode, u32 dev_addr, u32 regnum, u16 val) +{ + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, dev_addr); + phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, regnum); + phy_write(phydev, MII_LANTIQ_MMD_CTRL_REG, (mode << 14) | dev_addr); + return phy_write(phydev, MII_LANTIQ_MMD_REGDATA_REG, val); +} + +static int lantiq_of_load_led_config(struct phy_device *phydev, + struct device_node *of_node, + const struct lantiqphy_led_ctrl *leds, + u8 entries) +{ + u16 val; + int i; + int ret = 0; + + for (i = 0; i < entries; i++) { + if (!of_property_read_u16(of_node, leds[i].property, &val)) { + ret = lantiq_extended_write(phydev, OP_DATA, 0x1f, + leds[i].regnum, val); + if (ret) { + dev_err(&phydev->dev, "Error writing register 0x1f.%04x (%d)\n", + leds[i].regnum, ret); + break; + } + } + } + + return ret; +} + +static const struct lantiqphy_led_ctrl leds[] = { + { + .property = "led0h", + .regnum = 0x01e2, + }, + { + .property = "led0l", + .regnum = 0x01e3, + }, + { + .property = "led1h", + .regnum = 0x01e4, + }, + { + .property = "led1l", + .regnum = 0x01e5, + }, + { + .property = "led2h", + .regnum = 0x01e6, + }, + { + .property = "led2l", + .regnum = 0x01e7, + }, +}; + +static int lantiqphy_config_init(struct phy_device *phydev) +{ + struct device *dev = &phydev->dev; + struct device_node *of_node = dev->of_node; + u32 max_speed; + + if (!of_node && dev->parent->of_node) + of_node = dev->parent->of_node; + + if (of_node) { + lantiq_of_load_led_config(phydev, of_node, leds, + ARRAY_SIZE(leds)); + + if (!of_property_read_u32(of_node, "max-speed", + &max_speed)) { + /* The default values for phydev->supported are + * provided by the PHY driver "features" member, + * we want to reset to sane defaults fist before + * supporting higher speeds. + */ + phydev->supported &= PHY_DEFAULT_FEATURES; + + switch (max_speed) { + default: + break; + + case SPEED_1000: + phydev->supported |= PHY_1000BT_FEATURES; + case SPEED_100: + phydev->supported |= PHY_100BT_FEATURES; + case SPEED_10: + phydev->supported |= PHY_10BT_FEATURES; + } + } + } + return 0; +} + +static struct phy_driver lantiqphy_driver[] = { +{ + .phy_id = PHY_ID_PEF7071, + .phy_id_mask = 0x00fffffe, + .name = "Lantiq PEF7071", + .features = (PHY_GBIT_FEATURES | SUPPORTED_Pause), + .flags = PHY_HAS_MAGICANEG, + .config_init = lantiqphy_config_init, + .config_aneg = genphy_config_aneg, + .read_status = genphy_read_status, + .suspend = genphy_suspend, + .resume = genphy_resume, + .driver = { .owner = THIS_MODULE,}, +} }; + +static int __init lantiqphy_init(void) +{ + return phy_drivers_register(lantiqphy_driver, + ARRAY_SIZE(lantiqphy_driver)); +} + +static void __exit lantiqphy_exit(void) +{ + phy_drivers_unregister(lantiqphy_driver, + ARRAY_SIZE(lantiqphy_driver)); +} + +module_init(lantiqphy_init); +module_exit(lantiqphy_exit); + +MODULE_DESCRIPTION("Lantiq PHY driver"); +MODULE_AUTHOR("Alexander Stein <alexander.stein@systec-electronic.com>"); +MODULE_LICENSE("GPL"); + +static struct mdio_device_id __maybe_unused lantiq_tbl[] = { + { PHY_ID_PEF7071, 0x00fffffe }, + { } +}; +MODULE_DEVICE_TABLE(mdio, lantiq_tbl);
This currently only supports PEF7071 and allows to specify max-speed and is able to read the LED configuration from device-tree. Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> --- The main purpose for now is to set a LED configuration from device tree and to limit the maximum speed. The latter one in my case hardware limited. As MAC and it's link partner support 1000MBit/s they would try to use that but will eventually fail due to magnetics only supporting 100MBit/s. So limit the maximum link speed supported directly from the start. As this is a RFC I skipped the device tree binding doc. drivers/net/phy/Kconfig | 5 ++ drivers/net/phy/Makefile | 1 + drivers/net/phy/lantiq.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+) create mode 100644 drivers/net/phy/lantiq.c