Hi, On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote: > > > > On 18/06/2020 14:48, Doug Anderson wrote: > > Hi, > > > > On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla > > <srinivas.kandagatla@xxxxxxxxxx> wrote: > >> > >> +Adding SBoyd. > >> > >> On 17/06/2020 18:22, Doug Anderson wrote: > >>> Hi, > >>> > >>> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla > >>> <srinivas.kandagatla@xxxxxxxxxx> wrote: > >>>> > >>>> > >>>> > >>>> On 17/06/2020 15:51, Douglas Anderson wrote: > >>>>> From: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> > >>>>> > >>>>> On some systems it's possible to actually blow the fuses in the qfprom > >>>>> from the kernel. Add properties to support that. > >>>>> > >>>>> NOTE: Whether this is possible depends on the BIOS settings and > >>>>> whether the kernel has permissions here, so not all boards will be > >>>>> able to blow fuses in the kernel. > >>>>> > >>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> > >>>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> Changes in v3: > >>>>> - Add an extra reg range (at 0x6000 offset for SoCs checked) > >>>>> - Define two options for reg: 1 item or 4 items. > >>>>> - No reg-names. > >>>>> - Add "clocks" and "clock-names" to list of properties. > >>>>> - Clock is now "sec", not "secclk". > >>>>> - Add "vcc-supply" to list of properties. > >>>>> - Fixed up example. > >>>>> > >>>>> .../bindings/nvmem/qcom,qfprom.yaml | 45 ++++++++++++++++++- > >>>>> 1 file changed, 43 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > >>>>> index 5efa5e7c4d81..b195212c6193 100644 > >>>>> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > >>>>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > >>>>> @@ -17,8 +17,27 @@ properties: > >>>>> const: qcom,qfprom > >>>>> > >>>>> reg: > >>>>> - items: > >>>>> - - description: The corrected region. > >>>>> + # If the QFPROM is read-only OS image then only the corrected region > >>>>> + # needs to be provided. If the QFPROM is writable then all 4 regions > >>>>> + # must be provided. > >>>>> + oneOf: > >>>>> + - items: > >>>>> + - description: The corrected region. > >>>>> + - items: > >>>>> + - description: The corrected region. > >>>>> + - description: The raw region. > >>>>> + - description: The config region. > >>>>> + - description: The security control region. > >>>>> + > >>>>> + # Clock must be provided if QFPROM is writable from the OS image. > >>>>> + clocks: > >>>>> + maxItems: 1 > >>>> > >>>> > >>>>> + clock-names: > >>>>> + const: sec > >>>> > >>>> Do we need clock-names for just one clock here? > >>> > >>> I think technically you can get by without, but convention is that > >>> clock-names are always provided for clocks. It's talked about in the > >>> same link I sent that talked about reg-names: > >>> > >>> https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@xxxxxxxxxxxxxx/ > >>> > >> > >> TBH, This is total confusion!!! > >> > >> when to use "*-names" Device tree bindings is totally depended on Linux > >> Subsystem interfaces! > >> > >> And what is the starting point to draw this line? > > > > Definitely confusing and mostly because the dts stuff grew organically > > for a while there. It does feel like Rob is pretty clear on the > > current state of things and the policy in the link I provided, though. > > > > > >>> Specifically, Rob said: > >>> > >>>> That probably is because the clock binding has had clock-names from > >>>> the start (it may have been the first one). That was probably partly > >>>> due to the clock API also was mainly by name already if we want to > >>>> admit Linux influence on bindings > >>> > >>> Basically the standard way for getting clocks in Linux is > >>> clk_get(name). With just one clock you can call clk_get(NULL) and I > >>> believe that works, but when you add the 2nd clock then you have to > >>> switch APIs to one of the less-commonly-used variants. > >> > >> In previous NON-DT life clk_get api name argument comes from the clk > >> names that clk provider registered the clocks with. > >> > >> If I remember this correctly, the name that is refereed here for > >> clk_get() is old clkdev api based on clk_lookups and is not the same as > >> clk-names that we have in Device tree. Atleast in this case! > >> > >> clk-names has two objectives in DT: > >> 1> To find the index of the clock in the clocks DT property. > >> > >> 2> If actual clk name is specified then if "1" fails then name could > >> potentially fallback to use old clkdev based clk_lookups. > >> > >> In this specific case we have "sec" as clock-names which is totally used > >> for indexing into clocks property and it can not be used for (2) as > >> there is no clk named "sec" registered by any of the clk providers. > >> > >> So this does not justify the reasoning why "clock-names" should be used > >> while "reg-names" should not be used!. Both of them are going to be > >> finally used for indexing into there respective properties. > > > > Right, you just have to accept the fact that logic doesn't come into > > play here. For clocks, always use "clk-names" but also always use a > > consistent order (which is now more enforced by the schema checker). > > For "reg" almost never use "reg-names". > > > > 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. I won't say it's impossible to get by without clock-names when you have more than one clock, but (almost) nobody does it. It's hard to quickly come up with a good way to search for this, but skimming through: git grep -C5 'clocks.*,' -- arch/arm64/boot/dts ...you don't find too many examples of no clock-names and you find _lots_ of examples where clock-names are specified. One example that _does_ have multiple clocks and doesn't specify clock-names is "simple-framebuffer". Looking at the Linux driver you can see they have to use the special "of_clk_get()" variant to handle it. -Doug
