[PATCH v3] dm: uclass: don't assign aliased seq numbers
Michael Walle
michael at walle.cc
Thu Feb 20 21:16:52 CET 2020
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
More information about the U-Boot
mailing list