Message-ID: <6394b7e46a7967f4a5a6f77215e670e3e7a7ef10.camel@baylibre.com>
Date:   Mon, 10 Jun 2019 11:37:16 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Stephen Boyd <sboyd@...nel.org>,
        Alexandre Mergnat <amergnat@...libre.com>,
        mturquette@...libre.com
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        baylibre-upstreaming@...ups.io
Subject: Re: [PATCH] clk: fix clock global name usage.

On Thu, 2019-06-06 at 15:54 -0700, Stephen Boyd wrote:
> > > > Do you think we could come up with a priority order which makes the first
> > > > example work ?
> > > 
> > > Maybe? I'm open to suggestions.
> > > 
> > > > Something like:
> > > > 
> > > > if (hw) {
> > > >          /* use pointer */
> > > > } else if (name) {
> > > >          /* use legacy global names */
> > > 
> > > I don't imagine we can get rid of legacy name for a long time, so this
> > > can't be in this order. Otherwise we'll try to lookup the legacy name
> > > before trying the DT lookup and suffer performance hits doing a big
> > > global search while also skipping the DT lookup that we want drivers to
> > > use if they're more modern.
> > 
> > You'll try to look up the legacy name only if it is defined, in which case you
> > know you this is what you want to use, so I don't see the penalty.  Unless ...
> 
> We'll only try the legacy name if all else fails. We're hoping that
> .fw_name or .hw is used instead because those are faster than string
> comparisons on the entire tree.
> 
> > Are trying to support case where multiple fields among hw, name, fw_name, index
> > would be defined simultaneously ??
> 
> Yes.

Then this where we have different views ont he matter. I think this makes the
logic a lot more complex.

A controller should *know* if a clock is coming from 'somewhere else' and how.
I think we should not try multiple methods hoping one will eventually work.

I has already caught us now and I'm willing to bet we won't be the last.

> 
> > IMO, it would be weird and very confusing.
> 
> Let's expand the table.
> 
>     .fw_name   |  .index |  .name      | DT lookup? | fallback to legacy? 
>    +-----------+---------+-------------+----+----------------------------
>  1 | NULL      |    >= 0 |  NULL       |    Y       |         N
>  2 | NULL      |    -1   |  NULL       |    N       |         N
>  3 | non-NULL  |    -1   |  NULL       |    Y       |         N
>  4 | non-NULL  |    >= 0 |  NULL       |    Y       |         N
>  5 | NULL      |    >= 0 |  non-NULL   |    Y       |         Y
>  6 | NULL      |    -1   |  non-NULL   |    N       |         Y
>  7 | non-NULL  |    -1   |  non-NULL   |    Y       |         Y
>  8 | non-NULL  |    >= 0 |  non-NULL   |    Y       |         Y
> 
> And then if .hw is set we just don't even try to use the above table.
> 
> You want case #5 to skip the DT lookup because .fw_name is NULL, but the
> index is 0. I stared at this for a few minutes and I think you're
> arguing that we should only do the DT lookup when index is 0 if we can't
> fallback to a legacy lookup.
> 
> Initially that sounds OK, but then we'll get stuck with legacy lookups
> forever if someone doesn't put a matching .fw_name into the
> 'clock-names' property. We don't want that to happen. We want to get off
> legacy and into the new world where either .fw_name or .index is used to
> specify a parent.
> 
> From my perspective you're suffering from static initializers setting
> everything to 0 and that making the index 0 and "valid" for a DT lookup
> hitting up against not wanting to set anything in the structure
> unnecessarily. While at the same time, it's great that .fw_name is set
> to NULL by the static initializer, so it's a case of wanting the same
> thing to do different things.

Sure ... but we are also suffering from the complexity of the logic behind this
(and the table above)

When I define .hw, I don't need to initialize the rest but when I define .name I
must define (at least) .index to a specific value ... This is at least tricky,
and IMO, inconsistent.

> 
> If we would have made it so the DT index started at 1 then this wouldn't
> be a problem, but that's not possible. Or if we would have made a new
> clk flag that said CLK_USE_INDEX_FOR_DT then it could have gotten over
> this problem but that's basically the same as making the inverse set of
> drivers use -1 for the index to indicate they don't want a DT lookup.
> 
> I still believe the large majority of clk drivers will either use .hw or
> .fw_name to find parents, while falling back to the .name for legacy
> reasons. The .index field won't see much use, but we'll support it for
> the firmwares out there that don't use "clock-names". Similarly, we'll
> have only a handful of drivers that only want to specify .name and
> nothing else, and those few drivers can use .index = -1 to overcome
> this.

I'm sorry, I still find all this very confusing just to add a fallback
mechanism. It would be simpler to ask people to choose the method they.

The transition to DT can be done submitting DT part first and submitting the
clock driver part later on

Anyway, we'll define index to -1 since this how it is supposed to be. Thx for
your explanation

Powered by blists - more mailing lists