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>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
(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).
>
>>> 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.
>>
>> 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.
>
> 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.
>
> 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
--
Stefan Bethke <stb@lassitu.de> Fon +49 151 14070811
[-- Attachment #2 --]
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 *);
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?331547BD-7CFD-40B3-97C0-2C85B31D03FB>