linux-kernel - Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a separate file

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]

Message-ID: <20160629141324.GA24194@lunn.ch>
Date:	Wed, 29 Jun 2016 16:13:24 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Jon Mason <jon.mason@...adcom.com>
Cc:	zajec5@...il.com, davem@...emloft.net, f.fainelli@...il.com,
	hauke@...ke-m.de, bcm-kernel-feedback-list@...adcom.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 3/7] net: ethernet: bgmac: move BCMA MDIO Phy code into a
 separate file

Hi Jon

I know you are just refactoring code, but at some point it would be
good to take a closer look at this MDIO bus driver.

The MDIO bus driver should be generic, allowing access to all 32
addresses on the bus, if that makes sense. You could for example have
a B53 switch hanging off the MDIO bus.... The basic read/write
functions seem to allow this.

> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyinit */
> +static void bcma_mdio_phy_init(struct bcma_mdio *bcma_mdio)
> +{
> +	struct bcma_chipinfo *ci = &bcma_mdio->core->bus->chipinfo;
> +	u8 i;
> +
> +	if (ci->id == BCMA_CHIP_ID_BCM5356) {
> +		for (i = 0; i < 5; i++) {
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x008b);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x15, 0x0100);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x12, 0x2aaa);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);

It would be good to document what these writes are doing. Also, could
they be moved into the phy driver? Does the PHY implement an ID in
registers 2 and 3 that can be used to determine these are needed?

Also, why is it iterating over 5 PHYs? Is this actually a switch with
5 ports?

> +		}
> +	}
> +	if ((ci->id == BCMA_CHIP_ID_BCM5357 && ci->pkg != 10) ||
> +	    (ci->id == BCMA_CHIP_ID_BCM4749 && ci->pkg != 10) ||
> +	    (ci->id == BCMA_CHIP_ID_BCM53572 && ci->pkg != 9)) {
> +		struct bcma_drv_cc *cc = &bcma_mdio->core->bus->drv_cc;
> +
> +		bcma_chipco_chipctl_maskset(cc, 2, ~0xc0000000, 0);
> +		bcma_chipco_chipctl_maskset(cc, 4, ~0x80000000, 0);
> +		for (i = 0; i < 5; i++) {
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5284);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x0010);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000f);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x5296);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x1073);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9073);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x16, 0x52b6);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x17, 0x9273);
> +			bcma_mdio_phy_write(bcma_mdio, i, 0x1f, 0x000b);

Same comment here.

> +/* http://bcm-v4.sipsolutions.net/mac-gbit/gmac/chipphyreset */
> +static int bcma_mdio_phy_reset(struct mii_bus *bus)
> +{
> +	struct bcma_mdio *bcma_mdio = bus->priv;
> +	u8 phyaddr = bcma_mdio->phyaddr;
> +
> +	if (bcma_mdio->phyaddr == BGMAC_PHY_NOREGS)
> +		return 0;
> +
> +	bcma_mdio_phy_write(bcma_mdio, phyaddr, MII_BMCR, BMCR_RESET);
> +	udelay(100);
> +	if (bcma_mdio_phy_read(bcma_mdio, phyaddr, MII_BMCR) & BMCR_RESET)
> +		dev_err(&bcma_mdio->core->dev, "PHY reset failed\n");
> +	bcma_mdio_phy_init(bcma_mdio);

This appears to be resetting one PHY. What about the others? Why not
put this in the PHY driver?

> +struct mii_bus *bcma_mdio_mii_register(struct bcma_device *core, u8 phyaddr)
> +{
> +	struct bcma_mdio *bcma_mdio;
> +	struct mii_bus *mii_bus;
> +	int err;
> +
> +	bcma_mdio = kzalloc(sizeof(*bcma_mdio), GFP_KERNEL);
> +	if (!bcma_mdio)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mii_bus = mdiobus_alloc();
> +	if (!mii_bus) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +
> +	mii_bus->name = "bcma_mdio mii bus";
> +	sprintf(mii_bus->id, "%s-%d-%d", "bcma_mdio", core->bus->num,
> +		core->core_unit);
> +	mii_bus->priv = bcma_mdio;
> +	mii_bus->read = bcma_mdio_mii_read;
> +	mii_bus->write = bcma_mdio_mii_write;
> +	mii_bus->reset = bcma_mdio_phy_reset;
> +	mii_bus->parent = &core->dev;
> +	mii_bus->phy_mask = ~(1 << phyaddr);

Is there a need to limit this to just the one PHY?

> +
> +	bcma_mdio->core = core;
> +	bcma_mdio->phyaddr = phyaddr;
> +
> +	err = mdiobus_register(mii_bus);

Something which appears to be missing from the later patch which adds
device tree support. If you have a device node pointer, you should use
of_mdiobus_register() and document in the binding that PHYs should be
listed in the usual way.

       Andrew