Messages in this thread | | | Subject | Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses | From | Srinivas Kandagatla <> | Date | Thu, 18 Jun 2020 15:01:31 +0100 |
| |
On 18/06/2020 14:48, Doug Anderson wrote: > Hi, > > On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> 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@linaro.org> wrote: >>>> >>>> >>>> >>>> On 17/06/2020 15:51, Douglas Anderson wrote: >>>>> From: Ravi Kumar Bokka <rbokka@codeaurora.org> >>>>> >>>>> 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@codeaurora.org> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>>> --- >>>>> >>>>> 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@mail.gmail.com/ >>> >> >> 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!
> >> This also brings in greater confusion for both existing and while adding >> bindings with "*-names" for new interfaces. >> >> Rob, can you please provide some clarity and direction on when to >> use/not-use *-names properties! > > If I had to guess Rob will say that we shouldn't add more places where > the convention is to have "-names".
Confusion is not just about new bindings, but with the existing ones! :-)
--srini > > > I will put posting v4 of this patch on pause until this is resolved to > avoid fragmenting the discussion. > > > -Doug >
| |