| Message ID | 20191220132858.13027-1-michael@walle.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v2] dm: uclass: don't assign aliased seq numbers | expand |
Hi Michael, On Fri, 20 Dec 2019 at 06:29, Michael Walle <michael@walle.cc> wrote: > > If there are aliases for an uclass, set the base for the "dynamically" > allocated numbers next to the highest alias. > > Please note, that this might lead to holes in the sequences, depending > on the device tree. For example if there is only an alias "ethernet1", > the next device seq number would be 2. > > In particular this fixes a problem with boards which are using ethernet > aliases but also might have network add-in cards like the E1000. If the > board is started with the add-in card and depending on the order of the > drivers, the E1000 might occupy the first ethernet device and mess up > all the hardware addresses, because the devices are now shifted by one. > > Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> > Cc: Michal Simek <michal.simek@xilinx.com> > Signed-off-by: Michael Walle <michael@walle.cc> > Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> > Tested-by: Alex Marginean <alexandru.marginean@nxp.com> > Acked-by: Vladimir Oltean <olteanv@gmail.com> > --- > > As a side effect, this should also make the following commits > superfluous: > - 7f3289bf6d ("dm: device: Request next sequence number") > - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") > Although I don't understand the root cause of the said problem. > > Thomas, Michal, could you please test this and then I'd add a second > patch removing the old code. I think this is reasonable. We have discussed a possible rework of the logic to merge seq and req_seq, but I don't think we have any patches yet. Please can you add a test to your patch? You can put it in test-fdt.c for example. If you are reverting the other patches, could you please send patches for those? Regards, SImon
Hi Michael, On Wed, 29 Jan 2020 at 19:16, Simon Glass <sjg@chromium.org> wrote: > > Hi Michael, > > On Fri, 20 Dec 2019 at 06:29, Michael Walle <michael@walle.cc> wrote: > > > > If there are aliases for an uclass, set the base for the "dynamically" > > allocated numbers next to the highest alias. > > > > Please note, that this might lead to holes in the sequences, depending > > on the device tree. For example if there is only an alias "ethernet1", > > the next device seq number would be 2. > > > > In particular this fixes a problem with boards which are using ethernet > > aliases but also might have network add-in cards like the E1000. If the > > board is started with the add-in card and depending on the order of the > > drivers, the E1000 might occupy the first ethernet device and mess up > > all the hardware addresses, because the devices are now shifted by one. > > > > Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> > > Cc: Michal Simek <michal.simek@xilinx.com> > > Signed-off-by: Michael Walle <michael@walle.cc> > > Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> > > Tested-by: Alex Marginean <alexandru.marginean@nxp.com> > > Acked-by: Vladimir Oltean <olteanv@gmail.com> > > --- > > > > As a side effect, this should also make the following commits > > superfluous: > > - 7f3289bf6d ("dm: device: Request next sequence number") > > - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") > > Although I don't understand the root cause of the said problem. > > > > Thomas, Michal, could you please test this and then I'd add a second > > patch removing the old code. > > I think this is reasonable. We have discussed a possible rework of the > logic to merge seq and req_seq, but I don't think we have any patches > yet. > > Please can you add a test to your patch? You can put it in test-fdt.c > for example. > > If you are reverting the other patches, could you please send patches for those? One more thing...this actually breaks existing tests so please fix those also. Thanks, SImon
Hi Simon, Am 2020-01-30 03:16, schrieb Simon Glass: > Hi Michael, > > On Fri, 20 Dec 2019 at 06:29, Michael Walle <michael@walle.cc> wrote: >> >> If there are aliases for an uclass, set the base for the "dynamically" >> allocated numbers next to the highest alias. >> >> Please note, that this might lead to holes in the sequences, depending >> on the device tree. For example if there is only an alias "ethernet1", >> the next device seq number would be 2. >> >> In particular this fixes a problem with boards which are using >> ethernet >> aliases but also might have network add-in cards like the E1000. If >> the >> board is started with the add-in card and depending on the order of >> the >> drivers, the E1000 might occupy the first ethernet device and mess up >> all the hardware addresses, because the devices are now shifted by >> one. >> >> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> >> Cc: Michal Simek <michal.simek@xilinx.com> >> Signed-off-by: Michael Walle <michael@walle.cc> >> Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> >> Tested-by: Alex Marginean <alexandru.marginean@nxp.com> >> Acked-by: Vladimir Oltean <olteanv@gmail.com> >> --- >> >> As a side effect, this should also make the following commits >> superfluous: >> - 7f3289bf6d ("dm: device: Request next sequence number") >> - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >> Although I don't understand the root cause of the said problem. >> >> Thomas, Michal, could you please test this and then I'd add a second >> patch removing the old code. > > I think this is reasonable. We have discussed a possible rework of the > logic to merge seq and req_seq, but I don't think we have any patches > yet. > > Please can you add a test to your patch? You can put it in test-fdt.c > for example. Just did a new version. > If you are reverting the other patches, could you please send patches > for those? Unfortunatly, neither Thomas nor Michal has responded, so there would be no test if that would work. But I could certainly prepare two patches. -michael
On 03. 02. 20 18:16, Michael Walle wrote: > Hi Simon, > > Am 2020-01-30 03:16, schrieb Simon Glass: >> Hi Michael, >> >> On Fri, 20 Dec 2019 at 06:29, Michael Walle <michael@walle.cc> wrote: >>> >>> If there are aliases for an uclass, set the base for the "dynamically" >>> allocated numbers next to the highest alias. >>> >>> Please note, that this might lead to holes in the sequences, depending >>> on the device tree. For example if there is only an alias "ethernet1", >>> the next device seq number would be 2. >>> >>> In particular this fixes a problem with boards which are using ethernet >>> aliases but also might have network add-in cards like the E1000. If the >>> board is started with the add-in card and depending on the order of the >>> drivers, the E1000 might occupy the first ethernet device and mess up >>> all the hardware addresses, because the devices are now shifted by one. >>> >>> Cc: Thomas Fitzsimmons <fitzsim@fitzsim.org> >>> Cc: Michal Simek <michal.simek@xilinx.com> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> Reviewed-by: Alex Marginean <alexandru.marginean@nxp.com> >>> Tested-by: Alex Marginean <alexandru.marginean@nxp.com> >>> Acked-by: Vladimir Oltean <olteanv@gmail.com> >>> --- >>> >>> As a side effect, this should also make the following commits >>> superfluous: >>> - 7f3289bf6d ("dm: device: Request next sequence number") >>> - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >>> Although I don't understand the root cause of the said problem. >>> >>> Thomas, Michal, could you please test this and then I'd add a second >>> patch removing the old code. >> >> I think this is reasonable. We have discussed a possible rework of the >> logic to merge seq and req_seq, but I don't think we have any patches >> yet. >> >> Please can you add a test to your patch? You can put it in test-fdt.c >> for example. > > Just did a new version. > >> If you are reverting the other patches, could you please send patches >> for those? > > Unfortunatly, neither Thomas nor Michal has responded, so there would be > no test if that would work. But I could certainly prepare two patches. I still have this in my inbox to take a look and retest. I just don't have time to take a look at it now. I have tested this code on board with i2c mux where I was trying to change aliases. Thanks, Michal
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index c520ef113a..3c216221e0 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev) int uclass_resolve_seq(struct udevice *dev) { + struct uclass *uc = dev->uclass; + struct uclass_driver *uc_drv = uc->uc_drv; struct udevice *dup; - int seq; + int seq = 0; int ret; assert(dev->seq == -1); - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, dev->req_seq, - false, &dup); + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false, &dup); if (!ret) { dm_warn("Device '%s': seq %d is in use by '%s'\n", dev->name, dev->req_seq, dup->name); @@ -693,9 +694,17 @@ int uclass_resolve_seq(struct udevice *dev) return ret; } - for (seq = 0; seq < DM_MAX_SEQ; seq++) { - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq, - false, &dup); + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) && + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) { + /* + * dev_read_alias_highest_id() will return -1 if there no + * alias. Thus we can always add one. + */ + seq = dev_read_alias_highest_id(uc_drv->name) + 1; + } + + for (; seq < DM_MAX_SEQ; seq++) { + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup); if (ret == -ENODEV) break; if (ret)