| Message ID | 20200203171152.1639-1-michael@walle.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] dm: uclass: don't assign aliased seq numbers | expand |
On Mon, 3 Feb 2020 at 10:12, Michael Walle <michael at 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. > > 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> (added Joe for review fo network parts)
On 03. 02. 20 18:11, 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> > --- > > Please note that I've kept the R-b, T-b, and A-b tags although they were > for an older version. They only affects the drivers/core/uclass.c not the > test/dm/ part. OTOH none of the actual implementation has changed. > > I couldn't split the patch, otherwise the tests would fail. > > 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. > > 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 e529c54d8d..d448892a65 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 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) > 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 d59c449ce0..a4e5c64a1f 100644 > --- a/test/dm/test-fdt.c > +++ b/test/dm/test-fdt.c > @@ -359,20 +359,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> Thanks, Michal
Hi Michal, Am 2020-02-20 09:30, schrieb Michal Simek: > On 03. 02. 20 18:11, 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> >> --- >> >> Please note that I've kept the R-b, T-b, and A-b tags although they >> were >> for an older version. They only affects the drivers/core/uclass.c not >> the >> test/dm/ part. OTOH none of the actual implementation has changed. >> >> I couldn't split the patch, otherwise the tests would fail. >> >> 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. >> >> 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 e529c54d8d..d448892a65 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 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) >> 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 d59c449ce0..a4e5c64a1f 100644 >> --- a/test/dm/test-fdt.c >> +++ b/test/dm/test-fdt.c >> @@ -359,20 +359,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> Thanks! Just to be sure, did you test this patch or did you also revert 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") before testing? -michael
On 20. 02. 20 10:52, Michael Walle wrote: > Hi Michal, > > Am 2020-02-20 09:30, schrieb Michal Simek: >> On 03. 02. 20 18:11, 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> >>> --- >>> >>> Please note that I've kept the R-b, T-b, and A-b tags although they were >>> for an older version. They only affects the drivers/core/uclass.c not >>> the >>> test/dm/ part. OTOH none of the actual implementation has changed. >>> >>> I couldn't split the patch, otherwise the tests would fail. >>> >>> 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. >>> >>> 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 e529c54d8d..d448892a65 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 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) >>> 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 d59c449ce0..a4e5c64a1f 100644 >>> --- a/test/dm/test-fdt.c >>> +++ b/test/dm/test-fdt.c >>> @@ -359,20 +359,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> > > Thanks! > > Just to be sure, did you test this patch or did you also revert > ?61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") > before testing? I have applied this patch on the HEAD + my xilinx patches and run on zcu102 which has i2c muxes where every bus needs to have own id. Only i2c0/i2c1 had aliases that's why bus 0/1 had proper links. Others without alias continue to use 2/3/4/5... Then I changed i2c1 hw bus to be i2c5 alias and check that aliases are going from 6 up. Then apply your patch and check if this behavior stayed there. ZynqMP> i2c bus Bus 0: i2c at ff020000 (active 0) 20: gpio at 20, offset len 1, flags 0 21: gpio at 21, offset len 1, flags 0 75: i2c-mux at 75, offset len 1, flags 0 Bus 6: i2c at ff020000->i2c-mux at 75->i2c at 0 Bus 7: i2c at ff020000->i2c-mux at 75->i2c at 1 Bus 8: i2c at ff020000->i2c-mux at 75->i2c at 2 Bus 5: i2c at ff030000 (active 5) 74: i2c-mux at 74, offset len 1, flags 0 75: i2c-mux at 75, offset len 1, flags 0 Bus 9: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 9) 54: eeprom at 54, offset len 1, flags 0 Bus 10: i2c at ff030000->i2c-mux at 74->i2c at 1 Bus 11: i2c at ff030000->i2c-mux at 74->i2c at 2 Bus 12: i2c at ff030000->i2c-mux at 74->i2c at 3 Bus 13: i2c at ff030000->i2c-mux at 74->i2c at 4 Bus 14: i2c at ff030000->i2c-mux at 75->i2c at 0 Bus 15: i2c at ff030000->i2c-mux at 75->i2c at 1 Bus 16: i2c at ff030000->i2c-mux at 75->i2c at 2 Bus 17: i2c at ff030000->i2c-mux at 75->i2c at 3 Bus 18: i2c at ff030000->i2c-mux at 75->i2c at 4 Bus 19: i2c at ff030000->i2c-mux at 75->i2c at 5 Bus 20: i2c at ff030000->i2c-mux at 75->i2c at 6 Bus 21: i2c at ff030000->i2c-mux at 75->i2c at 7 I didn't revert the patch above. If I do it I see a lot of bus without proper number like this. ZynqMP> i2c bus Bus 0: i2c at ff020000 (active 0) 20: gpio at 20, offset len 1, flags 0 21: gpio at 21, offset len 1, flags 0 75: i2c-mux at 75, offset len 1, flags 0 Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 0 Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 1 Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 2 Bus 5: i2c at ff030000 (active 5) 74: i2c-mux at 74, offset len 1, flags 0 75: i2c-mux at 75, offset len 1, flags 0 Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 6) 54: eeprom at 54, offset len 1, flags 0 Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 1 Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 2 Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 3 Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 4 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 0 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 1 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 2 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 3 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 4 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 5 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 6 Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 7 Thanks, Michal
Hi Michal, Am 2020-02-20 11:14, schrieb Michal Simek: > On 20. 02. 20 10:52, Michael Walle wrote: >> Hi Michal, >> >> Am 2020-02-20 09:30, schrieb Michal Simek: >>> On 03. 02. 20 18:11, 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> >>>> --- >>>> >>>> Please note that I've kept the R-b, T-b, and A-b tags although they >>>> were >>>> for an older version. They only affects the drivers/core/uclass.c >>>> not >>>> the >>>> test/dm/ part. OTOH none of the actual implementation has changed. >>>> >>>> I couldn't split the patch, otherwise the tests would fail. >>>> >>>> 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. >>>> >>>> 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 e529c54d8d..d448892a65 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 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) >>>> 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 d59c449ce0..a4e5c64a1f 100644 >>>> --- a/test/dm/test-fdt.c >>>> +++ b/test/dm/test-fdt.c >>>> @@ -359,20 +359,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> >> >> Thanks! >> >> Just to be sure, did you test this patch or did you also revert >> ?61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >> before testing? > > I have applied this patch on the HEAD + my xilinx patches and run on > zcu102 which has i2c muxes where every bus needs to have own id. > Only i2c0/i2c1 had aliases that's why bus 0/1 had proper links. Others > without alias continue to use 2/3/4/5... > > Then I changed i2c1 hw bus to be i2c5 alias and check that aliases are > going from 6 up. > > Then apply your patch and check if this behavior stayed there. > > ZynqMP> i2c bus > Bus 0: i2c at ff020000 (active 0) > 20: gpio at 20, offset len 1, flags 0 > 21: gpio at 21, offset len 1, flags 0 > 75: i2c-mux at 75, offset len 1, flags 0 > Bus 6: i2c at ff020000->i2c-mux at 75->i2c at 0 > Bus 7: i2c at ff020000->i2c-mux at 75->i2c at 1 > Bus 8: i2c at ff020000->i2c-mux at 75->i2c at 2 > Bus 5: i2c at ff030000 (active 5) > 74: i2c-mux at 74, offset len 1, flags 0 > 75: i2c-mux at 75, offset len 1, flags 0 > Bus 9: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 9) > 54: eeprom at 54, offset len 1, flags 0 > Bus 10: i2c at ff030000->i2c-mux at 74->i2c at 1 > Bus 11: i2c at ff030000->i2c-mux at 74->i2c at 2 > Bus 12: i2c at ff030000->i2c-mux at 74->i2c at 3 > Bus 13: i2c at ff030000->i2c-mux at 74->i2c at 4 > Bus 14: i2c at ff030000->i2c-mux at 75->i2c at 0 > Bus 15: i2c at ff030000->i2c-mux at 75->i2c at 1 > Bus 16: i2c at ff030000->i2c-mux at 75->i2c at 2 > Bus 17: i2c at ff030000->i2c-mux at 75->i2c at 3 > Bus 18: i2c at ff030000->i2c-mux at 75->i2c at 4 > Bus 19: i2c at ff030000->i2c-mux at 75->i2c at 5 > Bus 20: i2c at ff030000->i2c-mux at 75->i2c at 6 > Bus 21: i2c at ff030000->i2c-mux at 75->i2c at 7 > > > I didn't revert the patch above. If I do it I see a lot of bus without > proper number like this. Mhh. the first number is req_req, so that explains why its -1. But I kinda see how this is involved in the "i2c dev" command. Looks like you can then request an unprobed device. So without the patch this wouldn't be possible anymore. This seems to be not only a problem for the i2c command, but I've also seen it in the mdio subsystem. Where you can't do raw mii/mdio accesses before the network device was probed, eg. you have to do a bootp and then the mdio/mii bus shows up. I guess we'd have a similar problem here. But regarding the initial question, so the patch is still necessary; at least if we don't want to change the "i2c dev <num>" behaviour. Thanks, -michael > ZynqMP> i2c bus > Bus 0: i2c at ff020000 (active 0) > 20: gpio at 20, offset len 1, flags 0 > 21: gpio at 21, offset len 1, flags 0 > 75: i2c-mux at 75, offset len 1, flags 0 > Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 0 > Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 1 > Bus -1: i2c at ff020000->i2c-mux at 75->i2c at 2 > Bus 5: i2c at ff030000 (active 5) > 74: i2c-mux at 74, offset len 1, flags 0 > 75: i2c-mux at 75, offset len 1, flags 0 > Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 0 (active 6) > 54: eeprom at 54, offset len 1, flags 0 > Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 1 > Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 2 > Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 3 > Bus -1: i2c at ff030000->i2c-mux at 74->i2c at 4 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 0 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 1 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 2 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 3 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 4 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 5 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 6 > Bus -1: i2c at ff030000->i2c-mux at 75->i2c at 7 > > > Thanks, > Michal
On 20. 02. 20 21:16, Michael Walle wrote: > > Hi Michal, > > Am 2020-02-20 11:14, schrieb Michal Simek: >> On 20. 02. 20 10:52, Michael Walle wrote: >>> Hi Michal, >>> >>> Am 2020-02-20 09:30, schrieb Michal Simek: >>>> On 03. 02. 20 18:11, 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> >>>>> --- >>>>> >>>>> Please note that I've kept the R-b, T-b, and A-b tags although they >>>>> were >>>>> for an older version. They only affects the drivers/core/uclass.c not >>>>> the >>>>> test/dm/ part. OTOH none of the actual implementation has changed. >>>>> >>>>> I couldn't split the patch, otherwise the tests would fail. >>>>> >>>>> 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. >>>>> >>>>> 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 e529c54d8d..d448892a65 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 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) >>>>> 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 d59c449ce0..a4e5c64a1f 100644 >>>>> --- a/test/dm/test-fdt.c >>>>> +++ b/test/dm/test-fdt.c >>>>> @@ -359,20 +359,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> >>> >>> Thanks! >>> >>> Just to be sure, did you test this patch or did you also revert >>> ?61607225d1 ("i2c: Fill req_seq in i2c_post_bind()") >>> before testing? >> >> I have applied this patch on the HEAD + my xilinx patches and run on >> zcu102 which has i2c muxes where every bus needs to have own id. >> Only i2c0/i2c1 had aliases that's why bus 0/1 had proper links. Others >> without alias continue to use 2/3/4/5... >> >> Then I changed i2c1 hw bus to be i2c5 alias and check that aliases are >> going from 6 up. >> >> Then apply your patch and check if this behavior stayed there. >> >> ZynqMP> i2c bus >> Bus 0:??? i2c at ff020000? (active 0) >> ?? 20: gpio at 20, offset len 1, flags 0 >> ?? 21: gpio at 21, offset len 1, flags 0 >> ?? 75: i2c-mux at 75, offset len 1, flags 0 >> Bus 6:??? i2c at ff020000->i2c-mux at 75->i2c at 0 >> Bus 7:??? i2c at ff020000->i2c-mux at 75->i2c at 1 >> Bus 8:??? i2c at ff020000->i2c-mux at 75->i2c at 2 >> Bus 5:??? i2c at ff030000? (active 5) >> ?? 74: i2c-mux at 74, offset len 1, flags 0 >> ?? 75: i2c-mux at 75, offset len 1, flags 0 >> Bus 9:??? i2c at ff030000->i2c-mux at 74->i2c at 0? (active 9) >> ?? 54: eeprom at 54, offset len 1, flags 0 >> Bus 10:??? i2c at ff030000->i2c-mux at 74->i2c at 1 >> Bus 11:??? i2c at ff030000->i2c-mux at 74->i2c at 2 >> Bus 12:??? i2c at ff030000->i2c-mux at 74->i2c at 3 >> Bus 13:??? i2c at ff030000->i2c-mux at 74->i2c at 4 >> Bus 14:??? i2c at ff030000->i2c-mux at 75->i2c at 0 >> Bus 15:??? i2c at ff030000->i2c-mux at 75->i2c at 1 >> Bus 16:??? i2c at ff030000->i2c-mux at 75->i2c at 2 >> Bus 17:??? i2c at ff030000->i2c-mux at 75->i2c at 3 >> Bus 18:??? i2c at ff030000->i2c-mux at 75->i2c at 4 >> Bus 19:??? i2c at ff030000->i2c-mux at 75->i2c at 5 >> Bus 20:??? i2c at ff030000->i2c-mux at 75->i2c at 6 >> Bus 21:??? i2c at ff030000->i2c-mux at 75->i2c at 7 >> >> >> I didn't revert the patch above. If I do it I see a lot of bus without >> proper number like this. > > Mhh. the first number is req_req, so that explains why its -1. > > But I kinda see how this is involved in the "i2c dev" command. Looks like > you can then request an unprobed device. So without the patch this > wouldn't be possible anymore. This seems to be not only a problem for > the i2c command, but I've also seen it in the mdio subsystem. Where you > can't do raw mii/mdio accesses before the network device was probed, eg. > you have to do a bootp and then the mdio/mii bus shows up. I guess we'd > have a similar problem here. that's about mdio registration when it is called. IIRC normally it is done as the part of probe and for that you need to start to use it. If MDIO has onw class and then you would solve this problem. Regarding i2c - you need to know that numbers to know where you want to go and without it you can't do anything. > > But regarding the initial question, so the patch is still necessary; > at least if we don't want to change the "i2c dev <num>" behaviour. As I said I can't see any breakage in usecase which I used for developing origin code that's why I am ok with it. There are other issues in i2c subsystems but that's different story. Thanks, Michal
Hi Michael, On Wed, 5 Feb 2020 at 13:05, Simon Glass <sjg at chromium.org> wrote: > > On Mon, 3 Feb 2020 at 10:12, Michael Walle <michael at 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. > > > > 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> > > (added Joe for review fo network parts) Unfortunately this breaks some ARM and PPC boards: https://gitlab.denx.de/u-boot/custodians/u-boot-dm/pipelines/2308 I've dropped it for now. Please can you take a look? Regards, Simon
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index e529c54d8d..d448892a65 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 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) 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 d59c449ce0..a4e5c64a1f 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -359,20 +359,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);