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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 19 Apr 2020 18:33:40 +0200
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>
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