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

[Michael Walle]

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