| Message ID | 20200303074738.3299-1-michael@walle.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] dm: uclass: don't assign aliased seq numbers | expand |
On 03. 03. 20 8:47, Michael Walle 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. > > Also adapt the test cases to the new handling and add test cases > checking the holes in the seq numbers. > > Signed-off-by: Michael Walle <michael at walle.cc> > Reviewed-by: Alex Marginean <alexandru.marginean at nxp.com> > Tested-by: Alex Marginean <alexandru.marginean at nxp.com> > Acked-by: Vladimir Oltean <olteanv at gmail.com> > Reviewed-by: Simon Glass <sjg at chromium.org> > --- > changes since v3: > - dev_read_alias_highest_id() is only available if CONFIG_OF_CONTROL is > set. Thus added an additional condition "CONFIG_IS_ENABLED(OF_CONTROL)", > thanks Simon. > > changes since v2: > - adapt/new test cases, thanks Simon > > changes since v1: > - move notice about superfluous commits from commit message to this > section. > - fix the comment style > > arch/sandbox/dts/test.dts | 4 ++-- > drivers/core/uclass.c | 21 +++++++++++++++------ > include/configs/sandbox.h | 6 +++--- > test/dm/eth.c | 14 +++++++------- > test/dm/test-fdt.c | 22 +++++++++++++++++----- > 5 files changed, 44 insertions(+), 23 deletions(-) > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > index 4a277934a7..915f337ae8 100644 > --- a/arch/sandbox/dts/test.dts > +++ b/arch/sandbox/dts/test.dts > @@ -19,8 +19,8 @@ > pci0 = &pci0; > pci1 = &pci1; > pci2 = &pci2; > - remoteproc1 = &rproc_1; > - remoteproc2 = &rproc_2; > + remoteproc0 = &rproc_1; > + remoteproc1 = &rproc_2; > rtc0 = &rtc_0; > rtc1 = &rtc_1; > spi0 = "/spi at 0"; > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index 58b19a4210..dab49fe627 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -688,13 +688,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); > @@ -706,9 +707,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(OF_CONTROL) && 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) > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h > index 1c13055cdc..b02c362fed 100644 > --- a/include/configs/sandbox.h > +++ b/include/configs/sandbox.h > @@ -97,9 +97,9 @@ > #endif > > #define SANDBOX_ETH_SETTINGS "ethaddr=00:00:11:22:33:44\0" \ > - "eth1addr=00:00:11:22:33:45\0" \ > - "eth3addr=00:00:11:22:33:46\0" \ > - "eth5addr=00:00:11:22:33:47\0" \ > + "eth3addr=00:00:11:22:33:45\0" \ > + "eth5addr=00:00:11:22:33:46\0" \ > + "eth6addr=00:00:11:22:33:47\0" \ > "ipaddr=1.2.3.4\0" > > #define MEM_LAYOUT_ENV_SETTINGS \ > diff --git a/test/dm/eth.c b/test/dm/eth.c > index ad5354b4bf..75315a0c6d 100644 > --- a/test/dm/eth.c > +++ b/test/dm/eth.c > @@ -47,7 +47,7 @@ static int dm_test_eth_alias(struct unit_test_state *uts) > ut_assertok(net_loop(PING)); > ut_asserteq_str("eth at 10002000", env_get("ethact")); > > - env_set("ethact", "eth1"); > + env_set("ethact", "eth6"); > ut_assertok(net_loop(PING)); > ut_asserteq_str("eth at 10004000", env_get("ethact")); > > @@ -104,7 +104,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) > const char *ethname[DM_TEST_ETH_NUM] = {"eth at 10002000", "eth at 10003000", > "sbe5", "eth at 10004000"}; > const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr", > - "eth3addr", "eth1addr"}; > + "eth3addr", "eth6addr"}; > char ethaddr[DM_TEST_ETH_NUM][18]; > int i; > > @@ -187,15 +187,15 @@ static int dm_test_eth_rotate(struct unit_test_state *uts) > > /* Invalidate eth1's MAC address */ > memset(ethaddr, '\0', sizeof(ethaddr)); > - strncpy(ethaddr, env_get("eth1addr"), 17); > - /* Must disable access protection for eth1addr before clearing */ > - env_set(".flags", "eth1addr"); > - env_set("eth1addr", NULL); > + strncpy(ethaddr, env_get("eth6addr"), 17); > + /* Must disable access protection for eth6addr before clearing */ > + env_set(".flags", "eth6addr"); > + env_set("eth6addr", NULL); > > retval = _dm_test_eth_rotate1(uts); > > /* Restore the env */ > - env_set("eth1addr", ethaddr); > + env_set("eth6addr", ethaddr); > env_set("ethrotate", NULL); > > if (!retval) { > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c > index 75ae08081c..23ce4339fa 100644 > --- a/test/dm/test-fdt.c > +++ b/test/dm/test-fdt.c > @@ -360,20 +360,32 @@ static int dm_test_fdt_uclass_seq(struct unit_test_state *uts) > ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 2, &dev)); > ut_asserteq_str("d-test", dev->name); > > - /* d-test actually gets 0 */ > - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 0, &dev)); > + /* > + * d-test actually gets 9, because thats the next free one after the > + * aliases. > + */ > + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 9, &dev)); > ut_asserteq_str("d-test", dev->name); > > - /* initially no one wants seq 1 */ > - ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, > + /* initially no one wants seq 10 */ > + ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, > &dev)); > ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); > ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 4, &dev)); > > /* But now that it is probed, we can find it */ > - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, &dev)); > + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, &dev)); > ut_asserteq_str("f-test", dev->name); > > + /* > + * And we should still have holes in our sequence numbers, that is 2 > + * and 4 should not be used. > + */ > + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 2, > + true, &dev)); > + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 4, > + true, &dev)); > + > return 0; > } > DM_TEST(dm_test_fdt_uclass_seq, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); > Tested-by: Michal Simek <michal.simek at xilinx.com> (zcu102-revA) Thanks, Michal
On 03. 03. 20 8:47, Michael Walle 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. > > Also adapt the test cases to the new handling and add test cases > checking the holes in the seq numbers. > > Signed-off-by: Michael Walle <michael at walle.cc> > Reviewed-by: Alex Marginean <alexandru.marginean at nxp.com> > Tested-by: Alex Marginean <alexandru.marginean at nxp.com> > Acked-by: Vladimir Oltean <olteanv at gmail.com> > Reviewed-by: Simon Glass <sjg at chromium.org> > --- > changes since v3: > - dev_read_alias_highest_id() is only available if CONFIG_OF_CONTROL is > set. Thus added an additional condition "CONFIG_IS_ENABLED(OF_CONTROL)", > thanks Simon. > > changes since v2: > - adapt/new test cases, thanks Simon > > changes since v1: > - move notice about superfluous commits from commit message to this > section. > - fix the comment style > > arch/sandbox/dts/test.dts | 4 ++-- > drivers/core/uclass.c | 21 +++++++++++++++------ > include/configs/sandbox.h | 6 +++--- > test/dm/eth.c | 14 +++++++------- > test/dm/test-fdt.c | 22 +++++++++++++++++----- > 5 files changed, 44 insertions(+), 23 deletions(-) > Applied to u-boot-dm/next, thanks!
Hi Simon, Am 2020-04-24 16:17, schrieb Michael Walle: > Hi Simon, > > Am 2020-04-20 01:38, schrieb Simon Glass: > > [..snip..] > >>> > uclass 31: eth >>> > 0 * smsc95xx_eth @ 3db69ac0, seq 0, (req -1) >>> >>> Shouldn't this be "req 0" if the ethernet alias is actually matched. >>> Does u-boot actually supports matching usb nodes to devices? If not, >>> shouldn't the alias be removed then? >>> >>> That being said, it is still strange why the bootloader doesn't find >>> ethernet-1 then. I've tried with my board, no native ethernet support >>> and an usb network dongle which works as expected (well the dongle >>> seems to have some issues to actually transfer frames). >> >> It is a bit strange. Removing the alias does not fix it though. > > Are you sure you removed the alias in the correct file? There are two, > could you please double check if is not contained in the resulting > device tree? > > dtc -I dtb -O dts dts/dt.dtb > > I just tested it on a rpi3b. and it works if i remove the alias. > >> So far as I know U-Boot doesn't work with the alias, since there is no >> driver for the "usb424,2514" compatible string. > > So it is actually correct behaviour of my patch. ethernet1 doesn't work > because there is no eth1addr. So I see three solutions: > > (1) make the matching work > (2) remove the alias > (3) set eth1addr instead of ethaddr Any news on this? Can I help somewhere? I'd go with (2). -michael
Hi Michael, On Tue, 19 May 2020 at 06:17, Michael Walle <michael at walle.cc> wrote: > > Hi Simon, > > Am 2020-04-24 16:17, schrieb Michael Walle: > > Hi Simon, > > > > Am 2020-04-20 01:38, schrieb Simon Glass: > > > > [..snip..] > > > >>> > uclass 31: eth > >>> > 0 * smsc95xx_eth @ 3db69ac0, seq 0, (req -1) > >>> > >>> Shouldn't this be "req 0" if the ethernet alias is actually matched. > >>> Does u-boot actually supports matching usb nodes to devices? If not, > >>> shouldn't the alias be removed then? > >>> > >>> That being said, it is still strange why the bootloader doesn't find > >>> ethernet-1 then. I've tried with my board, no native ethernet support > >>> and an usb network dongle which works as expected (well the dongle > >>> seems to have some issues to actually transfer frames). > >> > >> It is a bit strange. Removing the alias does not fix it though. > > > > Are you sure you removed the alias in the correct file? There are two, > > could you please double check if is not contained in the resulting > > device tree? > > > > dtc -I dtb -O dts dts/dt.dtb > > > > I just tested it on a rpi3b. and it works if i remove the alias. > > > >> So far as I know U-Boot doesn't work with the alias, since there is no > >> driver for the "usb424,2514" compatible string. > > > > So it is actually correct behaviour of my patch. ethernet1 doesn't work > > because there is no eth1addr. So I see three solutions: > > > > (1) make the matching work > > (2) remove the alias > > (3) set eth1addr instead of ethaddr > > Any news on this? Can I help somewhere? I'd go with (2). What is involved in (1)? Regards, Simon
Hi Simon, Am 2020-05-19 18:47, schrieb Simon Glass: > Hi Michael, > > On Tue, 19 May 2020 at 06:17, Michael Walle <michael at walle.cc> wrote: >> >> Hi Simon, >> >> Am 2020-04-24 16:17, schrieb Michael Walle: >> > Hi Simon, >> > >> > Am 2020-04-20 01:38, schrieb Simon Glass: >> > >> > [..snip..] >> > >> >>> > uclass 31: eth >> >>> > 0 * smsc95xx_eth @ 3db69ac0, seq 0, (req -1) >> >>> >> >>> Shouldn't this be "req 0" if the ethernet alias is actually matched. >> >>> Does u-boot actually supports matching usb nodes to devices? If not, >> >>> shouldn't the alias be removed then? >> >>> >> >>> That being said, it is still strange why the bootloader doesn't find >> >>> ethernet-1 then. I've tried with my board, no native ethernet support >> >>> and an usb network dongle which works as expected (well the dongle >> >>> seems to have some issues to actually transfer frames). >> >> >> >> It is a bit strange. Removing the alias does not fix it though. >> > >> > Are you sure you removed the alias in the correct file? There are two, >> > could you please double check if is not contained in the resulting >> > device tree? >> > >> > dtc -I dtb -O dts dts/dt.dtb >> > >> > I just tested it on a rpi3b. and it works if i remove the alias. >> > >> >> So far as I know U-Boot doesn't work with the alias, since there is no >> >> driver for the "usb424,2514" compatible string. >> > >> > So it is actually correct behaviour of my patch. ethernet1 doesn't work >> > because there is no eth1addr. So I see three solutions: >> > >> > (1) make the matching work >> > (2) remove the alias >> > (3) set eth1addr instead of ethaddr >> >> Any news on this? Can I help somewhere? I'd go with (2). > > What is involved in (1)? I've given it a try in the new v5 version. -michael
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 4a277934a7..915f337ae8 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -19,8 +19,8 @@ pci0 = &pci0; pci1 = &pci1; pci2 = &pci2; - remoteproc1 = &rproc_1; - remoteproc2 = &rproc_2; + remoteproc0 = &rproc_1; + remoteproc1 = &rproc_2; rtc0 = &rtc_0; rtc1 = &rtc_1; spi0 = "/spi at 0"; diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 58b19a4210..dab49fe627 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -688,13 +688,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); @@ -706,9 +707,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(OF_CONTROL) && 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) diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 1c13055cdc..b02c362fed 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -97,9 +97,9 @@ #endif #define SANDBOX_ETH_SETTINGS "ethaddr=00:00:11:22:33:44\0" \ - "eth1addr=00:00:11:22:33:45\0" \ - "eth3addr=00:00:11:22:33:46\0" \ - "eth5addr=00:00:11:22:33:47\0" \ + "eth3addr=00:00:11:22:33:45\0" \ + "eth5addr=00:00:11:22:33:46\0" \ + "eth6addr=00:00:11:22:33:47\0" \ "ipaddr=1.2.3.4\0" #define MEM_LAYOUT_ENV_SETTINGS \ diff --git a/test/dm/eth.c b/test/dm/eth.c index ad5354b4bf..75315a0c6d 100644 --- a/test/dm/eth.c +++ b/test/dm/eth.c @@ -47,7 +47,7 @@ static int dm_test_eth_alias(struct unit_test_state *uts) ut_assertok(net_loop(PING)); ut_asserteq_str("eth at 10002000", env_get("ethact")); - env_set("ethact", "eth1"); + env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth at 10004000", env_get("ethact")); @@ -104,7 +104,7 @@ static int dm_test_eth_act(struct unit_test_state *uts) const char *ethname[DM_TEST_ETH_NUM] = {"eth at 10002000", "eth at 10003000", "sbe5", "eth at 10004000"}; const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr", - "eth3addr", "eth1addr"}; + "eth3addr", "eth6addr"}; char ethaddr[DM_TEST_ETH_NUM][18]; int i; @@ -187,15 +187,15 @@ static int dm_test_eth_rotate(struct unit_test_state *uts) /* Invalidate eth1's MAC address */ memset(ethaddr, '\0', sizeof(ethaddr)); - strncpy(ethaddr, env_get("eth1addr"), 17); - /* Must disable access protection for eth1addr before clearing */ - env_set(".flags", "eth1addr"); - env_set("eth1addr", NULL); + strncpy(ethaddr, env_get("eth6addr"), 17); + /* Must disable access protection for eth6addr before clearing */ + env_set(".flags", "eth6addr"); + env_set("eth6addr", NULL); retval = _dm_test_eth_rotate1(uts); /* Restore the env */ - env_set("eth1addr", ethaddr); + env_set("eth6addr", ethaddr); env_set("ethrotate", NULL); if (!retval) { diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 75ae08081c..23ce4339fa 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -360,20 +360,32 @@ static int dm_test_fdt_uclass_seq(struct unit_test_state *uts) ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 2, &dev)); ut_asserteq_str("d-test", dev->name); - /* d-test actually gets 0 */ - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 0, &dev)); + /* + * d-test actually gets 9, because thats the next free one after the + * aliases. + */ + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 9, &dev)); ut_asserteq_str("d-test", dev->name); - /* initially no one wants seq 1 */ - ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, + /* initially no one wants seq 10 */ + ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, &dev)); ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev)); ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 4, &dev)); /* But now that it is probed, we can find it */ - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, &dev)); + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, &dev)); ut_asserteq_str("f-test", dev->name); + /* + * And we should still have holes in our sequence numbers, that is 2 + * and 4 should not be used. + */ + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 2, + true, &dev)); + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 4, + true, &dev)); + return 0; } DM_TEST(dm_test_fdt_uclass_seq, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);