Re: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support

From: Michael Walle
Date: Sun Apr 19 2020 - 12:33:46 EST


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