On 18/06/2020 18:25, Doug Anderson wrote:
Hi,
On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
Quoting Doug Anderson (2020-06-18 08:32:20)
Hi,
On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla
On the other note:
clock-names are not mandatory according to
Documentation/devicetree/bindings/clock/clock-bindings.txt
For this particular case where clock-names = "sec" is totally used for
indexing and nothing else!
So I guess in the one-clock case it's more optional and if you feel
strongly I'll get rid of clk-names here. ...but if we ever need
another clock we probably will want to add it back and (I could be
corrected) I believe it's convention to specify clk-names even with
one clock.
TL;DR: I suggest you call this "core" if you want to keep the
clock-name, or just drop it if there's only one clk and move on.
Ah, true. "core" sounds good.
It's not required to have clock-names with one clk, and indeed it's not
required to have clock-names at all. The multi clk scenario is a little
more difficult to handle because historically the clk_get() API has been
name based and not index based like platform resources. When there is
one clk the driver can pass NULL as the 'con_id' argument to clk_get()
and it will do the right thing. And when you have more than one clk you
can pass NULL still and get the first clk, that should be in the same
index, and then other clks by name.
So far nobody has added clk_get_by_index() but I suppose if it was
important the API could be added. Working with only legacy clkdev
lookups would fail of course, but clock-names could be fully deprecated
and kernel images may be smaller because we're not storing piles of
strings and doing string comparisons. Given that it's been this way for
a long time and we have DT schema checking it doesn't seem very
important to mandate anything one way or the other though. I certainly
don't feel good when I see of_clk_*() APIs being used by platform
drivers, but sometimes it is required.
To really put this into perspective, consider the fact that most drivers
have code that figures out what clk names to look for and then they pile
them into arrays and just turn them all on and off together. Providing
fine grained clk control here is a gigantic waste of time, and requiring
clock-names is just more hoops that driver authors feel they have to
jump through for $reasons. We have clk_bulk_get_all() for this, but that
doesn't solve the one rate changing clk among the sea of clk gates
problem. In general, driver authors don't care and we should probably be
providing a richer while simpler API to them that manages power state of
some handful of clks, regulators, and power domains for a device while
also letting them control various knobs like clk rate when necessary.
BTW, on qcom platforms they usually name clks "core" and "iface" for the
core clk and the interface clk used to access the registers of a device.
Sometimes there are esoteric ones like "axi". In theory this cuts down
on the number of strings the kernel keeps around but I like that it
helps provide continuity across drivers and DTs for their SoCs. If you
ask the hardware engineer what the clk name is for the hardware block
they'll tell you the globally unique clk name like
"gcc_qupv3_uart9_core_clk", which is the worst name to use.
OK, sounds about what I expected. I suppose the path of least
resistance would be to just drop clock-names. I guess I'm just
worried that down the road someone will want to specify the "iface"
clock too. If that ever happens, we're stuck with these options:
1. Be the first ones to require adding clk_get_by_index().
2. Use the frowned upon of_clk_get() API which allows getting by index.
3. Get the first clock with clk_get(NULL) and the second clock with
clk_get("iface") and figure out how to specify this happily in the
yaml.
If we just define clock-names now then we pretty much match the
pattern of everyone else.
Thanks to Stephen Boyd for his inputs and directions here!
I guess we have to live with "clock-names" for now for both consistency
and reasons detailed by Boyd.
Am okay with the clock-names!
--srini
Srinivas: reading all that if you still want me to drop clock-names
then I will. I'll use clk_get(NULL) to get the clock and if/when we
ever need an "iface" clock (maybe we never will?) we can figure it out
then.
-Doug
