[PATCH v3] dm: uclass: don't assign aliased seq numbers
Michal Simek
michal.simek at xilinx.com
Thu Feb 20 11:14:11 CET 2020
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
More information about the U-Boot
mailing list