Hi Simon, On Sun, Feb 15, 2015 at 9:59 AM, Simon Glass <s...@chromium.org> wrote: > > Hi Joe, > > On 13 February 2015 at 19:33, Joe Hershberger <joe.hershber...@gmail.com> wrote: > > On Thu, Feb 12, 2015 at 11:14 PM, Simon Glass <s...@chromium.org> wrote: > >> > >> Hi Joe, > >> > >> On 10 February 2015 at 23:08, Joe Hershberger < joe.hershber...@gmail.com> > >> wrote: > >> > Hi Simon, > >> > > >> > On Tue, Feb 10, 2015 at 10:39 PM, Simon Glass <s...@chromium.org> wrote: > >> >> > >> >> Hi Joe, > >> >> > >> >> On 10 February 2015 at 18:30, Joe Hershberger < joe.hershber...@ni.com> > >> >> wrote: > >> >> > Before this patch, if the sequence numbers were resolved before > >> >> > probe, > >> >> > this code would insist on defining new non-conflicting-with-itself > >> >> > seq > >> >> > numbers. Now any "non -1" seq number is accepted as already resolved. > >> >> > >> >> Can you explain what problem this solves? At present, when probing a > >> >> device, ->seq must be -1 (sort-of by definition since it doesn't exist > >> >> as an active device in the uclass). > >> > > >> > Please look at eth_post_bind() in patch 07/14. The Ethernet devices > >> > need to > >> > write their hardware addresses to the registers in bind (since it needs > >> > to > >> > happen regardless of the device being used so that Linux will see the > >> > MAC > >> > address). As such, the sequence number is needed to look up the > >> > ethaddr. In > >> > order to avoid probing all the devices to get the seq number resolved, I > >> > resolve it in post_bind to avoid the rest of the overhead (thus no > >> > longer > >> > probing in post_bind, which was one of the issues previously). Then > >> > when > >> > probe comes along, the seq is already resolved. That's why this patch > >> > is > >> > needed. > >> > >> OK I see. > >> > >> This is a bit messy. If the MAC address assignment is part of the bind > >> step then it shouldn't need the seq number. > > > > Not sure why you say that. The reason I need the seq number is because I > > need to look up the proper env variable for the MAC address. E.g. ethaddr, > > eth2addr, etc. The seq number select which one to read from the env. > > We should be able to do this after a probe. A device which is bound > but not probed does not have a sequence number, as things currently > stand. > > > > >> I can think of some poor ways to do this but a nice way is not obvious! > > > > Not sure what you're referring to here. What is "this" in this context? > > Figuring out the sequence number. > > > > >> One option would be probe all the Ethernet devices on startup. If > >> probe() only set up the hardware (including MAC address) then that > >> might work. It would be fairly fast since it wouldn't involve starting > >> up the link, etc. I suspect you are worried about a lot of Ethernet > >> devices sitting around probed by unused. I'm not sure if that matters > >> though. > > > > I had it probing the devices originally (by calling first and next) and you > > commented that it shouldn't happen until the devices are used. However, I > > That was because your code was probing things in the bind mehod. > > > don't think we can guarantee that all drivers that come later will have > > simple probe (since that's not part of the contract). I think I agree with > > your original statement that we should not probe. It seems more suitable to > > write the hwaddr in bind as a known and limited side effect. > > I don't like the idea of an ethernet device supporting writing its > hardware address before it is probed. Until it is probed we don't > really know it is there, nor where it is exactly (bus, memory > address). So I think writing the hardware address makes more sense > after probe. But probe should not happen as part of bind. It seems to > me it could happen in your eth_init().
OK. I can see why you prefer not to have the hardware accessed in bind. In order to probe the devices (but not from bind) I'll have to add back eth_initialize() and have it probe all of the devices. the "eth_init()" that you see here is what gets called when the network is enabled, so it certainly shouldn't go in eth_init(). I could potentially rename it to eth_start() or something. That would be clearer, but would break from the former naming for anyone familiar. > > The seq number resolution seems fairly well contained as I implemented it in > > bind. I simply call the core function and write the result to the device > > member. Then of course this patch to remove the assert. > > Yes it is well contained, but I still don't think it is right. If you > want to put '#ifndef CONFIG_DM_NET' around the assert in > uclass_resolve_seq() while we work it out, that is OK with me. I will work on making it correct instead of adding that. -Joe _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
