[PATCH 00/27] dm: Change the way sequence numbers are implemented

Fri Dec 11 08:42:06 CET 2020

On 12/11/20 8:28 AM, Michal Simek wrote:
> Hi Simon,
>
> On 10. 12. 20 18:46, Simon Glass wrote:
>> Hi Michal,
>>
>> On Thu, 10 Dec 2020 at 10:33, Michal Simek <michal.simek at xilinx.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 10. 12. 20 18:27, Simon Glass wrote:
>>>> Hi Michal,
>>>>
>>>> On Thu, 10 Dec 2020 at 00:34, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 09. 12. 20 17:30, Michael Walle wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> Am 2020-12-09 17:23, schrieb Simon Glass:
>>>>>>> On Tue, 8 Dec 2020 at 15:52, Michael Walle <michael at walle.cc> wrote:
>>>>>>>> Am 2020-11-30 02:53, schrieb Simon Glass:
>>>>>>>>> At present each device has two sequence numbers, with 'req_seq' being
>>>>>>>>> set up at bind time and 'seq' at probe time. The idea is that devices
>>>>>>>>> can 'request' a sequence number and then the conflicts are resolved
>>>>>>>>> when
>>>>>>>>> the device is probed.
>>>>>>>>>
>>>>>>>>> This makes things complicated in a few cases, since we don't really
>>>>>>>>> know
>>>>>>>>> (at bind time) what the sequence number will end up being. We want to
>>>>>>>>> honour the bind-time requests if at all possible, but in fact the only
>>>>>>>>> source of these at present is the devicetree aliases.
>>>>>>>>>
>>>>>>>>> Apart from the obvious need for sequence numbers to supports U-Boot's
>>>>>>>>> numbering on devices on the command line, the current scheme was
>>>>>>>>> designed to:
>>>>>>>>>
>>>>>>>>> - avoid calculating the sequence number until it is needed, to save
>>>>>>>>>    execution time
>>>>>>>>> - allow multiple devices to obtain a particular sequence number as
>>>>>>>> they
>>>>>>>>>    are probed and removed
>>>>>>>>> - retain a record of the 'requested' sequence number even if it turns
>>>>>>>>> out
>>>>>>>>>    that a device could not get it (to allow debugging and retrying)
>>>>>>>>>
>>>>>>>>> After some years using the current scheme it seems on balance that
>>>>>>>>> these
>>>>>>>>> goals don't have as much merit as first thought. The first point would
>>>>>>>>> be persuasive except that we end up reading the devicetree aliases at
>>>>>>>>> bind-time anyway. So the work of resolving the sequence numbers during
>>>>>>>>> probing is not that great. The second point hasn't really been an
>>>>>>>>> issue,
>>>>>>>>> as there is typically no contention for sequence numbers (boards tend
>>>>>>>>> to
>>>>>>>>> allocate them statically in the devicetree). Re the third point, we
>>>>>>>> can
>>>>>>>>> often figure out what was requested by looking at aliases, and in the
>>>>>>>>> cases where we can't, it doesn't seem to matter much.
>>>>>>>>>
>>>>>>>>> Since we have the devicetree available at bind time, we may as well
>>>>>>>>> just
>>>>>>>>> use it, in the hope that the required processing will turn out to be
>>>>>>>>> useful later (i.e. the device actually gets used). In addition, it is
>>>>>>>>> simpler to use a single sequence number, since it avoids confusion and
>>>>>>>>> some extra code.
>>>>>>>>>
>>>>>>>>> This series moves U-Boot to use a single, bind-time sequence number.
>>>>>>>>> All
>>>>>>>>> uclasses with the DM_UC_FLAG_SEQ_ALIAS flag enabled will assign
>>>>>>>>> sequence
>>>>>>>>> numbers to their devices, so that as soon as a device is bound, it has
>>>>>>>>> a
>>>>>>>>> sequence number. If a devicetree alias provides the number, it will be
>>>>>>>>> used. Otherwise, during initial binding, the first free number is
>>>>>>>> used.
>>>>>>>>
>>>>>>>> What does "first free number mean"?
>>>>>>>>
>>>>>>>> I have a device tree with the following aliases for network:
>>>>>>>>
>>>>>>>> aliases {
>>>>>>>>           ethernet0 = &enetc0;
>>>>>>>>           ethernet1 = &enetc1;
>>>>>>>>           ethernet2 = &enetc2;
>>>>>>>>           ethernet3 = &enetc6;
>>>>>>>> };
>>>>>>>>
>>>>>>>> The individual devices might be disabled, depending on the board variant
>>>>>>>> (which might also be dynamically determined during startup).
>>>>>>>
>>>>>>> By disabled, do you mean that they are marked 'status = "disabled"'?
>>>>>>
>>>>>> yes
>>>>>>
>>>>>>> If so, then they are ignored by DM and will not claim their number.
>>>>>>>
>>>>>>>>
>>>>>>>> My first smoke test with this series show the following:
>>>>>>>>
>>>>>>>>     uclass 32: eth
>>>>>>>>     0   * enetc-0 @ ffd40e60, seq 0
>>>>>>>>     1   * ax88179_eth @ ffd51f50, seq 1
>>>>>>>>
>>>>>>>> Looks like the usb ethernet device will get seq 1 assigned (after "usb
>>>>>>>> start"). Is this intended?
>>>>>>>>
>>>>>>>> If so, this is a problem, because for ethernet devices, the MAC address
>>>>>>>> is assigned according to the ethNaddr variable. And at least for this
>>>>>>>> board (kontron_sl28) the first four are reserved for the ones with the
>>>>>>>> alias entries. Thus I'd have expected that the usb device will get seq 4
>>>>>>>> assigned.
>>>>>>>
>>>>>>> OK, so you mean after all existing aliases, even if they did not bind.
>>>>>>> I think we can do that.
>>>>>>
>>>>>> Great, that will also match the current behavior. See
>>>>>> be1a6e94254af205bd67d69e3bdb26b161ccd72f ("dm: uclass: don't assign
>>>>>> aliased seq numbers")
>>>>>
>>>>> Also take a look at 83e4c7e9ffa57fe4116967999c223c952a46a78a
>>>>> which is more or less the same things as is done in linux
>>>>> by351d224f64afc1b3b359a1738b7d4600c7e64061
>>>>>
>>>>> And we are using it for i2c subsystem.
>>>>>
>>>>> If you look at Linux kernel i2c/spi subsystems they are using it for
>>>>> quite a while. Recently mmc subsystem starts to use it.
>>>>> On the other hand we had similar discussion around networking and it has
>>>>> never started to be used.
>>>>>
>>>>> In general make sense if you have uclass that it is recorded(based on
>>>>> aliases) the first highest free ID and start to use it for devices which
>>>>> are not listed or don't have record in aliases.
>>>>>
>>>>> That's IMHO the best predictable behavior we could reach. If you care
>>>>> about numbering scheme then your device should have alias.
>>>>>
>>>>> Also if there are devices which doesn't have alias keyword we should
>>>>> work with DT guys to get it listed in the spec.
>>>>
>>>> Do you mean the root of the name (e.g. i2c for i2c1)?
>>>
>>> yes assigned the root of the name to specific uclass.
>>
>> Oh gosh, I didn't even know they were in the DT spec.
>
> In DT spec you have recommended node names which is more or less fit
> with uclasses.
>
>
>>>
>>>>
>>>> Also has there been any discussion of using phandles instead of
>>>> strings? Perhaps we could create a new node with that approach.
>>>
>>> What do you mean by string?
>>> Aliases are not using phandles. It is just label converted to path.
>>> for example from dtc -I dtb -O dts.
>>>
>>>          aliases {
>>>                  ethernet0 = "/amba_pl/ethernet at 40c00000";
>>>                  i2c0 = "/amba_pl/i2c at 40800000";
>>>                  serial0 = "/amba_pl/serial at 40600000";
>>>          };
>>
>> Yes that's my point. It uses strings, which is inefficient. I feel we
>> could use phandles instead and solve a number of problems.
>
> If you want to change it we can't change it in one project only. And
> this discussion should be made against DT specification.
> Another thing is that that Linux is not capable to update aliases when
> for example DT overlay is applied. This part is completely ignored even
> if there is no collision. Nothing important for U-Boot but something to
> keep in mind.
> IIRC We don't support run time overlay applying to be able to add more
> nodes (but would be useful to have it too).

https://lkml.org/lkml/2016/12/20/510
[PATCH 0/4 v2] of/overlay: sysfs based ABI for dt overlays
suggested a mechanism to do so but was not merged.

Best regards

Heinrich