[U-Boot] [RFC PATCH v3 01/14] dm: core: Allow seq numbers to be resolved before probe

[Simon Glass]

Hi Joe,

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

I see. Maybe eth_probe_all_devices()?

>
>> > 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.

OK.

Regards,
Simon