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

[Simon Glass]

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.

>
> >
> > 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.

Regards,
Simon