Re: [PATCH 2/2] clk: vc5: Add support for optional load capacitance

From: Adam Ford
Date: Mon Jan 18 2021 - 10:30:59 EST


On Sat, Jan 16, 2021 at 3:55 PM Adam Ford <aford173@xxxxxxxxx> wrote:
>
> 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 <aford173@xxxxxxxxx>
>
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index 43db67337bc0..224118ca08fd 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -759,6 +759,72 @@ 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 lowest target we can hit is 9430, so exit if it's less */
> + if (femtofarads < 9430)
> + return 0;
> +
> + /*
> + * 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 - 9430, 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;
> +
> + 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;
> +
> + /*
> + * According to Renesas, bits [1:0] of VC5_XTAL_X1_LOAD_CAP
> + * and VC5_XTAL_X2_LOAD_CAP should always be 01b.
> + * Since the mapped_value is really the high 6 bits of 8,
> + * shift the value 2 places and or in the 0x01;
> + */
> +
> + mapped_value = (mapped_value << 2) | 0x01;
> + regmap_write(vc5->regmap, VC5_XTAL_X1_LOAD_CAP, mapped_value);
> + regmap_write(vc5->regmap, VC5_XTAL_X2_LOAD_CAP, mapped_value);

On second thought I'm going to change register write to a
read-modify-write since the low two bits are unclear and X1 and X2 low
bits are not exactly the same. Since the info is confusing, I can
cache VC5_XTAL_X1_LOAD_CAP, clear the upper 6 bits and then logic-or
the value. This way we don't have to guess about what the 0x01. It
also appears the 0x01 is only for one of the registers and not both.


> + }
> +
> + return 0;
> +}
> +
> static int vc5_update_slew(struct device_node *np_output,
> struct vc5_out_data *clk_out)
> {
> @@ -884,6 +950,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;
> --
> 2.25.1
>