dt-bindings: clk: versaclock5: Add optional load capacitance property
Subsystems (3)
PATCH v2 1/2 dt-bindings: clk: versaclock5: Add optional load capacitance property
There are two registers which can set the load capacitance for XTAL1 and XTAL2. These are optional registers when using an external crystal. Since XTAL1 and XTAL2 will set to the same value, update the binding to support a single property called xtal-load-femtofarads.
---
V2: No Change
Documentation/devicetree/bindings/clock/idt,versaclock5.yaml +6 -0
@@ -59,6 +59,12 @@ properties: minItems: 1 maxItems: 2 + idt,xtal-load-femtofarads: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 9000 + maximum: 22760 + description: Optional load capacitor for XTAL1 and XTAL2 + patternProperties: "^OUT[1-4]$": type: object
Signature · 1 line
2.25.1
PATCH v2 2/2 clk: vc5: Add support for optional load capacitance
There are two registers which can set the load capacitance for XTAL1 and XTAL2. These are optional registers when using an external crystal. Parse the device tree and set the corresponding registers accordingly.
---
V2: Make the math subtract 9000 since we have a DIV_ROUND_CLOSEST
This also allows us to remove the check for 9430 since values between 9000 and 9430 will round up and down.
Make write VC5_XTAL_X1_LOAD_CAP and VC5_XTAL_X2_LOAD_CAP a read-modify-write to not worry about the contents of bits[1:0].
drivers/clk/clk-versaclock5.c +75 -0
@@ -759,6 +759,74 @@ static int vc5_update_power(struct device_node *np_output,return 0 ; } +static int vc5_map_cap_value (u32 femtofarads) +{ +int mapped_value; + +/* +* The datasheet explicitly states 9000 - 25000 with 0.5pF +* steps, but the Programmer's guide shows the steps are 0.430pF. +* After getting feedback from Renesas, the .5pF steps were the +* goal, but 430nF was the actual values. +* Because of this, the actual range goes to 22760 instead of 25000 +*/ +if (femtofarads <9000 || femtofarads >22760 ) +return -EINVAL ; + +/* +* The Programmer's guide shows XTAL[5:0] but in reality, +* XTAL[0] and XTAL[1] are both LSB which makes the math +* strange. With clarfication from Renesas, setting the +* values should be simpler by ignoring XTAL[0] +*/ + + mapped_value =DIV_ROUND_CLOSEST (femtofarads -9000 ,430 ); + +/* +* Since the calculation ignores XTAL[0], there is one +* special case where mapped_value = 32. In reality, this means +* the real mapped value should be 111111b. In other clases, +* the mapped_value needs to be shifted 1 to the left. +*/ + +if (mapped_value >31 ) + mapped_value =0x3f ; +else + mapped_value <<=1 ; + +return mapped_value; +} +static int vc5_update_cap_load (struct device_node *node,struct vc5_driver_data *vc5) +{ +u32 value; +int mapped_value, cache; + +if (!of_property_read_u32 (node,"idt,xtal-load-femtofarads" , &value)) { + mapped_value =vc5_map_cap_value (value); +if (mapped_value <0 ) +return mapped_value; + +/* +* The mapped_value is really the high 6 bits of +* VC5_XTAL_X1_LOAD_CAP and VC5_XTAL_X2_LOAD_CAP, so +* shift the value 2 places. Read each register, wipe out +* the top 6 bits and, write the combined data back. +*/ + + mapped_value = (mapped_value <<2 ); + + cache =regmap_read (vc5->regmap,VC5_XTAL_X1_LOAD_CAP , &cache); + cache &=0x03 ; +regmap_write (vc5->regmap,VC5_XTAL_X1_LOAD_CAP , mapped_value | cache); + + cache =regmap_read (vc5->regmap,VC5_XTAL_X2_LOAD_CAP , &cache); + cache &=0x03 ; +regmap_write (vc5->regmap,VC5_XTAL_X2_LOAD_CAP , mapped_value | cache); + } + +return 0 ; +} +static int vc5_update_slew (struct device_node *np_output,struct vc5_out_data *clk_out) {
@@ -884,6 +952,13 @@ static int vc5_probe(struct i2c_client *client, const struct i2c_device_id *id)return -EINVAL ; } +/* Configure Optional Loading Capacitance for external XTAL */ +if (!(vc5->chip_info->flags &VC5_HAS_INTERNAL_XTAL )) { + ret =vc5_update_cap_load (client->dev.of_node, vc5); +if (ret) + goto err_clk_register; + } + init.name =kasprintf (GFP_KERNEL ,"%pOFn.mux" , client->dev.of_node); init.ops = &vc5_mux_ops; init.flags =0 ;
Signature · 1 line
2.25.1
Luca Ceresoli
maintainerRe: [PATCH V2 2/2] clk: vc5: Add support for optional load capacitance
Hi Adam,
Quoted 39 lines · Adam Ford
There are two registers which can set the load capacitance for XTAL1 and XTAL2. These are optional registers when using an external crystal. Parse the device tree and set the corresponding registers accordingly.
Signed-off-by Adam Ford <[email protected]>---
V2: Make the math subtract 9000 since we have aDIV_ROUND_CLOSEST
This also allows us to remove the check for 9430 since values between 9000 and 9430 will round up and down.
Make writeVC5_XTAL_X1_LOAD_CAPandVC5_XTAL_X2_LOAD_CAPa read-modify-write to not worry about the contents of bits[1:0].drivers/clk/clk-versaclock5.c +17
-0@@ -759,6 +759,74 @@ static int vc5_update_power(struct device_node *np_output,return 0 ; } +static int vc5_map_cap_value (u32 femtofarads) +{ +int mapped_value; + +/* +* The datasheet explicitly states 9000 - 25000 with 0.5pF +* steps, but the Programmer's guide shows the steps are 0.430pF. +* After getting feedback from Renesas, the .5pF steps were the +* goal, but 430nF was the actual values. +* Because of this, the actual range goes to 22760 instead of 25000 +*/ +if (femtofarads <9000 || femtofarads >22760 ) +return -EINVAL ; + +/* +* The Programmer's guide shows XTAL[5:0] but in reality, +* XTAL[0] and XTAL[1] are both LSB which makes the math
drivers/clk/clk-versaclock5.c static int vc5_update_power(struct device_node *np_output, · line 788
Quoted 10 lines
drivers/clk/clk-versaclock5.c +10
-0+* strange. With clarfication from Renesas, setting the +* values should be simpler by ignoring XTAL[0] +*/ + + mapped_value =DIV_ROUND_CLOSEST (femtofarads -9000 ,430 ); + +/* +* Since the calculation ignores XTAL[0], there is one +* special case where mapped_value = 32. In reality, this means +* the real mapped value should be 111111b. In other clases,
s/clases/cases/
Quoted 18 lines
drivers/clk/clk-versaclock5.c +18
-0+* the mapped_value needs to be shifted 1 to the left. +*/ + +if (mapped_value >31 ) + mapped_value =0x3f ; +else + mapped_value <<=1 ; + +return mapped_value; +} +static int vc5_update_cap_load (struct device_node *node,struct vc5_driver_data *vc5) +{ +u32 value; +int mapped_value, cache; + +if (!of_property_read_u32 (node,"idt,xtal-load-femtofarads" , &value)) { + mapped_value =vc5_map_cap_value (value); +if (mapped_value <0 )
drivers/clk/clk-versaclock5.c static int vc5_update_power(struct device_node *np_output, · line 816
Quoted 10 lines
drivers/clk/clk-versaclock5.c +10
-0+return mapped_value; + +/* +* The mapped_value is really the high 6 bits of +* VC5_XTAL_X1_LOAD_CAP and VC5_XTAL_X2_LOAD_CAP, so +* shift the value 2 places. Read each register, wipe out +* the top 6 bits and, write the combined data back. +*/ + + mapped_value = (mapped_value <<2 );
Nit: the comment is about this specific line of code, remove the extra empty line. Also in other places above.
It's incredible how we need more comments than lines of code, but it's deserved as the settings are weird and the docs are fuzzy and contradictory. Thanks for taking care of writing them.
drivers/clk/clk-versaclock5.c static int vc5_update_power(struct device_node *np_output, · line 824
Quoted 8 lines
drivers/clk/clk-versaclock5.c +8
-0+ + cache =regmap_read (vc5->regmap,VC5_XTAL_X1_LOAD_CAP , &cache); + cache &=0x03 ; +regmap_write (vc5->regmap,VC5_XTAL_X1_LOAD_CAP , mapped_value | cache); + + cache =regmap_read (vc5->regmap,VC5_XTAL_X2_LOAD_CAP , &cache); + cache &=0x03 ; +regmap_write (vc5->regmap,VC5_XTAL_X2_LOAD_CAP , mapped_value | cache);
With regmap_update_bits() or some other regmap call you can write each of these in a single statement.
Signature · 1 line
Luca
Luca Ceresoli
maintainerHi Adam,
Quoted 15 lines · Adam Ford
There are two registers which can set the load capacitance for XTAL1 and XTAL2. These are optional registers when using an external crystal. Since XTAL1 and XTAL2 will set to the same value, update the binding to support a single property called xtal-load-femtofarads.
Signed-off-by Adam Ford <[email protected]>---
V2: No ChangeDocumentation/devicetree/bindings/clock/idt,versaclock5.yaml +0
-0
Documentation/devicetree/bindings/clock/idt,versaclock5.yaml properties: · line 63
Quoted 5 lines
Documentation/devicetree/bindings/clock/idt,versaclock5.yaml +2
-0@@ -59,6 +59,12 @@ properties: minItems: 1 maxItems: 2 + idt,xtal-load-femtofarads: + $ref: /schemas/types.yaml#/definitions/uint32
"Vendor specific properties having a standard unit suffix don't need a type." -- Documentation/devicetree/bindings/example-schema.yaml
Overall looks good.
Signature · 1 line
Luca
Quoted 22 lines · Luca Ceresoli
Hi Adam,
Quoted 18 lines · Adam Ford
There are two registers which can set the load capacitance for XTAL1 and XTAL2. These are optional registers when using an external crystal. Since XTAL1 and XTAL2 will set to the same value, update the binding to support a single property called xtal-load-femtofarads.
Signed-off-by Adam Ford <[email protected]>---
V2: No ChangeDocumentation/devicetree/bindings/clock/idt,versaclock5.yaml +0
-0@@ -59,6 +59,12 @@ properties: minItems: 1 maxItems: 2
Quoted 6 lines
Documentation/devicetree/bindings/clock/idt,versaclock5.yaml properties: · line 63
Quoted 2 lines
Documentation/devicetree/bindings/clock/idt,versaclock5.yaml +2
-0+ idt,xtal-load-femtofarads: + $ref: /schemas/types.yaml#/definitions/uint32"Vendor specific properties having a standard unit suffix don't need a type." --
Documentation/devicetree/bindings/example-schema.yaml
I tried to remove the "$ref: /schemas/types.yaml#/definitions/uint32" but when I ran the test to make the yaml files, it threw an error, so I put it back.
adam
Quoted 4 lines
Overall looks good.
--
Luca