2023-02-13 09:51:17

by Javier Carrasco

[permalink] [raw]
Subject: [PATCH 0/2] pcf85363: support for quartz-load-femtofarads

These patches add support for the quartz-load-femtofarads property in
the pcf85363 device driver and a description of the property's possible
values.

The driver has been tested with a PCF85263ATT RTC and a CTS3-32.768-12.5-20
oscillator that needs a 12.5 pF load capacitor. With no property
support the 7 pF default value leads to at least 2 Hz output frequency
deviations, while setting the right value the deviation decreased to
0.15 Hz. These measurements were made with a high precision oscilloscope
(SIGLENT SDS5104X).

This modification does not affect existing designs where the
quartz-load-femtofarads is not defined because in that case the default
value is used.

Javier Carrasco (2):
rtc: pcf85363: add support for the quartz-load-femtofarads property
dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for
pcf85263 and pcf85363

.../devicetree/bindings/rtc/nxp,pcf8563.yaml | 20 ++++++++--
drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++-
2 files changed, 53 insertions(+), 4 deletions(-)

--
2.37.2


Javier Carrasco
Research and Development

Wolfvision GmbH
Oberes Ried 14 | 6833 Klaus | Austria
Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>

Website: wolfvision.com <http://www.wolfvision.com>
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria



2023-02-13 09:51:22

by Javier Carrasco

[permalink] [raw]
Subject: [PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
the oscillator control register (address 25h).

Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c05b722f0060..941f9264cf0a 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -101,6 +101,10 @@
#define PIN_IO_INTA_OUT 2
#define PIN_IO_INTA_HIZ 3

+#define OSC_CAP_SEL GENMASK(1, 0)
+#define OSC_CAP_6000 0x01
+#define OSC_CAP_12500 0x02
+
#define STOP_EN_STOP BIT(0)

#define RESET_CPR 0xa4
@@ -117,6 +121,32 @@ struct pcf85x63_config {
unsigned int num_nvram;
};

+static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
+{
+ u32 load = 7000;
+ u8 value = 0;
+
+ of_property_read_u32(node, "quartz-load-femtofarads", &load);
+
+ switch (load) {
+ default:
+ dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
+ load);
+ fallthrough;
+ case 7000:
+ break;
+ case 6000:
+ value |= OSC_CAP_6000;
+ break;
+ case 12500:
+ value |= OSC_CAP_12500;
+ break;
+ }
+
+ return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
+ OSC_CAP_SEL, value);
+}
+
static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
{
struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
@@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
.reg_write = pcf85363_nvram_write,
},
};
- int ret, i;
+ int ret, i, err;

if (data)
config = data;
@@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
if (IS_ERR(pcf85363->rtc))
return PTR_ERR(pcf85363->rtc);

+ err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);
+ if (err < 0)
+ dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
+ err);
+
pcf85363->rtc->ops = &rtc_ops;
pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
--
2.37.2


Javier Carrasco
Research and Development

Wolfvision GmbH
Oberes Ried 14 | 6833 Klaus | Austria
Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>

Website: wolfvision.com <http://www.wolfvision.com>
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria


2023-02-13 09:51:26

by Javier Carrasco

[permalink] [raw]
Subject: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

These RTCs are handled by the pcf85363 device driver, which now supports
the quartz-load-femtofarads property.

Signed-off-by: Javier Carrasco <[email protected]>
---
.../devicetree/bindings/rtc/nxp,pcf8563.yaml | 20 ++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
index a98b72752349..aac7f7565ba7 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
@@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
maintainers:
- Alexandre Belloni <[email protected]>

-allOf:
- - $ref: rtc.yaml#
-
properties:
compatible:
enum:
@@ -37,6 +34,23 @@ properties:
start-year: true
wakeup-source: true

+allOf:
+ - $ref: rtc.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - nxp,pcf85263
+ - nxp,pcf85363
+ then:
+ properties:
+ quartz-load-femtofarads:
+ description:
+ The capacitive load of the quartz(x-tal).
+ enum: [6000, 7000, 12500]
+ default: 7000
+
required:
- compatible
- reg
--
2.37.2


Javier Carrasco
Research and Development

Wolfvision GmbH
Oberes Ried 14 | 6833 Klaus | Austria
Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>

Website: wolfvision.com <http://www.wolfvision.com>
Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria


2023-02-13 12:24:34

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

On 13/02/2023 10:50, Javier Carrasco wrote:
> The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
> be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
> the oscillator control register (address 25h).
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c05b722f0060..941f9264cf0a 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -101,6 +101,10 @@
> #define PIN_IO_INTA_OUT 2
> #define PIN_IO_INTA_HIZ 3
>
> +#define OSC_CAP_SEL GENMASK(1, 0)
> +#define OSC_CAP_6000 0x01
> +#define OSC_CAP_12500 0x02
> +
> #define STOP_EN_STOP BIT(0)
>
> #define RESET_CPR 0xa4
> @@ -117,6 +121,32 @@ struct pcf85x63_config {
> unsigned int num_nvram;
> };
>
> +static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
> +{
> + u32 load = 7000;
> + u8 value = 0;
> +
> + of_property_read_u32(node, "quartz-load-femtofarads", &load);
> +
> + switch (load) {
> + default:
> + dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
> + load);
> + fallthrough;
> + case 7000:
> + break;
> + case 6000:
> + value |= OSC_CAP_6000;
> + break;
> + case 12500:
> + value |= OSC_CAP_12500;
> + break;
> + }
> +
> + return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
> + OSC_CAP_SEL, value);
> +}
> +
> static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
> struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> @@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
> .reg_write = pcf85363_nvram_write,
> },
> };
> - int ret, i;
> + int ret, i, err;
>
> if (data)
> config = data;
> @@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
> if (IS_ERR(pcf85363->rtc))
> return PTR_ERR(pcf85363->rtc);
>
> + err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);

Aren't you updating it for all variants? But the property is marked as
not valid for them.

Best regards,
Krzysztof


2023-02-13 12:25:47

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

On 13/02/2023 10:50, Javier Carrasco wrote:
> These RTCs are handled by the pcf85363 device driver, which now supports
> the quartz-load-femtofarads property.
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> .../devicetree/bindings/rtc/nxp,pcf8563.yaml | 20 ++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> index a98b72752349..aac7f7565ba7 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
> maintainers:
> - Alexandre Belloni <[email protected]>
>
> -allOf:
> - - $ref: rtc.yaml#
> -
> properties:
> compatible:
> enum:
> @@ -37,6 +34,23 @@ properties:
> start-year: true
> wakeup-source: true
>
> +allOf:
> + - $ref: rtc.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - nxp,pcf85263
> + - nxp,pcf85363
> + then:
> + properties:
> + quartz-load-femtofarads:
> + description:
> + The capacitive load of the quartz(x-tal).
> + enum: [6000, 7000, 12500]
> + default: 7000

I don't think this will work. If you tested your DTS, you would see errors.

Please define the property in top-level and disallow it for other
variants. Other way to make it working would be to switch to
unevaluatedProperties, but defining properties in allOf:if:then: is not
that readable.

Best regards,
Krzysztof


2023-02-13 17:03:51

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH 1/2] rtc: pcf85363: add support for the quartz-load-femtofarads property

On 13/02/2023 10:50:17+0100, Javier Carrasco wrote:
> The quartz oscillator load capacitance of the PCF85263 and PCF85363 can
> be adjusted to 6 pF, 7 pF (default) and 12.5 pF with the CL[1:0] bits in
> the oscillator control register (address 25h).
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> drivers/rtc/rtc-pcf85363.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c05b722f0060..941f9264cf0a 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -101,6 +101,10 @@
> #define PIN_IO_INTA_OUT 2
> #define PIN_IO_INTA_HIZ 3
>
> +#define OSC_CAP_SEL GENMASK(1, 0)
> +#define OSC_CAP_6000 0x01
> +#define OSC_CAP_12500 0x02
> +
> #define STOP_EN_STOP BIT(0)
>
> #define RESET_CPR 0xa4
> @@ -117,6 +121,32 @@ struct pcf85x63_config {
> unsigned int num_nvram;
> };
>
> +static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
> +{
> + u32 load = 7000;
> + u8 value = 0;
> +
> + of_property_read_u32(node, "quartz-load-femtofarads", &load);
> +
> + switch (load) {
> + default:
> + dev_warn(&pcf85363->rtc->dev, "Unknown quartz-load-femtofarads value: %d. Assuming 7000",
> + load);
> + fallthrough;
> + case 7000:
> + break;
> + case 6000:
> + value |= OSC_CAP_6000;

Why are you using the |= operator?

> + break;
> + case 12500:
> + value |= OSC_CAP_12500;
> + break;
> + }
> +
> + return regmap_update_bits(pcf85363->regmap, CTRL_OSCILLATOR,
> + OSC_CAP_SEL, value);
> +}
> +
> static int pcf85363_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
> struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> @@ -372,7 +402,7 @@ static int pcf85363_probe(struct i2c_client *client)
> .reg_write = pcf85363_nvram_write,
> },
> };
> - int ret, i;
> + int ret, i, err;
>
> if (data)
> config = data;
> @@ -394,6 +424,11 @@ static int pcf85363_probe(struct i2c_client *client)
> if (IS_ERR(pcf85363->rtc))
> return PTR_ERR(pcf85363->rtc);
>
> + err = pcf85363_load_capacitance(pcf85363, client->dev.of_node);
> + if (err < 0)
> + dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
> + err);
> +
> pcf85363->rtc->ops = &rtc_ops;
> pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
> --
> 2.37.2
>
>
> Javier Carrasco
> Research and Development
>
> Wolfvision GmbH
> Oberes Ried 14 | 6833 Klaus | Austria
> Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>
>
> Website: wolfvision.com <http://www.wolfvision.com>
> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
>

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-02-13 17:09:12

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

Hello,

Krzysztof's confusion is because you are changing the binding for
nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363

On 13/02/2023 10:50:18+0100, Javier Carrasco wrote:
> These RTCs are handled by the pcf85363 device driver, which now supports
> the quartz-load-femtofarads property.
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> .../devicetree/bindings/rtc/nxp,pcf8563.yaml | 20 ++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> index a98b72752349..aac7f7565ba7 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
> maintainers:
> - Alexandre Belloni <[email protected]>
>
> -allOf:
> - - $ref: rtc.yaml#
> -
> properties:
> compatible:
> enum:
> @@ -37,6 +34,23 @@ properties:
> start-year: true
> wakeup-source: true
>
> +allOf:
> + - $ref: rtc.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - nxp,pcf85263
> + - nxp,pcf85363
> + then:
> + properties:
> + quartz-load-femtofarads:
> + description:
> + The capacitive load of the quartz(x-tal).
> + enum: [6000, 7000, 12500]
> + default: 7000
> +
> required:
> - compatible
> - reg
> --
> 2.37.2
>
>
> Javier Carrasco
> Research and Development
>
> Wolfvision GmbH
> Oberes Ried 14 | 6833 Klaus | Austria
> Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>
>
> Website: wolfvision.com <http://www.wolfvision.com>
> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
>

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-02-14 11:41:16

by Javier Carrasco

[permalink] [raw]
Subject: Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

Hello,

Sorry for the previous mail in html format with annoying signatures.

> Hello,
>
> Krzysztof's confusion is because you are changing the binding for
> nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363

If that is ok I would propose a new bindings file nxp,pcf85363 for
pcf85263 and pcf85363 in the next version.

Best regards,

Javier Carrasco

>
> On 13/02/2023 10:50:18+0100, Javier Carrasco wrote:
>> These RTCs are handled by the pcf85363 device driver, which now supports
>> the quartz-load-femtofarads property.
>>
>> Signed-off-by: Javier Carrasco <[email protected]>
>> ---
>> .../devicetree/bindings/rtc/nxp,pcf8563.yaml | 20 ++++++++++++++++---
>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> index a98b72752349..aac7f7565ba7 100644
>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8563.yaml
>> @@ -9,9 +9,6 @@ title: Philips PCF8563/Epson RTC8564 Real Time Clock
>> maintainers:
>> - Alexandre Belloni <[email protected]>
>>
>> -allOf:
>> - - $ref: rtc.yaml#
>> -
>> properties:
>> compatible:
>> enum:
>> @@ -37,6 +34,23 @@ properties:
>> start-year: true
>> wakeup-source: true
>>
>> +allOf:
>> + - $ref: rtc.yaml#
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - nxp,pcf85263
>> + - nxp,pcf85363
>> + then:
>> + properties:
>> + quartz-load-femtofarads:
>> + description:
>> + The capacitive load of the quartz(x-tal).
>> + enum: [6000, 7000, 12500]
>> + default: 7000
>> +
>> required:
>> - compatible
>> - reg
>> --
>> 2.37.2
>>
>>
>> Javier Carrasco
>> Research and Development
>>
>> Wolfvision GmbH
>> Oberes Ried 14 | 6833 Klaus | Austria
>> Tel: +43 5523 52250 <tel:+43552352250> | Mail: [email protected] <mailto:[email protected]>
>>
>> Website: wolfvision.com <http://www.wolfvision.com>
>> Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
>>
>


2023-02-14 12:11:15

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/2] dt-bindings: rtc: nxp,pcf8563: add quartz-load-femtofarads for pcf85263 and pcf85363

On 14/02/2023 12:41, Javier Carrasco Cruz wrote:
> Hello,
>
> Sorry for the previous mail in html format with annoying signatures.
>
>> Hello,
>>
>> Krzysztof's confusion is because you are changing the binding for
>> nxp,pcf8563 while adding support for the nxp,pcf85263/nxp,pcf85363
>
> If that is ok I would propose a new bindings file nxp,pcf85363 for
> pcf85263 and pcf85363 in the next version.

Could be or still could be fixed in this binding. Indeed my comment in
the driver came from confusion that there are two drivers but one
binding. Having two bindings might be simpler and more readable in such
case.

Best regards,
Krzysztof