[PATCH v3] dm: uclass: don't assign aliased seq numbers
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@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 ---
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@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@10002000", env_get("ethact"));
- env_set("ethact", "eth1"); + env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@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);
On Mon, 3 Feb 2020 at 10:12, 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.
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@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
Reviewed-by: Simon Glass sjg@chromium.org
(added Joe for review fo network parts)
Hi Michael,
On Wed, 5 Feb 2020 at 13:05, Simon Glass sjg@chromium.org wrote:
On Mon, 3 Feb 2020 at 10:12, 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.
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@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
Reviewed-by: Simon Glass sjg@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
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@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
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;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);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@10002000", env_get("ethact"));
- env_set("ethact", "eth1");
- env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@10004000"}; const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr",
"eth3addr", "eth1addr"};
"eth3addr", "eth6addr"};@@ -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@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@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
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;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);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@10002000", env_get("ethact"));
- env_set("ethact", "eth1");
- env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@10004000"}; const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr",
"eth3addr", "eth1addr"};
"eth3addr", "eth6addr"};@@ -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@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@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
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@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@10002000", env_get("ethact"));
- env_set("ethact", "eth1"); + env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@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@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@ff020000 (active 0) 20: gpio@20, offset len 1, flags 0 21: gpio@21, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 6: i2c@ff020000->i2c-mux@75->i2c@0 Bus 7: i2c@ff020000->i2c-mux@75->i2c@1 Bus 8: i2c@ff020000->i2c-mux@75->i2c@2 Bus 5: i2c@ff030000 (active 5) 74: i2c-mux@74, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 9: i2c@ff030000->i2c-mux@74->i2c@0 (active 9) 54: eeprom@54, offset len 1, flags 0 Bus 10: i2c@ff030000->i2c-mux@74->i2c@1 Bus 11: i2c@ff030000->i2c-mux@74->i2c@2 Bus 12: i2c@ff030000->i2c-mux@74->i2c@3 Bus 13: i2c@ff030000->i2c-mux@74->i2c@4 Bus 14: i2c@ff030000->i2c-mux@75->i2c@0 Bus 15: i2c@ff030000->i2c-mux@75->i2c@1 Bus 16: i2c@ff030000->i2c-mux@75->i2c@2 Bus 17: i2c@ff030000->i2c-mux@75->i2c@3 Bus 18: i2c@ff030000->i2c-mux@75->i2c@4 Bus 19: i2c@ff030000->i2c-mux@75->i2c@5 Bus 20: i2c@ff030000->i2c-mux@75->i2c@6 Bus 21: i2c@ff030000->i2c-mux@75->i2c@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@ff020000 (active 0) 20: gpio@20, offset len 1, flags 0 21: gpio@21, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus -1: i2c@ff020000->i2c-mux@75->i2c@0 Bus -1: i2c@ff020000->i2c-mux@75->i2c@1 Bus -1: i2c@ff020000->i2c-mux@75->i2c@2 Bus 5: i2c@ff030000 (active 5) 74: i2c-mux@74, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus -1: i2c@ff030000->i2c-mux@74->i2c@0 (active 6) 54: eeprom@54, offset len 1, flags 0 Bus -1: i2c@ff030000->i2c-mux@74->i2c@1 Bus -1: i2c@ff030000->i2c-mux@74->i2c@2 Bus -1: i2c@ff030000->i2c-mux@74->i2c@3 Bus -1: i2c@ff030000->i2c-mux@74->i2c@4 Bus -1: i2c@ff030000->i2c-mux@75->i2c@0 Bus -1: i2c@ff030000->i2c-mux@75->i2c@1 Bus -1: i2c@ff030000->i2c-mux@75->i2c@2 Bus -1: i2c@ff030000->i2c-mux@75->i2c@3 Bus -1: i2c@ff030000->i2c-mux@75->i2c@4 Bus -1: i2c@ff030000->i2c-mux@75->i2c@5 Bus -1: i2c@ff030000->i2c-mux@75->i2c@6 Bus -1: i2c@ff030000->i2c-mux@75->i2c@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@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
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@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@10002000", env_get("ethact"));
- env_set("ethact", "eth1"); + env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@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@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@ff020000 (active 0) 20: gpio@20, offset len 1, flags 0 21: gpio@21, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 6: i2c@ff020000->i2c-mux@75->i2c@0 Bus 7: i2c@ff020000->i2c-mux@75->i2c@1 Bus 8: i2c@ff020000->i2c-mux@75->i2c@2 Bus 5: i2c@ff030000 (active 5) 74: i2c-mux@74, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 9: i2c@ff030000->i2c-mux@74->i2c@0 (active 9) 54: eeprom@54, offset len 1, flags 0 Bus 10: i2c@ff030000->i2c-mux@74->i2c@1 Bus 11: i2c@ff030000->i2c-mux@74->i2c@2 Bus 12: i2c@ff030000->i2c-mux@74->i2c@3 Bus 13: i2c@ff030000->i2c-mux@74->i2c@4 Bus 14: i2c@ff030000->i2c-mux@75->i2c@0 Bus 15: i2c@ff030000->i2c-mux@75->i2c@1 Bus 16: i2c@ff030000->i2c-mux@75->i2c@2 Bus 17: i2c@ff030000->i2c-mux@75->i2c@3 Bus 18: i2c@ff030000->i2c-mux@75->i2c@4 Bus 19: i2c@ff030000->i2c-mux@75->i2c@5 Bus 20: i2c@ff030000->i2c-mux@75->i2c@6 Bus 21: i2c@ff030000->i2c-mux@75->i2c@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@ff020000 (active 0) 20: gpio@20, offset len 1, flags 0 21: gpio@21, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus -1: i2c@ff020000->i2c-mux@75->i2c@0 Bus -1: i2c@ff020000->i2c-mux@75->i2c@1 Bus -1: i2c@ff020000->i2c-mux@75->i2c@2 Bus 5: i2c@ff030000 (active 5) 74: i2c-mux@74, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus -1: i2c@ff030000->i2c-mux@74->i2c@0 (active 6) 54: eeprom@54, offset len 1, flags 0 Bus -1: i2c@ff030000->i2c-mux@74->i2c@1 Bus -1: i2c@ff030000->i2c-mux@74->i2c@2 Bus -1: i2c@ff030000->i2c-mux@74->i2c@3 Bus -1: i2c@ff030000->i2c-mux@74->i2c@4 Bus -1: i2c@ff030000->i2c-mux@75->i2c@0 Bus -1: i2c@ff030000->i2c-mux@75->i2c@1 Bus -1: i2c@ff030000->i2c-mux@75->i2c@2 Bus -1: i2c@ff030000->i2c-mux@75->i2c@3 Bus -1: i2c@ff030000->i2c-mux@75->i2c@4 Bus -1: i2c@ff030000->i2c-mux@75->i2c@5 Bus -1: i2c@ff030000->i2c-mux@75->i2c@6 Bus -1: i2c@ff030000->i2c-mux@75->i2c@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@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
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@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@10002000", env_get("ethact"));
- env_set("ethact", "eth1"); + env_set("ethact", "eth6"); ut_assertok(net_loop(PING)); ut_asserteq_str("eth@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@10002000", "eth@10003000", "sbe5", "eth@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@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@ff020000 (active 0) 20: gpio@20, offset len 1, flags 0 21: gpio@21, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 6: i2c@ff020000->i2c-mux@75->i2c@0 Bus 7: i2c@ff020000->i2c-mux@75->i2c@1 Bus 8: i2c@ff020000->i2c-mux@75->i2c@2 Bus 5: i2c@ff030000 (active 5) 74: i2c-mux@74, offset len 1, flags 0 75: i2c-mux@75, offset len 1, flags 0 Bus 9: i2c@ff030000->i2c-mux@74->i2c@0 (active 9) 54: eeprom@54, offset len 1, flags 0 Bus 10: i2c@ff030000->i2c-mux@74->i2c@1 Bus 11: i2c@ff030000->i2c-mux@74->i2c@2 Bus 12: i2c@ff030000->i2c-mux@74->i2c@3 Bus 13: i2c@ff030000->i2c-mux@74->i2c@4 Bus 14: i2c@ff030000->i2c-mux@75->i2c@0 Bus 15: i2c@ff030000->i2c-mux@75->i2c@1 Bus 16: i2c@ff030000->i2c-mux@75->i2c@2 Bus 17: i2c@ff030000->i2c-mux@75->i2c@3 Bus 18: i2c@ff030000->i2c-mux@75->i2c@4 Bus 19: i2c@ff030000->i2c-mux@75->i2c@5 Bus 20: i2c@ff030000->i2c-mux@75->i2c@6 Bus 21: i2c@ff030000->i2c-mux@75->i2c@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
participants (3)
-
Michael Walle -
Michal Simek -
Simon Glass