| Message ID | 20210106173900.388758-1-aford173@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC,1/2] dt-bindings: clk: versaclock5: Add load capacitance properties | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | fail | build log |
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".
Hi Adam, On 06/01/21 18:39, 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. Parse the device tree and set the > corresponding registers accordingly. No need to repeat the first 2 sentences, they are already in patch 1. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > drivers/clk/clk-versaclock5.c | 64 +++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index 43db67337bc0..445abc3731fb 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -759,6 +759,63 @@ 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 */ > + if ((femtofarads < 9000) || (femtofarads > 25000)) > + return -EINVAL; > + > + /* The lowest target we can hit is 9430, so exit if it's less */ > + if (femtofarads < 9430) > + return 0; > + > + /* > + * According to VersaClock 6E Programming Guide, there are 6 > + * bits which translate to 64 entries in XTAL registers 12 and > + * 13. Because bits 0 and 1 increase the capacitance the > + * same, some of the values can be repeated. Plugging this > + * into a spreadsheet and generating a trendline, the output > + * equation becomes x = (y-9098.29) / 216.44, where 'y' is > + * the desired capacitance in femtofarads, and x is the value > + * of XTAL[5:0]. > + * To help with rounding, do fixed point math > + */ > + femtofarads *= 100; > + mapped_value = (femtofarads - 909829) / 21644; Thanks for the extensive comment, but I am confused. Not by your code which is very clean and readable, but by the chip documentation (disclaimer: I haven't read it in full depth). The 5P49V6965 datasheet at page 17 clearly states capacitance can be increased in 0.5 pF steps. The "VersaClock 6E Family Register Descriptions and Programming Guide" at page 18 shows a table that allows 0.43 pF. Can you clarify how the thing works? > + > + /* > + * The datasheet states, the maximum capacitance is 25000, > + * but the programmer guide shows a max value is 22832, > + * so values higher values could overflow, so cap it. > + */ The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps leads to 25000. Now I am more confused than before. > + mapped_value = max(mapped_value/100, 0x3f); Uhm, min()? > + > + return mapped_value; > +} > +static int vc5_update_cap_load(struct device_node *node, struct vc5_driver_data *vc5) > +{ > + u32 value, mapped_value; > + > + if (!of_property_read_u32(node, "idt,xtal1-load-femtofarads", &value)) { > + mapped_value = vc5_map_cap_value(value); > + if (mapped_value < 0) > + return mapped_value; > + > + regmap_write(vc5->regmap, VC5_XTAL_X1_LOAD_CAP, (mapped_value << 2)); > + } > + > + if (!of_property_read_u32(node, "idt,xtal2-load-femtofarads", &value)) { > + mapped_value = vc5_map_cap_value(value); > + if (mapped_value < 0) > + return mapped_value; > + regmap_write(vc5->regmap, VC5_XTAL_X2_LOAD_CAP, (mapped_value << 2)); > + } > + > + return 0; > +} > + > static int vc5_update_slew(struct device_node *np_output, > struct vc5_out_data *clk_out) > { > @@ -884,6 +941,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; > Overall LGTM.
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
On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Adam, > > On 06/01/21 18:39, 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. Parse the device tree and set the > > corresponding registers accordingly. > > No need to repeat the first 2 sentences, they are already in patch 1. The reason I did that was because if someone does a git log on the individual file, they'd see the comment. While it's redundant not, it might not be as obvious in the future when looking back. Not everyone reviews the history of the binding, but the source files' git logs usually have some value. However, if you want me to drop it or rephrase it, I can do that. > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > drivers/clk/clk-versaclock5.c | 64 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 64 insertions(+) > > > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > > index 43db67337bc0..445abc3731fb 100644 > > --- a/drivers/clk/clk-versaclock5.c > > +++ b/drivers/clk/clk-versaclock5.c > > @@ -759,6 +759,63 @@ 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 */ > > + if ((femtofarads < 9000) || (femtofarads > 25000)) > > + return -EINVAL; > > + > > + /* The lowest target we can hit is 9430, so exit if it's less */ > > + if (femtofarads < 9430) > > + return 0; > > + > > + /* > > + * According to VersaClock 6E Programming Guide, there are 6 > > + * bits which translate to 64 entries in XTAL registers 12 and > > + * 13. Because bits 0 and 1 increase the capacitance the > > + * same, some of the values can be repeated. Plugging this > > + * into a spreadsheet and generating a trendline, the output > > + * equation becomes x = (y-9098.29) / 216.44, where 'y' is > > + * the desired capacitance in femtofarads, and x is the value > > + * of XTAL[5:0]. > > + * To help with rounding, do fixed point math > > + */ > > + femtofarads *= 100; > > + mapped_value = (femtofarads - 909829) / 21644; > > Thanks for the extensive comment, but I am confused. Not by your code > which is very clean and readable, but by the chip documentation > (disclaimer: I haven't read it in full depth). I was confused too since the datasheet and programmers manual differ a bit. > > The 5P49V6965 datasheet at page 17 clearly states capacitance can be > increased in 0.5 pF steps. The "VersaClock 6E Family Register > Descriptions and Programming Guide" at page 18 shows a table that allows > 0.43 pF. Can you clarify how the thing works? I used the Versaclock 6E doc which is based on the following: BIT 5 - Add 6.92pF BIT 4 - Add 3.46pF BIT 3 - Add 1.73pF BIT 2 - Add 0.86pF Bit 1 - Add 0.43pF Bit 0 - Add 0.43pF Because the Datasheet starts at 9pF, the math I used, assumes these numbers are added to 9pF. Because the datasheet shows the increments are in .5pF increments, the 430nF seems close. The datasheet shows 9pF - 25pF and based on the programmer table, we could get close to 25pF by enabling all bits and adding 9pF, however the math doesn't quite hit 25pF. For what it's worth I needed around 11.5pF, and with this patch, the hardware engineer said our ppm went from around 70 ppm to around 4ppm. > > > + > > + /* > > + * The datasheet states, the maximum capacitance is 25000, > > + * but the programmer guide shows a max value is 22832, > > + * so values higher values could overflow, so cap it. > > + */ > > The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps > leads to 25000. Now I am more confused than before. I agree. It would be nice to get some clarification from Renesas. > > > + mapped_value = max(mapped_value/100, 0x3f); > > Uhm, min()? Oops! You're absolutely right. > > > + > > + return mapped_value; > > +} > > +static int vc5_update_cap_load(struct device_node *node, struct vc5_driver_data *vc5) > > +{ > > + u32 value, mapped_value; > > + > > + if (!of_property_read_u32(node, "idt,xtal1-load-femtofarads", &value)) { > > + mapped_value = vc5_map_cap_value(value); > > + if (mapped_value < 0) > > + return mapped_value; > > + > > + regmap_write(vc5->regmap, VC5_XTAL_X1_LOAD_CAP, (mapped_value << 2)); > > + } > > + > > + if (!of_property_read_u32(node, "idt,xtal2-load-femtofarads", &value)) { > > + mapped_value = vc5_map_cap_value(value); > > + if (mapped_value < 0) > > + return mapped_value; > > + regmap_write(vc5->regmap, VC5_XTAL_X2_LOAD_CAP, (mapped_value << 2)); > > + } > > + > > + return 0; > > +} > > + > > static int vc5_update_slew(struct device_node *np_output, > > struct vc5_out_data *clk_out) > > { > > @@ -884,6 +941,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; > > > > Overall LGTM. Thanks! adam > > -- > Luca
Hi Adam, On 09/01/21 04:00, 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:39, 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. Parse the device tree and set the >>> corresponding registers accordingly. >> >> No need to repeat the first 2 sentences, they are already in patch 1. > > The reason I did that was because if someone does a git log on the > individual file, they'd see the comment. While it's redundant not, it > might not be as obvious in the future when looking back. Not > everyone reviews the history of the binding, but the source files' git > logs usually have some value. However, if you want me to drop it or > rephrase it, I can do that. Makes sense, I had never considered that before. >>> +static int vc5_map_cap_value(u32 femtofarads) >>> +{ >>> + int mapped_value; >>> + >>> + /* The datasheet explicitly states 9000 - 25000 */ >>> + if ((femtofarads < 9000) || (femtofarads > 25000)) >>> + return -EINVAL; >>> + >>> + /* The lowest target we can hit is 9430, so exit if it's less */ >>> + if (femtofarads < 9430) >>> + return 0; >>> + >>> + /* >>> + * According to VersaClock 6E Programming Guide, there are 6 >>> + * bits which translate to 64 entries in XTAL registers 12 and >>> + * 13. Because bits 0 and 1 increase the capacitance the >>> + * same, some of the values can be repeated. Plugging this >>> + * into a spreadsheet and generating a trendline, the output >>> + * equation becomes x = (y-9098.29) / 216.44, where 'y' is >>> + * the desired capacitance in femtofarads, and x is the value >>> + * of XTAL[5:0]. >>> + * To help with rounding, do fixed point math >>> + */ >>> + femtofarads *= 100; >>> + mapped_value = (femtofarads - 909829) / 21644; >> >> Thanks for the extensive comment, but I am confused. Not by your code >> which is very clean and readable, but by the chip documentation >> (disclaimer: I haven't read it in full depth). > > I was confused too since the datasheet and programmers manual differ a bit. >> >> The 5P49V6965 datasheet at page 17 clearly states capacitance can be >> increased in 0.5 pF steps. The "VersaClock 6E Family Register >> Descriptions and Programming Guide" at page 18 shows a table that allows >> 0.43 pF. Can you clarify how the thing works? > > I used the Versaclock 6E doc which is based on the following: > > BIT 5 - Add 6.92pF > BIT 4 - Add 3.46pF > BIT 3 - Add 1.73pF > BIT 2 - Add 0.86pF > Bit 1 - Add 0.43pF > Bit 0 - Add 0.43pF > > Because the Datasheet starts at 9pF, the math I used, assumes these > numbers are added to 9pF. > Because the datasheet shows the increments are in .5pF increments, the > 430nF seems close. The datasheet shows 9pF - 25pF and based on the > programmer table, we could get close to 25pF by enabling all bits and > adding 9pF, however the math doesn't quite hit 25pF. > > For what it's worth I needed around 11.5pF, and with this patch, the > hardware engineer said our ppm went from around 70 ppm to around 4ppm. Did he measure what happens if you set the register according to the 0.5 pF interpretation? Does it improve? I understand the difference is probably olwer than the noise, but who knows. >>> + >>> + /* >>> + * The datasheet states, the maximum capacitance is 25000, >>> + * but the programmer guide shows a max value is 22832, >>> + * so values higher values could overflow, so cap it. >>> + */ >> >> The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps >> leads to 25000. Now I am more confused than before. > > I agree. It would be nice to get some clarification from Renesas. Definitely. Do you have access to some support from them? I don't think I have, but I can ask next week. Regards.
On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Adam, > > On 09/01/21 04:00, 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:39, 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. Parse the device tree and set the > >>> corresponding registers accordingly. > >> > >> No need to repeat the first 2 sentences, they are already in patch 1. > > > > The reason I did that was because if someone does a git log on the > > individual file, they'd see the comment. While it's redundant not, it > > might not be as obvious in the future when looking back. Not > > everyone reviews the history of the binding, but the source files' git > > logs usually have some value. However, if you want me to drop it or > > rephrase it, I can do that. > > Makes sense, I had never considered that before. > > >>> +static int vc5_map_cap_value(u32 femtofarads) > >>> +{ > >>> + int mapped_value; > >>> + > >>> + /* The datasheet explicitly states 9000 - 25000 */ > >>> + if ((femtofarads < 9000) || (femtofarads > 25000)) > >>> + return -EINVAL; > >>> + > >>> + /* The lowest target we can hit is 9430, so exit if it's less */ > >>> + if (femtofarads < 9430) > >>> + return 0; > >>> + > >>> + /* > >>> + * According to VersaClock 6E Programming Guide, there are 6 > >>> + * bits which translate to 64 entries in XTAL registers 12 and > >>> + * 13. Because bits 0 and 1 increase the capacitance the > >>> + * same, some of the values can be repeated. Plugging this > >>> + * into a spreadsheet and generating a trendline, the output > >>> + * equation becomes x = (y-9098.29) / 216.44, where 'y' is > >>> + * the desired capacitance in femtofarads, and x is the value > >>> + * of XTAL[5:0]. > >>> + * To help with rounding, do fixed point math > >>> + */ > >>> + femtofarads *= 100; > >>> + mapped_value = (femtofarads - 909829) / 21644; > >> > >> Thanks for the extensive comment, but I am confused. Not by your code > >> which is very clean and readable, but by the chip documentation > >> (disclaimer: I haven't read it in full depth). > > > > I was confused too since the datasheet and programmers manual differ a bit. > >> > >> The 5P49V6965 datasheet at page 17 clearly states capacitance can be > >> increased in 0.5 pF steps. The "VersaClock 6E Family Register > >> Descriptions and Programming Guide" at page 18 shows a table that allows > >> 0.43 pF. Can you clarify how the thing works? > > > > I used the Versaclock 6E doc which is based on the following: > > > > BIT 5 - Add 6.92pF > > BIT 4 - Add 3.46pF > > BIT 3 - Add 1.73pF > > BIT 2 - Add 0.86pF > > Bit 1 - Add 0.43pF > > Bit 0 - Add 0.43pF > > > > Because the Datasheet starts at 9pF, the math I used, assumes these > > numbers are added to 9pF. > > Because the datasheet shows the increments are in .5pF increments, the > > 430nF seems close. The datasheet shows 9pF - 25pF and based on the > > programmer table, we could get close to 25pF by enabling all bits and > > adding 9pF, however the math doesn't quite hit 25pF. > > > > For what it's worth I needed around 11.5pF, and with this patch, the > > hardware engineer said our ppm went from around 70 ppm to around 4ppm. > > Did he measure what happens if you set the register according to the 0.5 > pF interpretation? Does it improve? I understand the difference is > probably olwer than the noise, but who knows. > > >>> + > >>> + /* > >>> + * The datasheet states, the maximum capacitance is 25000, > >>> + * but the programmer guide shows a max value is 22832, > >>> + * so values higher values could overflow, so cap it. > >>> + */ > >> > >> The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps > >> leads to 25000. Now I am more confused than before. > > > > I agree. It would be nice to get some clarification from Renesas. > > Definitely. Do you have access to some support from them? Luca, We reached out to Renesas with the following questions: 1) I'm seeing a discrepancy between the datasheet and programming guide we have for the Versaclock 6e in regard to the crystal load programming registers. The datasheet for the 5P49V6965A000NLGI indicates a 9pF minimum with 0.5pF steps, while the programming guide says that the lower two register bits both add 0.43pF, which would make the equation: Ci = 9pF + 0.43pF * XTAL[5:1] instead of Ci = 9pF + 0.5pF * XTAL[5:0] as is published in the datasheet. 2) The programming guide shows that the default setting is 01b, but the note says it's reserved, use D1 D0 = 00. Can you confirm that we should set switch mode to 00 instead of the default 01? And we got the following answers: 1) The first one with 0.43pF steps is the correct one. Ci = 9pF + 0.43pF * XTAL[5:1] 0.5pF steps was the design target. When measuring actual silicon, we found 0.43pF steps. There are 6 bits reserved for the CL setting but bits 0 and 1 have the same 0.43pF step. So it is actually 5 bits with an extra LSB of 0.43pF. 2) Please use D1 D0 = 01. The “00” is a typo….. Based on the above response I think we should always assume XTAL bit 0 is 0, and only use XTAL[5:1] which should make the math go easier, because the desired value in femtofarads would just be offset by 9000 and divided by 430 and that value would be shifted 3 places instead fo two, and the fixed-point math calculation can go away. In addition to that, I would also need to make sure that D0 is set to 1, so instead of just writing the shifted XTAL value, I would also have to do a logic OR with 1 to set the low bit. I talked with the hardware guys from work who also suggested that we always write the same value to X1 and X2, so I can remove the X1 and X2 references from the bindings. Does that work for you? adam > I don't think I have, but I can ask next week. > > Regards. > -- > Luca
Hi Adam, On 11/01/21 17:40, Adam Ford wrote: > On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi Adam, >> >> On 09/01/21 04:00, 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:39, 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. Parse the device tree and set the >>>>> corresponding registers accordingly. >>>> >>>> No need to repeat the first 2 sentences, they are already in patch 1. >>> >>> The reason I did that was because if someone does a git log on the >>> individual file, they'd see the comment. While it's redundant not, it >>> might not be as obvious in the future when looking back. Not >>> everyone reviews the history of the binding, but the source files' git >>> logs usually have some value. However, if you want me to drop it or >>> rephrase it, I can do that. >> >> Makes sense, I had never considered that before. >> >>>>> +static int vc5_map_cap_value(u32 femtofarads) >>>>> +{ >>>>> + int mapped_value; >>>>> + >>>>> + /* The datasheet explicitly states 9000 - 25000 */ >>>>> + if ((femtofarads < 9000) || (femtofarads > 25000)) >>>>> + return -EINVAL; >>>>> + >>>>> + /* The lowest target we can hit is 9430, so exit if it's less */ >>>>> + if (femtofarads < 9430) >>>>> + return 0; >>>>> + >>>>> + /* >>>>> + * According to VersaClock 6E Programming Guide, there are 6 >>>>> + * bits which translate to 64 entries in XTAL registers 12 and >>>>> + * 13. Because bits 0 and 1 increase the capacitance the >>>>> + * same, some of the values can be repeated. Plugging this >>>>> + * into a spreadsheet and generating a trendline, the output >>>>> + * equation becomes x = (y-9098.29) / 216.44, where 'y' is >>>>> + * the desired capacitance in femtofarads, and x is the value >>>>> + * of XTAL[5:0]. >>>>> + * To help with rounding, do fixed point math >>>>> + */ >>>>> + femtofarads *= 100; >>>>> + mapped_value = (femtofarads - 909829) / 21644; >>>> >>>> Thanks for the extensive comment, but I am confused. Not by your code >>>> which is very clean and readable, but by the chip documentation >>>> (disclaimer: I haven't read it in full depth). >>> >>> I was confused too since the datasheet and programmers manual differ a bit. >>>> >>>> The 5P49V6965 datasheet at page 17 clearly states capacitance can be >>>> increased in 0.5 pF steps. The "VersaClock 6E Family Register >>>> Descriptions and Programming Guide" at page 18 shows a table that allows >>>> 0.43 pF. Can you clarify how the thing works? >>> >>> I used the Versaclock 6E doc which is based on the following: >>> >>> BIT 5 - Add 6.92pF >>> BIT 4 - Add 3.46pF >>> BIT 3 - Add 1.73pF >>> BIT 2 - Add 0.86pF >>> Bit 1 - Add 0.43pF >>> Bit 0 - Add 0.43pF >>> >>> Because the Datasheet starts at 9pF, the math I used, assumes these >>> numbers are added to 9pF. >>> Because the datasheet shows the increments are in .5pF increments, the >>> 430nF seems close. The datasheet shows 9pF - 25pF and based on the >>> programmer table, we could get close to 25pF by enabling all bits and >>> adding 9pF, however the math doesn't quite hit 25pF. >>> >>> For what it's worth I needed around 11.5pF, and with this patch, the >>> hardware engineer said our ppm went from around 70 ppm to around 4ppm. >> >> Did he measure what happens if you set the register according to the 0.5 >> pF interpretation? Does it improve? I understand the difference is >> probably olwer than the noise, but who knows. >> >>>>> + >>>>> + /* >>>>> + * The datasheet states, the maximum capacitance is 25000, >>>>> + * but the programmer guide shows a max value is 22832, >>>>> + * so values higher values could overflow, so cap it. >>>>> + */ >>>> >>>> The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps >>>> leads to 25000. Now I am more confused than before. >>> >>> I agree. It would be nice to get some clarification from Renesas. >> >> Definitely. Do you have access to some support from them? > > Luca, > > We reached out to Renesas with the following questions: > > 1) > I'm seeing a discrepancy between the datasheet and programming guide > we have for the Versaclock 6e in regard to the crystal load > programming registers. The datasheet for the 5P49V6965A000NLGI > indicates a 9pF minimum with 0.5pF steps, while the programming guide > says that the lower two register bits both add 0.43pF, which would > make the equation: > > Ci = 9pF + 0.43pF * XTAL[5:1] instead of > Ci = 9pF + 0.5pF * XTAL[5:0] as is published in the datasheet. > > 2) The programming guide shows that the default setting is 01b, but > the note says it's reserved, use D1 D0 = 00. Can you confirm that we > should set switch mode to 00 instead of the default 01? > > And we got the following answers: > > 1) > The first one with 0.43pF steps is the correct one. Ci = 9pF + > 0.43pF * XTAL[5:1] > 0.5pF steps was the design target. When measuring actual > silicon, we found 0.43pF steps. > > There are 6 bits reserved for the CL setting but bits 0 and 1 > have the same 0.43pF step. So it is actually 5 bits with an extra LSB > of 0.43pF. > > 2) > Please use D1 D0 = 01. The “00” is a typo….. Great thing you got all those info from Renesas! > > Based on the above response I think we should always assume XTAL bit 0 > is 0, and only use XTAL[5:1] which should make the math go easier, > because the desired value in femtofarads would just be offset by 9000 > and divided by 430 and that value would be shifted 3 places instead fo > two, and the fixed-point math calculation can go away. > > In addition to that, I would also need to make sure that D0 is set to > 1, so instead of just writing the shifted XTAL value, I would also > have to do a logic OR with 1 to set the low bit. > > I talked with the hardware guys from work who also suggested that we > always write the same value to X1 and X2, so I can remove the X1 and > X2 references from the bindings. > > Does that work for you? Yes. We are only losing the ability to set 9 + (0.43 * 32) pF using all bits. I'm OK with that. Should it be needed in the future we can just add it as a special case, maybe just add a comment saying that, like "XTAL[5:0] = b111111 not supported".
On Tue, Jan 12, 2021 at 10:45 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: > > Hi Adam, > > On 11/01/21 17:40, Adam Ford wrote: > > On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: > >> > >> Hi Adam, > >> > >> On 09/01/21 04:00, 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:39, 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. Parse the device tree and set the > >>>>> corresponding registers accordingly. > >>>> > >>>> No need to repeat the first 2 sentences, they are already in patch 1. > >>> > >>> The reason I did that was because if someone does a git log on the > >>> individual file, they'd see the comment. While it's redundant not, it > >>> might not be as obvious in the future when looking back. Not > >>> everyone reviews the history of the binding, but the source files' git > >>> logs usually have some value. However, if you want me to drop it or > >>> rephrase it, I can do that. > >> > >> Makes sense, I had never considered that before. > >> > >>>>> +static int vc5_map_cap_value(u32 femtofarads) > >>>>> +{ > >>>>> + int mapped_value; > >>>>> + > >>>>> + /* The datasheet explicitly states 9000 - 25000 */ > >>>>> + if ((femtofarads < 9000) || (femtofarads > 25000)) > >>>>> + return -EINVAL; > >>>>> + > >>>>> + /* The lowest target we can hit is 9430, so exit if it's less */ > >>>>> + if (femtofarads < 9430) > >>>>> + return 0; > >>>>> + > >>>>> + /* > >>>>> + * According to VersaClock 6E Programming Guide, there are 6 > >>>>> + * bits which translate to 64 entries in XTAL registers 12 and > >>>>> + * 13. Because bits 0 and 1 increase the capacitance the > >>>>> + * same, some of the values can be repeated. Plugging this > >>>>> + * into a spreadsheet and generating a trendline, the output > >>>>> + * equation becomes x = (y-9098.29) / 216.44, where 'y' is > >>>>> + * the desired capacitance in femtofarads, and x is the value > >>>>> + * of XTAL[5:0]. > >>>>> + * To help with rounding, do fixed point math > >>>>> + */ > >>>>> + femtofarads *= 100; > >>>>> + mapped_value = (femtofarads - 909829) / 21644; > >>>> > >>>> Thanks for the extensive comment, but I am confused. Not by your code > >>>> which is very clean and readable, but by the chip documentation > >>>> (disclaimer: I haven't read it in full depth). > >>> > >>> I was confused too since the datasheet and programmers manual differ a bit. > >>>> > >>>> The 5P49V6965 datasheet at page 17 clearly states capacitance can be > >>>> increased in 0.5 pF steps. The "VersaClock 6E Family Register > >>>> Descriptions and Programming Guide" at page 18 shows a table that allows > >>>> 0.43 pF. Can you clarify how the thing works? > >>> > >>> I used the Versaclock 6E doc which is based on the following: > >>> > >>> BIT 5 - Add 6.92pF > >>> BIT 4 - Add 3.46pF > >>> BIT 3 - Add 1.73pF > >>> BIT 2 - Add 0.86pF > >>> Bit 1 - Add 0.43pF > >>> Bit 0 - Add 0.43pF > >>> > >>> Because the Datasheet starts at 9pF, the math I used, assumes these > >>> numbers are added to 9pF. > >>> Because the datasheet shows the increments are in .5pF increments, the > >>> 430nF seems close. The datasheet shows 9pF - 25pF and based on the > >>> programmer table, we could get close to 25pF by enabling all bits and > >>> adding 9pF, however the math doesn't quite hit 25pF. > >>> > >>> For what it's worth I needed around 11.5pF, and with this patch, the > >>> hardware engineer said our ppm went from around 70 ppm to around 4ppm. > >> > >> Did he measure what happens if you set the register according to the 0.5 > >> pF interpretation? Does it improve? I understand the difference is > >> probably olwer than the noise, but who knows. > >> > >>>>> + > >>>>> + /* > >>>>> + * The datasheet states, the maximum capacitance is 25000, > >>>>> + * but the programmer guide shows a max value is 22832, > >>>>> + * so values higher values could overflow, so cap it. > >>>>> + */ > >>>> > >>>> The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps > >>>> leads to 25000. Now I am more confused than before. > >>> > >>> I agree. It would be nice to get some clarification from Renesas. > >> > >> Definitely. Do you have access to some support from them? > > > > Luca, > > > > We reached out to Renesas with the following questions: > > > > 1) > > I'm seeing a discrepancy between the datasheet and programming guide > > we have for the Versaclock 6e in regard to the crystal load > > programming registers. The datasheet for the 5P49V6965A000NLGI > > indicates a 9pF minimum with 0.5pF steps, while the programming guide > > says that the lower two register bits both add 0.43pF, which would > > make the equation: > > > > Ci = 9pF + 0.43pF * XTAL[5:1] instead of > > Ci = 9pF + 0.5pF * XTAL[5:0] as is published in the datasheet. > > > > 2) The programming guide shows that the default setting is 01b, but > > the note says it's reserved, use D1 D0 = 00. Can you confirm that we > > should set switch mode to 00 instead of the default 01? > > > > And we got the following answers: > > > > 1) > > The first one with 0.43pF steps is the correct one. Ci = 9pF + > > 0.43pF * XTAL[5:1] > > 0.5pF steps was the design target. When measuring actual > > silicon, we found 0.43pF steps. > > > > There are 6 bits reserved for the CL setting but bits 0 and 1 > > have the same 0.43pF step. So it is actually 5 bits with an extra LSB > > of 0.43pF. > > > > 2) > > Please use D1 D0 = 01. The “00” is a typo….. > > Great thing you got all those info from Renesas! > > > > > Based on the above response I think we should always assume XTAL bit 0 > > is 0, and only use XTAL[5:1] which should make the math go easier, > > because the desired value in femtofarads would just be offset by 9000 > > and divided by 430 and that value would be shifted 3 places instead fo > > two, and the fixed-point math calculation can go away. > > > > In addition to that, I would also need to make sure that D0 is set to > > 1, so instead of just writing the shifted XTAL value, I would also > > have to do a logic OR with 1 to set the low bit. > > > > I talked with the hardware guys from work who also suggested that we > > always write the same value to X1 and X2, so I can remove the X1 and > > X2 references from the bindings. > > > > Does that work for you? > > Yes. > > We are only losing the ability to set 9 + (0.43 * 32) pF using all bits. We'd be doing this with XTAL[5:1] which mathematically makes more sense. > I'm OK with that. Should it be needed in the future we can just add it > as a special case, maybe just add a comment saying that, like "XTAL[5:0] > = b111111 not supported". XTAL[0] won't be supported at all by my updated algorithm, not just b111111 adam > > -- > Luca
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 >
Quoting Adam Ford (2021-01-06 09:39:00) > 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@gmail.com> > --- > drivers/clk/clk-versaclock5.c | 64 +++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index 43db67337bc0..445abc3731fb 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -759,6 +759,63 @@ 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 */ > + if ((femtofarads < 9000) || (femtofarads > 25000)) Please remove useless parenthesis. > + return -EINVAL; > + > + /* The lowest target we can hit is 9430, so exit if it's less */ > + if (femtofarads < 9430) > + return 0; > + > + /* > + * According to VersaClock 6E Programming Guide, there are 6 > + * bits which translate to 64 entries in XTAL registers 12 and > + * 13. Because bits 0 and 1 increase the capacitance the > + * same, some of the values can be repeated. Plugging this > + * into a spreadsheet and generating a trendline, the output > + * equation becomes x = (y-9098.29) / 216.44, where 'y' is > + * the desired capacitance in femtofarads, and x is the value > + * of XTAL[5:0]. > + * To help with rounding, do fixed point math > + */ > + femtofarads *= 100; > + mapped_value = (femtofarads - 909829) / 21644; > + > + /* > + * The datasheet states, the maximum capacitance is 25000, > + * but the programmer guide shows a max value is 22832, > + * so values higher values could overflow, so cap it. > + */ > + mapped_value = max(mapped_value/100, 0x3f); > + > + return mapped_value; > +} > +static int vc5_update_cap_load(struct device_node *node, struct vc5_driver_data *vc5) > +{ > + u32 value, mapped_value; > + > + if (!of_property_read_u32(node, "idt,xtal1-load-femtofarads", &value)) { > + mapped_value = vc5_map_cap_value(value); > + if (mapped_value < 0) How can it be less than 0? It's unsigned. > + return mapped_value; > + > + regmap_write(vc5->regmap, VC5_XTAL_X1_LOAD_CAP, (mapped_value << 2)); > + } > + > + if (!of_property_read_u32(node, "idt,xtal2-load-femtofarads", &value)) { > + mapped_value = vc5_map_cap_value(value); > + if (mapped_value < 0) Same! > + return mapped_value; > + regmap_write(vc5->regmap, VC5_XTAL_X2_LOAD_CAP, (mapped_value << 2)); > + } > + > + return 0; > +} > + > static int vc5_update_slew(struct device_node *np_output, > struct vc5_out_data *clk_out) > {
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 > >
Hi Adam, On 12/01/21 18:00, Adam Ford wrote: > On Tue, Jan 12, 2021 at 10:45 AM Luca Ceresoli <luca@lucaceresoli.net> wrote: >> >> Hi Adam, >> >> On 11/01/21 17:40, Adam Ford wrote: >>> On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@lucaceresoli.net> wrote: >>>> >>>> Hi Adam, >>>> >>>> On 09/01/21 04:00, 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:39, 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. Parse the device tree and set the >>>>>>> corresponding registers accordingly. >>>>>> >>>>>> No need to repeat the first 2 sentences, they are already in patch 1. >>>>> >>>>> The reason I did that was because if someone does a git log on the >>>>> individual file, they'd see the comment. While it's redundant not, it >>>>> might not be as obvious in the future when looking back. Not >>>>> everyone reviews the history of the binding, but the source files' git >>>>> logs usually have some value. However, if you want me to drop it or >>>>> rephrase it, I can do that. >>>> >>>> Makes sense, I had never considered that before. >>>> >>>>>>> +static int vc5_map_cap_value(u32 femtofarads) >>>>>>> +{ >>>>>>> + int mapped_value; >>>>>>> + >>>>>>> + /* The datasheet explicitly states 9000 - 25000 */ >>>>>>> + if ((femtofarads < 9000) || (femtofarads > 25000)) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + /* The lowest target we can hit is 9430, so exit if it's less */ >>>>>>> + if (femtofarads < 9430) >>>>>>> + return 0; >>>>>>> + >>>>>>> + /* >>>>>>> + * According to VersaClock 6E Programming Guide, there are 6 >>>>>>> + * bits which translate to 64 entries in XTAL registers 12 and >>>>>>> + * 13. Because bits 0 and 1 increase the capacitance the >>>>>>> + * same, some of the values can be repeated. Plugging this >>>>>>> + * into a spreadsheet and generating a trendline, the output >>>>>>> + * equation becomes x = (y-9098.29) / 216.44, where 'y' is >>>>>>> + * the desired capacitance in femtofarads, and x is the value >>>>>>> + * of XTAL[5:0]. >>>>>>> + * To help with rounding, do fixed point math >>>>>>> + */ >>>>>>> + femtofarads *= 100; >>>>>>> + mapped_value = (femtofarads - 909829) / 21644; >>>>>> >>>>>> Thanks for the extensive comment, but I am confused. Not by your code >>>>>> which is very clean and readable, but by the chip documentation >>>>>> (disclaimer: I haven't read it in full depth). >>>>> >>>>> I was confused too since the datasheet and programmers manual differ a bit. >>>>>> >>>>>> The 5P49V6965 datasheet at page 17 clearly states capacitance can be >>>>>> increased in 0.5 pF steps. The "VersaClock 6E Family Register >>>>>> Descriptions and Programming Guide" at page 18 shows a table that allows >>>>>> 0.43 pF. Can you clarify how the thing works? >>>>> >>>>> I used the Versaclock 6E doc which is based on the following: >>>>> >>>>> BIT 5 - Add 6.92pF >>>>> BIT 4 - Add 3.46pF >>>>> BIT 3 - Add 1.73pF >>>>> BIT 2 - Add 0.86pF >>>>> Bit 1 - Add 0.43pF >>>>> Bit 0 - Add 0.43pF >>>>> >>>>> Because the Datasheet starts at 9pF, the math I used, assumes these >>>>> numbers are added to 9pF. >>>>> Because the datasheet shows the increments are in .5pF increments, the >>>>> 430nF seems close. The datasheet shows 9pF - 25pF and based on the >>>>> programmer table, we could get close to 25pF by enabling all bits and >>>>> adding 9pF, however the math doesn't quite hit 25pF. >>>>> >>>>> For what it's worth I needed around 11.5pF, and with this patch, the >>>>> hardware engineer said our ppm went from around 70 ppm to around 4ppm. >>>> >>>> Did he measure what happens if you set the register according to the 0.5 >>>> pF interpretation? Does it improve? I understand the difference is >>>> probably olwer than the noise, but who knows. >>>> >>>>>>> + >>>>>>> + /* >>>>>>> + * The datasheet states, the maximum capacitance is 25000, >>>>>>> + * but the programmer guide shows a max value is 22832, >>>>>>> + * so values higher values could overflow, so cap it. >>>>>>> + */ >>>>>> >>>>>> The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps >>>>>> leads to 25000. Now I am more confused than before. >>>>> >>>>> I agree. It would be nice to get some clarification from Renesas. >>>> >>>> Definitely. Do you have access to some support from them? >>> >>> Luca, >>> >>> We reached out to Renesas with the following questions: >>> >>> 1) >>> I'm seeing a discrepancy between the datasheet and programming guide >>> we have for the Versaclock 6e in regard to the crystal load >>> programming registers. The datasheet for the 5P49V6965A000NLGI >>> indicates a 9pF minimum with 0.5pF steps, while the programming guide >>> says that the lower two register bits both add 0.43pF, which would >>> make the equation: >>> >>> Ci = 9pF + 0.43pF * XTAL[5:1] instead of >>> Ci = 9pF + 0.5pF * XTAL[5:0] as is published in the datasheet. >>> >>> 2) The programming guide shows that the default setting is 01b, but >>> the note says it's reserved, use D1 D0 = 00. Can you confirm that we >>> should set switch mode to 00 instead of the default 01? >>> >>> And we got the following answers: >>> >>> 1) >>> The first one with 0.43pF steps is the correct one. Ci = 9pF + >>> 0.43pF * XTAL[5:1] >>> 0.5pF steps was the design target. When measuring actual >>> silicon, we found 0.43pF steps. >>> >>> There are 6 bits reserved for the CL setting but bits 0 and 1 >>> have the same 0.43pF step. So it is actually 5 bits with an extra LSB >>> of 0.43pF. >>> >>> 2) >>> Please use D1 D0 = 01. The “00” is a typo….. >> >> Great thing you got all those info from Renesas! >> >>> >>> Based on the above response I think we should always assume XTAL bit 0 >>> is 0, and only use XTAL[5:1] which should make the math go easier, >>> because the desired value in femtofarads would just be offset by 9000 >>> and divided by 430 and that value would be shifted 3 places instead fo >>> two, and the fixed-point math calculation can go away. >>> >>> In addition to that, I would also need to make sure that D0 is set to >>> 1, so instead of just writing the shifted XTAL value, I would also >>> have to do a logic OR with 1 to set the low bit. >>> >>> I talked with the hardware guys from work who also suggested that we >>> always write the same value to X1 and X2, so I can remove the X1 and >>> X2 references from the bindings. >>> >>> Does that work for you? >> >> Yes. >> >> We are only losing the ability to set 9 + (0.43 * 32) pF using all bits. > > We'd be doing this with XTAL[5:1] which mathematically makes more sense. > >> I'm OK with that. Should it be needed in the future we can just add it >> as a special case, maybe just add a comment saying that, like "XTAL[5:0] >> = b111111 not supported". > > XTAL[0] won't be supported at all by my updated algorithm, not just b111111 I think we are saying the same thing in two different ways. According to table 28 of the "Regiter description and programming guide", we have: Ci = 9 + (0.43 * N) pF where N can be any integer between 0 and 32 (inclusive). Your proposed implementation is: reg_0x12 = (N << 3) | 0x1; i.e. using only XTAL bits [5:1], not [0], so we can have any value in the range [0..31], but not 32. This is OK for me. Should we need value 32 at some point we can simply augment it as: if (N == 32) reg_0x12 = (b111111 << 2) | 0x1; else reg_0x12 = (N << 3) | 0x1; // your current proposal Among all register XTAL values having XTAL[0] == 1, only b111111 is interesting. All other values have an equivalent with XTAL[0] == 0. Correct?
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
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.
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
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(+)