netdev - Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for OpenBlocks 600

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: <1504315323.4974.98.camel@kernel.crashing.org>
Date:   Sat, 02 Sep 2017 11:22:03 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for
 OpenBlocks 600

On Fri, 2017-09-01 at 17:35 -0700, Florian Fainelli wrote:
> On 08/31/2017 09:44 PM, Benjamin Herrenschmidt wrote:
> > The vendor patches initialize those registers to get the
> > PHY working properly.
> > 
> > Sadly I don't have that PHY spec and whatever Broadcom PHY
> > code we already have don't seem to document these two shadow
> > registers (unless I miscalculated the address) so I'm keeping
> > this as "vendor magic for that board". The vendor has long
> > abandoned that product, but I find it handy to test ppc405
> > kernels and so would like to keep it alive upstream :-)
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> > ---
> > 
> > Note: Ideally, the whole driver should switch over to the
> > generic PHY layer. However this is a much bigger undertaking
> > which requires access to a bunch of HW to test, and for which
> > I have neither the time nor the HW available these days.
> 
> Yes it sure does and the function names are so close, it is almost
> irresistible not to do it.

I think there's some common ancestry :-)

That said, I'm weary of doing it without proper testing, especially
those old cell blades which I'm not sure I still have a functional
one, and whatever is using gpcs...

Cheers,
Ben.

> 
> > 
> > (Some of the HW could prove hard to find ...)
> > ---
> >  drivers/net/ethernet/ibm/emac/phy.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/ibm/emac/phy.c b/drivers/net/ethernet/ibm/emac/phy.c
> > index 35865d05fccd..daa10de542fb 100644
> > --- a/drivers/net/ethernet/ibm/emac/phy.c
> > +++ b/drivers/net/ethernet/ibm/emac/phy.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/mii.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/delay.h>
> > +#include <linux/of.h>
> >  
> >  #include "emac.h"
> >  #include "phy.h"
> > @@ -363,6 +364,34 @@ static struct mii_phy_def bcm5248_phy_def = {
> >  	.ops		= &generic_phy_ops
> >  };
> >  
> > +static int bcm5482_init(struct mii_phy *phy)
> > +{
> > +	if (!of_machine_is_compatible("plathome,obs600"))
> > +		return 0;
> 
> You can probably include brcmphy.h and pull the definition for at least
> 0x1c: MII_BCM54XX_SHD

Yup.

> > +
> > +	/* Magic inits from vendor original patches */
> > +	phy_write(phy, 0x1c, 0xa410);
> 
> What you are doing here is write to shadow register 9 (9 << 10) which is
> the LED control register, and making the activity LED be driven on
> activity/link as opposed to just activity. So this can probably be
> written as:

Ok so I really don't *need* that in fact.


> 	phy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |
> 		  MII_BCM54XX_SHD_VAL(9) | MII_BCM54XX_SHD_DATA(BIT(4));
> 
> > +	phy_write(phy, 0x1c, 0x8804);
> 
> And here you are writing to the spare control 1 register and setting bit
> 2 (which appears reserved but this is not clear) which would be enabling
> the activity LED for 10BaseT or no link which can be written as:
> 
> 	phy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |
> 		  MII_BCM54XX_SHD_VAL(2) | MII_BCM4XX_SHD_DATA(BIT(2));
> 
> So basically you are touching registers that only affect LED
> configuration and should not be doing anything else...

I wonder if I need to bother at all then. I was worried it was related
to actual function of the device, but if it's just LEDs, I think I may
as well just drop it.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct mii_phy_ops bcm5482_phy_ops = {
> > +	.init		= bcm5482_init,
> > +	.setup_aneg	= genmii_setup_aneg,
> > +	.setup_forced	= genmii_setup_forced,
> > +	.poll_link	= genmii_poll_link,
> > +	.read_link	= genmii_read_link
> > +};
> > +
> > +static struct mii_phy_def bcm5482_phy_def = {
> > +
> > +	.phy_id		= 0x0143bcb0,
> > +	.phy_id_mask	= 0x0ffffff0,
> > +	.name		= "BCM5482 Gigabit Ethernet",
> > +	.ops		= &bcm5482_phy_ops
> > +};
> > +
> >  static int m88e1111_init(struct mii_phy *phy)
> >  {
> >  	pr_debug("%s: Marvell 88E1111 Ethernet\n", __func__);
> > @@ -499,6 +528,7 @@ static struct mii_phy_def *mii_phy_table[] = {
> >  	&et1011c_phy_def,
> >  	&cis8201_phy_def,
> >  	&bcm5248_phy_def,
> > +	&bcm5482_phy_def,
> >  	&m88e1111_phy_def,
> >  	&m88e1112_phy_def,
> >  	&ar8035_phy_def,
> > 
> 
>