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

[Joe Hershberger]

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.

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