Date: Fri, 6 Jan 2012 14:03:27 +0100 From: Stefan Bethke <stb@lassitu.de> To: Marius Strobl <marius@alchemy.franken.de> Cc: freebsd-arch@freebsd.org Subject: Re: Extending sys/dev/mii Message-ID: <331547BD-7CFD-40B3-97C0-2C85B31D03FB@lassitu.de> In-Reply-To: <47ABA638-7E08-4350-A03C-3D4A23BF2D7E@lassitu.de> References: <8D025847-4BE4-4B2C-87D7-97E72CC9D325@lassitu.de> <20120104215930.GM90831@alchemy.franken.de> <47ABA638-7E08-4350-A03C-3D4A23BF2D7E@lassitu.de>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail-3--344543017
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=us-ascii
(Now with actual patch attached. Thanks ray!)
Am 05.01.2012 um 21:52 schrieb Stefan Bethke:
> The problem with this is that the miibus instance might not be a =
(transitive) child of the ethernet driver that has the MII that needs to =
be adjusted to the new PHY settings. And since the method does not =
provide any parameters about which phy or miibus did issue the method, =
or which ifp it applies to, bubbling it up won't work (that the scenario =
where the PHY for arge0 is connected to the switch's MDIO, which is =
attached to arge1's MDIO).
>=20
>>> Since the parent will now be the mdiobus, miibus needs effectively =
two attachments, one to the provider of the MDIO access, the other for =
the ethernet interface. I propose to associate the ethernet interface =
by a modified mii_attach() function that takes a device_t (of the =
ethernet driver) instead of the two callback function pointers.
>>=20
>> Please elaborate on why these changes are technically necessary
>> to implement what you are trying do. Otherwise I prefer to avoid
>> them given the rototilling they'd cause.
>=20
> Necessary is a strong word. Right now, I'm trying to understand how a =
sensible change would even look like, and which combination of glue code =
and miibus changes make the most sense.
>=20
> Let me see if I can come up with a prototype patch the next couple of =
days, so we don't have to theorize about the changes that might or might =
not be necessary.
Here's a patch that causes zero rototilling, if I'm not mistaken.
The patch implements the split between the MDIO access and notifications =
posted to the ethernet interface device that has the MII that needs to =
be adjusted in accordance with the PHY autonegotiation results. I've =
added a field to the ivars struct and not the softc, because the softc =
is included by many network drivers, while the ivars are private to =
mii.c For this reason, I believe this change is API and ABI compatible, =
and likely can be MFCed. (I believe MFCing is not high on the priority =
list because many other parts in sys/mips would need to be MFCed first =
for all the Atheros platforms to become fully usable, but Adrian can =
correct me.)
A second patch will implement a separate MDIO bus driver, but I haven't =
finished that yet. It s completely independent of the above change.
Stefan
--=20
Stefan Bethke <stb@lassitu.de> Fon +49 151 14070811
--Apple-Mail-3--344543017
Content-Disposition: attachment;
filename=mii.diff
Content-Type: application/octet-stream;
name="mii.diff"
Content-Transfer-Encoding: 7bit
diff --git a/sys/dev/mii/mii.c b/sys/dev/mii/mii.c
index f85c579..ef9421b 100644
--- a/sys/dev/mii/mii.c
+++ b/sys/dev/mii/mii.c
@@ -106,6 +106,7 @@ driver_t miibus_driver = {
struct miibus_ivars {
struct ifnet *ifp;
+ device_t ifdev;
ifm_change_cb_t ifmedia_upd;
ifm_stat_cb_t ifmedia_sts;
u_int mii_flags;
@@ -300,11 +301,10 @@ miibus_writereg(device_t dev, int phy, int reg, int data)
static void
miibus_statchg(device_t dev)
{
- device_t parent;
+ struct miibus_ivars *ivars = device_get_ivars(dev);
struct mii_data *mii;
- parent = device_get_parent(dev);
- MIIBUS_STATCHG(parent);
+ MIIBUS_STATCHG(ivars->ifdev);
mii = device_get_softc(dev);
mii->mii_ifp->if_baudrate = ifmedia_baudrate(mii->mii_media_active);
@@ -313,12 +313,11 @@ miibus_statchg(device_t dev)
static void
miibus_linkchg(device_t dev)
{
+ struct miibus_ivars *ivars = device_get_ivars(dev);
struct mii_data *mii;
- device_t parent;
int link_state;
- parent = device_get_parent(dev);
- MIIBUS_LINKCHG(parent);
+ MIIBUS_LINKCHG(ivars->ifdev);
mii = device_get_softc(dev);
@@ -335,12 +334,13 @@ miibus_linkchg(device_t dev)
static void
miibus_mediainit(device_t dev)
{
+ struct miibus_ivars *ivars = device_get_ivars(dev);
struct mii_data *mii;
struct ifmedia_entry *m;
int media = 0;
- /* Poke the parent in case it has any media of its own to add. */
- MIIBUS_MEDIAINIT(device_get_parent(dev));
+ /* Poke the interface device in case it has any media of its own to add. */
+ MIIBUS_MEDIAINIT(ivars->ifdev);
mii = device_get_softc(dev);
LIST_FOREACH(m, &mii->mii_media.ifm_list, ifm_list) {
@@ -361,6 +361,22 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
ifm_change_cb_t ifmedia_upd, ifm_stat_cb_t ifmedia_sts, int capmask,
int phyloc, int offloc, int flags)
{
+ return mii_attach_interface(dev, dev, miibus, ifp, ifmedia_upd,
+ ifmedia_sts, capmask, phyloc, offloc, flags);
+}
+
+/*
+ * Helper function used by network interface drivers. The mdiodev device
+ * becomes the parent of the miibus and provides access to the MDIO master
+ * that communicates with the PHY(s). The ifdev device is the network
+ * interface driver that will receive MIIBUS_STATCHG, MIIBUS_LINKCHG, and
+ * MIIBUS_MEDIAINIT messages. Both devices can be the same.
+ */
+int
+mii_attach_interface(device_t mdiodev, device_t ifdev, device_t *miibus, struct ifnet *ifp,
+ ifm_change_cb_t ifmedia_upd, ifm_stat_cb_t ifmedia_sts, int capmask,
+ int phyloc, int offloc, int flags)
+{
struct miibus_ivars *ivars;
struct mii_attach_args *args, ma;
device_t *children, phy;
@@ -395,10 +411,11 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
if (ivars == NULL)
return (ENOMEM);
ivars->ifp = ifp;
+ ivars->ifdev = ifdev;
ivars->ifmedia_upd = ifmedia_upd;
ivars->ifmedia_sts = ifmedia_sts;
ivars->mii_flags = flags;
- *miibus = device_add_child(dev, "miibus", -1);
+ *miibus = device_add_child(mdiodev, "miibus", -1);
if (*miibus == NULL) {
rv = ENXIO;
goto fail;
@@ -453,7 +470,7 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
* many braindead PHYs report 0/0 in their ID registers,
* so we test for media in the BMSR.
*/
- bmsr = MIIBUS_READREG(dev, ma.mii_phyno, MII_BMSR);
+ bmsr = MIIBUS_READREG(mdiodev, ma.mii_phyno, MII_BMSR);
if (bmsr == 0 || bmsr == 0xffff ||
(bmsr & (BMSR_EXTSTAT | BMSR_MEDIAMASK)) == 0) {
/* Assume no PHY at this address. */
@@ -478,8 +495,8 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
* the `ukphy' driver, as we have no ID information to
* match on.
*/
- ma.mii_id1 = MIIBUS_READREG(dev, ma.mii_phyno, MII_PHYIDR1);
- ma.mii_id2 = MIIBUS_READREG(dev, ma.mii_phyno, MII_PHYIDR2);
+ ma.mii_id1 = MIIBUS_READREG(mdiodev, ma.mii_phyno, MII_PHYIDR1);
+ ma.mii_id2 = MIIBUS_READREG(mdiodev, ma.mii_phyno, MII_PHYIDR2);
ma.mii_offset = ivars->mii_offset;
args = malloc(sizeof(struct mii_attach_args), M_DEVBUF,
@@ -511,7 +528,7 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
rv = ENXIO;
goto fail;
}
- rv = bus_generic_attach(dev);
+ rv = bus_generic_attach(mdiodev);
if (rv != 0)
goto fail;
@@ -526,7 +543,7 @@ mii_attach(device_t dev, device_t *miibus, struct ifnet *ifp,
fail:
if (*miibus != NULL)
- device_delete_child(dev, *miibus);
+ device_delete_child(mdiodev, *miibus);
free(ivars, M_DEVBUF);
if (first != 0)
*miibus = NULL;
diff --git a/sys/dev/mii/miivar.h b/sys/dev/mii/miivar.h
index 34b0e9ed..029e6a8 100644
--- a/sys/dev/mii/miivar.h
+++ b/sys/dev/mii/miivar.h
@@ -248,6 +248,8 @@ extern driver_t miibus_driver;
int mii_attach(device_t, device_t *, struct ifnet *, ifm_change_cb_t,
ifm_stat_cb_t, int, int, int, int);
+int mii_attach_interface(device_t, device_t, device_t *, struct ifnet *,
+ ifm_change_cb_t, ifm_stat_cb_t, int, int, int, int);
void mii_down(struct mii_data *);
int mii_mediachg(struct mii_data *);
void mii_tick(struct mii_data *);
--Apple-Mail-3--344543017--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?331547BD-7CFD-40B3-97C0-2C85B31D03FB>