Hi Simon,
On Sun, Feb 15, 2015 at 9:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 13 February 2015 at 19:33, Joe Hershberger joe.hershberger@gmail.com
wrote:
On Thu, Feb 12, 2015 at 11:14 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 10 February 2015 at 23:08, Joe Hershberger <
joe.hershberger@gmail.com>
wrote:
Hi Simon,
On Tue, Feb 10, 2015 at 10:39 PM, Simon Glass sjg@chromium.org
wrote:
Hi Joe,
On 10 February 2015 at 18:30, Joe Hershberger <
joe.hershberger@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