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

[Michal Simek]

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.

> 
> 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";
        };


Thanks,
Michal