[v3,2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

Commit Message

Doug Anderson June 17, 2020, 2:51 p.m. UTC

From: Ravi Kumar Bokka <[e-mail]>

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 <[e-mail]>
Signed-off-by: Douglas Anderson <[e-mail]>
---

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(-)

Comments

Srinivas Kandagatla June 17, 2020, 3:19 p.m. UTC | #1

On 17/06/2020 15:51, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Douglas Anderson <[e-mail]>
> ---
> 
> 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?

> +
> +  # Supply reference must be provided if QFPROM is writable from the OS image.
> +  vcc-supply:
> +    description: Our power supply.
>   
>     # Needed if any child nodes are present.
>     "#address-cells":
> @@ -31,6 +50,28 @@ required:
>      - reg
>   
>   examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    efuse@784000 {
> +      compatible = "qcom,qfprom";
> +      reg = <0 0x00784000 0 0x8ff>,
> +            <0 0x00780000 0 0x7a0>,
> +            <0 0x00782000 0 0x100>,
> +            <0 0x00786000 0 0x1fff>;
> +      clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> +      clock-names = "sec";
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      vcc-supply = <&vreg_l11a_1p8>;
> +
> +      hstx-trim-primary@25b {
> +        reg = <0x25b 0x1>;
> +        bits = <1 3>;
> +      };
> +    };
> +
>     - |
>       efuse@784000 {
>         compatible = "qcom,qfprom";
>

Doug Anderson June 17, 2020, 5:22 p.m. UTC | #2

Hi,

On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
<[e-mail]> wrote:
>
>
>
> On 17/06/2020 15:51, Douglas Anderson wrote:
> > From: Ravi Kumar Bokka <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > ---
> >
> > 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+=[e-mail]/

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.

-Doug

Srinivas Kandagatla June 18, 2020, 10:10 a.m. UTC | #3

+Adding SBoyd.

On 17/06/2020 18:22, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> <[e-mail]> wrote:
>>
>>
>>
>> On 17/06/2020 15:51, Douglas Anderson wrote:
>>> From: Ravi Kumar Bokka <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Douglas Anderson <[e-mail]>
>>> ---
>>>
>>> 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+=[e-mail]/
> 

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?


> 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.

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!


Thanks,
srini
> 
> -Doug
>

Doug Anderson June 18, 2020, 1:48 p.m. UTC | #4

Hi,

On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
<[e-mail]> wrote:
>
> +Adding SBoyd.
>
> On 17/06/2020 18:22, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> > <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 17/06/2020 15:51, Douglas Anderson wrote:
> >>> From: Ravi Kumar Bokka <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Signed-off-by: Douglas Anderson <[e-mail]>
> >>> ---
> >>>
> >>> 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+=[e-mail]/
> >
>
> 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".


> 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".


I will put posting v4 of this patch on pause until this is resolved to
avoid fragmenting the discussion.


-Doug

Srinivas Kandagatla June 18, 2020, 2:01 p.m. UTC | #5

On 18/06/2020 14:48, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
> <[e-mail]> wrote:
>>
>> +Adding SBoyd.
>>
>> On 17/06/2020 18:22, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
>>> <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 17/06/2020 15:51, Douglas Anderson wrote:
>>>>> From: Ravi Kumar Bokka <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Douglas Anderson <[e-mail]>
>>>>> ---
>>>>>
>>>>> 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+=[e-mail]/
>>>
>>
>> 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
>

Doug Anderson June 18, 2020, 3:32 p.m. UTC | #6

Hi,

On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla
<[e-mail]> wrote:
>
>
>
> On 18/06/2020 14:48, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
> > <[e-mail]> wrote:
> >>
> >> +Adding SBoyd.
> >>
> >> On 17/06/2020 18:22, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> >>> <[e-mail]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 17/06/2020 15:51, Douglas Anderson wrote:
> >>>>> From: Ravi Kumar Bokka <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> Signed-off-by: Douglas Anderson <[e-mail]>
> >>>>> ---
> >>>>>
> >>>>> 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+=[e-mail]/
> >>>
> >>
> >> 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

Doug Anderson June 18, 2020, 5:25 p.m. UTC | #7

Hi,

On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <[e-mail]> 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.


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

Srinivas Kandagatla June 19, 2020, 9:22 a.m. UTC | #8

On 18/06/2020 18:25, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <[e-mail]> 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
>

Patch

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
+
+  # Supply reference must be provided if QFPROM is writable from the OS image.
+  vcc-supply:
+    description: Our power supply.
 
   # Needed if any child nodes are present.
   "#address-cells":
@@ -31,6 +50,28 @@  required:
    - reg
 
 examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    efuse@784000 {
+      compatible = "qcom,qfprom";
+      reg = <0 0x00784000 0 0x8ff>,
+            <0 0x00780000 0 0x7a0>,
+            <0 0x00782000 0 0x100>,
+            <0 0x00786000 0 0x1fff>;
+      clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+      clock-names = "sec";
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      vcc-supply = <&vreg_l11a_1p8>;
+
+      hstx-trim-primary@25b {
+        reg = <0x25b 0x1>;
+        bits = <1 3>;
+      };
+    };
+
   - |
     efuse@784000 {
       compatible = "qcom,qfprom";