Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 13 Nov 2011 23:59:07 +0100
From:      Marius Strobl <marius@alchemy.franken.de>
To:        Aleksandr Rybalko <ray@ddteam.net>
Cc:        embedded@freebsd.org
Subject:   Re: Ethernet switch framework
Message-ID:  <20111113225907.GS93221@alchemy.franken.de>
In-Reply-To: <20111111004514.04bc27be.ray@ddteam.net>
References:  <20111110014904.0e8caf2c.ray@ddteam.net> <CAJ-Vmo=r0sL4rZ_MuB_G0zJaa8ThLW-EWc8yVpURiiEXK3CTYw@mail.gmail.com> <20111111004514.04bc27be.ray@ddteam.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 11, 2011 at 12:45:14AM +0200, Aleksandr Rybalko wrote:
> Hi embedders,  
> 
> Invite Marius to discussion if Marius don't have objections :)
> 
> On Thu, 10 Nov 2011 08:17:46 -0800
> Adrian Chadd <adrian@freebsd.org> wrote:
> 
> > Hi!
> > 
> > This is great. Between you, gonzo@ and loos, I think we finally will
> > have something to throw into -HEAD.
> > 
> > The miibus changes though - why did you need those? Can you run them
> > by marius@ and see what he thinks?
> > 
> > 
> > Adrian
> 
> Copy miibus diff here to easily scope it
> Index: mii.c
> ===================================================================
> --- mii.c	(revision 227410)
> +++ mii.c	(working copy)
> @@ -310,7 +310,13 @@
>  	struct mii_attach_args ma, *args;
>  	device_t *children, phy;
>  	int bmsr, first, i, nchildren, offset, phymax, phymin, rv;
> +	uint32_t phymask;
> +	const char *value;
> +	char *key;
>  
> +	phymask = 0xfffffffful;
> +	key = "phyXX";
> +
>  	if (phyloc != MII_PHY_ANY && offloc != MII_OFFSET_ANY) {
>  		printf("%s: phyloc and offloc specified\n", __func__);
>  		return (EINVAL);
> @@ -366,6 +372,9 @@
>  
>  	ma.mii_capmask = capmask;
>  
> +	resource_int_value(device_get_name(*miibus), device_get_unit
> (*miibus),
> +	    "phymask", &phymask);
> +
>  	phy = NULL;
>  	offset = 0;
>  	for (ma.mii_phyno = phymin; ma.mii_phyno <= phymax;
> ma.mii_phyno++) { @@ -390,6 +399,26 @@
>  		}
>  
>  		/*
> +		 * If phymask don't have corresponding bit set, then
> this PHY
> +		 * id marked as muted, skip to next id.
> +		 */
> +		if (!(phymask & (1 << ma.mii_phyno)))
> +			continue;
> +
> +		/*
> +		 * Check if we have hinted driver. If so, attach it.
> +		 */
> +		value = NULL;
> +		sprintf(key, "phy%d", ma.mii_phyno);
> +		if (resource_string_value(device_get_name(*miibus),
> +		    device_get_unit(*miibus), key, &value) == 0) {
> +
> +			ma.mii_id1 = 0;
> +			ma.mii_id2 = 0;
> +			goto attach_hinted;
> +		}
> +
> +		/*
>  		 * Check to see if there is a PHY at this address.
> Note,
>  		 * many braindead PHYs report 0/0 in their ID
> registers,
>  		 * so we test for media in the BMSR.
> @@ -416,13 +445,14 @@
>  	ma.mii_id1 = MIIBUS_READREG(dev, ma.mii_phyno,MII_PHYIDR1);
> 	ma.mii_id2 = MIIBUS_READREG(dev, ma.mii_phyno,MII_PHYIDR2); 
> +attach_hinted:
>  		ma.mii_offset = offset;
>  		args = malloc(sizeof(struct mii_attach_args), M_DEVBUF,
>  		    M_NOWAIT);
>  		if (args == NULL)
>  			goto skip;
>  		bcopy((char *)&ma, (char *)args, sizeof(ma));
> -		phy = device_add_child(*miibus, NULL, -1);
> +		phy = device_add_child(*miibus, value, -1);
>  		if (phy == NULL) {
>  			free(args, M_DEVBUF);
>  			goto skip;
> ===================================================================
> 
> So this diff have to parts:
> 
> 1. cosmetic one - fetching and apply phymask from hints. Why I need it?
> Because switches like Atheros AR8x16 map their registers into MDIO
> space. So if we not filter it, we will have random set of PHYs on
> miibusX. Mostly ukphy, but even ukphy making read to PHY registers and
> this may corrupt switch driver data (registers which Read/Clear).
> 
> 2. Second is easy and cheap way to pre-assign phy driver to
> some PHY ID on MDIO. I need it because 
> 	a) some switches have no ID in MII_PHYIDR1 and MII_PHYIDR2
> registers (BCM5325, AR8x16);
> 	b) in cases when AR8x16 on MDIO attached to many NIC's, after
> first instance of driver initialize switch, second miibus instance
> don't found any PHY's.
> 	c) D-Link DSR-500[1000] (Cavium Octeon CN5010 based), CN5010
> have single MDIO bus for 3 Ethernet interfaces. First and second NICs
> attached to MII0 and MII1 ports of BCM53115, but third attached to
> real PHY :)
> 
> Last case I configuring with following hints:
> hint.miibus.0.phymask="0x00000001"
> hint.miibus.0.phy0="switch"
> hint.miibus.2.phymask="0x00000020"
> hint.miibus.2.phy5="plumbphy"
> hint.miibus.3.phymask="0x00000100"
> 
> Maybe we have better way?
> 
> Will wait for any opinion.
> 

Apart from some style issues I'm fine with the phymask part, actually
I've nearly the same patch in one of my trees. I think we can do better
regarding hinted PHYs though. I was hoping to be able to look into this
over the week-end but unfortunately two other committers came up with
more urgent stuff. I hope to get to it over the next couple of days.

Marius




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111113225907.GS93221>