[PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Mike Looijmans]

Add bindings for a fixed-rate clock that retrieves its rate from an
NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
or similar device.

Component shortages lead to boards being shipped with different clock
crystals, based on what was available at the time. The clock frequency
was written to EEPROM at production time. Systems can adapt to a wide
range of input frequencies using the clock framework, but this required
us to patch the devicetree at runtime or use some custom driver. This
provides a more generic solution.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

Changes in v2:
Changed "fixed-clock" into "nvmem-clock" in dts example
Add minItems:1 to nvmem-cell-names

 .../bindings/clock/nvmem-clock.yaml           | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nvmem-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/nvmem-clock.yaml b/Documentation/devicetree/bindings/clock/nvmem-clock.yaml
new file mode 100644
index 000000000000..af96a5e9372d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nvmem-clock.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/nvmem-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Simple fixed-rate clock source from NVMEM
+
+maintainers:
+  - Mike Looijmans <mike.looijmans@topic.nl>
+
+description:
+  Provides a clock rate from NVMEM. Typical usage is that the factory places a
+  crystal on the board and writes the rate into an EEPROM or EFUSE. If some math
+  is required, one can add a fixed-factor clock using this clock as input.
+
+properties:
+  compatible:
+    const: nvmem-clock
+
+  "#clock-cells":
+    const: 0
+
+  nvmem-cells:
+    minItems: 1
+    maxItems: 2
+    description:
+      Reads clock-frequency and optionally clock-accuracy from an NVMEM provider
+      in binary native integer format. The size of the NVMEM cell can be 1, 2, 4
+      or 8 bytes.
+
+  nvmem-cell-names:
+    minItems: 1
+    items:
+      - const: clock-frequency
+      - const: clock-accuracy
+
+  clock-accuracy:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      accuracy of clock in ppb (parts per billion). Alternative for providing
+      this through nvmem, the nvmem provided value takes precedence.
+
+  clock-output-names:
+    maxItems: 1
+
+required:
+  - compatible
+  - "#clock-cells"
+  - nvmem-cells
+  - nvmem-cell-names
+
+additionalProperties: false
+
+examples:
+  - |
+    xtal {
+      compatible = "nvmem-clock";
+      #clock-cells = <0>;
+      nvmem-cells = <&efuse_xtal_freq>;
+      nvmem-cell-names = "clock-frequency";
+    };
+...
-- 
2.17.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail

[PATCH v2 2/2] clk: Add nvmem-clock provider

[Mike Looijmans]

Adds a fixed-rate clock that retrieves its rate from an NVMEM provider.
This allows to store clock settings in EEPROM or EFUSE or similar device.

Component shortages lead to boards being shipped with different clock
crystals, based on what was available at the time. The clock frequency
was written to EEPROM at production time. Systems can adapt to a wide
range of input frequencies usign the clock framework, but this required
us to patch the devicetree at runtime or use some custom driver. This
provides a more generic solution.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

(no changes since v1)

 drivers/clk/Kconfig     |   7 +++
 drivers/clk/Makefile    |   1 +
 drivers/clk/clk-nvmem.c | 107 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/clk/clk-nvmem.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 016814e15536..63f165473481 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -447,6 +447,13 @@ config COMMON_CLK_FIXED_MMIO
 	help
 	  Support for Memory Mapped IO Fixed clocks
 
+config COMMON_CLK_NVMEM
+	bool "Clock driver for NVMEM provided frequency"
+	depends on COMMON_CLK && OF
+	help
+	  This driver allows a clock frequency to be provided by NVMEM data, for
+	  example in an EEPROM, by fuses or other non-volatile storage.
+
 config COMMON_CLK_K210
 	bool "Clock driver for the Canaan Kendryte K210 SoC"
 	depends on OF && RISCV && SOC_CANAAN
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 0aebef17edc6..aef1361e5085 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
 obj-$(CONFIG_ARCH_NPCM7XX)	    	+= clk-npcm7xx.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
+obj-$(CONFIG_COMMON_CLK_NVMEM)		+= clk-nvmem.o
 obj-$(CONFIG_COMMON_CLK_OXNAS)		+= clk-oxnas.o
 obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_CLK_LS1028A_PLLDIG)	+= clk-plldig.o
diff --git a/drivers/clk/clk-nvmem.c b/drivers/clk/clk-nvmem.c
new file mode 100644
index 000000000000..870f04a3954f
--- /dev/null
+++ b/drivers/clk/clk-nvmem.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Fixed rate clock that reads its frequency from NVMEM
+ *
+ * Copyright (C) 2023 Topic Embedded Products
+ * Mike Looijmans <mike.looijmans@topic.nl>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+static long nvmemclk_retrieve(struct device_node *node, const char *name)
+{
+	struct nvmem_cell *cell;
+	const void *data;
+	size_t len;
+	long ret;
+
+	cell = of_nvmem_cell_get(node, name);
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	data = nvmem_cell_read(cell, &len);
+	nvmem_cell_put(cell);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	switch (len) {
+	case 1:
+		ret = *(u8 *)data;
+		break;
+	case 2:
+		ret = *(u16 *)data;
+		break;
+	case 4:
+		ret = *(u32 *)data;
+		break;
+	case 8:
+		ret = *(u64 *)data;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	kfree(data);
+
+	return ret;
+}
+
+static int nvmemclk_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	const char *clk_name = node->name;
+	struct clk_hw *hw;
+	u32 rate;
+	u32 accuracy = 0;
+	long value;
+
+	value = nvmemclk_retrieve(node, "clock-frequency");
+	if (value < 0)
+		return dev_err_probe(&pdev->dev, value,
+				     "failed to retrieve clock-frequency\n");
+	rate = value;
+
+	/* clock-accuracy can be provided by either NVMEM or property */
+	value = nvmemclk_retrieve(node, "clock-accuracy");
+	if (value > 0)
+		accuracy = value;
+	else
+		of_property_read_u32(node, "clock-accuracy", &accuracy);
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	hw = clk_hw_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
+						    0, rate, accuracy);
+	if (IS_ERR(hw))
+		return dev_err_probe(&pdev->dev, PTR_ERR(hw),
+				     "Failed to register clock %s\n", clk_name);
+
+	return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_simple_get,
+					   hw);
+}
+
+static const struct of_device_id of_nvmemclk_ids[] = {
+	{ .compatible = "nvmem-clock" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, of_nvmemclk_ids);
+
+static struct platform_driver nvmemclk_driver = {
+	.driver = {
+		.name = "nvmem-clock",
+		.of_match_table = of_nvmemclk_ids,
+	},
+	.probe = nvmemclk_probe,
+};
+
+module_platform_driver(nvmemclk_driver);
+
+MODULE_DESCRIPTION("NVMEM clock driver");
+MODULE_AUTHOR("Mike Looijmans <mike.looijmans@topic.nl>");
+MODULE_LICENSE("GPL");
-- 
2.17.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]

Hey Mike,

I have no comments about whether this is a good idea or not, I'll leave
that Stephen, Krzysztof etc..

On Fri, May 26, 2023 at 04:38:06PM +0200, Mike Looijmans wrote:
> Add bindings for a fixed-rate clock that retrieves its rate from an
> NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
> or similar device.
> 
> Component shortages lead to boards being shipped with different clock
> crystals, based on what was available at the time. The clock frequency
> was written to EEPROM at production time. Systems can adapt to a wide
> range of input frequencies using the clock framework, but this required
> us to patch the devicetree at runtime or use some custom driver. This
> provides a more generic solution.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
> Changes in v2:
> Changed "fixed-clock" into "nvmem-clock" in dts example
> Add minItems:1 to nvmem-cell-names
> 
>  .../bindings/clock/nvmem-clock.yaml           | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/nvmem-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/nvmem-clock.yaml b/Documentation/devicetree/bindings/clock/nvmem-clock.yaml
> new file mode 100644
> index 000000000000..af96a5e9372d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvmem-clock.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/nvmem-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Simple fixed-rate clock source from NVMEM
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +description:
> +  Provides a clock rate from NVMEM. Typical usage is that the factory places a
> +  crystal on the board and writes the rate into an EEPROM or EFUSE. If some math
> +  is required, one can add a fixed-factor clock using this clock as input.
> +
> +properties:
> +  compatible:
> +    const: nvmem-clock
> +
> +  "#clock-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    minItems: 1
> +    maxItems: 2
> +    description:
> +      Reads clock-frequency and optionally clock-accuracy from an NVMEM provider
> +      in binary native integer format. The size of the NVMEM cell can be 1, 2, 4
> +      or 8 bytes.

How is that size communicated to the driver? Some nvmem magic just takes
care of it?

> +
> +  nvmem-cell-names:
> +    minItems: 1
> +    items:
> +      - const: clock-frequency
> +      - const: clock-accuracy
> +
> +  clock-accuracy:

I think the common pattern is to add a unit suffix for things like
this, which I guess would be -ppb?

Cheers,
Conor.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      accuracy of clock in ppb (parts per billion). Alternative for providing
> +      this through nvmem, the nvmem provided value takes precedence.
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - nvmem-cells
> +  - nvmem-cell-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    xtal {
> +      compatible = "nvmem-clock";
> +      #clock-cells = <0>;
> +      nvmem-cells = <&efuse_xtal_freq>;
> +      nvmem-cell-names = "clock-frequency";
> +    };
> +...
> -- 
> 2.17.1
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topicproducts.com
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Krzysztof Kozlowski]

Resending as my previous email probably got lost. If you got it twice,
apologies.


On 26/05/2023 16:38, Mike Looijmans wrote:
> Add bindings for a fixed-rate clock that retrieves its rate from an
> NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
> or similar device.
> 
> Component shortages lead to boards being shipped with different clock
> crystals, based on what was available at the time. The clock frequency
> was written to EEPROM at production time. Systems can adapt to a wide
> range of input frequencies using the clock framework, but this required
> us to patch the devicetree at runtime or use some custom driver. This
> provides a more generic solution.

This does not look like real hardware. I mean, the clock does not fetch
its rate from nvmem, right? It's the Linux which does it, so basically
you described here driver, not hardware.

Extend existing fixed-clock bindings to allow reading frequency via
nvmem cells.



Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Mike Looijmans]

On 31-05-2023 21:27, Krzysztof Kozlowski wrote:
> On 26/05/2023 16:38, Mike Looijmans wrote:
>> Add bindings for a fixed-rate clock that retrieves its rate from an
>> NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
>> or similar device.
>>
>> Component shortages lead to boards being shipped with different clock
>> crystals, based on what was available at the time. The clock frequency
>> was written to EEPROM at production time. Systems can adapt to a wide
>> range of input frequencies using the clock framework, but this required
>> us to patch the devicetree at runtime or use some custom driver. This
>> provides a more generic solution.
> This does not look like real hardware. I mean, the clock does not fetch
> its rate from nvmem, right? It's the Linux which does it, so basically
> you described here driver, not hardware.
Right, this just reads a setting from an NVMEM provider.
> Extend existing fixed-clock bindings to allow reading frequency via
> nvmem cells.

I just tried and implemented this, but it does not work. The reason is 
that the fixed-clock implementation returns "void" in its 
of_fixed_clk_setup() init function. The nvmem provider returns 
EPROBE_DEFER because it isn't ready at this early stage, and this error 
will not be propagated up because of the "void" signature. Thus, it's 
never retried and the clock just disappears.


> Best regards,
> Krzysztof
>

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Re: [PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Krzysztof Kozlowski]

On 05/06/2023 12:01, Mike Looijmans wrote:
> On 31-05-2023 21:27, Krzysztof Kozlowski wrote:
>> On 26/05/2023 16:38, Mike Looijmans wrote:
>>> Add bindings for a fixed-rate clock that retrieves its rate from an
>>> NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
>>> or similar device.
>>>
>>> Component shortages lead to boards being shipped with different clock
>>> crystals, based on what was available at the time. The clock frequency
>>> was written to EEPROM at production time. Systems can adapt to a wide
>>> range of input frequencies using the clock framework, but this required
>>> us to patch the devicetree at runtime or use some custom driver. This
>>> provides a more generic solution.
>> This does not look like real hardware. I mean, the clock does not fetch
>> its rate from nvmem, right? It's the Linux which does it, so basically
>> you described here driver, not hardware.
> Right, this just reads a setting from an NVMEM provider.
>> Extend existing fixed-clock bindings to allow reading frequency via
>> nvmem cells.
> 
> I just tried and implemented this, but it does not work. The reason is 
> that the fixed-clock implementation returns "void" in its 
> of_fixed_clk_setup() init function. The nvmem provider returns 
> EPROBE_DEFER because it isn't ready at this early stage, and this error 
> will not be propagated up because of the "void" signature. Thus, it's 
> never retried and the clock just disappears.

Linux driver problems are not a reason to add bindings for virtual
hardware...

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: clock: Add nvmem-clock

[Mike Looijmans]

On 05-06-2023 12:13, Krzysztof Kozlowski wrote:
> On 05/06/2023 12:01, Mike Looijmans wrote:
>> On 31-05-2023 21:27, Krzysztof Kozlowski wrote:
>>> On 26/05/2023 16:38, Mike Looijmans wrote:
>>>> Add bindings for a fixed-rate clock that retrieves its rate from an
>>>> NVMEM provider. This allows to store clock settings in EEPROM or EFUSE
>>>> or similar device.
>>>>
>>>> Component shortages lead to boards being shipped with different clock
>>>> crystals, based on what was available at the time. The clock frequency
>>>> was written to EEPROM at production time. Systems can adapt to a wide
>>>> range of input frequencies using the clock framework, but this required
>>>> us to patch the devicetree at runtime or use some custom driver. This
>>>> provides a more generic solution.
>>> This does not look like real hardware. I mean, the clock does not fetch
>>> its rate from nvmem, right? It's the Linux which does it, so basically
>>> you described here driver, not hardware.
>> Right, this just reads a setting from an NVMEM provider.
>>> Extend existing fixed-clock bindings to allow reading frequency via
>>> nvmem cells.
>> I just tried and implemented this, but it does not work. The reason is
>> that the fixed-clock implementation returns "void" in its
>> of_fixed_clk_setup() init function. The nvmem provider returns
>> EPROBE_DEFER because it isn't ready at this early stage, and this error
>> will not be propagated up because of the "void" signature. Thus, it's
>> never retried and the clock just disappears.
> Linux driver problems are not a reason to add bindings for virtual
> hardware...
>
Okay, so what are you proposing?

The implementation needs a compatible string other than "fixed-clock", 
because of the way the fixed-clock driver is being loaded (it can never 
be deferred).

So I should add another compatible string to fixed-clock.yaml? That'd 
make the syntax in fixed-clock.yaml rather awkward, since some 
properties would only apply to some compatible strings?

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl