| Message ID | 1423618233-11397-2-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | RFC |
| Delegated to: | Simon Glass |
| Headers | show |
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). > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> > > --- > > Changes in v3: > -Add seq patch to dm core > > Changes in v2: None > > drivers/core/uclass.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index 289a5d2..2d8b6f8 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -366,7 +366,9 @@ int uclass_resolve_seq(struct udevice *dev) > int seq; > int ret; > > - assert(dev->seq == -1); > + if (dev->seq != -1) > + return dev->seq; > + > ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, > false, &dup); > if (!ret) { > -- > 1.7.11.5 > Regards, Simon
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. Thanks, -Joe
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. I can think of some poor ways to do this but a nice way is not obvious! 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. Regards, Simon
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. > 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? > 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 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. 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. Thanks, -Joe
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(). > > 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. Regards, Simon
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
Hi Joe, On 16 February 2015 at 21:17, Joe Hershberger <joe.hershberger@gmail.com> wrote: > 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. 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
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 289a5d2..2d8b6f8 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -366,7 +366,9 @@ int uclass_resolve_seq(struct udevice *dev) int seq; int ret; - assert(dev->seq == -1); + if (dev->seq != -1) + return dev->seq; + ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, false, &dup); if (!ret) {
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. Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> --- Changes in v3: -Add seq patch to dm core Changes in v2: None drivers/core/uclass.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)