Message ID | 1523541336-145953-1-git-send-email-phil@raspberrypi.org |
---|---|
Headers | show |
Series | lan78xx: Read configuration from Device Tree | expand |
On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote: > Add two new Device Tree properties: > * microchip,eee-enabled - a boolean to enable EEE > * microchip,tx-lpi-timer - time in microseconds to wait after TX goes > idle before entering the low power state > (default 600) Hi Phil This looks wrong. What should happen is that the MAC driver calls phy_init_eee() to find out if the PHY supports EEE. There should be no need to look in device tree. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On 12/04/2018 15:06, Andrew Lunn wrote: > Hi Phil > >> - ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo); >> - ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi); >> + mac_addr = of_get_mac_address(dev->udev->dev.of_node); > > It might be better to use the higher level eth_platform_get_mac_address(). OK - I'll take your word for it. Phil -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote: > Add support for DT property "microchip,led-modes", a vector of two > cells (u32s) in the range 0-15, each of which sets the mode for one > of the two LEDs. Some possible values are: > > 0=link/activity 1=link1000/activity > 2=link100/activity 3=link10/activity > 4=link100/1000/activity 5=link10/1000/activity > 6=link10/100/activity 14=off 15=on > > Also use the presence of the DT property to indicate that the > LEDs should be enabled - necessary in the event that no valid OTP > or EEPROM is available. I'm not a fan of this, but at the moment, we don't have anything better. Please follow what mscc does, add a header file for the LED settings. Andrew > > Signed-off-by: Phil Elwell <phil@raspberrypi.org> > --- > drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index d98397b..ffb483d 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) > { > int ret; > u32 mii_adv; > + u32 led_modes[2]; > struct phy_device *phydev; > > phydev = phy_find_first(dev->mdiobus); > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) > (void)lan78xx_set_eee(dev->net, &edata); > } > > + if (!of_property_read_u32_array(dev->udev->dev.of_node, > + "microchip,led-modes", > + led_modes, ARRAY_SIZE(led_modes))) { > + u32 reg; > + int i; > + > + reg = phy_read(phydev, 0x1d); > + for (i = 0; i < ARRAY_SIZE(led_modes); i++) { > + reg &= ~(0xf << (i * 4)); > + reg |= (led_modes[i] & 0xf) << (i * 4); > + } Please add range checks for led_modes[i] and return -EINVAL if the check fails. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On 12/04/2018 15:26, Andrew Lunn wrote: > On Thu, Apr 12, 2018 at 02:55:35PM +0100, Phil Elwell wrote: >> Add support for DT property "microchip,led-modes", a vector of two >> cells (u32s) in the range 0-15, each of which sets the mode for one >> of the two LEDs. Some possible values are: >> >> 0=link/activity 1=link1000/activity >> 2=link100/activity 3=link10/activity >> 4=link100/1000/activity 5=link10/1000/activity >> 6=link10/100/activity 14=off 15=on >> >> Also use the presence of the DT property to indicate that the >> LEDs should be enabled - necessary in the event that no valid OTP >> or EEPROM is available. > > I'm not a fan of this, but at the moment, we don't have anything > better. > > Please follow what mscc does, add a header file for the LED settings. Good idea. > >> >> Signed-off-by: Phil Elwell <phil@raspberrypi.org> >> --- >> drivers/net/usb/lan78xx.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c >> index d98397b..ffb483d 100644 >> --- a/drivers/net/usb/lan78xx.c >> +++ b/drivers/net/usb/lan78xx.c >> @@ -2008,6 +2008,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) >> { >> int ret; >> u32 mii_adv; >> + u32 led_modes[2]; >> struct phy_device *phydev; >> >> phydev = phy_find_first(dev->mdiobus); >> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) >> (void)lan78xx_set_eee(dev->net, &edata); >> } >> >> + if (!of_property_read_u32_array(dev->udev->dev.of_node, >> + "microchip,led-modes", >> + led_modes, ARRAY_SIZE(led_modes))) { >> + u32 reg; >> + int i; >> + >> + reg = phy_read(phydev, 0x1d); >> + for (i = 0; i < ARRAY_SIZE(led_modes); i++) { >> + reg &= ~(0xf << (i * 4)); >> + reg |= (led_modes[i] & 0xf) << (i * 4); >> + } > > Please add range checks for led_modes[i] and return -EINVAL if the > check fails. Will do. Thanks, Phil -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) > (void)lan78xx_set_eee(dev->net, &edata); > } > > + if (!of_property_read_u32_array(dev->udev->dev.of_node, > + "microchip,led-modes", > + led_modes, ARRAY_SIZE(led_modes))) { > + u32 reg; > + int i; > + > + reg = phy_read(phydev, 0x1d); > + for (i = 0; i < ARRAY_SIZE(led_modes); i++) { > + reg &= ~(0xf << (i * 4)); > + reg |= (led_modes[i] & 0xf) << (i * 4); > + } > + (void)phy_write(phydev, 0x1d, reg); Poking PHY registers directly from the MAC driver is not always a good idea. This MAC driver does that in a few places :-( What do we know about the PHY? It is built into the device or is it external? If it is external, how do you know the LED register is at 0x1d? The safest place to do this is in the PHY driver, and place these OF properties into the PHY node. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew, On 12/04/2018 15:16, Andrew Lunn wrote: > On Thu, Apr 12, 2018 at 02:55:34PM +0100, Phil Elwell wrote: >> Add two new Device Tree properties: >> * microchip,eee-enabled - a boolean to enable EEE >> * microchip,tx-lpi-timer - time in microseconds to wait after TX goes >> idle before entering the low power state >> (default 600) > > Hi Phil > > This looks wrong. > > What should happen is that the MAC driver calls phy_init_eee() to find > out if the PHY supports EEE. There should be no need to look in device > tree. If the driver should be calling phy_init_eee to initialise EEE operation then I'm fine with that (although I notice that the TI cpsw calls phy_ethtool_set_eee but I don't see it calling phy_init_eee). However, it sounds like I need to keep my DT toggle of the EEE enablement and parameters downstream. Phil -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) > > (void)lan78xx_set_eee(dev->net, &edata); > > } > > > > + if (!of_property_read_u32_array(dev->udev->dev.of_node, > > + "microchip,led-modes", > > + led_modes, ARRAY_SIZE(led_modes))) { > > + u32 reg; > > + int i; > > + > > + reg = phy_read(phydev, 0x1d); > > + for (i = 0; i < ARRAY_SIZE(led_modes); i++) { > > + reg &= ~(0xf << (i * 4)); > > + reg |= (led_modes[i] & 0xf) << (i * 4); > > + } > > + (void)phy_write(phydev, 0x1d, reg); > > Poking PHY registers directly from the MAC driver is not always a good > idea. This MAC driver does that in a few places :-( Agree but, some are for workaround unfortunately. > What do we know about the PHY? It is built into the device or is it > external? If it is external, how do you know the LED register is at > 0x1d? This register is not defined in include/linux/microchipphy.h. :( Also agree that there parts should be applied to internal PHY only. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html