Messages in this thread | | | Date | Sun, 19 Apr 2020 18:33:40 +0200 | From | Michael Walle <> | Subject | Re: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support |
| |
Am 2020-04-19 17:49, schrieb Andrew Lunn: > On Sun, Apr 19, 2020 at 12:12:48PM +0200, Michael Walle wrote: > > Hi Michael > >> +static int bcm54140_b0_workaround(struct phy_device *phydev) >> +{ >> + int spare3; >> + int ret; > > Could you add a comment about what this is working around?
sure
> >> +static int bcm54140_phy_probe(struct phy_device *phydev) >> +{ >> + struct bcm54140_phy_priv *priv; >> + int ret; >> + >> + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + phydev->priv = priv; >> + >> + ret = bcm54140_get_base_addr_and_port(phydev); >> + if (ret) >> + return ret; >> + >> + dev_info(&phydev->mdio.dev, >> + "probed (port %d, base PHY address %d)\n", >> + priv->port, priv->base_addr); > > phydev_dbg() ? Do we need to see this message four times?
ok. every phy will have a different port. And keep in mind, that you might have less than four ports/PHYs here. So I'd like to keep that as a phydev_dbg() if you agree.
> >> + >> + return 0; >> +} >> + >> +static int bcm54140_config_init(struct phy_device *phydev) >> +{ >> + u16 reg = 0xffff; >> + int ret; >> + >> + /* Apply hardware errata */ >> + ret = bcm54140_b0_workaround(phydev); >> + if (ret) >> + return ret; >> + >> + /* Unmask events we are interested in. */ >> + reg &= ~(BCM54140_RDB_INT_DUPLEX | >> + BCM54140_RDB_INT_SPEED | >> + BCM54140_RDB_INT_LINK); >> + ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_IMR, reg); >> + if (ret) >> + return ret; >> + >> + /* LED1=LINKSPD[1], LED2=LINKSPD[2], LED3=ACTIVITY */ >> + ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE1, >> + 0, BCM54140_RDB_SPARE1_LSLM); >> + if (ret) >> + return ret; > > What are the reset default for LEDs? Can the LEDs be configured via > strapping pins? There is currently no good solution for this. Whatever > you pick will be wrong for somebody else. At minimum, strapping pins, > if they exist, should not be overridden.
Fair enough. There are no strapping options, just the "default behaviour", where LED1/2 indicates the speed, and LED3 just activity (no link indication). And I just noticed that in this case the comment above is wrong, because it is actually link/activity. Further, there are myriad configuration options which I didn't want to encode altogether. So I've just chosen the typical one (which actually matches our hardware), ie. to have the "activity/led mode". The application note mentions some other concrete modes, but I don't know if its worth implementing them. Maybe we can have a enumeration of some distinct modes? Ie.
broadcom,led-mode = <BCM54140_NO_CHANGE>; broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;
-michael
| |