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>