[RFC,1/2] dt-bindings: clk: versaclock5: Add load capacitance properties
diff mbox series

Message ID 20210106173900.388758-1-aford173@gmail.com
State New, archived
Headers show
Series
  • [RFC,1/2] dt-bindings: clk: versaclock5: Add load capacitance properties
Related show

Commit Message

Adam Ford Jan. 6, 2021, 5:38 p.m. UTC
There are two registers which can set the load capacitance for
XTAL1 and XTAL2. These are optional registers when using an
external crystal.  Update the bindings to support them.

Signed-off-by: Adam Ford <aford173@gmail.com>
---
 .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Luca Ceresoli Jan. 8, 2021, 10:49 p.m. UTC | #1
Hi Adam,

On 06/01/21 18:38, Adam Ford wrote:
> There are two registers which can set the load capacitance for
> XTAL1 and XTAL2. These are optional registers when using an
> external crystal.  Update the bindings to support them.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 2ac1131fd922..e5e55ffb266e 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -59,6 +59,18 @@ properties:
>      minItems: 1
>      maxItems: 2
>  
> +  idt,xtal1-load-femtofarads:

I wonder whether we should have a common, vendor independent property.
In mainline we have xtal-load-pf (ti,cdce925.txt bindings) which has no
vendor prefix. However I don't know how much common it is to need
different loads for x1 and x2. Any hardware engineer around?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 9000
> +    maximum: 25000
> +    description: Optional loading capacitor for XTAL1

Nit: I think the common wording is "load capacitor", not "loading
capacitor".
Adam Ford Jan. 9, 2021, 2:48 a.m. UTC | #2
On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>
> Hi Adam,
>
> On 06/01/21 18:38, Adam Ford wrote:
> > There are two registers which can set the load capacitance for
> > XTAL1 and XTAL2. These are optional registers when using an
> > external crystal.  Update the bindings to support them.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > index 2ac1131fd922..e5e55ffb266e 100644
> > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > @@ -59,6 +59,18 @@ properties:
> >      minItems: 1
> >      maxItems: 2
> >
> > +  idt,xtal1-load-femtofarads:
>
> I wonder whether we should have a common, vendor independent property.

That would be nice.

> In mainline we have xtal-load-pf (ti,cdce925.txt bindings) which has no
> vendor prefix. However I don't know how much common it is to need

rtc-pcf85063.c uses  quartz-load-femtofarads, so there is already some
discrepancy.

Since the unit of measure here is femtofarads, using pF in the name seems wrong.
We need to read the data as a u32, so femtofarads works better than
pF, which would require a decimal point.

> different loads for x1 and x2. Any hardware engineer around?

I talked to a hardware engineer where I work, and he said it makes
sense to keep them the same.  I only separated them because there are
two registers, and I assumed there might be a reason to have X1 and X2
be different, but I'm ok with reading one value and writing it to two
different registers.

adam
>
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 9000
> > +    maximum: 25000
> > +    description: Optional loading capacitor for XTAL1
>
> Nit: I think the common wording is "load capacitor", not "loading
> capacitor".
>
> --
> Luca
Rob Herring Jan. 13, 2021, 3:16 a.m. UTC | #3
On Wed, Jan 06, 2021 at 11:38:59AM -0600, Adam Ford wrote:
> There are two registers which can set the load capacitance for
> XTAL1 and XTAL2. These are optional registers when using an
> external crystal.  Update the bindings to support them.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> index 2ac1131fd922..e5e55ffb266e 100644
> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> @@ -59,6 +59,18 @@ properties:
>      minItems: 1
>      maxItems: 2
>  
> +  idt,xtal1-load-femtofarads:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Already has a type, so you can drop the $ref.

> +    minimum: 9000
> +    maximum: 25000
> +    description: Optional loading capacitor for XTAL1
> +
> +  idt,xtal2-load-femtofarads:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 9000
> +    maximum: 25000
> +    description: Optional loading capacitor for XTAL2
> +
>  patternProperties:
>    "^OUT[1-4]$":
>      type: object
> -- 
> 2.25.1
>
Adam Ford Jan. 13, 2021, 12:31 p.m. UTC | #4
On Tue, Jan 12, 2021 at 9:16 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jan 06, 2021 at 11:38:59AM -0600, Adam Ford wrote:
> > There are two registers which can set the load capacitance for
> > XTAL1 and XTAL2. These are optional registers when using an
> > external crystal.  Update the bindings to support them.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > index 2ac1131fd922..e5e55ffb266e 100644
> > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > @@ -59,6 +59,18 @@ properties:
> >      minItems: 1
> >      maxItems: 2
> >
> > +  idt,xtal1-load-femtofarads:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> Already has a type, so you can drop the $ref.
>
> > +    minimum: 9000
> > +    maximum: 25000

Luca,

Do you want the range to the 9000 - 25000 per the datasheet, or should
I use the max value based on the programmer guide?  Currently, my
intent was to cap the value to 11111b, so anyone who writes 23000,
24000, or 25000 will all be the same value based on the feedback I got
from Renesas.

adam
> > +    description: Optional loading capacitor for XTAL1
> > +
> > +  idt,xtal2-load-femtofarads:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 9000
> > +    maximum: 25000
> > +    description: Optional loading capacitor for XTAL2
> > +
> >  patternProperties:
> >    "^OUT[1-4]$":
> >      type: object
> > --
> > 2.25.1
> >
Luca Ceresoli Jan. 13, 2021, 2:36 p.m. UTC | #5
Hi Adam,

On 13/01/21 13:31, Adam Ford wrote:
> On Tue, Jan 12, 2021 at 9:16 PM Rob Herring <robh@kernel.org> wrote:
>>
>> On Wed, Jan 06, 2021 at 11:38:59AM -0600, Adam Ford wrote:
>>> There are two registers which can set the load capacitance for
>>> XTAL1 and XTAL2. These are optional registers when using an
>>> external crystal.  Update the bindings to support them.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> ---
>>>  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> index 2ac1131fd922..e5e55ffb266e 100644
>>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> @@ -59,6 +59,18 @@ properties:
>>>      minItems: 1
>>>      maxItems: 2
>>>
>>> +  idt,xtal1-load-femtofarads:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Already has a type, so you can drop the $ref.
>>
>>> +    minimum: 9000
>>> +    maximum: 25000
> 
> Luca,
> 
> Do you want the range to the 9000 - 25000 per the datasheet, or should
> I use the max value based on the programmer guide?  Currently, my
> intent was to cap the value to 11111b, so anyone who writes 23000,
> 24000, or 25000 will all be the same value based on the feedback I got
> from Renesas.

DT should describe the HW, so I'd use the same range that can be set in
hardware, regardless of driver support. Thus it should be:

9000 - [9000 + 430 * 32] = 9000 - 22760
Luca Ceresoli Jan. 13, 2021, 2:39 p.m. UTC | #6
Hi Adam,

On 09/01/21 03:48, Adam Ford wrote:
> On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli <luca@lucaceresoli.net> wrote:
>>
>> Hi Adam,
>>
>> On 06/01/21 18:38, Adam Ford wrote:
>>> There are two registers which can set the load capacitance for
>>> XTAL1 and XTAL2. These are optional registers when using an
>>> external crystal.  Update the bindings to support them.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> ---
>>>  .../devicetree/bindings/clock/idt,versaclock5.yaml   | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> index 2ac1131fd922..e5e55ffb266e 100644
>>> --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
>>> @@ -59,6 +59,18 @@ properties:
>>>      minItems: 1
>>>      maxItems: 2
>>>
>>> +  idt,xtal1-load-femtofarads:
>>
>> I wonder whether we should have a common, vendor independent property.
> 
> That would be nice.
> 
>> In mainline we have xtal-load-pf (ti,cdce925.txt bindings) which has no
>> vendor prefix. However I don't know how much common it is to need
> 
> rtc-pcf85063.c uses  quartz-load-femtofarads, so there is already some
> discrepancy.
> 
> Since the unit of measure here is femtofarads, using pF in the name seems wrong.
> We need to read the data as a u32, so femtofarads works better than
> pF, which would require a decimal point.
> 
>> different loads for x1 and x2. Any hardware engineer around?
> 
> I talked to a hardware engineer where I work, and he said it makes
> sense to keep them the same.  I only separated them because there are
> two registers, and I assumed there might be a reason to have X1 and X2
> be different, but I'm ok with reading one value and writing it to two
> different registers.

If both your HW engineer and the Renesas docs say setting different
values is not useful in real life, and other drivers don't set different
values as well, it looks like that is the reasonable way. I think it
also increases likelihood of establishing a unique property name to be
used for all future chips.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
index 2ac1131fd922..e5e55ffb266e 100644
--- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
+++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
@@ -59,6 +59,18 @@  properties:
     minItems: 1
     maxItems: 2
 
+  idt,xtal1-load-femtofarads:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 9000
+    maximum: 25000
+    description: Optional loading capacitor for XTAL1
+
+  idt,xtal2-load-femtofarads:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 9000
+    maximum: 25000
+    description: Optional loading capacitor for XTAL2
+
 patternProperties:
   "^OUT[1-4]$":
     type: object