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

[Michael Walle <michael@walle.cc>]

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