Messages in this thread | | | Subject | Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses | From | Srinivas Kandagatla <> | Date | Fri, 19 Jun 2020 10:22:17 +0100 |
| |
On 18/06/2020 18:25, Doug Anderson wrote: > Hi, > > On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <sboyd@kernel.org> 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 >
| |