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] [day] [month] [year] [list]

Message-ID: <8478a8bb5542f8e40fa17a003893f08d@walle.cc>
Date:   Sun, 19 Apr 2020 18:56:58 +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 18:49, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 06:33:40PM +0200, Michael Walle wrote:
>> >
>> > > +
>> > > +	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>;
> 
> Configuring LEDs is a mess at the moment. No two PHYs do it the
> same. For a long time i've had a TODO item to make PHY LEDs work just
> like every other LED in linux, and you can set trigger actions which
> are then implemented in hardware.
> 
> We have been pushing back on adding DT properties, it just makes the
> problem worse. If reset defaults are good enough for you, please leave
> it at that.

Unfortunately not. We need the link/act, which I presume will also be
used by most other users, thus the driver enables this setting. I don't
know any board which just have an activity led, that is just off and
blinks for a short time if there is RX or TX, which is the reset default
setting of this PHY.

-michael