Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
On Thu, 2017-04-06 at 12:46 -0700, Florian Fainelli wrote: > > I thought a while ago we could add some dev flag to prevent the link > > watch from doing that, but never got to look into it myself and > > apparently neither did Gavin. > > It sounds like a similar situation to e.g: 802.1x, you want to let some > frames make it through and you need a working link for that, but you > can't quite flag the device as UP entirely. Maybe you can find a way to > re-use IFF_LOWER_UP to that purpose and only set that flag based on the > NC-SI frames indicating link state? Possibly yes, I wasn't familiar with the difference between "carrier" and IFF_LOWER_UP so far. I'll have a look or have Gavin do so. It would be nice to reflect that properly so things like fallover or bonding do the right thing. On the other hand I yet have to see a system where the BMC has more than one NC-SI link to a NIC (what they usually have is that NC-SI is connected to multiple NICs which is a different matter and should be handled within the NC-SI stack). Anyway, I'll look into it further down the track along with other improvements I'd like to do to the NC-SI stack such as properly setting up multicast and vlan filters in the peer NIC and various cleanups. Cheers, Ben.
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
On 04/04/2017 11:31 PM, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-04 at 23:02 -0700, Florian Fainelli wrote: > >> We don't necessarily have a phydev attached when using NC-SI, so it was >>> easier to have the core code path not have to go fishing for those >>> settings in different places based on whether we're using NC-SI or not. >> >> Oh right, I missed that part. Is there a reason why NC-SI does not have >> a PHY device attached? If not, could you somehow model the link using a >> fixed PHY (which appears to Linux as a normal phy_device) just to keep >> things simple. > > Hrm ... maybe another day if you don't mind ;-) > > First NC-SI isn't really a PHY it's a cross-over RMII connection > to another NIC. > > Now we could make it a phydev using a "fixed" PHY I suppose, that just > "represents" the other end. That would be a way to do it. It would need > to have the link permanently up however (see below). > > That said I do want to tackle making it some kind of pseudo-PHY that > actually reflects the state of the remote end (especially the link > state, ie. up/down). > > However there are a couple of issues to tackle if we do that. Well > mostly one annoying one: > > NC-SI needs to talk to the remote NIC via specific ethernet frames. > > With the current link watch code however, if we reflect the remote link > to the local NIC link via netif_carrier_on/off, we end up deactivating > the device on link off and thus preventing the NC-SI stack from talking > to the peer NIC at all. > > I thought a while ago we could add some dev flag to prevent the link > watch from doing that, but never got to look into it myself and > apparently neither did Gavin. It sounds like a similar situation to e.g: 802.1x, you want to let some frames make it through and you need a working link for that, but you can't quite flag the device as UP entirely. Maybe you can find a way to re-use IFF_LOWER_UP to that purpose and only set that flag based on the NC-SI frames indicating link state? -- Florian
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
From: Benjamin Herrenschmidt Date: Wed, 5 Apr 2017 12:28:40 +1000 > This is version 2 of the first batch of updates to the > ftgmac100 driver. > > Essentially: > > - A few misc cleanups > - Fixing link speed & duplex handling (including dealing with >an Aspeed requirement to double reset the controller when >the speed changes) > - And addition of a reset task workqueue which will be used >for delaying the re-initialization of the controller > - Fixing a number of issues with how interrupts and NAPI >are dealt with. > > Subsequent batches will rework and improve the rx path, the > tx path, and add a bunch of features and fixes. > > Version 2 addresses some review comments to patches 5 and 10 > (see version history in the respective emails). Series applied.
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
On Tue, 2017-04-04 at 23:02 -0700, Florian Fainelli wrote: > We don't necessarily have a phydev attached when using NC-SI, so it was > > easier to have the core code path not have to go fishing for those > > settings in different places based on whether we're using NC-SI or not. > > Oh right, I missed that part. Is there a reason why NC-SI does not have > a PHY device attached? If not, could you somehow model the link using a > fixed PHY (which appears to Linux as a normal phy_device) just to keep > things simple. Hrm ... maybe another day if you don't mind ;-) First NC-SI isn't really a PHY it's a cross-over RMII connection to another NIC. Now we could make it a phydev using a "fixed" PHY I suppose, that just "represents" the other end. That would be a way to do it. It would need to have the link permanently up however (see below). That said I do want to tackle making it some kind of pseudo-PHY that actually reflects the state of the remote end (especially the link state, ie. up/down). However there are a couple of issues to tackle if we do that. Well mostly one annoying one: NC-SI needs to talk to the remote NIC via specific ethernet frames. With the current link watch code however, if we reflect the remote link to the local NIC link via netif_carrier_on/off, we end up deactivating the device on link off and thus preventing the NC-SI stack from talking to the peer NIC at all. I thought a while ago we could add some dev flag to prevent the link watch from doing that, but never got to look into it myself and apparently neither did Gavin. So yes, those are worthwhile improvements and I can probably tackle them once I've unpiled a dozen other train wrecks from my plate ;) However I'd like to not block this series further since it's not actually making things any worse than they are. > > > - the need to reset the HW during link changes is just ... well too bad > > > > Yup but there's little choice. The HW wants it. I don't see any real > > point in optimizing that path mind you. Losing a few packets around > > a link change isn't going to hurt and it keeps the code a lot simpler > > by having a single "re-init" path. > > I was just merely trying to say nicely: what a nicely broken piece of HW > (there were other adjectives coming to mind), and I do understand the pain. :-) At least I got a register spec (and little more) :-) It looks like those Aspeed BMCs are the only game in town for BMC chips these days and they use that "interesting" IP block from Faraday so this is probably here to stay, at least for a while. Another "interesting" attribute of that piece of c^Hhw is its handling of receive descriptors. It doesn't "count" how many are free. It has to constantly "read" the head descriptor in the RX ring to check the own bit. So you have to setup a HW timer for the chip to go "poll" on your memory. It's pretty insane. At least for TX there's an MMIO you can poke to tell it to go fetch more. There's sort-of one for RX but it doesn't seem to do what you would expect, or I did something wrong when playing with it. It's not like it would have been hard to have a counter, which is incremented by writing a value to a register so Linux can "provide" descriptors by writing the number freed in there. So the chip never really knows how many free descriptors it has which also means it cannot do flow control based on that, only on the FIFO threshold. With a 2K only FIFO that's interesting. Anyway, it sort-of works. Without my patches I maxed out at about 80Mbit/s iperf on a gigabit link with the AST2500 eval board (ARM11 800Mhz base). With my patches I get to about 400Mbit/s. Cheers. Ben.
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
On 04/04/2017 10:53 PM, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-04 at 21:21 -0700, Florian Fainelli wrote: >> >> This looks pretty good to me, two minor things: >> >> - most drivers keep track of the old status/duplex/pause/link variables >> instead of the current one which is already available within struct >> phy_device, any particular reason for not doing like the other drivers? > > We don't necessarily have a phydev attached when using NC-SI, so it was > easier to have the core code path not have to go fishing for those > settings in different places based on whether we're using NC-SI or not. Oh right, I missed that part. Is there a reason why NC-SI does not have a PHY device attached? If not, could you somehow model the link using a fixed PHY (which appears to Linux as a normal phy_device) just to keep things simple. > >> - the need to reset the HW during link changes is just ... well too bad > > Yup but there's little choice. The HW wants it. I don't see any real > point in optimizing that path mind you. Losing a few packets around > a link change isn't going to hurt and it keeps the code a lot simpler > by having a single "re-init" path. I was just merely trying to say nicely: what a nicely broken piece of HW (there were other adjectives coming to mind), and I do understand the pain. > >> With that: >> >>> Reviewed-by: Florian Fainelli > > Thanks ! > > I'll post batch 2 in the next couple of days which tackles the RX path. Cool, looking forward to that! -- Florian
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
On Tue, 2017-04-04 at 21:21 -0700, Florian Fainelli wrote: > > This looks pretty good to me, two minor things: > > - most drivers keep track of the old status/duplex/pause/link variables > instead of the current one which is already available within struct > phy_device, any particular reason for not doing like the other drivers? We don't necessarily have a phydev attached when using NC-SI, so it was easier to have the core code path not have to go fishing for those settings in different places based on whether we're using NC-SI or not. > - the need to reset the HW during link changes is just ... well too bad Yup but there's little choice. The HW wants it. I don't see any real point in optimizing that path mind you. Losing a few packets around a link change isn't going to hurt and it keeps the code a lot simpler by having a single "re-init" path. > With that: > > > Reviewed-by: Florian Fainelli Thanks ! I'll post batch 2 in the next couple of days which tackles the RX path. Cheers, Ben.
Re: [PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
Salut Benjamin, Le 04/04/17 à 19:28, Benjamin Herrenschmidt a écrit : > This is version 2 of the first batch of updates to the > ftgmac100 driver. > > Essentially: > > - A few misc cleanups > - Fixing link speed & duplex handling (including dealing with >an Aspeed requirement to double reset the controller when >the speed changes) > - And addition of a reset task workqueue which will be used >for delaying the re-initialization of the controller > - Fixing a number of issues with how interrupts and NAPI >are dealt with. > > Subsequent batches will rework and improve the rx path, the > tx path, and add a bunch of features and fixes. > > Version 2 addresses some review comments to patches 5 and 10 > (see version history in the respective emails). > This looks pretty good to me, two minor things: - most drivers keep track of the old status/duplex/pause/link variables instead of the current one which is already available within struct phy_device, any particular reason for not doing like the other drivers? - the need to reset the HW during link changes is just ... well too bad With that: Reviewed-by: Florian Fainelli -- Florian
[PATCH v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts
This is version 2 of the first batch of updates to the ftgmac100 driver. Essentially: - A few misc cleanups - Fixing link speed & duplex handling (including dealing with an Aspeed requirement to double reset the controller when the speed changes) - And addition of a reset task workqueue which will be used for delaying the re-initialization of the controller - Fixing a number of issues with how interrupts and NAPI are dealt with. Subsequent batches will rework and improve the rx path, the tx path, and add a bunch of features and fixes. Version 2 addresses some review comments to patches 5 and 10 (see version history in the respective emails).
