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

[Andrew Lunn <andrew@lunn.ch>]

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