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 <mich...@walle.cc> >>> Reviewed-by: Alex Marginean <alexandru.margin...@nxp.com> >>> Tested-by: Alex Marginean <alexandru.margin...@nxp.com> >>> Acked-by: Vladimir Oltean <olte...@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.si...@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