| Message ID | 20150914103207.GH21084@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
14.09.2015 13:32, Russell King - ARM Linux ?????: > I've been bringing up the mainline kernel on a new board, which has > an Marvell SoC with a mvneta interface connected to a Marvell DSA > switch. The DSA switch is a 88E6176. > > In the DT block for the interface, I specify: > > ethernet@... { > phy-mode = "sgmii"; > status = "okay"; > > fixed-link { > speed = <1000>; > full-duplex; > }; > }; > > However, this doesn't work without patching mvneta to disable the > autonegotiation and forcing the link up: > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index 62e48bc0cb23..e1698731e429 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp) > val |= MVNETA_GMAC_INBAND_AN_ENABLE | > MVNETA_GMAC_AN_SPEED_EN | > MVNETA_GMAC_AN_DUPLEX_EN; > + /* We appear to need the interface forced for DSA switches */ > + val &= ~(MVNETA_GMAC_AN_DUPLEX_EN | > + MVNETA_GMAC_AN_SPEED_EN); > + val |= MVNETA_GMAC_FORCE_LINK_PASS; > mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); > val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER); > val |= MVNETA_GMAC_1MS_CLOCK_ENABLE; Hello Russell, just to make sure, aren't you missing this by any chance: https://lkml.org/lkml/2015/7/20/710
On Mon, Sep 14, 2015 at 02:06:13PM +0300, Stas Sergeev wrote: > 14.09.2015 13:32, Russell King - ARM Linux ?????: > >I've been bringing up the mainline kernel on a new board, which has > >an Marvell SoC with a mvneta interface connected to a Marvell DSA > >switch. The DSA switch is a 88E6176. > > > >In the DT block for the interface, I specify: > > > > ethernet@... { > > phy-mode = "sgmii"; > > status = "okay"; > > > > fixed-link { > > speed = <1000>; > > full-duplex; > > }; > > }; > > > >However, this doesn't work without patching mvneta to disable the > >autonegotiation and forcing the link up: > > > >diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > >index 62e48bc0cb23..e1698731e429 100644 > >--- a/drivers/net/ethernet/marvell/mvneta.c > >+++ b/drivers/net/ethernet/marvell/mvneta.c > >@@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp) > > val |= MVNETA_GMAC_INBAND_AN_ENABLE | > > MVNETA_GMAC_AN_SPEED_EN | > > MVNETA_GMAC_AN_DUPLEX_EN; > >+ /* We appear to need the interface forced for DSA switches */ > >+ val &= ~(MVNETA_GMAC_AN_DUPLEX_EN | > >+ MVNETA_GMAC_AN_SPEED_EN); > >+ val |= MVNETA_GMAC_FORCE_LINK_PASS; > > mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); > > val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER); > > val |= MVNETA_GMAC_1MS_CLOCK_ENABLE; > Hello Russell, just to make sure, aren't you missing this by any chance: > https://lkml.org/lkml/2015/7/20/710 Thanks, I think that will solve it. I have to wonder why that patch (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's billed as a regression that occurred in the previous merge window, and given that it was sent in July, and we're now in September. As it wasn't in v4.2, it looks like it should be a stable candidate. David, any objections to having the stable guys pick this regression fix up, if not already done so?
From: Russell King - ARM Linux <linux@arm.linux.org.uk> Date: Mon, 14 Sep 2015 12:42:09 +0100 > Thanks, I think that will solve it. I have to wonder why that patch > (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's > billed as a regression that occurred in the previous merge window, and > given that it was sent in July, and we're now in September. As it > wasn't in v4.2, it looks like it should be a stable candidate. The series had a whole bunch of non bug fixes in it and we were in the final phases of 4.2, in which case I defer to applying patches to net-next only unless I'm told otherwise. It's up the the patch/series author to let me know that an important regression fix is hidden in there, but they should have submitted it seperately from the rest in that kind of situation anyways. > David, any objections to having the stable guys pick this regression > fix up, if not already done so? More than this patch is needed, the one before it (3/4) instantiates the necessary property in the DT, for example. I can queue up the whole series for -stable if you want.
On 17/09/15 15:12, David Miller wrote: > From: Russell King - ARM Linux <linux@arm.linux.org.uk> > Date: Mon, 14 Sep 2015 12:42:09 +0100 > >> Thanks, I think that will solve it. I have to wonder why that patch >> (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's >> billed as a regression that occurred in the previous merge window, and >> given that it was sent in July, and we're now in September. As it >> wasn't in v4.2, it looks like it should be a stable candidate. > > The series had a whole bunch of non bug fixes in it and we were in > the final phases of 4.2, in which case I defer to applying patches > to net-next only unless I'm told otherwise. To your defense, Staas and I kept arguing for a while, slowing the entire process down until we agreed on a proper solution, the submission was targeting your 'net' tree, but I did not realize until now that these got applied to 'net-next'. > > It's up the the patch/series author to let me know that an important > regression fix is hidden in there, but they should have submitted > it seperately from the rest in that kind of situation anyways. > >> David, any objections to having the stable guys pick this regression >> fix up, if not already done so? > > More than this patch is needed, the one before it (3/4) instantiates > the necessary property in the DT, for example. > > I can queue up the whole series for -stable if you want. I think this would be a good thing, mvneta-based platforms are fairly popular. Thank you!
On Thu, Sep 17, 2015 at 03:12:47PM -0700, David Miller wrote: > From: Russell King - ARM Linux <linux@arm.linux.org.uk> > Date: Mon, 14 Sep 2015 12:42:09 +0100 > > > Thanks, I think that will solve it. I have to wonder why that patch > > (f8af8e6eb9509 in mainline) didn't made it into v4.2 though, as it's > > billed as a regression that occurred in the previous merge window, and > > given that it was sent in July, and we're now in September. As it > > wasn't in v4.2, it looks like it should be a stable candidate. > > The series had a whole bunch of non bug fixes in it and we were in > the final phases of 4.2, in which case I defer to applying patches > to net-next only unless I'm told otherwise. > > It's up the the patch/series author to let me know that an important > regression fix is hidden in there, but they should have submitted > it seperately from the rest in that kind of situation anyways. > > > David, any objections to having the stable guys pick this regression > > fix up, if not already done so? > > More than this patch is needed, the one before it (3/4) instantiates > the necessary property in the DT, for example. > > I can queue up the whole series for -stable if you want. Sorry in advance for this rambling reply... I'm not entirely certain that'd be a good idea at the moment, for a number of reasons, which are coming up because I'm looking at getting a SFP cage supported with mvneta hardware. 1. Serdes gigabit ethernet links have two operating modes for in-band "negotiation" - Cisco SGMII format, and 1000base-X format. Both use exactly the same encoding on the wire, the only differences between them are the contents of a 16-bit configuration word and how each end of the link handles that. SFP can use either format depending on the module hot-plugged in - fiber modules will normally use 1000base-X, but copper modules which contain a PHY may use either SGMII or 1000base-X. (Fiber modules for 100baseFX will probably use SGMII though.) The issue there is two-fold: that the new DT property just says it's "in-band" or "auto" but there's no way to specify the format of the in-band configuration. 2. With Serdes, the PCS layer of the PHY, which does the autonegotiation, is moved to the MAC. When connected to a SGMII PHY, the PHY may report over the Serdes connection the Cisco SGMII configuration word which instructs the MAC how to configure itself. It's not "negotiation" by any means, but "phy telling the MAC how to configure itself" word. Having "in-band" enabled pretty much requires the use of the "fixed-link" property, which seems to be a total hack around the PCS layer being in the MAC - the "fixed-link" phy is no longer fixed, but is used as a means to convey the negotiated results from the MAC side PCS to the software-emulated PHY, only to have them pop back out into the MAC driver. If you specify "in-band" without a "fixed-link" but have other MACs making use of the fixed-link support, all hell breaks loose, because mvneta will call the fixed-link update function with the real phy with the in-band results, and this can hit a fixed-link PHY for some other network adapter. The fixed-link PHY code makes no attempt to validate that the phy_device passed in really is a fixed-link phy and not a MDIO phy. 3. Having DT specify a fixed-link with parameters along with in-band negotiation results in the fixed-link parameters being ignored. This means if a fixed-link DT declaration specifies a speed, that declaration will be ignored. What I'm basically saying is that: phy-mode = "sgmii"; fixed-link { speed = <1000>; }; specifies a fixed-speed serdes link at 1000mbps, but: phy-mode = "sgmii"; managed = "in-band-status"; fixed-link { speed = <1000>; }; does not fix the speed at all. _But_ using the in-band status property fundamentally requires this for mvneta to behave correctly: phy-mode = "sgmii"; managed = "in-band-status"; fixed-link { }; with _no_ phy node. 4. Going back to the SFP problem, the link is only up when the SFP module pins indicate that there's no transmitter fault, no loss of signal _and_ the PCS layer at the MAC indicates that it has completed negotiation. This pretty much rules out trying to emulate a SFP cage as a software-based PHY. I've code right now doing exactly that, and it results in netif_carrier_on() being called far too early. What I don't know is how many generations of the mvneta hardware have support for both serdes modes, but what I'm basically saying is that the solution we now have seems to be somewhat lacking - maybe it should have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the ability to add additional modes later. The other point I'm making above is that I'm forming the opinion that the existing PHY layer isn't flexible enough for supporting SFP, and I need some way to represent at least part of the autonegotiation at the MAC level without involving the PHY level - especially when considering that a real PHY might be inside the SFP cage which can be talked to over I2C. This is the problem I'm presently grappling with, and it's taking lots of thought right now. I'm aware of other drivers in the kernel which support SFP, each using their own implementations to support that. Lastly, while looking at this, I've a small stack of patches for the PHY code resolving some of the issues I've mentioned above, and fixing broken reference counting and mdio bus module removal issues: phy: fixed-phy: properly validate phy in fixed_phy_update_state() net: fix phy refcounting in a bunch of drivers of_mdio: fix MDIO phy device refcounting phy: add proper phy struct device refcounting phy: fix mdiobus module safety phy: fix of_mdio_find_bus() device refcount leak I hope to be able to send those out in the next few days - they have nothing to do with SFP itself but are the results of looking through the PHY code.
From: Florian Fainelli <f.fainelli@gmail.com> Date: Thu, 17 Sep 2015 16:02:41 -0700 > On 17/09/15 15:12, David Miller wrote: >> I can queue up the whole series for -stable if you want. > > I think this would be a good thing, mvneta-based platforms are fairly > popular. Done.
On 17/09/15 16:14, Russell King - ARM Linux wrote: [snip] > with _no_ phy node. > > 4. Going back to the SFP problem, the link is only up when the SFP > module pins indicate that there's no transmitter fault, no loss of > signal _and_ the PCS layer at the MAC indicates that it has completed > negotiation. This pretty much rules out trying to emulate a SFP cage > as a software-based PHY. I've code right now doing exactly that, and > it results in netif_carrier_on() being called far too early. Andrew recently added support for having the fixed PHY emulated PHY poll a GPIO to determine link status. If this information comes differently (e.g: via MMIO/MAC registers) in your case, I guess we could either extend the fixed PHY to support that scheme, or just open-code the carrier state change in the mvneta driver. > > What I don't know is how many generations of the mvneta hardware have > support for both serdes modes, but what I'm basically saying is that > the solution we now have seems to be somewhat lacking - maybe it should > have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the > ability to add additional modes later. Staas and I had quite some discussions about this topic, and I think I changed my mind at least once about his initial proposal, but I seem to recall that I suggested making the auto-negotiation property something more flexible than it currently is (or maybe Staas did, and my mind plays tricks on me). > > The other point I'm making above is that I'm forming the opinion that > the existing PHY layer isn't flexible enough for supporting SFP, and I > need some way to represent at least part of the autonegotiation at the > MAC level without involving the PHY level - especially when considering > that a real PHY might be inside the SFP cage which can be talked to > over I2C. This is a fair conclusion, the PHY library is really designed to support well 10/100/1000 PHYs, anything else, including 10Gbits PHY are not that well supported. Anything that is not MDIO is not that well supported, and the fixed PHY has its limitations. It is unclear to me how much of the PHY library: state machine, link management, device abstraction, and ethtool interface is going to be useful if you have a SFP instead of a MDIO-connected PHY. > > This is the problem I'm presently grappling with, and it's taking lots > of thought right now. I'm aware of other drivers in the kernel which > support SFP, each using their own implementations to support that. > > > Lastly, while looking at this, I've a small stack of patches for the PHY > code resolving some of the issues I've mentioned above, and fixing broken > reference counting and mdio bus module removal issues: > > phy: fixed-phy: properly validate phy in fixed_phy_update_state() > net: fix phy refcounting in a bunch of drivers > of_mdio: fix MDIO phy device refcounting > phy: add proper phy struct device refcounting > phy: fix mdiobus module safety > phy: fix of_mdio_find_bus() device refcount leak > > I hope to be able to send those out in the next few days - they have > nothing to do with SFP itself but are the results of looking through the > PHY code. >
On Thu, Sep 17, 2015 at 04:36:44PM -0700, Florian Fainelli wrote: > On 17/09/15 16:14, Russell King - ARM Linux wrote: > > [snip] > > > with _no_ phy node. > > > > 4. Going back to the SFP problem, the link is only up when the SFP > > module pins indicate that there's no transmitter fault, no loss of > > signal _and_ the PCS layer at the MAC indicates that it has completed > > negotiation. This pretty much rules out trying to emulate a SFP cage > > as a software-based PHY. I've code right now doing exactly that, and > > it results in netif_carrier_on() being called far too early. > > Andrew recently added support for having the fixed PHY emulated PHY poll > a GPIO to determine link status. If this information comes differently > (e.g: via MMIO/MAC registers) in your case, I guess we could either > extend the fixed PHY to support that scheme, or just open-code the > carrier state change in the mvneta driver. It's not that simple. The "is the carrier up" is the logical and of: - no TX fault (at SFP level) - no RX loss (at SFP level) - Serdes says link is up (at MAC level) If TX fault is asserted, then there is error recovery that can be done, so it's not as simple as the MAC monitoring that GPIO - unless we want to do what's already been done in other MAC drivers, and integrate the SFP driver logic into the MAC driver rather than doing the same thing and having it as a library. > This is a fair conclusion, the PHY library is really designed to support > well 10/100/1000 PHYs, anything else, including 10Gbits PHY are not that > well supported. Anything that is not MDIO is not that well supported, > and the fixed PHY has its limitations. > > It is unclear to me how much of the PHY library: state machine, link > management, device abstraction, and ethtool interface is going to be > useful if you have a SFP instead of a MDIO-connected PHY. Well, it gets even _more_ fun when you consider these, which are all a very real possibility thanks to the hardware designs out there: MAC ---SGMII---> Phy ---> Copper MAC ---1000baseX---> Phy ---> 1000baseT Copper MAC ---SGMII---> SFP ---> Phy ---> Copper MAC ---SGMII---> SFP ---> Phy ---> Optical MAC ---1000baseX---> SFP ---> Optical .---1000base-X---> SFP ---> Optical MAC ---SGMII---> Phy `---> Copper .---SGMII---> SFP ---> Phy ---> Optical MAC ---SGMII---> Phy `---> Copper .---1000base-X---> SFP ---> Phy ---> 1000baseT Copper MAC ---SGMII---> Phy `---> Copper .---SGMII---> SFP ---> Phy ---> Copper MAC ---SGMII---> Phy `---> Copper and when you realise that you can talk to all the PHYs in those trees. In the last four cases, the first level phy decides between the copper connection and the "other" connection based on programmable priority. At the moment, I only have two optical SFP modules, which fall into the 5th setup above - you can't use SGMII with "dumb" optical modules as SGMII's control word is an instruction from the PHY to the MAC about the link properties, it isn't any sort of negotiation. What I'm worried about is having to redo the DT properties, and if the change makes it into stable trees, then we have to support the existing design as well as whatever solution to this problem.
18.09.2015 02:14, Russell King - ARM Linux ?????: > 3. Having DT specify a fixed-link with parameters along with in-band > negotiation results in the fixed-link parameters being ignored. > This means if a fixed-link DT declaration specifies a speed, that > declaration will be ignored. What I'm basically saying is that: > > phy-mode = "sgmii"; > fixed-link { > speed = <1000>; > }; > > specifies a fixed-speed serdes link at 1000mbps, but: > > phy-mode = "sgmii"; > managed = "in-band-status"; > fixed-link { > speed = <1000>; > }; > > does not fix the speed at all. I think the fixed-link DT parsing should be moved from of_mdio.c to fixed-phy.c. Then fixed-phy will be able to back up the initial values and use them whenever needed. I can even do such a simple patch, but since it is not strictly speaking a regression from my changes, I'd rather prefer someone else do. :) > _But_ using the in-band status > property fundamentally requires this for mvneta to behave correctly: > > phy-mode = "sgmii"; > managed = "in-band-status"; > fixed-link { > }; > > with _no_ phy node. I don't understand this one. At least for me it works without empty fixed-link. There may be some bug. > > 4. Going back to the SFP problem, the link is only up when the SFP > module pins indicate that there's no transmitter fault, no loss of > signal _and_ the PCS layer at the MAC indicates that it has completed > negotiation. This pretty much rules out trying to emulate a SFP cage > as a software-based PHY. I've code right now doing exactly that, and > it results in netif_carrier_on() being called far too early. > > What I don't know is how many generations of the mvneta hardware have > support for both serdes modes, but what I'm basically saying is that > the solution we now have seems to be somewhat lacking - maybe it should > have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the > ability to add additional modes later. Well, you need to be quick with such a change, esp now when the patch was queued to -stable. What alternatives do we have here? Will the following work too? phy-mode = "1000base-x"; managed = "in-band-status";
On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: > 18.09.2015 02:14, Russell King - ARM Linux ?????: > > _But_ using the in-band status > > property fundamentally requires this for mvneta to behave correctly: > > > > phy-mode = "sgmii"; > > managed = "in-band-status"; > > fixed-link { > > }; > > > > with _no_ phy node. > I don't understand this one. > At least for me it works without empty fixed-link. > There may be some bug. if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); if (pp->use_inband_status && (cause_misc & (MVNETA_CAUSE_PHY_STATUS_CHANGE | MVNETA_CAUSE_LINK_CHANGE | MVNETA_CAUSE_PSC_SYNC_CHANGE))) { mvneta_fixed_link_update(pp, pp->phy_dev); } pp->use_inband_status is set when managed = "in-band-status" is set. We detect changes in the in-band status, and call mvneta_fixed_link_update(): mvneta_fixed_link_update() reads the status, and communicates that into the fixed-link phy: u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); ... code setting status.* values from gmac_stat ... changed.link = 1; changed.speed = 1; changed.duplex = 1; fixed_phy_update_state(phy, &status, &changed); fixed_phy_update_state() then looks up the phy in its list, comparing only the address: if (!phydev || !phydev->bus) return -EINVAL; list_for_each_entry(fp, &fmb->phys, node) { if (fp->addr == phydev->addr) { updating fp->* with the new state, calling fixed_phy_update_regs(). This updates the fixed-link phy emulated registers, and phylib then notices the change in link status, and notifies the netdevice attached to the PHY it found of the change. Now, one of two things happens as a result of this: 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link phy to update its "fixed-link" properties, and the "not so fixed" phy changes its parameters according to the new status. 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link phy, the fixed-link phy state is updated with the new parameters, and some other net device in the system changes link state - the settings are not communicated back to the mvneta instance which changed link state. 3. If pp->phy_dev is a MDIO phy but does not match a fixed-link phy, nothing happens and fixed_phy_update_state() returns -ENOENT. Again, the settings are not communicated back to the mvneta instance which changed link state. Now, a fixed-link phy is only created in mvneta when there is no MDIO phy specified, but when there is a fixed-link specification in DT: phy_node = of_parse_phandle(dn, "phy", 0); if (!phy_node) { if (!of_phy_is_fixed_link(dn)) { dev_err(&pdev->dev, "no PHY specified\n"); err = -ENODEV; goto err_free_irq; } err = of_phy_register_fixed_link(dn); if (err < 0) { dev_err(&pdev->dev, "cannot register fixed PHY\n"); goto err_free_irq; } If there's neither a MDIO PHY nor a fixed-link, then the network driver fails to initialise the device. > > What I don't know is how many generations of the mvneta hardware have > > support for both serdes modes, but what I'm basically saying is that > > the solution we now have seems to be somewhat lacking - maybe it should > > have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the > > ability to add additional modes later. > > Well, you need to be quick with such a change, esp now when the patch > was queued to -stable. > What alternatives do we have here? > Will the following work too? > phy-mode = "1000base-x"; > managed = "in-band-status"; There's no chance of being "quick" here - the issues are complex and not trivial to solve in a day - I've already spent all week thinking about the issues here, and I'm only _just_ starting to come up with a potential solution this morning, though I haven't yet assessed whether it'll be feasible. The problem I have with the above is that it fixes the phy mode to either SGMII or 1000base-X, whereas what we need for the SFP case is to have the down-link object tell the MAC driver whether it wants SGMII or 1000base-X mode.
18.09.2015 15:13, Russell King - ARM Linux ?????: > On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: >> 18.09.2015 02:14, Russell King - ARM Linux ?????: >>> _But_ using the in-band status >>> property fundamentally requires this for mvneta to behave correctly: >>> >>> phy-mode = "sgmii"; >>> managed = "in-band-status"; >>> fixed-link { >>> }; >>> >>> with _no_ phy node. >> I don't understand this one. >> At least for me it works without empty fixed-link. >> There may be some bug. > > if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { > u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); > > mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); > if (pp->use_inband_status && (cause_misc & > (MVNETA_CAUSE_PHY_STATUS_CHANGE | > MVNETA_CAUSE_LINK_CHANGE | > MVNETA_CAUSE_PSC_SYNC_CHANGE))) { > mvneta_fixed_link_update(pp, pp->phy_dev); > } > > pp->use_inband_status is set when managed = "in-band-status" is set. > We detect changes in the in-band status, and call mvneta_fixed_link_update(): > > mvneta_fixed_link_update() reads the status, and communicates that into > the fixed-link phy: > > u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); > > ... code setting status.* values from gmac_stat ... > changed.link = 1; > changed.speed = 1; > changed.duplex = 1; > fixed_phy_update_state(phy, &status, &changed); > > fixed_phy_update_state() then looks up the phy in its list, comparing only > the address: > > if (!phydev || !phydev->bus) > return -EINVAL; > > list_for_each_entry(fp, &fmb->phys, node) { > if (fp->addr == phydev->addr) { > > updating fp->* with the new state, calling fixed_phy_update_regs(). This > updates the fixed-link phy emulated registers, and phylib then notices > the change in link status, and notifies the netdevice attached to the > PHY it found of the change. > > Now, one of two things happens as a result of this: > > 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link > phy to update its "fixed-link" properties, and the "not so fixed" phy > changes its parameters according to the new status. > > 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link > phy, Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? I don't think MDIO PHYs can appear there. If they can - the bug is very nasty. Have you seen exactly when/why that happens? > Now, a fixed-link phy is only created in mvneta when there is no MDIO phy > specified, but when there is a fixed-link specification in DT: > > phy_node = of_parse_phandle(dn, "phy", 0); > if (!phy_node) { > if (!of_phy_is_fixed_link(dn)) { > dev_err(&pdev->dev, "no PHY specified\n"); > err = -ENODEV; > goto err_free_irq; > } > > err = of_phy_register_fixed_link(dn); > if (err < 0) { > dev_err(&pdev->dev, "cannot register fixed PHY\n"); > goto err_free_irq; > } > > If there's neither a MDIO PHY nor a fixed-link, then the network driver > fails to initialise the device. But it does. In fact, of_mdio.c has this code: err = of_property_read_string(np, "managed", &managed); if (err == 0) { if (strcmp(managed, "in-band-status") == 0) { /* status is zeroed, namely its .link member */ phy = fixed_phy_register(PHY_POLL, &status, np); return IS_ERR(phy) ? PTR_ERR(phy) : 0; } } Which is exactly for the case you describe. >>> What I don't know is how many generations of the mvneta hardware have >>> support for both serdes modes, but what I'm basically saying is that >>> the solution we now have seems to be somewhat lacking - maybe it should >>> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the >>> ability to add additional modes later. >> >> Well, you need to be quick with such a change, esp now when the patch >> was queued to -stable. >> What alternatives do we have here? >> Will the following work too? >> phy-mode = "1000base-x"; >> managed = "in-band-status"; > > There's no chance of being "quick" here - the issues are complex and not > trivial to solve in a day - I've already spent all week thinking about > the issues here, and I'm only _just_ starting to come up with a potential > solution this morning, though I haven't yet assessed whether it'll be > feasible. > > The problem I have with the above is that it fixes the phy mode to either > SGMII or 1000base-X, whereas what we need for the SFP case is to have the > down-link object tell the MAC driver whether it wants SGMII or 1000base-X > mode. Not that I understand that SFP business at all. Maybe if some downlink tells the MAC what autoneg protocol will be used, you can have: phy-mode = "1000base-x" | "sgmii" | "serdes-auto"; managed = "in-band-status"; and "serdes-auto" will use either "1000base-x" or "sgmii", depending on what the downlink says?
On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: > 18.09.2015 15:13, Russell King - ARM Linux ?????: > > On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: > >> 18.09.2015 02:14, Russell King - ARM Linux ?????: > >>> _But_ using the in-band status > >>> property fundamentally requires this for mvneta to behave correctly: > >>> > >>> phy-mode = "sgmii"; > >>> managed = "in-band-status"; > >>> fixed-link { > >>> }; > >>> > >>> with _no_ phy node. > >> I don't understand this one. > >> At least for me it works without empty fixed-link. > >> There may be some bug. > > > > if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { > > u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); > > > > mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); > > if (pp->use_inband_status && (cause_misc & > > (MVNETA_CAUSE_PHY_STATUS_CHANGE | > > MVNETA_CAUSE_LINK_CHANGE | > > MVNETA_CAUSE_PSC_SYNC_CHANGE))) { > > mvneta_fixed_link_update(pp, pp->phy_dev); > > } > > > > pp->use_inband_status is set when managed = "in-band-status" is set. > > We detect changes in the in-band status, and call mvneta_fixed_link_update(): > > > > mvneta_fixed_link_update() reads the status, and communicates that into > > the fixed-link phy: > > > > u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); > > > > ... code setting status.* values from gmac_stat ... > > changed.link = 1; > > changed.speed = 1; > > changed.duplex = 1; > > fixed_phy_update_state(phy, &status, &changed); > > > > fixed_phy_update_state() then looks up the phy in its list, comparing only > > the address: > > > > if (!phydev || !phydev->bus) > > return -EINVAL; > > > > list_for_each_entry(fp, &fmb->phys, node) { > > if (fp->addr == phydev->addr) { > > > > updating fp->* with the new state, calling fixed_phy_update_regs(). This > > updates the fixed-link phy emulated registers, and phylib then notices > > the change in link status, and notifies the netdevice attached to the > > PHY it found of the change. > > > > Now, one of two things happens as a result of this: > > > > 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link > > phy to update its "fixed-link" properties, and the "not so fixed" phy > > changes its parameters according to the new status. > > > > 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link > > phy, > Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? > I don't think MDIO PHYs can appear there. If they can - the bug is > very nasty. Have you seen exactly when/why that happens? I think I explained it fully - please follow the code paths I've detailed above. Specifically, look at this code: if (!phydev || !phydev->bus) return -EINVAL; list_for_each_entry(fp, &fmb->phys, node) { if (fp->addr == phydev->addr) { Consider what the effect is if you have a MDIO phy at address 0 on eth0 which has in-band-status enabled. Now if you have a fixed-link phy on the fixed-link bus at address 0 connected to eth1. Bring eth1 up, everything works as one would expect, it gains the fixed link settings. Now bring eth0 up. Because there's no distinction in mvneta between a fixed-link phy and a MDIO phy, it ends up calling fixed_phy_update_state() with the MDIO phy, which has phydev->addr = 0. The fixed-link code scans every phy on the fixed-link bus, looking for one with address 0, and it finds that, and modifies the state of that phy. eth1 now gains the settings that eth0 communicated into the fixed-link phy layer. > > Now, a fixed-link phy is only created in mvneta when there is no MDIO phy > > specified, but when there is a fixed-link specification in DT: > > > > phy_node = of_parse_phandle(dn, "phy", 0); > > if (!phy_node) { > > if (!of_phy_is_fixed_link(dn)) { > > dev_err(&pdev->dev, "no PHY specified\n"); > > err = -ENODEV; > > goto err_free_irq; > > } > > > > err = of_phy_register_fixed_link(dn); > > if (err < 0) { > > dev_err(&pdev->dev, "cannot register fixed PHY\n"); > > goto err_free_irq; > > } > > > > If there's neither a MDIO PHY nor a fixed-link, then the network driver > > fails to initialise the device. > But it does. Please, look again at the code I quoted above. > In fact, of_mdio.c has this code: > > err = of_property_read_string(np, "managed", &managed); > if (err == 0) { > if (strcmp(managed, "in-band-status") == 0) { > /* status is zeroed, namely its .link member */ > phy = fixed_phy_register(PHY_POLL, &status, np); > return IS_ERR(phy) ? PTR_ERR(phy) : 0; > } > } > > Which is exactly for the case you describe. That code is in of_phy_register_fixed_link(). That code will _NOT_ be reached if a MDIO phy is specified. Again, please read the code. > >>> What I don't know is how many generations of the mvneta hardware have > >>> support for both serdes modes, but what I'm basically saying is that > >>> the solution we now have seems to be somewhat lacking - maybe it should > >>> have been "auto", "in-band-sgmii" and "in-band-1000base-x" with the > >>> ability to add additional modes later. > >> > >> Well, you need to be quick with such a change, esp now when the patch > >> was queued to -stable. > >> What alternatives do we have here? > >> Will the following work too? > >> phy-mode = "1000base-x"; > >> managed = "in-band-status"; > > > > There's no chance of being "quick" here - the issues are complex and not > > trivial to solve in a day - I've already spent all week thinking about > > the issues here, and I'm only _just_ starting to come up with a potential > > solution this morning, though I haven't yet assessed whether it'll be > > feasible. > > > > The problem I have with the above is that it fixes the phy mode to either > > SGMII or 1000base-X, whereas what we need for the SFP case is to have the > > down-link object tell the MAC driver whether it wants SGMII or 1000base-X > > mode. > Not that I understand that SFP business at all. > Maybe if some downlink tells the MAC what autoneg protocol will > be used, you can have: > phy-mode = "1000base-x" | "sgmii" | "serdes-auto"; > managed = "in-band-status"; > > and "serdes-auto" will use either "1000base-x" or "sgmii", depending > on what the downlink says? Maybe, but rather than guessing and getting it wrong, let's wait until we know what kind of a solution is necessary here. Rushing this will only create another design mistake and an even larger can of worms.
18.09.2015 16:12, Russell King - ARM Linux ?????: > On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: >> 18.09.2015 15:13, Russell King - ARM Linux ?????: >>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: >>>> 18.09.2015 02:14, Russell King - ARM Linux ?????: >>>>> _But_ using the in-band status >>>>> property fundamentally requires this for mvneta to behave correctly: >>>>> >>>>> phy-mode = "sgmii"; >>>>> managed = "in-band-status"; >>>>> fixed-link { >>>>> }; >>>>> >>>>> with _no_ phy node. >>>> I don't understand this one. >>>> At least for me it works without empty fixed-link. >>>> There may be some bug. >>> >>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { >>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); >>> >>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); >>> if (pp->use_inband_status && (cause_misc & >>> (MVNETA_CAUSE_PHY_STATUS_CHANGE | >>> MVNETA_CAUSE_LINK_CHANGE | >>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) { >>> mvneta_fixed_link_update(pp, pp->phy_dev); >>> } >>> >>> pp->use_inband_status is set when managed = "in-band-status" is set. >>> We detect changes in the in-band status, and call mvneta_fixed_link_update(): >>> >>> mvneta_fixed_link_update() reads the status, and communicates that into >>> the fixed-link phy: >>> >>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); >>> >>> ... code setting status.* values from gmac_stat ... >>> changed.link = 1; >>> changed.speed = 1; >>> changed.duplex = 1; >>> fixed_phy_update_state(phy, &status, &changed); >>> >>> fixed_phy_update_state() then looks up the phy in its list, comparing only >>> the address: >>> >>> if (!phydev || !phydev->bus) >>> return -EINVAL; >>> >>> list_for_each_entry(fp, &fmb->phys, node) { >>> if (fp->addr == phydev->addr) { >>> >>> updating fp->* with the new state, calling fixed_phy_update_regs(). This >>> updates the fixed-link phy emulated registers, and phylib then notices >>> the change in link status, and notifies the netdevice attached to the >>> PHY it found of the change. >>> >>> Now, one of two things happens as a result of this: >>> >>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link >>> phy to update its "fixed-link" properties, and the "not so fixed" phy >>> changes its parameters according to the new status. >>> >>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link >>> phy, >> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? >> I don't think MDIO PHYs can appear there. If they can - the bug is >> very nasty. Have you seen exactly when/why that happens? > > I think I explained it fully - please follow the code paths I've detailed > above. I try. What I don't understand is why both PHYs get the same address on the "Fixed MDIO bus". > > Specifically, look at this code: > > if (!phydev || !phydev->bus) > return -EINVAL; > > list_for_each_entry(fp, &fmb->phys, node) { > if (fp->addr == phydev->addr) { > > Consider what the effect is if you have a MDIO phy at address 0 on eth0 > which has in-band-status enabled. So as I understand, you have MDIO phy with DT looking like this: ethernet@70000 { status = "okay"; phy-mode = "sgmii"; managed = "in-band-status"; } W/O either "phy" of "fixed-link" nodes. Correct? mvneta calls of_phy_register_fixed_link(dn) on it after not finding the "phy" node. And it will do the same with the second non-MDIO phy. What I don't see is how do they get the same addr on the same bus, could you please clarify that a bit? >>> Now, a fixed-link phy is only created in mvneta when there is no MDIO phy >>> specified, but when there is a fixed-link specification in DT: >>> >>> phy_node = of_parse_phandle(dn, "phy", 0); >>> if (!phy_node) { >>> if (!of_phy_is_fixed_link(dn)) { >>> dev_err(&pdev->dev, "no PHY specified\n"); >>> err = -ENODEV; >>> goto err_free_irq; >>> } >>> >>> err = of_phy_register_fixed_link(dn); >>> if (err < 0) { >>> dev_err(&pdev->dev, "cannot register fixed PHY\n"); >>> goto err_free_irq; >>> } >>> >>> If there's neither a MDIO PHY nor a fixed-link, then the network driver >>> fails to initialise the device. >> But it does. > > Please, look again at the code I quoted above. > >> In fact, of_mdio.c has this code: >> >> err = of_property_read_string(np, "managed", &managed); >> if (err == 0) { >> if (strcmp(managed, "in-band-status") == 0) { >> /* status is zeroed, namely its .link member */ >> phy = fixed_phy_register(PHY_POLL, &status, np); >> return IS_ERR(phy) ? PTR_ERR(phy) : 0; >> } >> } >> >> Which is exactly for the case you describe. > > That code is in of_phy_register_fixed_link(). That code will _NOT_ be > reached if a MDIO phy is specified. Again, please read the code. I think the DT quote is missing here for me to understand. This code is for managed = "in-band-status", which is what I thought you are talking about. Could you please quote the DT that creates 2 PHYs with the same addr on fmb? I'll try it here and from that will understand. >> Maybe if some downlink tells the MAC what autoneg protocol will >> be used, you can have: >> phy-mode = "1000base-x" | "sgmii" | "serdes-auto"; >> managed = "in-band-status"; >> >> and "serdes-auto" will use either "1000base-x" or "sgmii", depending >> on what the downlink says? > > Maybe, but rather than guessing and getting it wrong, let's wait until > we know what kind of a solution is necessary here. Rushing this will > only create another design mistake and an even larger can of worms. That was just an idea of how to get it without changing the current values of the "managed" property.
On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote: > 18.09.2015 16:12, Russell King - ARM Linux ?????: > > On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: > >> 18.09.2015 15:13, Russell King - ARM Linux ?????: > >>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: > >>>> 18.09.2015 02:14, Russell King - ARM Linux ?????: > >>>>> _But_ using the in-band status > >>>>> property fundamentally requires this for mvneta to behave correctly: > >>>>> > >>>>> phy-mode = "sgmii"; > >>>>> managed = "in-band-status"; > >>>>> fixed-link { > >>>>> }; > >>>>> > >>>>> with _no_ phy node. > >>>> I don't understand this one. > >>>> At least for me it works without empty fixed-link. > >>>> There may be some bug. > >>> > >>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { > >>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); > >>> > >>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); > >>> if (pp->use_inband_status && (cause_misc & > >>> (MVNETA_CAUSE_PHY_STATUS_CHANGE | > >>> MVNETA_CAUSE_LINK_CHANGE | > >>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) { > >>> mvneta_fixed_link_update(pp, pp->phy_dev); > >>> } > >>> > >>> pp->use_inband_status is set when managed = "in-band-status" is set. > >>> We detect changes in the in-band status, and call mvneta_fixed_link_update(): > >>> > >>> mvneta_fixed_link_update() reads the status, and communicates that into > >>> the fixed-link phy: > >>> > >>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); > >>> > >>> ... code setting status.* values from gmac_stat ... > >>> changed.link = 1; > >>> changed.speed = 1; > >>> changed.duplex = 1; > >>> fixed_phy_update_state(phy, &status, &changed); > >>> > >>> fixed_phy_update_state() then looks up the phy in its list, comparing only > >>> the address: > >>> > >>> if (!phydev || !phydev->bus) > >>> return -EINVAL; > >>> > >>> list_for_each_entry(fp, &fmb->phys, node) { > >>> if (fp->addr == phydev->addr) { > >>> > >>> updating fp->* with the new state, calling fixed_phy_update_regs(). This > >>> updates the fixed-link phy emulated registers, and phylib then notices > >>> the change in link status, and notifies the netdevice attached to the > >>> PHY it found of the change. > >>> > >>> Now, one of two things happens as a result of this: > >>> > >>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link > >>> phy to update its "fixed-link" properties, and the "not so fixed" phy > >>> changes its parameters according to the new status. > >>> > >>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link > >>> phy, > >> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? > >> I don't think MDIO PHYs can appear there. If they can - the bug is > >> very nasty. Have you seen exactly when/why that happens? > > > > I think I explained it fully - please follow the code paths I've detailed > > above. > I try. What I don't understand is why both PHYs get the > same address on the "Fixed MDIO bus". They aren't both on the fixed MDIO bus - that's the whole point I'm making. They get the same phydev->addr but on _different_ buses. > > Specifically, look at this code: > > > > if (!phydev || !phydev->bus) > > return -EINVAL; > > > > list_for_each_entry(fp, &fmb->phys, node) { > > if (fp->addr == phydev->addr) { Look at this closely - at what point is there any validation that "phydev" is actually a fixed-link phy? There is no validation done. The only criteria there are: - phydev is not NULL - phydev->bus is not NULL (which is true of any registered phy) - phydev->addr matches _any_ fixed-link phy. I've already sent a patch earlier today to address this issue. If you place a WARN_ON(fp->phydev != phydev) then that'll show you when it incorrectly matches. > > Consider what the effect is if you have a MDIO phy at address 0 on eth0 > > which has in-band-status enabled. > So as I understand, you have MDIO phy with DT looking like this: > ethernet@70000 { > status = "okay"; > phy-mode = "sgmii"; > managed = "in-band-status"; > } > W/O either "phy" of "fixed-link" nodes. Correct? > mvneta calls of_phy_register_fixed_link(dn) on it after not > finding the "phy" node. And it will do the same with the second > non-MDIO phy. What I don't see is how do they get the same addr > on the same bus, could you please clarify that a bit? mdio@72004 { phy_dedicated: ethernet-phy@0 { reg = <0>; }; }; eth1: ethernet@30000 { phy = <&phy_dedicated>; phy-mode = "sgmii"; managed = "in-band-status"; }; eth0: ethernet@70000 { phy-mode = "sgmii"; fixed-link { speed = 1000; full-duplex; }; }; Bring eth0 up first, everything works. Then, bring eth1 up, and eth0 goes down. This happens because when eth1 is brought up, eth1's mvneta calls into fixed_phy_update_state() with a pointer to the "phy_dedicated" PHY at address 0. fixed_phy_update_state() scans the fixed-link phys for one at address 0, and finds the fixed-link phy associated with eth0. This causes the fixed link code to change the settings for eth0. As I have already shown in my previous setup diagrams, it is _entirely_ reasonable to use in-band status with SGMII with a phy attached.
18.09.2015 16:57, Russell King - ARM Linux ?????: > On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote: >> 18.09.2015 16:12, Russell King - ARM Linux ?????: >>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: >>>> 18.09.2015 15:13, Russell King - ARM Linux ?????: >>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: >>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????: >>>>>>> _But_ using the in-band status >>>>>>> property fundamentally requires this for mvneta to behave correctly: >>>>>>> >>>>>>> phy-mode = "sgmii"; >>>>>>> managed = "in-band-status"; >>>>>>> fixed-link { >>>>>>> }; >>>>>>> >>>>>>> with _no_ phy node. >>>>>> I don't understand this one. >>>>>> At least for me it works without empty fixed-link. >>>>>> There may be some bug. >>>>> >>>>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { >>>>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); >>>>> >>>>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); >>>>> if (pp->use_inband_status && (cause_misc & >>>>> (MVNETA_CAUSE_PHY_STATUS_CHANGE | >>>>> MVNETA_CAUSE_LINK_CHANGE | >>>>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) { >>>>> mvneta_fixed_link_update(pp, pp->phy_dev); >>>>> } >>>>> >>>>> pp->use_inband_status is set when managed = "in-band-status" is set. >>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update(): >>>>> >>>>> mvneta_fixed_link_update() reads the status, and communicates that into >>>>> the fixed-link phy: >>>>> >>>>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); >>>>> >>>>> ... code setting status.* values from gmac_stat ... >>>>> changed.link = 1; >>>>> changed.speed = 1; >>>>> changed.duplex = 1; >>>>> fixed_phy_update_state(phy, &status, &changed); >>>>> >>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only >>>>> the address: >>>>> >>>>> if (!phydev || !phydev->bus) >>>>> return -EINVAL; >>>>> >>>>> list_for_each_entry(fp, &fmb->phys, node) { >>>>> if (fp->addr == phydev->addr) { >>>>> >>>>> updating fp->* with the new state, calling fixed_phy_update_regs(). This >>>>> updates the fixed-link phy emulated registers, and phylib then notices >>>>> the change in link status, and notifies the netdevice attached to the >>>>> PHY it found of the change. >>>>> >>>>> Now, one of two things happens as a result of this: >>>>> >>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link >>>>> phy to update its "fixed-link" properties, and the "not so fixed" phy >>>>> changes its parameters according to the new status. >>>>> >>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link >>>>> phy, >>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? >>>> I don't think MDIO PHYs can appear there. If they can - the bug is >>>> very nasty. Have you seen exactly when/why that happens? >>> >>> I think I explained it fully - please follow the code paths I've detailed >>> above. >> I try. What I don't understand is why both PHYs get the >> same address on the "Fixed MDIO bus". > > They aren't both on the fixed MDIO bus - that's the whole point I'm > making. They get the same phydev->addr but on _different_ buses. So you have an MDIO phy for which mvneta seems to have use_inband_status==true, correct? AFAICS if it has use_inband_status==true, then it went through of_phy_register_fixed_link(dn), which, in case of managed="in-band-status", was created by fixed_phy_register(), in which case it should be on the "fixed MDIO bus". So if they are on a _different_ buses, then I would expect for the MDIO phy to have use_inband_status==false. You described the update status path very precisely in your prev mail, but this doesn't help because what I don't understand is the particular _setup_ path that leads to use_inband_status==true yet the phy is not on the fmb. >>> Specifically, look at this code: >>> >>> if (!phydev || !phydev->bus) >>> return -EINVAL; >>> >>> list_for_each_entry(fp, &fmb->phys, node) { >>> if (fp->addr == phydev->addr) { > > Look at this closely - at what point is there any validation that "phydev" > is actually a fixed-link phy? There is no validation done. The only > criteria there are: > - phydev is not NULL > - phydev->bus is not NULL (which is true of any registered phy) > - phydev->addr matches _any_ fixed-link phy. > > I've already sent a patch earlier today to address this issue. > > If you place a WARN_ON(fp->phydev != phydev) then that'll show you > when it incorrectly matches. Yes, I realize if the MDIO phy is passed there, then all you say will happen. That's pretty trivial and doesn't need any explanations. But how was that phydev created, not with fixed_phy_register() I suppose? Then it should have use_inband_status==false, right? >>> Consider what the effect is if you have a MDIO phy at address 0 on eth0 >>> which has in-band-status enabled. >> So as I understand, you have MDIO phy with DT looking like this: >> ethernet@70000 { >> status = "okay"; >> phy-mode = "sgmii"; >> managed = "in-band-status"; >> } >> W/O either "phy" of "fixed-link" nodes. Correct? >> mvneta calls of_phy_register_fixed_link(dn) on it after not >> finding the "phy" node. And it will do the same with the second >> non-MDIO phy. What I don't see is how do they get the same addr >> on the same bus, could you please clarify that a bit? > > mdio@72004 { > phy_dedicated: ethernet-phy@0 { > reg = <0>; > }; > }; > > eth1: ethernet@30000 { > phy = <&phy_dedicated>; > phy-mode = "sgmii"; > managed = "in-band-status"; > }; > > eth0: ethernet@70000 { > phy-mode = "sgmii"; > fixed-link { > speed = 1000; > full-duplex; > }; > }; > > Bring eth0 up first, everything works. Then, bring eth1 up, and eth0 > goes down. I don't have an MDIO phy here, but I'll make an attempt to replicate this setup. > This happens because when eth1 is brought up, eth1's mvneta calls into > fixed_phy_update_state() with a pointer to the "phy_dedicated" PHY at > address 0. fixed_phy_update_state() scans the fixed-link phys for one > at address 0, and finds the fixed-link phy associated with eth0. Unfortunately this is not what needs to be explained. I am confused about the setup path, not update path.
On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote: > 18.09.2015 16:57, Russell King - ARM Linux ?????: > > On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote: > >> 18.09.2015 16:12, Russell King - ARM Linux ?????: > >>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: > >>>> 18.09.2015 15:13, Russell King - ARM Linux ?????: > >>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: > >>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????: > >>>>>>> _But_ using the in-band status > >>>>>>> property fundamentally requires this for mvneta to behave correctly: > >>>>>>> > >>>>>>> phy-mode = "sgmii"; > >>>>>>> managed = "in-band-status"; > >>>>>>> fixed-link { > >>>>>>> }; > >>>>>>> > >>>>>>> with _no_ phy node. > >>>>>> I don't understand this one. > >>>>>> At least for me it works without empty fixed-link. > >>>>>> There may be some bug. > >>>>> > >>>>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { > >>>>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); > >>>>> > >>>>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); > >>>>> if (pp->use_inband_status && (cause_misc & > >>>>> (MVNETA_CAUSE_PHY_STATUS_CHANGE | > >>>>> MVNETA_CAUSE_LINK_CHANGE | > >>>>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) { > >>>>> mvneta_fixed_link_update(pp, pp->phy_dev); > >>>>> } > >>>>> > >>>>> pp->use_inband_status is set when managed = "in-band-status" is set. > >>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update(): > >>>>> > >>>>> mvneta_fixed_link_update() reads the status, and communicates that into > >>>>> the fixed-link phy: > >>>>> > >>>>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); > >>>>> > >>>>> ... code setting status.* values from gmac_stat ... > >>>>> changed.link = 1; > >>>>> changed.speed = 1; > >>>>> changed.duplex = 1; > >>>>> fixed_phy_update_state(phy, &status, &changed); > >>>>> > >>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only > >>>>> the address: > >>>>> > >>>>> if (!phydev || !phydev->bus) > >>>>> return -EINVAL; > >>>>> > >>>>> list_for_each_entry(fp, &fmb->phys, node) { > >>>>> if (fp->addr == phydev->addr) { > >>>>> > >>>>> updating fp->* with the new state, calling fixed_phy_update_regs(). This > >>>>> updates the fixed-link phy emulated registers, and phylib then notices > >>>>> the change in link status, and notifies the netdevice attached to the > >>>>> PHY it found of the change. > >>>>> > >>>>> Now, one of two things happens as a result of this: > >>>>> > >>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link > >>>>> phy to update its "fixed-link" properties, and the "not so fixed" phy > >>>>> changes its parameters according to the new status. > >>>>> > >>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link > >>>>> phy, > >>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? > >>>> I don't think MDIO PHYs can appear there. If they can - the bug is > >>>> very nasty. Have you seen exactly when/why that happens? > >>> > >>> I think I explained it fully - please follow the code paths I've detailed > >>> above. > >> I try. What I don't understand is why both PHYs get the > >> same address on the "Fixed MDIO bus". > > > > They aren't both on the fixed MDIO bus - that's the whole point I'm > > making. They get the same phydev->addr but on _different_ buses. > > So you have an MDIO phy for which mvneta seems to have > use_inband_status==true, correct? That is one very real possibililty. Cisco SGMII in-band status is for a SGMII PHY to inform the MAC about the properties of the link to which the PHY is attached. So, specifying "managed = in-band-status" along with a real PHY is very much a _real_ possibility, as I've previously explained. > AFAICS if it has use_inband_status==true, > then it went through of_phy_register_fixed_link(dn), That's totally incorrect. The test for setting use_inband_status in mvneta is: err = of_property_read_string(dn, "managed", &managed); pp->use_inband_status = (err == 0 && strcmp(managed, "in-band-status") == 0); So, use_inband_status can be set _whatever_. It doesn't matter if there's a fixed-phy being used, or whether a real MDIO phy has been specified. The _actual_ situation I had when I encountered the problem was: eth0 - connected to a DSA switch eth1 - connected to SFP "phy" with in-band-status enabled (for 1000base-X) where this phy is sitting on its own virtual MDIO bus. eth2 - connected to a RGMII phy. At boot, eth2 is brought up by DHCP, and eth0 is configured up as part of the switch initialisation. eth0 comes up fine. Then, I manually brought up eth1, very unexpectedly eth0 immediately went down - and this was completely repeatable, caused by this problem. > You described the update status path very precisely in your prev mail, > but this doesn't help because what I don't understand is the particular > _setup_ path that leads to use_inband_status==true yet the phy is not > on the fmb. I think I covered that in the email to which you're replying, where I show you that I have a "phy=" node within the ethernet definition. That means: phy_node = of_parse_phandle(dn, "phy", 0); will return non-NULL, and that means: if (!phy_node) { if (!of_phy_is_fixed_link(dn)) { dev_err(&pdev->dev, "no PHY specified\n"); err = -ENODEV; goto err_free_irq; } err = of_phy_register_fixed_link(dn); the code here is never reached. I'm failing to see what the problem is - the code you keep referring me to won't be called in the situation I'm describing.
18.09.2015 18:43, Russell King - ARM Linux ?????: > On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote: >> 18.09.2015 16:57, Russell King - ARM Linux ?????: >>> On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote: >>>> 18.09.2015 16:12, Russell King - ARM Linux ?????: >>>>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote: >>>>>> 18.09.2015 15:13, Russell King - ARM Linux ?????: >>>>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote: >>>>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????: >>>>>>>>> _But_ using the in-band status >>>>>>>>> property fundamentally requires this for mvneta to behave correctly: >>>>>>>>> >>>>>>>>> phy-mode = "sgmii"; >>>>>>>>> managed = "in-band-status"; >>>>>>>>> fixed-link { >>>>>>>>> }; >>>>>>>>> >>>>>>>>> with _no_ phy node. >>>>>>>> I don't understand this one. >>>>>>>> At least for me it works without empty fixed-link. >>>>>>>> There may be some bug. >>>>>>> >>>>>>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) { >>>>>>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE); >>>>>>> >>>>>>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0); >>>>>>> if (pp->use_inband_status && (cause_misc & >>>>>>> (MVNETA_CAUSE_PHY_STATUS_CHANGE | >>>>>>> MVNETA_CAUSE_LINK_CHANGE | >>>>>>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) { >>>>>>> mvneta_fixed_link_update(pp, pp->phy_dev); >>>>>>> } >>>>>>> >>>>>>> pp->use_inband_status is set when managed = "in-band-status" is set. >>>>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update(): >>>>>>> >>>>>>> mvneta_fixed_link_update() reads the status, and communicates that into >>>>>>> the fixed-link phy: >>>>>>> >>>>>>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS); >>>>>>> >>>>>>> ... code setting status.* values from gmac_stat ... >>>>>>> changed.link = 1; >>>>>>> changed.speed = 1; >>>>>>> changed.duplex = 1; >>>>>>> fixed_phy_update_state(phy, &status, &changed); >>>>>>> >>>>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only >>>>>>> the address: >>>>>>> >>>>>>> if (!phydev || !phydev->bus) >>>>>>> return -EINVAL; >>>>>>> >>>>>>> list_for_each_entry(fp, &fmb->phys, node) { >>>>>>> if (fp->addr == phydev->addr) { >>>>>>> >>>>>>> updating fp->* with the new state, calling fixed_phy_update_regs(). This >>>>>>> updates the fixed-link phy emulated registers, and phylib then notices >>>>>>> the change in link status, and notifies the netdevice attached to the >>>>>>> PHY it found of the change. >>>>>>> >>>>>>> Now, one of two things happens as a result of this: >>>>>>> >>>>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link >>>>>>> phy to update its "fixed-link" properties, and the "not so fixed" phy >>>>>>> changes its parameters according to the new status. >>>>>>> >>>>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link >>>>>>> phy, >>>>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"? >>>>>> I don't think MDIO PHYs can appear there. If they can - the bug is >>>>>> very nasty. Have you seen exactly when/why that happens? >>>>> >>>>> I think I explained it fully - please follow the code paths I've detailed >>>>> above. >>>> I try. What I don't understand is why both PHYs get the >>>> same address on the "Fixed MDIO bus". >>> >>> They aren't both on the fixed MDIO bus - that's the whole point I'm >>> making. They get the same phydev->addr but on _different_ buses. >> >> So you have an MDIO phy for which mvneta seems to have >> use_inband_status==true, correct? > > That is one very real possibililty. Cisco SGMII in-band status is for a > SGMII PHY to inform the MAC about the properties of the link to which the > PHY is attached. > > So, specifying "managed = in-band-status" along with a real PHY is very > much a _real_ possibility, as I've previously explained. > >> AFAICS if it has use_inband_status==true, >> then it went through of_phy_register_fixed_link(dn), > > That's totally incorrect. The test for setting use_inband_status in > mvneta is: > > err = of_property_read_string(dn, "managed", &managed); > pp->use_inband_status = (err == 0 && > strcmp(managed, "in-band-status") == 0); Arrrr! I was looking at the branch without the last patch applied, so it occurred to me as pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && fixed_phy; Sorry for that. So we seem to indeed have a nasty regression with the patch that just went to stable. :( Great news. Thanks for you time. I still have problems with this part though: > If there's neither a MDIO PHY nor a fixed-link, then the network driver > fails to initialise the device. I think I am looking into the right source this time, seems like if we don't have both but still have managed="in-band-status", that should go the fixed-link path and still work... no?
On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote: > 18.09.2015 18:43, Russell King - ARM Linux ?????: > > On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote: > >> AFAICS if it has use_inband_status==true, > >> then it went through of_phy_register_fixed_link(dn), > > > > That's totally incorrect. The test for setting use_inband_status in > > mvneta is: > > > > err = of_property_read_string(dn, "managed", &managed); > > pp->use_inband_status = (err == 0 && > > strcmp(managed, "in-band-status") == 0); > > Arrrr! I was looking at the branch without the last > patch applied, so it occurred to me as > > pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && > fixed_phy; > > Sorry for that. Yay :) > So we seem to indeed have a nasty regression with the patch > that just went to stable. :( Great news. Yes. > Thanks for you time. > > I still have problems with this part though: > > If there's neither a MDIO PHY nor a fixed-link, then the network driver > > fails to initialise the device. > > I think I am looking into the right source this time, seems like > if we don't have both but still have managed="in-band-status", that > should go the fixed-link path and still work... no? If we have no fixed-link and no phy, then you're correct. However, I really don't like the idea of abusing "fixed-link" as a method to generate an ethtool/miitool/miidiag compatible output for this, but I'm willing to let that pass for the moment. :)
On 18/09/15 10:22, Russell King - ARM Linux wrote: > On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote: >> 18.09.2015 18:43, Russell King - ARM Linux ?????: >>> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote: >>>> AFAICS if it has use_inband_status==true, >>>> then it went through of_phy_register_fixed_link(dn), >>> >>> That's totally incorrect. The test for setting use_inband_status in >>> mvneta is: >>> >>> err = of_property_read_string(dn, "managed", &managed); >>> pp->use_inband_status = (err == 0 && >>> strcmp(managed, "in-band-status") == 0); >> >> Arrrr! I was looking at the branch without the last >> patch applied, so it occurred to me as >> >> pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && >> fixed_phy; >> >> Sorry for that. > > Yay :) > >> So we seem to indeed have a nasty regression with the patch >> that just went to stable. :( Great news. > > Yes. > >> Thanks for you time. >> >> I still have problems with this part though: >>> If there's neither a MDIO PHY nor a fixed-link, then the network driver >>> fails to initialise the device. >> >> I think I am looking into the right source this time, seems like >> if we don't have both but still have managed="in-band-status", that >> should go the fixed-link path and still work... no? > > If we have no fixed-link and no phy, then you're correct. > > However, I really don't like the idea of abusing "fixed-link" as a > method to generate an ethtool/miitool/miidiag compatible output for > this, but I'm willing to let that pass for the moment. :) It is not just for that, you get all the goodies from the PHY library without modifying your Ethernet MAC driver specifically for it: phy_connect, adjust_link and phy_ethtool_{set,get}. The solution that was judged being the less intrusive back then was to provide MII-like registers, getting you all user-land to work transparently for free, but now that I think about it, having this "MII" translation seems a bit unnecessary, if not confusing. It may be better to remove some of register update logic and just assign phydev members directly...
18.09.2015 20:30, Florian Fainelli ?????: > On 18/09/15 10:22, Russell King - ARM Linux wrote: >> On Fri, Sep 18, 2015 at 07:04:09PM +0300, Stas Sergeev wrote: >>> 18.09.2015 18:43, Russell King - ARM Linux ?????: >>>> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote: >>>>> AFAICS if it has use_inband_status==true, >>>>> then it went through of_phy_register_fixed_link(dn), >>>> That's totally incorrect. The test for setting use_inband_status in >>>> mvneta is: >>>> >>>> err = of_property_read_string(dn, "managed", &managed); >>>> pp->use_inband_status = (err == 0 && >>>> strcmp(managed, "in-band-status") == 0); >>> Arrrr! I was looking at the branch without the last >>> patch applied, so it occurred to me as >>> >>> pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && >>> fixed_phy; >>> >>> Sorry for that. >> Yay :) >> >>> So we seem to indeed have a nasty regression with the patch >>> that just went to stable. :( Great news. >> Yes. >> >>> Thanks for you time. >>> >>> I still have problems with this part though: >>>> If there's neither a MDIO PHY nor a fixed-link, then the network driver >>>> fails to initialise the device. >>> I think I am looking into the right source this time, seems like >>> if we don't have both but still have managed="in-band-status", that >>> should go the fixed-link path and still work... no? >> If we have no fixed-link and no phy, then you're correct. >> >> However, I really don't like the idea of abusing "fixed-link" as a >> method to generate an ethtool/miitool/miidiag compatible output for >> this, but I'm willing to let that pass for the moment. :) > It is not just for that, you get all the goodies from the PHY library > without modifying your Ethernet MAC driver specifically for it: > phy_connect, adjust_link and phy_ethtool_{set,get}. But you still need to modify the MAC driver, and in a bit nasty way. Eg when you want the AN to work right, you need to do of_phy_find_device() and feed the current AN settings to fixed-phy manually, or, after phy_connect(), you'll run with wrong link state for some time (and there is still a bit of race anyway, as the link state could change between of_phy_find_device() and phy_connect()). So I am not sure what would the best API look like, but my overall impression was it is currently not the one.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 62e48bc0cb23..e1698731e429 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -1010,6 +1010,10 @@ static void mvneta_defaults_set(struct mvneta_port *pp) val |= MVNETA_GMAC_INBAND_AN_ENABLE | MVNETA_GMAC_AN_SPEED_EN | MVNETA_GMAC_AN_DUPLEX_EN; + /* We appear to need the interface forced for DSA switches */ + val &= ~(MVNETA_GMAC_AN_DUPLEX_EN | + MVNETA_GMAC_AN_SPEED_EN); + val |= MVNETA_GMAC_FORCE_LINK_PASS; mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER); val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;
I've been bringing up the mainline kernel on a new board, which has an Marvell SoC with a mvneta interface connected to a Marvell DSA switch. The DSA switch is a 88E6176. In the DT block for the interface, I specify: ethernet@... { phy-mode = "sgmii"; status = "okay"; fixed-link { speed = <1000>; full-duplex; }; }; However, this doesn't work without patching mvneta to disable the autonegotiation and forcing the link up: As Marvell likes to keep all their documentation secret, it's very hard to know what's going on, and why this would be necessary. However, when running the board under a kernel built from Marvell's code base, it sets this register in a similar way. Since the link is definitely a fixed-link operating at 1G, full duplex, with no autonegotiation, I think using the fixed-link in DT is correct, but the mvneta driver is wrong to abuse fixed-link to mean "SGMII but with in-band autonegotiation". The other issue I've seen is that even with the fixed-link settings, I have seen via ethtool that the link is reported as 10mbit - because mvneta_fixed_link_update() changes the settings on the fixed link. So, fixed-link doesn't seem to be quite as fixed as "fixed" says it should be. Shouldn't SGMII (which is always gigabit) be treated as gigabit with in-band negotiation when phy-mode = "sgmii" but no fixed-link, but a real fixed speed, other parameters and forced up when phy-mode = "sgmii" and there is a fixed link. It seems to me that there's been a design mistake here, and my fear is that as we seem to have had this mistake in the tree since April, it's now almost impossible to support this setup without breaking DT compatibility. However, it could be that the switch is misconfigured, and some register bit somewhere isn't set to indicate that it should provide in-band signalling on its SGMII interface. I've trawled the net looking at various bits of Marvell code for driving their switches, and I see nothing which would cause them to enable in-band signalling, but maybe I'm missing something. Here's the switch registers - the mvneta is connected to port 5: GLOBAL GLOBAL2 0 1 2 3 4 5 6 0: c844 0 100f 100f 100f 100f 100f e09 e07 1: 0 0 3 3 3 3 3 3e 3 2: 0 ffff 0 0 0 0 0 0 0 3: 0 ffff 1761 1761 1761 1761 1761 1761 1761 4: 6000 258 430 430 430 430 430 373f 430 5: 0 ff 0 0 0 0 0 0 0 6: 0 1f0f 7e 7d 7b 77 6f 505f 3f 7: 0 707f 0 0 0 0 0 0 0 8: 0 7800 2080 2080 2080 2080 2080 2080 2080 9: 0 1600 1 1 1 1 1 1 1 a: 148 0 0 0 0 0 0 0 0 b: 6000 1000 1 2 4 8 10 20 40 c: 0 7f 0 0 0 0 0 0 0 d: 0 502 0 0 0 0 0 0 0 e: 0 0 0 0 0 0 0 0 0 f: 0 f00 dada dada dada dada dada dada dada 10: 0 0 0 0 0 0 0 0 0 11: 0 0 0 0 0 0 0 0 0 12: 5555 0 0 0 0 0 0 0 0 13: 5555 0 0 0 0 0 0 0 0 14: aaaa 400 0 0 0 0 0 0 0 15: aaaa 0 0 0 0 0 0 0 0 16: ffff 0 33 33 33 33 33 33 0 17: ffff 0 0 0 0 0 0 0 0 18: fa41 1884 3210 3210 3210 3210 3210 3210 3210 19: 0 1e1 7654 7654 7654 7654 7654 7654 7654 1a: 5550 0 0 0 0 0 0 0 0 1b: 1ff f869 8000 8000 8000 8000 8000 8000 8000 1c: 0 0 0 0 0 0 0 0 0 1d: 1000 0 0 0 0 0 0 0 0 1e: 0 0 0 0 0 0 0 0 0 1f: 0 0 0 0 0 0 0 0 0 The DSA switch is configured in DT with: dsa@0 { compatible = "marvell,dsa"; dsa,ethernet = <ð1>; dsa,mii-bus = <&mdio>; #address-cells = <2>; #size-cells = <0>; switch@0 { #address-cells = <1>; #size-cells = <0>; reg = <4 0>; ... port@5 { reg = <5>; label = "cpu"; }; }; };