Skip to contentSkip to conversation

Add Davicom phy + reset-gpios

PATCH Applied Sam Ravnborg 4 participants 22 messages Aug 22, 2018 – Sep 26, 2018

Subsystems (8)
lore
Share & tools
lore
#0 lore raw

v4 cover Add Davicom phy + reset-gpios

Changes in v4:
- Either register a mdio node or phy child nodes
- Check for valid phy node before registering the phy node This is like the kernel does it
- Also recognize compatible="ethernet-phy-id*" as PHY's again like the kernel, but with a simpler implementation, as we do not try to read the actual phy_id

Changes in v3:
- Consider the v3 a new implmentation - almost nothing was left from v2. Thus also invalidate any former review - sorry.
- Dropped the need for the mdio {} node, as this node is only used when there is dedicated HW support for the mdio.
- Added so PHY child nodes to ethernet nodes are registered, and as part of registration the PHY nodes are reset if the reset-gpios property is present.
- Further dropped that the parsed info is stored in the bus, as we only do reset once, thus there is no need to save the info from the DT
- Futher dropped helper function to reset. They was not needed
- Named the gpio with the name of the PHY node
in the DT. This makes it unique and easier to recognize.

Changes in v2:
- Added patch to enable Davicom PHY on at91sam9263ek - evaluation kit
- Fix so we do reset before comunicating with the PHY
- Rename to mdio_reset()
- Reference correct binding file in commit log (mdio.txt)
- Tested on at91sam9263ek
The at91sam9263ek kit do not require the reset like my proprietary board, so no DT changes required

Intro:
The following patches was necessary to get networking operational on my proprietary target.
The target is at91sam9263 based with a Davicom PHY.

The Davicom PHY is a straight copy form the Linux kernel with the interrupt routine removed and minor adjustments to the rest.

The davicom PHY would not work until it had seen a reset cycle - which I think may be an artifact of the board design.

To fix the reset issue I have implemented support for the reset-gpios binding (see net/phy.txt bindings). A minimal implmentation was done, just enough to get my target running.

I could have implemented something in macb - but I preferred the more generic solution.

Also included are a patch that for the at91sam9263ek evaluation board. The patch adds several extra tools that are usefull for testing, and enable the Davicom PHY.

Sam

Sam Ravnborg (3):
phylib: add Davicom PHY support
phylib: add support for reset-gpios
at91sam9263ek: add PHY, miitool etc. to config

 arch/arm/configs/at91sam9263ek_defconfig |  12 ++-
 drivers/net/phy/Kconfig                  |   5 ++
 drivers/net/phy/Makefile                 |   1 +
 drivers/net/phy/davicom.c                | 140 +++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c               |  97 ++++++++++++++++++++-
 5 files changed, 253 insertions(+), 2 deletions(-)

_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox

#1 lore raw

PATCH v4 1/3 phylib: add Davicom PHY support

Based on driver from Linux kernel 4.18.0-rc4

---
 drivers/net/phy/Kconfig   |   5 ++
 drivers/net/phy/Makefile  |   1 +
 drivers/net/phy/davicom.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 drivers/net/phy/davicom.c
drivers/net/phy/Kconfig +5 -0
index cda752b65..79fb917ee 100644
@@ -18,6 +18,11 @@ config AT803X_PHY
 	---help---
 	  Currently supports the AT8030, AT8031 and AT8035 PHYs.
 
+config DAVICOM_PHY
+	bool "Driver for Davicom PHYs"
+	---help---
+	  Currently supports dm9161e and dm9131
+
 config LXT_PHY
 	bool "Driver for the Intel LXT PHYs"
 	---help---
drivers/net/phy/Makefile +1 -0
index 30b20f8ee..4424054d9 100644
@@ -1,6 +1,7 @@
 obj-y += phy.o mdio_bus.o
 obj-$(CONFIG_AR8327N_PHY)	+= ar8327.o
 obj-$(CONFIG_AT803X_PHY)	+= at803x.o
+obj-$(CONFIG_DAVICOM_PHY)	+= davicom.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
 obj-$(CONFIG_MICREL_PHY)	+= micrel.o
drivers/net/phy/davicom.c new file +140 -0

140 lines not on this page · raw message

Signature · 7 lines
2.12.0


_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
#2 lore raw

PATCH v4 2/3 phylib: add support for reset-gpios

In reply to Sam Ravnborg

Add minimal support for reset-gpios in the PHY node.

Example DT that uses this:

	macb0: ethernet@fffbc000 {
		phy-mode = "rmii";
		#address-cells = <1>;
		#size-cells = <0>;

		ethphy0: ethernet-phy@1 {
			reg = <3>;
			reset-gpios = <&pioE 17 GPIO_ACTIVE_LOW>;
			reset-assert-us = <1000>;
			reset-deassert-us = <2000>;
		};
	};

The reset is required to get the Davicom PHY operational on my proprietary board, and is assumed relevant for other boards too.

The PHY is reset when we read the info from DT,
before the phy_id is retreived.

The bindings are documented in dts/Bindings/net/phy.txt.

A side-effect of this patch is that we may see phy devices created from the DT due to the extra call to:

of_mdiobus_register()

with the ethernet node as argument.

The logic to determine if a child node is a PHY node is a simpler version compared to the one used in the kernel. The kernel have a whitelist of compatible strings that is not included in the barebox version.
They can be added later if needed.

---
 drivers/net/phy/mdio_bus.c | 96 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)
drivers/net/phy/mdio_bus.c +95 -1
index d209716a1..636bdd1db 100644
@@ -17,14 +17,19 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <common.h>
+#include <of_gpio.h>
 #include <driver.h>
 #include <init.h>
+#include <gpio.h>
 #include <clock.h>
 #include <net.h>
 #include <errno.h>
 #include <linux/phy.h>
 #include <linux/err.h>
 
+#define DEFAULT_GPIO_RESET_ASSERT       1000      /* us */
+#define DEFAULT_GPIO_RESET_DEASSERT     1000      /* us */
+
 LIST_HEAD(mii_bus_list);
 
 int mdiobus_detect(struct device_d *dev)
@@ -82,6 +87,82 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 	return 0;
 }
 
+/*
+ * Node is considered a PHY node if:
+ * o Compatible string of "ethernet-phy-idX.X"
+ * o Compatible string of "ethernet-phy-ieee802.3-c45"
+ * o Compatible string of "ethernet-phy-ieee802.3-c22"
+ * o No compatibility string
+ *
+ * A device which is not a phy is expected to have a compatible string
+ * indicating what sort of device it is.
+ */
+static bool of_mdiobus_child_is_phy(struct device_node *np)
+{
+	struct property *prop;
+	const char *cp;
+
+	of_property_for_each_string(np, "compatible", prop, cp) {
+                if (!strncmp(cp, "ethernet-phy-id", strlen("ethernet-phy-id")))
+			return true;
+        }
+
+	if (of_device_is_compatible(np, "ethernet-phy-ieee802.3-c45"))
+		return true;
+
+	if (of_device_is_compatible(np, "ethernet-phy-ieee802.3-c22"))
+		return true;
+
+	if (!of_find_property(np, "compatible", NULL))
+		return true;
+
+	return false;
+}
+
+/*
+ * Reset the PHY, based on DT info
+ *
+ * If np is a phy node, and the phy node contains a reset-gpios property
+ * then reset the phy.
+ */
+static void of_mdiobus_reset_phy(struct mii_bus *bus, struct device_node *np)
+{
+	enum of_gpio_flags of_flags;
+	u32 reset_deassert_delay;
+	u32 reset_assert_delay;
+	unsigned long flags;
+	int gpio;
+	int ret;
+
+	gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &of_flags);
+	if (!gpio_is_valid(gpio))
+		return;
+
+	flags = GPIOF_OUT_INIT_INACTIVE;
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	ret = gpio_request_one(gpio, flags, np->name);
+	if (ret) {
+		dev_err(&bus->dev, "failed to request reset gpio for: %s\n",
+			np->name);
+		return;
+	}
+
+	reset_assert_delay = DEFAULT_GPIO_RESET_ASSERT;
+	of_property_read_u32(np, "reset-assert-us", &reset_assert_delay);
+
+	reset_deassert_delay = DEFAULT_GPIO_RESET_DEASSERT;
+	of_property_read_u32(np, "reset-deassert-us", &reset_deassert_delay);
+
+	/* reset the PHY */
+	dev_dbg(&bus->dev, "reset PHY with GPIO: %d\n", gpio);
+	gpio_set_active(gpio, 1);
+	udelay(reset_assert_delay);
+	gpio_set_active(gpio, 0);
+	udelay(reset_deassert_delay);
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -111,6 +192,10 @@ static int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 			continue;
 		}
 
+		if (!of_mdiobus_child_is_phy(child))
+			continue;
+
+		of_mdiobus_reset_phy(mdio, child);
 		of_mdiobus_register_phy(mdio, child, addr);
 	}
 
@@ -154,8 +239,17 @@ int mdiobus_register(struct mii_bus *bus)
 
 	pr_info("%s: probed\n", dev_name(&bus->dev));
 
-	if (bus->dev.device_node)
+	if (bus->dev.device_node) {
+		/* Register PHY's as child node to mdio node */
 		of_mdiobus_register(bus, bus->dev.device_node);
+	}
+	else if (bus->parent->device_node) {
+		/*
+		 * Register PHY's as child node to the ethernet node,
+		 * if there was no mdio node
+		 */
+		of_mdiobus_register(bus, bus->parent->device_node);
+	}
 
 	return 0;
 }
Signature · 7 lines
2.12.0


_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
#3 lore raw

PATCH v4 3/3 at91sam9263ek: add PHY, miitool etc. to config

In reply to Sam Ravnborg

Added Davicom phy to the config now it is supported. Add miitool to show the phy info.
Also add a bunch of extra commands that can be useful when handling the evaluation board.

---
 arch/arm/configs/at91sam9263ek_defconfig | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
arch/arm/configs/at91sam9263ek_defconfig +11 -1
index e8ad841fa..45c6f79de 100644
@@ -18,7 +18,7 @@ CONFIG_BOOTM_OFTREE=y
 CONFIG_BOOTM_OFTREE_UIMAGE=y
 CONFIG_CONSOLE_ACTIVATE_ALL=y
 CONFIG_DEFAULT_ENVIRONMENT_GENERIC=y
-# CONFIG_CMD_ARM_CPUINFO is not set
+CONFIG_CMD_DMESG=y
 CONFIG_LONGHELP=y
 CONFIG_CMD_IOMEM=y
 CONFIG_CMD_MEMINFO=y
@@ -33,22 +33,32 @@ CONFIG_CMD_PRINTENV=y
 CONFIG_CMD_SAVEENV=y
 CONFIG_CMD_SLEEP=y
 CONFIG_CMD_DHCP=y
+CONFIG_CMD_HOST=y
+CONFIG_NET_CMD_IFUP=y
+CONFIG_CMD_MIITOOL=y
 CONFIG_CMD_PING=y
 CONFIG_CMD_TFTP=y
 CONFIG_CMD_EDIT=y
 CONFIG_CMD_SPLASH=y
+CONFIG_CMD_FBTEST=y
 CONFIG_CMD_READLINE=y
 CONFIG_CMD_TIMEOUT=y
 CONFIG_CMD_CLK=y
+CONFIG_CMD_DETECT=y
 CONFIG_CMD_FLASH=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_LED=y
 CONFIG_CMD_LED_TRIGGER=y
+CONFIG_CMD_OF_NODE=y
+CONFIG_CMD_OF_PROPERTY=y
+CONFIG_CMD_OF_DISPLAY_TIMINGS=y
+CONFIG_CMD_OF_FIXUP_STATUS=y
 CONFIG_CMD_OFTREE=y
 CONFIG_NET=y
 CONFIG_NET_NFS=y
 CONFIG_OF_BAREBOX_DRIVERS=y
 CONFIG_DRIVER_NET_MACB=y
+CONFIG_DAVICOM_PHY=y
 # CONFIG_SPI is not set
 CONFIG_MTD=y
 # CONFIG_MTD_OOB_DEVICE is not set
Signature · 7 lines
2.12.0


_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
maintainer Applied
#4 lore raw
In reply to Sam Ravnborg
Quoted 7 lines · Sam Ravnborg

Changes in v4:
- Either register a mdio node or phy child nodes
- Check for valid phy node before registering the phy node This is like the kernel does it
- Also recognize compatible="ethernet-phy-id*" as PHY's again like the kernel, but with a simpler implementation, as we do not try to read the actual phy_id

Applied, thanks

Sascha

Quoted 68 lines

Changes in v3:
- Consider the v3 a new implmentation - almost nothing was left from v2. Thus also invalidate any former review - sorry.
- Dropped the need for the mdio {} node, as this node is only used when there is dedicated HW support for the mdio.
- Added so PHY child nodes to ethernet nodes are registered, and as part of registration the PHY nodes are reset if the reset-gpios property is present.
- Further dropped that the parsed info is stored in the bus, as we only do reset once, thus there is no need to save the info from the DT
- Futher dropped helper function to reset. They was not needed
- Named the gpio with the name of the PHY node
in the DT. This makes it unique and easier to recognize.

Changes in v2:
- Added patch to enable Davicom PHY on at91sam9263ek - evaluation kit
- Fix so we do reset before comunicating with the PHY
- Rename to mdio_reset()
- Reference correct binding file in commit log (mdio.txt)
- Tested on at91sam9263ek
The at91sam9263ek kit do not require the reset like my proprietary board, so no DT changes required

Intro:
The following patches was necessary to get networking operational on my proprietary target.
The target is at91sam9263 based with a Davicom PHY.

The Davicom PHY is a straight copy form the Linux kernel with the interrupt routine removed and minor adjustments to the rest.

The davicom PHY would not work until it had seen a reset cycle - which I think may be an artifact of the board design.

To fix the reset issue I have implemented support for the reset-gpios binding (see net/phy.txt bindings). A minimal implmentation was done, just enough to get my target running.

I could have implemented something in macb - but I preferred the more generic solution.

Also included are a patch that for the at91sam9263ek evaluation board. The patch adds several extra tools that are usefull for testing, and enable the Davicom PHY.

Sam

Sam Ravnborg (3):
phylib: add Davicom PHY support
phylib: add support for reset-gpios
at91sam9263ek: add PHY, miitool etc. to config

 arch/arm/configs/at91sam9263ek_defconfig |  12 ++-
 drivers/net/phy/Kconfig                  |   5 ++
 drivers/net/phy/Makefile                 |   1 +
 drivers/net/phy/davicom.c                | 140 +++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c               |  97 ++++++++++++++++++++-
 5 files changed, 253 insertions(+), 2 deletions(-)

_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox

Signature · 9 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
#5 lore raw

cover add quartz load support to NXP rtc drivers

In reply to Sam Ravnborg

The current rtc drivers for NXP pcf8523 and pcf85063 have different configuration of the quartz load.

pcf8523 hardcode the quartz load to 12.5 Pf
pcf85063 uses the reset default of 7 pF

Introduce a new property to the bindings for the two rtc devices to specify if the quartz load is 12.5 pF.

Update all in-kernel DT files using the pcf8523 to specify this property. And finally update the drivers to support the property.

NOTE:
This has the side-effect that any users of the pcf8523 driver using out-of-tree DTS files needs to add the new property to get the same behaviour as before.

Sam

Søren Andersen (5):
dt-binding: rtci-pcf8523: add binding with quartz_load property
dt-binding: rtc-pcf85063: add binding with quartz load property
dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes
rtc: pcf8523: add quartz load configuration from DT
rtc: pcf85063: add quartz load configuration from DT

 .../devicetree/bindings/rtc/nxp,pcf85063.txt       | 19 +++++++++++++
 .../devicetree/bindings/rtc/nxp,pcf8523.txt        | 19 +++++++++++++
 .../devicetree/bindings/trivial-devices.txt        |  2 --
 arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts       |  1 +
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts         |  1 +
 arch/arm/boot/dts/imx6q-h100.dts                   |  1 +
 arch/arm/boot/dts/imx6q-novena.dts                 |  1 +
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi             |  1 +
 arch/arm/boot/dts/imx6qdl-hummingboard.dtsi        |  1 +
 arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi       |  1 +
 drivers/rtc/rtc-pcf85063.c                         | 31 ++++++++++++++++++++++
 drivers/rtc/rtc-pcf8523.c                          |  6 ++---
 12 files changed, 79 insertions(+), 5 deletions(-)
#6 lore raw

PATCH 1/5 dt-binding: rtci-pcf8523: add quartz_load property

In reply to Sam Ravnborg

From: Søren Andersen <[email protected]>

The NXP pcf8523 supports two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf8523 needs to know the size of the quartz load, otherwise the the RTC will have a bad precision.

The default for the rtc (after power-on) is 7 pF.
Add a property that tells if the external capacitor is 12.5 pF.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +19 -0
new file mode 100644
index 000000000000..7c5e93f5077c
@@ -0,0 +1,19 @@
+* NXP PCF8523 Real Time Clock
+
+NXP PCF8523 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf8523".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf8523: pcf8523@68 {
+	compatible = "nxp,pcf85063";
+	reg = <0x68>;
+	nxp,quartz_load_12.5pF;
+};
Documentation/devicetree/bindings/trivial-devices.txt +0 -1
index 763a2808a95c..297dc82b67ad 100644
@@ -169,7 +169,6 @@ nxp,pca9556		Octal SMBus and I2C registered interface
 nxp,pca9557		8-bit I2C-bus and SMBus I/O port with reset
 nxp,pcf2127		Real-time clock
 nxp,pcf2129		Real-time clock
-nxp,pcf8523		Real-time Clock
 nxp,pcf8563		Real-time clock/calendar
 nxp,pcf85063		Tiny Real-Time Clock
 oki,ml86v7667		OKI ML86V7667 video decoder
Signature · 1 line
2.12.0
#7 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

Hi all.

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 19

Quoted 11 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +11 -0
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf8523: pcf8523@68 {
+	compatible = "nxp,pcf85063";
+	reg = <0x68>;
+	nxp,quartz_load_12.5pF;
+};

The property described in the bindings uses capital F. But the intent was to use a lower-case f, so the property is all lower-case.

Will fix in v2.
This goes for both this binding file and the nxp,pcf85063.txt. The dts files and the implmentation uses lower-case f already.

Sam

#8 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

In reply to Sam Ravnborg

Hi,

You can remove 'rtci-' from the subject.

Quoted 32 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

The NXP pcf8523 supports two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf8523 needs to know the size of the quartz load, otherwise the the RTC will have a bad precision.

The default for the rtc (after power-on) is 7 pF.
Add a property that tells if the external capacitor is 12.5 pF.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +2 -0
new file mode 100644
index 000000000000..7c5e93f5077c
@@ -0,0 +1,19 @@
+* NXP PCF8523 Real Time Clock
+

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 12

Quoted 10 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +10 -0
+NXP PCF8523 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf8523".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

nxp,quartz_load_fF and this would be either 7000 or 12500.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#9 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

Hi Alexandre.

Quoted 3 lines · Alexandre Belloni

Hi,

You can remove 'rtci-' from the subject.

The 'i' part was me fooling around in vi.
After submitting this serie I read the proper subject would be:
(from bindings/submitting-patches.txt)

dt-bindings: rtc: <short description>

I will use this in next submission so it is clear this is rtc related.

Quoted 49 lines · Sam Ravnborg
Quoted 32 lines

From: Søren Andersen <[email protected]>

The NXP pcf8523 supports two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf8523 needs to know the size of the quartz load, otherwise the the RTC will have a bad precision.

The default for the rtc (after power-on) is 7 pF.
Add a property that tells if the external capacitor is 12.5 pF.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +2 -0
new file mode 100644
index 000000000000..7c5e93f5077c
@@ -0,0 +1,19 @@
+* NXP PCF8523 Real Time Clock
+

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 12

Quoted 10 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +10 -0
+NXP PCF8523 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf8523".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

Quoted 1 line

nxp,quartz_load_fF and this would be either 7000 or 12500.

We had is implmented like this (using pF) in the beginning but then went for the simpler property.
Will add a fF property as you suggest and avoid breaking the existing drivers. We will check a few of the boards to see if the current configuration of the pcf8523 driver looks wrong, and if so we will print
the warnings as suggested.

I think the above covers feedback on all patches.
And thanks for the feedback!

Sam

#10 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

Quoted 12 lines · Sam Ravnborg
Quoted 5 lines

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

nxp,quartz_load_fF and this would be either 7000 or 12500.

We had is implmented like this (using pF) in the beginning but then went for the simpler property.
Will add a fF property as you suggest and avoid breaking the existing drivers. We will check a few of the boards to see if the current configuration of the pcf8523 driver looks wrong, and if so we will print
the warnings as suggested.

Quoted 3 lines

I think the above covers feedback on all patches.
And thanks for the feedback!

Hint: look at the cubox-i and the hummingboard schematics. I'm definitely not an analog expert but the two capacitors on the cubox-i are making me think that the setting should be different from the hummingboard.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer
#11 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

In reply to Alexandre Belloni
Quoted 52 lines · Alexandre Belloni

Hi,

You can remove 'rtci-' from the subject.

Quoted 32 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

The NXP pcf8523 supports two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf8523 needs to know the size of the quartz load, otherwise the the RTC will have a bad precision.

The default for the rtc (after power-on) is 7 pF.
Add a property that tells if the external capacitor is 12.5 pF.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +2 -0
new file mode 100644
index 000000000000..7c5e93f5077c
@@ -0,0 +1,19 @@
+* NXP PCF8523 Real Time Clock
+

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 12

Quoted 10 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +10 -0
+NXP PCF8523 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf8523".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

Quoted 1 line

nxp,quartz_load_fF and this would be either 7000 or 12500.

Use '-' rather than '_'.

There's already a defined unit suffix of '-picofarads'. If you need '-femtofarads', then please add to property-units.txt. Though I'd prefer not as wouldn't a value of 12 (or 13) be close enough for the needs here?

Rob

#12 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

Quoted 9 lines · Rob Herring
Quoted 9 lines

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 12

Quoted 4 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +4 -0
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

Quoted 9 lines
Quoted 1 line

nxp,quartz_load_fF and this would be either 7000 or 12500.

Use '-' rather than '_'.

There's already a defined unit suffix of '-picofarads'. If you need '-femtofarads', then please add to property-units.txt. Though I'd prefer not as wouldn't a value of 12 (or 13) be close enough for the needs here?

For that particular RTC, yes but the isl1208 has 0.5pF steps and the x1205 has 0.25pf steps. I'd prefer having a single generic property for all the RTCs: quartz-load-femtofarads.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#13 lore raw

Re: [PATCH v1 1/5] dt-binding: rtci-pcf8523: add quartz_load property

Quoted 20 lines · Alexandre Belloni
Quoted 9 lines · Rob Herring
Quoted 9 lines

Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt line 12

Quoted 4 lines
Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt new file +4 -0
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+

The boolean properties usually don't work well for RTCs because people usually want to keep any previous configuration that may have been done at the factory or in the bootloader so I would use:

Quoted 9 lines
Quoted 1 line

nxp,quartz_load_fF and this would be either 7000 or 12500.

Use '-' rather than '_'.

There's already a defined unit suffix of '-picofarads'. If you need '-femtofarads', then please add to property-units.txt. Though I'd prefer not as wouldn't a value of 12 (or 13) be close enough for the needs here?

Quoted 3 lines

For that particular RTC, yes but the isl1208 has 0.5pF steps and the x1205 has 0.25pf steps. I'd prefer having a single generic property for all the RTCs: quartz-load-femtofarads.

I will change to use this, and update property-units.txt too, when I get back to the patch set in a week or so.

Sam

#14 lore raw

PATCH 2/5 dt-binding: rtc-pcf85063: add quartz load property

In reply to Sam Ravnborg

From: Søren Andersen <[email protected]>

The NXP pcf85063 support two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf85063 needs to know the size of the external capacitor, otherwise the RTC will have a bad precision (hours/week).

The power-on default is 7 pF
Add a property that tells if the external capacitor is 12.5 pF

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 .../devicetree/bindings/rtc/nxp,pcf85063.txt          | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt new file +19 -0
new file mode 100644
index 000000000000..22ebb2ce52c1
@@ -0,0 +1,19 @@
+* NXP PCF85063 Real Time Clock
+
+NXP PCF85063 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf85063".
+- reg: I2C address for chip.
+
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf85063: pcf85063@51 {
+	compatible = "nxp,pcf85063";
+	reg = <0x51>;
+	nxp,quartz_load_12.5pF;
+};
Documentation/devicetree/bindings/trivial-devices.txt +0 -1
index 297dc82b67ad..440632682e31 100644
@@ -170,7 +170,6 @@ nxp,pca9557		8-bit I2C-bus and SMBus I/O port with reset
 nxp,pcf2127		Real-time clock
 nxp,pcf2129		Real-time clock
 nxp,pcf8563		Real-time clock/calendar
-nxp,pcf85063		Tiny Real-Time Clock
 oki,ml86v7667		OKI ML86V7667 video decoder
 ovti,ov5642		OV5642: Color CMOS QSXGA (5-megapixel) Image Sensor with OmniBSI and Embedded TrueFocus
 pericom,pt7c4338	Real-time Clock Module
Signature · 1 line
2.12.0
#15 lore raw

Re: [PATCH v1 2/5] dt-binding: rtc-pcf85063: add quartz load property

Hi,

You can remove 'rtc-' from the part name in the subject.

Quoted 38 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

The NXP pcf85063 support two different quartz loads.
- 7 pF (default)
- 12.5 pF (minimum power consumption)

The pcf85063 needs to know the size of the external capacitor, otherwise the RTC will have a bad precision (hours/week).

The power-on default is 7 pF
Add a property that tells if the external capacitor is 12.5 pF

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
---
 .../devicetree/bindings/rtc/nxp,pcf85063.txt          | 19 +++++++++++++++++++
 Documentation/devicetree/bindings/trivial-devices.txt |  1 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt
Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt new file +8 -0
new file mode 100644
index 000000000000..22ebb2ce52c1
@@ -0,0 +1,19 @@
+* NXP PCF85063 Real Time Clock
+
+NXP PCF85063 Real Time Clock
+
+Required properties:
+- compatible: Should contain "nxp,pcf85063".
+- reg: I2C address for chip.
+

Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt line 18

Quoted 10 lines
Documentation/devicetree/bindings/rtc/nxp,pcf85063.txt new file +10 -0
+Optional property:
+- nxp,quartz_load_12.5pF: The capacitive load on the quartz is 12.5 pF,
+  which differ from the default value of 7 pF
+
+Example:
+
+pcf85063: pcf85063@51 {
+	compatible = "nxp,pcf85063";
+	reg = <0x51>;
+	nxp,quartz_load_12.5pF;

Same comment as the previous patch, a value in fF is more useful. It could also probably be made a generic rtc property.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#16 lore raw

PATCH 3/5 dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes

In reply to Sam Ravnborg

From: Søren Andersen <[email protected]>

Add "nxp,quartz_load_12.5pf" to all pcf8523 nodes.

The device tree binding for the pcf8523 now includes
a property "nxp,quartz_load_12.5pf" to specify
that the connected x-tal has an internal capacitance of 12.5 pF.
Add this to all pcf8523 nodes, because this was implicit before, at least in the Linux kernel where the driver was hardcoded to set the rtc to use 12.5 pF.

Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Joel Stanley <[email protected]>
Cc Andrew Jeffery <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Russell King <[email protected]>
---
 arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts | 1 +
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts   | 1 +
 arch/arm/boot/dts/imx6q-h100.dts             | 1 +
 arch/arm/boot/dts/imx6q-novena.dts           | 1 +
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi       | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard.dtsi  | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi | 1 +
 7 files changed, 7 insertions(+)
arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts +1 -0
index d598b6391362..d1a0e3521e2b 100644
@@ -200,6 +200,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	tmp75@48 {
arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1 -0
index 2c5aa90a546d..b5088414e610 100644
@@ -201,6 +201,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	ucd90160@64 {
arch/arm/boot/dts/imx6q-h100.dts +1 -0
index 714e09e04dcb..3b6116069b11 100644
@@ -173,6 +173,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: sgtl5000@a {
arch/arm/boot/dts/imx6q-novena.dts +1 -0
index fcd824dc485b..6a2eaaa6e220 100644
@@ -257,6 +257,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sbs_battery: bq20z75@b {
arch/arm/boot/dts/imx6qdl-cubox-i.dtsi +1 -0
index 9332a31e6c8b..e0f601bce3d3 100644
@@ -143,6 +143,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 };
 
arch/arm/boot/dts/imx6qdl-hummingboard.dtsi +1 -0
index 0e64016e765f..7848b818078e 100644
@@ -189,6 +189,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	/* Pro baseboard model */
arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi +1 -0
index c413f9c3540f..7bc264234085 100644
@@ -222,6 +222,7 @@
 	pcf8523: rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: codec@a {
Signature · 1 line
2.12.0
#17 lore raw

Re: [PATCH v1 3/5] dts: add nxp,quartz_load_12.5pf to all pcf8523 nodes

Quoted 4 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

Add "nxp,quartz_load_12.5pf" to all pcf8523 nodes.

Quoted 8 lines

The device tree binding for the pcf8523 now includes
a property "nxp,quartz_load_12.5pf" to specify
that the connected x-tal has an internal capacitance of 12.5 pF.
Add this to all pcf8523 nodes, because this was implicit before, at least in the Linux kernel where the driver was hardcoded to set the rtc to use 12.5 pF.

Well, this patch is not needed as we should not break the DT ABI. See comment on the next patch...

Quoted 107 lines
Cc Rob Herring <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Joel Stanley <[email protected]>
Cc Andrew Jeffery <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Russell King <[email protected]>
---
 arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts | 1 +
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts   | 1 +
 arch/arm/boot/dts/imx6q-h100.dts             | 1 +
 arch/arm/boot/dts/imx6q-novena.dts           | 1 +
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi       | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard.dtsi  | 1 +
 arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi | 1 +
 7 files changed, 7 insertions(+)
arch/arm/boot/dts/aspeed-bmc-opp-lanyang.dts +1 -0
index d598b6391362..d1a0e3521e2b 100644
@@ -200,6 +200,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	tmp75@48 {
arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1 -0
index 2c5aa90a546d..b5088414e610 100644
@@ -201,6 +201,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	ucd90160@64 {
arch/arm/boot/dts/imx6q-h100.dts +1 -0
index 714e09e04dcb..3b6116069b11 100644
@@ -173,6 +173,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: sgtl5000@a {
arch/arm/boot/dts/imx6q-novena.dts +1 -0
index fcd824dc485b..6a2eaaa6e220 100644
@@ -257,6 +257,7 @@
 	rtc: pcf8523@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sbs_battery: bq20z75@b {
arch/arm/boot/dts/imx6qdl-cubox-i.dtsi +1 -0
index 9332a31e6c8b..e0f601bce3d3 100644
@@ -143,6 +143,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 };
 
arch/arm/boot/dts/imx6qdl-hummingboard.dtsi +1 -0
index 0e64016e765f..7848b818078e 100644
@@ -189,6 +189,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	/* Pro baseboard model */
arch/arm/boot/dts/imx6qdl-hummingboard2.dtsi +1 -0
index c413f9c3540f..7bc264234085 100644
@@ -222,6 +222,7 @@
 	pcf8523: rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		nxp,quartz_load_12.5pf;
 	};
 
 	sgtl5000: codec@a {
Signature · 1 line
2.12.0
Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#18 lore raw

PATCH 4/5 rtc: pcf8523: external capacitor configuration

In reply to Sam Ravnborg

From: Søren Andersen <[email protected]>

Add support for specifying the quartz load in the DT node. The pcf8523 may use either a 7 pF or an 12.5 pF xtal. If the rtc has the wrong configuration the time will drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used. This is a change compared to before where the driver assumed 12.5 pF.

Note: all kernel dts trees are updated with the new property.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
---
 drivers/rtc/rtc-pcf8523.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/rtc/rtc-pcf8523.c +3 -3
index 453615f8ac9a..3c19b4d21a2d 100644
@@ -85,7 +85,7 @@ static int pcf8523_write(struct i2c_client *client, u8 reg, u8 value)
 	return 0;
 }
 
-static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
+static int pcf8523_select_capacitance(struct i2c_client *client)
 {
 	u8 value;
 	int err;
@@ -94,7 +94,7 @@ static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
 	if (err < 0)
 		return err;
 
-	if (!high)
+	if (!device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
 		value &= ~REG_CONTROL1_CAP_SEL;
 	else
 		value |= REG_CONTROL1_CAP_SEL;
@@ -331,7 +331,7 @@ static int pcf8523_probe(struct i2c_client *client,
 	if (!pcf)
 		return -ENOMEM;
 
-	err = pcf8523_select_capacitance(client, true);
+	err = pcf8523_select_capacitance(client);
 	if (err < 0)
 		return err;
 
Signature · 1 line
2.12.0
#19 lore raw

Re: [PATCH v1 4/5] rtc: pcf8523: external capacitor configuration

Quoted 34 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

Add support for specifying the quartz load in the DT node. The pcf8523 may use either a 7 pF or an 12.5 pF xtal. If the rtc has the wrong configuration the time will drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used. This is a change compared to before where the driver assumed 12.5 pF.

Note: all kernel dts trees are updated with the new property.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
---
 drivers/rtc/rtc-pcf8523.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/rtc/rtc-pcf8523.c +1 -1
index 453615f8ac9a..3c19b4d21a2d 100644
@@ -85,7 +85,7 @@ static int pcf8523_write(struct i2c_client *client, u8 reg, u8 value)
 	return 0;
 }
 
-static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
+static int pcf8523_select_capacitance(struct i2c_client *client)
 {
 	u8 value;
 	int err;

drivers/rtc/rtc-pcf8523.c static int pcf8523_select_capacitance(struct i2c_client *client, bool high) · line 97

Quoted 5 lines
drivers/rtc/rtc-pcf8523.c +1 -1
@@ -94,7 +94,7 @@ static int pcf8523_select_capacitance(struct i2c_client *client, bool high)
 	if (err < 0)
 		return err;
 
-	if (!high)
+	if (!device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))

OK, this is super annoying we unfortunately can't change what was done previously without breaking the DT ABI. Honestly, I suspect the 12.5pF value is wrong for most boards anyway.

What I would do is use the value from DT (so either 7000fF or 12500fF) and set it if present.
If not present, set 12.5pF and print a warning that this differs from the RTC default.

Hopefully, at some point, we could leave it as it is as the value should persist across reboot.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#20 lore raw

PATCH 5/5 rtc: pcf85063: external capacitor configuration

In reply to Sam Ravnborg

From: Søren Andersen <[email protected]>

Add support for specifying the quartz load in the DT node. The pcf85063 may use either a 7 pF or an 12.5 pF xtal. If the rtc has the wrong configuration the time will drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
---
 drivers/rtc/rtc-pcf85063.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
drivers/rtc/rtc-pcf85063.c +31 -0
index 283c2335b01b..38163446664f 100644
@@ -27,6 +27,7 @@
 */
 
 #define PCF85063_REG_CTRL1		0x00 /* status */
+#define PCF85063_REG_CTRL1_CAP_SEL	BIT(0)
 #define PCF85063_REG_CTRL1_STOP		BIT(5)
 #define PCF85063_REG_CTRL2		0x01
 
@@ -180,6 +181,31 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {
 	.set_time	= pcf85063_rtc_set_time
 };
 
+static int pcf85063_select_capacitance(struct i2c_client *client)
+{
+	int rc;
+	u8 reg;
+
+	rc = i2c_smbus_read_byte_data(client, PCF85063_REG_CTRL1);
+	if (rc < 0) {
+		dev_err(&client->dev, "Failing to read Control1 reg\n");
+		return -EIO;
+	}
+
+	if (device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
+		reg = rc |= PCF85063_REG_CTRL1_CAP_SEL;
+	else
+		reg = rc &= ~PCF85063_REG_CTRL1_CAP_SEL;
+
+	rc = i2c_smbus_write_byte_data(client, PCF85063_REG_CTRL1, reg);
+	if (rc < 0) {
+		dev_err(&client->dev, "Failing to configure device\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int pcf85063_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
@@ -197,6 +223,11 @@ static int pcf85063_probe(struct i2c_client *client,
 		return err;
 	}
 
+	err = pcf85063_select_capacitance(client);
+	if (err < 0)
+		dev_warn(&client->dev,
+			 "Capacitance  setup failed. Trying to continue\n");
+
 	rtc = devm_rtc_device_register(&client->dev,
 				       pcf85063_driver.driver.name,
 				       &pcf85063_rtc_ops, THIS_MODULE);
Signature · 1 line
2.12.0
#21 lore raw

Re: [PATCH v1 5/5] rtc: pcf85063: external capacitor configuration

Quoted 38 lines · Sam Ravnborg

From: Søren Andersen <[email protected]>

Add support for specifying the quartz load in the DT node. The pcf85063 may use either a 7 pF or an 12.5 pF xtal. If the rtc has the wrong configuration the time will drift several hours/week.

If nothing is specified in DT then the factory default of 7 pF is used.

Cc Alessandro Zummo <[email protected]>
Cc Alexandre Belloni <[email protected]>
---
 drivers/rtc/rtc-pcf85063.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
drivers/rtc/rtc-pcf85063.c +6 -0
index 283c2335b01b..38163446664f 100644
@@ -27,6 +27,7 @@
 */
 
 #define PCF85063_REG_CTRL1		0x00 /* status */
+#define PCF85063_REG_CTRL1_CAP_SEL	BIT(0)
 #define PCF85063_REG_CTRL1_STOP		BIT(5)
 #define PCF85063_REG_CTRL2		0x01
 
@@ -180,6 +181,31 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {
 	.set_time	= pcf85063_rtc_set_time
 };
 
+static int pcf85063_select_capacitance(struct i2c_client *client)
+{
+	int rc;
+	u8 reg;
+

drivers/rtc/rtc-pcf85063.c static const struct rtc_class_ops pcf85063_rtc_ops = { · line 198

Quoted 10 lines
drivers/rtc/rtc-pcf85063.c +10 -0
+	rc = i2c_smbus_read_byte_data(client, PCF85063_REG_CTRL1);
+	if (rc < 0) {
+		dev_err(&client->dev, "Failing to read Control1 reg\n");
+		return -EIO;
+	}
+
+	if (device_property_present(&client->dev, "nxp,quartz_load_12.5pf"))
+		reg = rc |= PCF85063_REG_CTRL1_CAP_SEL;
+	else
+		reg = rc &= ~PCF85063_REG_CTRL1_CAP_SEL;

That one is more straight forward, set it to the value from DT or leave it alone.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com