[PATCH 0/2] Support for Apple SoCs' NCO blocks

[PATCH 0/2] Support for Apple SoCs' NCO blocks

[Martin Povišer]

Hi all,

I am submitting a common clock driver for Numerically Controlled Oscillator (NCO)
blocks on recent Apple ARM64 SoCs. The NCO driver will be used for audio
support on these platforms which is work-in-progress.

I suppose the only noteworthy thing is the blocks divide an input clock
with a clock cycle counter in part implemented as a LFSR. For that reason
the driver upfront calculates a table of LFSR states to be able to program
the dividers.

Best,
Martin

Martin Povišer (2):
  dt-bindings: clock: Add Apple NCO
  clk: clk-apple-nco: Add driver for Apple NCO

 .../devicetree/bindings/clock/apple,nco.yaml  |  70 ++++
 drivers/clk/Kconfig                           |   9 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-apple-nco.c                   | 299 ++++++++++++++++++
 4 files changed, 379 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
 create mode 100644 drivers/clk/clk-apple-nco.c

--
2.33.0

[PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Martin Povišer]

The NCO block found on Apple SoCs is a programmable clock generator
performing fractional division of a high frequency input clock.

Signed-off-by: Martin Povišer <povik@protonmail.com>
---
 .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml

diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
new file mode 100644
index 000000000000..5029824ab179
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/apple,nco.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple SoCs' NCO block
+
+maintainers:
+  - Martin Povišer <povik@protonmail.com>
+
+description: |
+  The NCO (Numerically Controlled Oscillator) block found on Apple SoCs
+  such as the t8103 (M1) is a programmable clock generator performing
+  fractional division of a high frequency input clock.
+
+  It carries a number of independent channels and is typically used for
+  generation of audio bitclocks.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - apple,t6000-nco
+        - apple,t8103-nco
+      - const: apple,nco
+
+  apple,nchannels:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The number of output channels the NCO block has been
+      synthesized for.
+
+  clocks:
+    description:
+      Specifies the reference clock from which the output clocks
+      are derived through fractional division.
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - apple,nchannels
+  - clocks
+  - '#clock-cells'
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    nco_clkref: clock-ref {
+      compatible = "fixed-clock";
+      #clock-cells = <0>;
+      clock-frequency = <900000000>;
+      clock-output-names = "nco-ref";
+    };
+
+    nco: clock-generator@23b044000 {
+      compatible = "apple,t8103-nco", "apple,nco";
+      reg = <0x3b044000 0x14000>;
+      #clock-cells = <1>;
+      clocks = <&nco_clkref>;
+      apple,nchannels = <5>;
+    };
--
2.33.0

[PATCH 2/2] clk: clk-apple-nco: Add driver for Apple NCO

[Martin Povišer]

Add a common clock driver for NCO blocks found on Apple SoCs where they
are typically the generators of audio clocks.

Signed-off-by: Martin Povišer <povik@protonmail.com>
---
 drivers/clk/Kconfig         |   9 ++
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-apple-nco.c | 299 ++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/clk/clk-apple-nco.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c5b3dc97396a..d2b3d40de29d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -390,6 +390,15 @@ config COMMON_CLK_K210
 	help
 	  Support for the Canaan Kendryte K210 RISC-V SoC clocks.

+config COMMON_CLK_APPLE_NCO
+	bool "Clock driver for Apple SoC NCOs"
+	depends on ARCH_APPLE || COMPILE_TEST
+	default ARCH_APPLE
+	help
+	  This driver supports NCO (Numerically Controlled Oscillator) blocks
+	  found on Apple SoCs such as t8103 (M1). The blocks are typically
+	  generators of audio clocks.
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/analogbits/Kconfig"
 source "drivers/clk/baikal-t1/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 6afe36bd2c0a..0f39db8664cc 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -17,6 +17,7 @@ endif

 # hardware specific clock types
 # please keep this section sorted lexicographically by file path name
+obj-$(CONFIG_COMMON_CLK_APPLE_NCO)  	+= clk-apple-nco.o
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
diff --git a/drivers/clk/clk-apple-nco.c b/drivers/clk/clk-apple-nco.c
new file mode 100644
index 000000000000..152901f6a40d
--- /dev/null
+++ b/drivers/clk/clk-apple-nco.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple NCO (Numerically Controlled Oscillator) clock driver
+ *
+ * Driver for an SoC block found on t8103 (M1) and other Apple chips
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_clk.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+#define NCO_CHANNEL_STRIDE	0x4000
+
+#define REG_CTRL	0
+#define CTRL_ENABLE	BIT(31)
+#define REG_DIV		4
+#define DIV_FINE	GENMASK(1, 0)
+#define DIV_COARSE	GENMASK(12, 2)
+#define REG_INC1	8
+#define REG_INC2	12
+#define REG_ACCINIT	16
+
+struct nco_tables;
+
+struct nco_channel {
+	void __iomem *base;
+	struct nco_tables *tbl;
+	struct clk_hw hw;
+};
+
+#define to_nco_channel(_hw) container_of(_hw, struct nco_channel, hw)
+
+#define LFSR_POLY	0xa01
+#define LFSR_INIT	0x7ff
+#define LFSR_LEN	11
+#define LFSR_PERIOD	((1 << LFSR_LEN) - 1)
+#define LFSR_TBLSIZE	(1 << LFSR_LEN)
+
+/* The minimal attainable coarse divisor (first value in table) */
+#define COARSE_DIV_OFFSET 2
+
+struct nco_tables {
+	u16 fwd[LFSR_TBLSIZE];
+	u16 inv[LFSR_TBLSIZE];
+};
+
+static int nco_enable(struct clk_hw *hw);
+static void nco_disable(struct clk_hw *hw);
+static int nco_is_enabled(struct clk_hw *hw);
+
+static void nco_compute_tables(struct nco_tables *tbl)
+{
+	int i;
+	u32 state = LFSR_INIT;
+
+	/*
+	 * Go through the states of a galois LFSR and build
+	 * a coarse divisor translation table.
+	 */
+	for (i = LFSR_PERIOD; i > 0; i--) {
+		if (state & 1)
+			state = (state >> 1) ^ (LFSR_POLY >> 1);
+		else
+			state = (state >> 1);
+		tbl->fwd[i] = state;
+		tbl->inv[state] = i;
+	}
+
+	/* Zero value is special-cased */
+	tbl->fwd[0] = 0;
+	tbl->inv[0] = 0;
+}
+
+static bool nco_div_check(int div)
+{
+	int coarse = div / 4;
+	return coarse >= COARSE_DIV_OFFSET &&
+		coarse < COARSE_DIV_OFFSET + LFSR_TBLSIZE;
+}
+
+static u32 nco_div_translate(struct nco_tables *tbl, int div)
+{
+	int coarse = div / 4;
+
+	if (WARN_ON(!nco_div_check(div)))
+		return 0;
+
+	return FIELD_PREP(DIV_COARSE, tbl->fwd[coarse - COARSE_DIV_OFFSET]) |
+			FIELD_PREP(DIV_FINE, div % 4);
+}
+
+static int nco_div_translate_inv(struct nco_tables *tbl, int regval)
+{
+	int coarse, fine;
+
+	coarse = tbl->inv[FIELD_GET(DIV_COARSE, regval)] + COARSE_DIV_OFFSET;
+	fine = FIELD_GET(DIV_FINE, regval);
+
+	return coarse * 4 + fine;
+}
+
+static int nco_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	struct nco_channel *chan = to_nco_channel(hw);
+	u32 div;
+	s32 inc1, inc2;
+	bool was_enabled;
+
+	was_enabled = nco_is_enabled(hw);
+	nco_disable(hw);
+
+	div = 2 * parent_rate / rate;
+	inc1 = 2 * parent_rate - div * rate;
+	inc2 = -((s32) (rate - inc1));
+
+	if (!nco_div_check(div))
+		return -EINVAL;
+
+	div = nco_div_translate(chan->tbl, div);
+
+	writel_relaxed(div,  chan->base + REG_DIV);
+	writel_relaxed(inc1, chan->base + REG_INC1);
+	writel_relaxed(inc2, chan->base + REG_INC2);
+	writel_relaxed(1 << 31, chan->base + REG_ACCINIT);
+
+	if (was_enabled)
+		nco_enable(hw);
+
+	return 0;
+}
+
+static unsigned long nco_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	struct nco_channel *chan = to_nco_channel(hw);
+	u32 div;
+	s32 inc1, inc2, incbase;
+
+	div = nco_div_translate_inv(chan->tbl,
+			readl_relaxed(chan->base + REG_DIV));
+
+	inc1 = readl_relaxed(chan->base + REG_INC1);
+	inc2 = readl_relaxed(chan->base + REG_INC2);
+
+	/*
+	 * We don't support wraparound of accumulator
+	 * nor the edge case of both increments being zero
+	 */
+	if (inc1 < 0 || inc2 > 0 || (inc1 == 0 && inc2 == 0))
+		return 0;
+
+	/* Scale both sides of division by incbase to maintain precision */
+	incbase = inc1 - inc2;
+
+	return div_u64(((u64) parent_rate) * 2 * incbase,
+			((u64) div) * incbase + inc1);
+}
+
+static long nco_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *parent_rate)
+{
+	unsigned long lo = *parent_rate / (COARSE_DIV_OFFSET + LFSR_TBLSIZE) + 1;
+	unsigned long hi = *parent_rate / COARSE_DIV_OFFSET;
+
+	return clamp(rate, lo, hi);
+}
+
+static int nco_enable(struct clk_hw *hw)
+{
+	struct nco_channel *chan = to_nco_channel(hw);
+	u32 val;
+
+	val = readl_relaxed(chan->base + REG_CTRL);
+	writel_relaxed(val | CTRL_ENABLE, chan->base + REG_CTRL);
+	return 0;
+}
+
+static void nco_disable(struct clk_hw *hw)
+{
+	struct nco_channel *chan = to_nco_channel(hw);
+	u32 val;
+
+	val = readl_relaxed(chan->base + REG_CTRL);
+	writel_relaxed(val & ~CTRL_ENABLE, chan->base + REG_CTRL);
+}
+
+static int nco_is_enabled(struct clk_hw *hw)
+{
+	struct nco_channel *chan = to_nco_channel(hw);
+
+	return (readl_relaxed(chan->base + REG_CTRL) & CTRL_ENABLE) != 0;
+}
+
+static const struct clk_ops nco_ops = {
+	.set_rate = nco_set_rate,
+	.recalc_rate = nco_recalc_rate,
+	.round_rate = nco_round_rate,
+	.enable = nco_enable,
+	.disable = nco_disable,
+	.is_enabled = nco_is_enabled,
+};
+
+static int apple_nco_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct clk_init_data init;
+	struct clk_hw_onecell_data *onecell_data;
+	const char *parent_name;
+	void __iomem *regs;
+	struct nco_tables *tbl;
+	int nchannels;
+	int ret, i;
+
+	ret = of_property_read_u32(np, "apple,nchannels", &nchannels);
+	if (ret) {
+		dev_err(&pdev->dev, "missing or invalid apple,nchannels property\n");
+		return -EINVAL;
+	}
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	if (of_clk_get_parent_count(np) != 1)
+		return -EINVAL;
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (!parent_name)
+		return -EINVAL;
+
+	onecell_data = devm_kzalloc(&pdev->dev, struct_size(onecell_data, hws,
+							nchannels), GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+	onecell_data->num = nchannels;
+
+	tbl = devm_kzalloc(&pdev->dev, sizeof(*tbl), GFP_KERNEL);
+	if (!tbl)
+		return -ENOMEM;
+	nco_compute_tables(tbl);
+
+	for (i = 0; i < nchannels; i++) {
+		struct nco_channel *chan;
+
+		chan = devm_kzalloc(&pdev->dev, sizeof(*chan), GFP_KERNEL);
+		if (!chan)
+			return -ENOMEM;
+		chan->base = regs + NCO_CHANNEL_STRIDE*i;
+		chan->tbl = tbl;
+
+		memset(&init, 0, sizeof(init));
+		init.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+						"%s-%d", np->name, i);
+		init.ops = &nco_ops;
+		init.num_parents = 1;
+		init.parent_names = &parent_name;
+		init.flags = 0;
+
+		chan->hw.init = &init;
+		ret = devm_clk_hw_register(&pdev->dev, &chan->hw);
+		if (ret)
+			return ret;
+
+		onecell_data->hws[i] = &chan->hw;
+	}
+
+	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+							onecell_data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id apple_nco_ids[] = {
+	{ .compatible = "apple,nco" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, apple_nco_ids)
+
+static struct platform_driver apple_nco_driver = {
+	.driver = {
+		.name = "apple-nco",
+		.of_match_table = apple_nco_ids,
+	},
+	.probe = apple_nco_probe,
+};
+module_platform_driver(apple_nco_driver);
+
+MODULE_AUTHOR("Martin Povišer <povik@protonmail.com>");
+MODULE_DESCRIPTION("Clock driver for NCO blocks on Apple SoCs");
+MODULE_LICENSE("GPL v2");
--
2.33.0

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On Tue, 14 Dec 2021 12:02:48 +0000, Martin Povišer wrote:
> The NCO block found on Apple SoCs is a programmable clock generator
> performing fractional division of a high frequency input clock.
> 
> Signed-off-by: Martin Povišer <povik@protonmail.com>
> ---
>  .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/clock/apple,nco.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1567695

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Rob Herring]

On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote:
> The NCO block found on Apple SoCs is a programmable clock generator
> performing fractional division of a high frequency input clock.
> 
> Signed-off-by: Martin Povišer <povik@protonmail.com>
> ---
>  .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
> new file mode 100644
> index 000000000000..5029824ab179
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/apple,nco.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple SoCs' NCO block
> +
> +maintainers:
> +  - Martin Povišer <povik@protonmail.com>
> +
> +description: |
> +  The NCO (Numerically Controlled Oscillator) block found on Apple SoCs
> +  such as the t8103 (M1) is a programmable clock generator performing
> +  fractional division of a high frequency input clock.
> +
> +  It carries a number of independent channels and is typically used for
> +  generation of audio bitclocks.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - apple,t6000-nco
> +        - apple,t8103-nco
> +      - const: apple,nco
> +
> +  apple,nchannels:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The number of output channels the NCO block has been
> +      synthesized for.

I'd assume there is some max number?

Do you really need to know this? If this is just to validate the clock 
cell values are less than this, then just drop that and the property. 
It's not the kernel's job to validate the DT.

> +
> +  clocks:
> +    description:
> +      Specifies the reference clock from which the output clocks
> +      are derived through fractional division.
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - apple,nchannels
> +  - clocks
> +  - '#clock-cells'
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    nco_clkref: clock-ref {
> +      compatible = "fixed-clock";
> +      #clock-cells = <0>;
> +      clock-frequency = <900000000>;
> +      clock-output-names = "nco-ref";
> +    };
> +
> +    nco: clock-generator@23b044000 {

clock-controller@...

> +      compatible = "apple,t8103-nco", "apple,nco";
> +      reg = <0x3b044000 0x14000>;

You really have 0x14000 worth of registers here because all of that 
will be mapped into virtual memory? Doesn't matter so much on 64-bit, 
but it did for 32-bit.

> +      #clock-cells = <1>;
> +      clocks = <&nco_clkref>;
> +      apple,nchannels = <5>;
> +    };
> --
> 2.33.0
> 
> 
> 

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Martin Povišer]

Hi Rob,

> On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote:
>> The NCO block found on Apple SoCs is a programmable clock generator
>> performing fractional division of a high frequency input clock.
>>
>> Signed-off-by: Martin Povišer <povik@protonmail.com>
>> ---
>> .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
>> 1 file changed, 70 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>> new file mode 100644
>> index 000000000000..5029824ab179
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>> @@ -0,0 +1,70 @@

>> +
>> +  apple,nchannels:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      The number of output channels the NCO block has been
>> +      synthesized for.
>
> I'd assume there is some max number?

There might be some limit to the underlying IP but we wouldn’t know.
What we know about the hardware comes from blackbox reversing, and that
doesn't suggest a particular limit to the number of channels we might
see on the SoC block in future.

> Do you really need to know this? If this is just to validate the clock
> cell values are less than this, then just drop that and the property.
> It's not the kernel's job to validate the DT.

Well strictly speaking the driver could do clock registration on-demand
at the cost of additional book-keeping, in which case we could drop
the property, but I would prefer we don’t do that. Rather than providing
validation the property simplifies drivers.

Another option is calculating the no. of channels from size of the reg
range, but I assume that’s worse than having the nchannels property.

>> +
>> +    nco: clock-generator@23b044000 {
>
> clock-controller@...

Okay, will change.

>
>> +      compatible = "apple,t8103-nco", "apple,nco";
>> +      reg = <0x3b044000 0x14000>;
>
> You really have 0x14000 worth of registers here because all of that
> will be mapped into virtual memory? Doesn't matter so much on 64-bit,
> but it did for 32-bit.

There is about 5 registers per channel with 0x4000 stride between them,
blame Apple (or Samsung? I don’t know...).

--
Martin

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Rob Herring]

On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote:
>
> Hi Rob,
>
> > On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote:
> >> The NCO block found on Apple SoCs is a programmable clock generator
> >> performing fractional division of a high frequency input clock.
> >>
> >> Signed-off-by: Martin Povišer <povik@protonmail.com>
> >> ---
> >> .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
> >> 1 file changed, 70 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
> >> new file mode 100644
> >> index 000000000000..5029824ab179
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
> >> @@ -0,0 +1,70 @@
>
> >> +
> >> +  apple,nchannels:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description:
> >> +      The number of output channels the NCO block has been
> >> +      synthesized for.
> >
> > I'd assume there is some max number?
>
> There might be some limit to the underlying IP but we wouldn’t know.
> What we know about the hardware comes from blackbox reversing, and that
> doesn't suggest a particular limit to the number of channels we might
> see on the SoC block in future.

All the more reason to not put the size in the DT, but imply from the
compatible. Unless it varies by instance...

Though I guess you would need DT updates anyways to use the new clock.

> > Do you really need to know this? If this is just to validate the clock
> > cell values are less than this, then just drop that and the property.
> > It's not the kernel's job to validate the DT.
>
> Well strictly speaking the driver could do clock registration on-demand
> at the cost of additional book-keeping, in which case we could drop
> the property, but I would prefer we don’t do that. Rather than providing
> validation the property simplifies drivers.
>
> Another option is calculating the no. of channels from size of the reg
> range, but I assume that’s worse than having the nchannels property.
>
> >> +
> >> +    nco: clock-generator@23b044000 {
> >
> > clock-controller@...
>
> Okay, will change.
>
> >
> >> +      compatible = "apple,t8103-nco", "apple,nco";
> >> +      reg = <0x3b044000 0x14000>;
> >
> > You really have 0x14000 worth of registers here because all of that
> > will be mapped into virtual memory? Doesn't matter so much on 64-bit,
> > but it did for 32-bit.
>
> There is about 5 registers per channel with 0x4000 stride between them,
> blame Apple (or Samsung? I don’t know...).

I would think you could walk the 0x4000 until you hit registers that
behave differently.

The register size / 0x4000 gives you the number of channels, too.

Another question, how do you know this is 1 block with N channels vs.
N blocks just happening to be next to each other in the memory map?

Rob

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Martin Povišer]

> On 15. 12. 2021, at 0:53, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote:
>>
>> Hi Rob,
>>
>>> On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote:
>>>> The NCO block found on Apple SoCs is a programmable clock generator
>>>> performing fractional division of a high frequency input clock.
>>>>
>>>> Signed-off-by: Martin Povišer <povik@protonmail.com>
>>>> ---
>>>> .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
>>>> 1 file changed, 70 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>> new file mode 100644
>>>> index 000000000000..5029824ab179
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>> @@ -0,0 +1,70 @@
>>
>>>> +
>>>> +  apple,nchannels:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      The number of output channels the NCO block has been
>>>> +      synthesized for.
>>>
>>> I'd assume there is some max number?
>>
>> There might be some limit to the underlying IP but we wouldn’t know.
>> What we know about the hardware comes from blackbox reversing, and that
>> doesn't suggest a particular limit to the number of channels we might
>> see on the SoC block in future.
>
> All the more reason to not put the size in the DT, but imply from the
> compatible. Unless it varies by instance...
>
> Though I guess you would need DT updates anyways to use the new clock.
>
>>> Do you really need to know this? If this is just to validate the clock
>>> cell values are less than this, then just drop that and the property.
>>> It's not the kernel's job to validate the DT.
>>
>> Well strictly speaking the driver could do clock registration on-demand
>> at the cost of additional book-keeping, in which case we could drop
>> the property, but I would prefer we don’t do that. Rather than providing
>> validation the property simplifies drivers.
>>
>> Another option is calculating the no. of channels from size of the reg
>> range, but I assume that’s worse than having the nchannels property.
>>
>>>> +
>>>> +    nco: clock-generator@23b044000 {
>>>
>>> clock-controller@...
>>
>> Okay, will change.
>>
>>>
>>>> +      compatible = "apple,t8103-nco", "apple,nco";
>>>> +      reg = <0x3b044000 0x14000>;
>>>
>>> You really have 0x14000 worth of registers here because all of that
>>> will be mapped into virtual memory? Doesn't matter so much on 64-bit,
>>> but it did for 32-bit.
>>
>> There is about 5 registers per channel with 0x4000 stride between them,
>> blame Apple (or Samsung? I don’t know...).
>
> I would think you could walk the 0x4000 until you hit registers that
> behave differently.
>
> The register size / 0x4000 gives you the number of channels, too.

Right now that’s what I am inclined to use in v2.

> Another question, how do you know this is 1 block with N channels vs.
> N blocks just happening to be next to each other in the memory map?

We don’t. We only see Apple describe it as such in their devicetree, and
so far for all practical purposes it could be one block.

I guess if we derive the number of channels from register size, there’s
the fallback of breaking up the nodes per channel in future.

Martin

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Sven Peter]

On Wed, Dec 15, 2021, at 09:28, Martin Povišer wrote:
>> On 15. 12. 2021, at 0:53, Rob Herring <robh@kernel.org> wrote:
>>
>> On Tue, Dec 14, 2021 at 2:08 PM Martin Povišer <povik@protonmail.com> wrote:
>>>
>>> Hi Rob,
>>>
>>>> On 14. 12. 2021, at 16:40, Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Tue, Dec 14, 2021 at 12:02:48PM +0000, Martin Povišer wrote:
>>>>> The NCO block found on Apple SoCs is a programmable clock generator
>>>>> performing fractional division of a high frequency input clock.
>>>>>
>>>>> Signed-off-by: Martin Povišer <povik@protonmail.com>
>>>>> ---
>>>>> .../devicetree/bindings/clock/apple,nco.yaml  | 70 +++++++++++++++++++
>>>>> 1 file changed, 70 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/apple,nco.yaml b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..5029824ab179
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/apple,nco.yaml
>>>>> @@ -0,0 +1,70 @@
>>>
>>>>> +
>>>>> +  apple,nchannels:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      The number of output channels the NCO block has been
>>>>> +      synthesized for.
>>>>
>>>> I'd assume there is some max number?
>>>
>>> There might be some limit to the underlying IP but we wouldn’t know.
>>> What we know about the hardware comes from blackbox reversing, and that
>>> doesn't suggest a particular limit to the number of channels we might
>>> see on the SoC block in future.
>>
>> All the more reason to not put the size in the DT, but imply from the
>> compatible. Unless it varies by instance...
>>
>> Though I guess you would need DT updates anyways to use the new clock.
>>
>>>> Do you really need to know this? If this is just to validate the clock
>>>> cell values are less than this, then just drop that and the property.
>>>> It's not the kernel's job to validate the DT.
>>>
>>> Well strictly speaking the driver could do clock registration on-demand
>>> at the cost of additional book-keeping, in which case we could drop
>>> the property, but I would prefer we don’t do that. Rather than providing
>>> validation the property simplifies drivers.
>>>
>>> Another option is calculating the no. of channels from size of the reg
>>> range, but I assume that’s worse than having the nchannels property.
>>>
>>>>> +
>>>>> +    nco: clock-generator@23b044000 {
>>>>
>>>> clock-controller@...
>>>
>>> Okay, will change.
>>>
>>>>
>>>>> +      compatible = "apple,t8103-nco", "apple,nco";
>>>>> +      reg = <0x3b044000 0x14000>;
>>>>
>>>> You really have 0x14000 worth of registers here because all of that
>>>> will be mapped into virtual memory? Doesn't matter so much on 64-bit,
>>>> but it did for 32-bit.
>>>
>>> There is about 5 registers per channel with 0x4000 stride between them,
>>> blame Apple (or Samsung? I don’t know...).
>>
>> I would think you could walk the 0x4000 until you hit registers that
>> behave differently.
>>
>> The register size / 0x4000 gives you the number of channels, too.
>
> Right now that’s what I am inclined to use in v2.
>
>> Another question, how do you know this is 1 block with N channels vs.
>> N blocks just happening to be next to each other in the memory map?
>
> We don’t. We only see Apple describe it as such in their devicetree, and
> so far for all practical purposes it could be one block.

Fwiw, the Apple device tree cannot be trusted in general. It also pretends
that two IOMMUs that need to programmed identically are a single dive,
sometimes includes MMIO ranges that are much too large and also contains
at least a single "virtual" device that only exists for what I assume
to be a workaround for some XNU quirk(s). (the GPU IOMMU has a separate node
with no MMIO or anything which only attaches a small shim driver that then
calls back into the main GPU driver. That device is also only used from within
that GPU driver.).

Are there any dependencies between these individual channels?
Is there some common initialization required for all of them?

From a quick glance and my uninformed opinion it looks like these are
separate to me. They only all need this LSFR table which could still be
shared.


Sven

Re: [PATCH 2/2] clk: clk-apple-nco: Add driver for Apple NCO

[Sven Peter]

Hi,

Thanks for working on the entire audio stack! I don't know much
about the clock framework and just have a few nits below.

On Tue, Dec 14, 2021, at 13:02, Martin Povišer wrote:
> Add a common clock driver for NCO blocks found on Apple SoCs where they
> are typically the generators of audio clocks.
>
> Signed-off-by: Martin Povišer <povik@protonmail.com>
> ---
>  drivers/clk/Kconfig         |   9 ++
>  drivers/clk/Makefile        |   1 +
>  drivers/clk/clk-apple-nco.c | 299 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 309 insertions(+)
>  create mode 100644 drivers/clk/clk-apple-nco.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c5b3dc97396a..d2b3d40de29d 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -390,6 +390,15 @@ config COMMON_CLK_K210
>  	help
>  	  Support for the Canaan Kendryte K210 RISC-V SoC clocks.
>
> +config COMMON_CLK_APPLE_NCO
> +	bool "Clock driver for Apple SoC NCOs"
> +	depends on ARCH_APPLE || COMPILE_TEST
> +	default ARCH_APPLE
> +	help
> +	  This driver supports NCO (Numerically Controlled Oscillator) blocks
> +	  found on Apple SoCs such as t8103 (M1). The blocks are typically
> +	  generators of audio clocks.
> +
>  source "drivers/clk/actions/Kconfig"
>  source "drivers/clk/analogbits/Kconfig"
>  source "drivers/clk/baikal-t1/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6afe36bd2c0a..0f39db8664cc 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -17,6 +17,7 @@ endif
>
>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file path name
> +obj-$(CONFIG_COMMON_CLK_APPLE_NCO)  	+= clk-apple-nco.o
>  obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
>  obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
>  obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
> diff --git a/drivers/clk/clk-apple-nco.c b/drivers/clk/clk-apple-nco.c
> new file mode 100644
> index 000000000000..152901f6a40d
> --- /dev/null
> +++ b/drivers/clk/clk-apple-nco.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple NCO (Numerically Controlled Oscillator) clock driver
> + *
> + * Driver for an SoC block found on t8103 (M1) and other Apple chips
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +#define NCO_CHANNEL_STRIDE	0x4000
> +
> +#define REG_CTRL	0
> +#define CTRL_ENABLE	BIT(31)
> +#define REG_DIV		4
> +#define DIV_FINE	GENMASK(1, 0)
> +#define DIV_COARSE	GENMASK(12, 2)
> +#define REG_INC1	8
> +#define REG_INC2	12
> +#define REG_ACCINIT	16
> +
> +struct nco_tables;
> +
> +struct nco_channel {
> +	void __iomem *base;
> +	struct nco_tables *tbl;
> +	struct clk_hw hw;
> +};
> +
> +#define to_nco_channel(_hw) container_of(_hw, struct nco_channel, hw)
> +
> +#define LFSR_POLY	0xa01
> +#define LFSR_INIT	0x7ff
> +#define LFSR_LEN	11
> +#define LFSR_PERIOD	((1 << LFSR_LEN) - 1)
> +#define LFSR_TBLSIZE	(1 << LFSR_LEN)
> +
> +/* The minimal attainable coarse divisor (first value in table) */
> +#define COARSE_DIV_OFFSET 2
> +
> +struct nco_tables {
> +	u16 fwd[LFSR_TBLSIZE];
> +	u16 inv[LFSR_TBLSIZE];
> +};
> +
> +static int nco_enable(struct clk_hw *hw);
> +static void nco_disable(struct clk_hw *hw);
> +static int nco_is_enabled(struct clk_hw *hw);

I think you can drop these forward-declarations if you move these
functions a bit to the top.

> +
> +static void nco_compute_tables(struct nco_tables *tbl)
> +{
> +	int i;
> +	u32 state = LFSR_INIT;
> +
> +	/*
> +	 * Go through the states of a galois LFSR and build
> +	 * a coarse divisor translation table.
> +	 */
> +	for (i = LFSR_PERIOD; i > 0; i--) {
> +		if (state & 1)
> +			state = (state >> 1) ^ (LFSR_POLY >> 1);
> +		else
> +			state = (state >> 1);
> +		tbl->fwd[i] = state;
> +		tbl->inv[state] = i;
> +	}
> +
> +	/* Zero value is special-cased */
> +	tbl->fwd[0] = 0;
> +	tbl->inv[0] = 0;
> +}
> +
> +static bool nco_div_check(int div)
> +{
> +	int coarse = div / 4;
> +	return coarse >= COARSE_DIV_OFFSET &&
> +		coarse < COARSE_DIV_OFFSET + LFSR_TBLSIZE;
> +}
> +
> +static u32 nco_div_translate(struct nco_tables *tbl, int div)
> +{
> +	int coarse = div / 4;
> +
> +	if (WARN_ON(!nco_div_check(div)))
> +		return 0;
> +
> +	return FIELD_PREP(DIV_COARSE, tbl->fwd[coarse - COARSE_DIV_OFFSET]) |
> +			FIELD_PREP(DIV_FINE, div % 4);
> +}
> +
> +static int nco_div_translate_inv(struct nco_tables *tbl, int regval)
> +{
> +	int coarse, fine;
> +
> +	coarse = tbl->inv[FIELD_GET(DIV_COARSE, regval)] + COARSE_DIV_OFFSET;
> +	fine = FIELD_GET(DIV_FINE, regval);
> +
> +	return coarse * 4 + fine;
> +}
> +
> +static int nco_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct nco_channel *chan = to_nco_channel(hw);
> +	u32 div;
> +	s32 inc1, inc2;
> +	bool was_enabled;
> +
> +	was_enabled = nco_is_enabled(hw);
> +	nco_disable(hw);
> +
> +	div = 2 * parent_rate / rate;
> +	inc1 = 2 * parent_rate - div * rate;
> +	inc2 = -((s32) (rate - inc1));
> +
> +	if (!nco_div_check(div))
> +		return -EINVAL;
> +
> +	div = nco_div_translate(chan->tbl, div);

You end up doing nco_div_check twice here and this is also the only
place you use nco_div_translate.
Maybe just drop the check above, remove the WARN_ON in nco_div_translate
and use if (!div) return -EINVAL; here.

> +
> +	writel_relaxed(div,  chan->base + REG_DIV);
> +	writel_relaxed(inc1, chan->base + REG_INC1);
> +	writel_relaxed(inc2, chan->base + REG_INC2);
> +	writel_relaxed(1 << 31, chan->base + REG_ACCINIT);

What does that magic number 1 << 31 represent?

> +
> +	if (was_enabled)
> +		nco_enable(hw);
> +
> +	return 0;
> +}
> +
> +static unsigned long nco_recalc_rate(struct clk_hw *hw,
> +				unsigned long parent_rate)
> +{
> +	struct nco_channel *chan = to_nco_channel(hw);
> +	u32 div;
> +	s32 inc1, inc2, incbase;
> +
> +	div = nco_div_translate_inv(chan->tbl,
> +			readl_relaxed(chan->base + REG_DIV));
> +
> +	inc1 = readl_relaxed(chan->base + REG_INC1);
> +	inc2 = readl_relaxed(chan->base + REG_INC2);
> +
> +	/*
> +	 * We don't support wraparound of accumulator
> +	 * nor the edge case of both increments being zero
> +	 */
> +	if (inc1 < 0 || inc2 > 0 || (inc1 == 0 && inc2 == 0))
> +		return 0;
> +
> +	/* Scale both sides of division by incbase to maintain precision */
> +	incbase = inc1 - inc2;
> +
> +	return div_u64(((u64) parent_rate) * 2 * incbase,
> +			((u64) div) * incbase + inc1);
> +}
> +
> +static long nco_round_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long *parent_rate)
> +{
> +	unsigned long lo = *parent_rate / (COARSE_DIV_OFFSET + LFSR_TBLSIZE) + 1;
> +	unsigned long hi = *parent_rate / COARSE_DIV_OFFSET;
> +
> +	return clamp(rate, lo, hi);
> +}
> +
> +static int nco_enable(struct clk_hw *hw)
> +{
> +	struct nco_channel *chan = to_nco_channel(hw);
> +	u32 val;
> +
> +	val = readl_relaxed(chan->base + REG_CTRL);
> +	writel_relaxed(val | CTRL_ENABLE, chan->base + REG_CTRL);
> +	return 0;
> +}
> +
> +static void nco_disable(struct clk_hw *hw)
> +{
> +	struct nco_channel *chan = to_nco_channel(hw);
> +	u32 val;
> +
> +	val = readl_relaxed(chan->base + REG_CTRL);
> +	writel_relaxed(val & ~CTRL_ENABLE, chan->base + REG_CTRL);
> +}
> +
> +static int nco_is_enabled(struct clk_hw *hw)
> +{
> +	struct nco_channel *chan = to_nco_channel(hw);
> +
> +	return (readl_relaxed(chan->base + REG_CTRL) & CTRL_ENABLE) != 0;
> +}
> +
> +static const struct clk_ops nco_ops = {
> +	.set_rate = nco_set_rate,
> +	.recalc_rate = nco_recalc_rate,
> +	.round_rate = nco_round_rate,
> +	.enable = nco_enable,
> +	.disable = nco_disable,
> +	.is_enabled = nco_is_enabled,
> +};
> +
> +static int apple_nco_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct clk_init_data init;
> +	struct clk_hw_onecell_data *onecell_data;
> +	const char *parent_name;
> +	void __iomem *regs;
> +	struct nco_tables *tbl;
> +	int nchannels;
> +	int ret, i;
> +
> +	ret = of_property_read_u32(np, "apple,nchannels", &nchannels);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing or invalid apple,nchannels property\n");
> +		return -EINVAL;
> +	}
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	if (of_clk_get_parent_count(np) != 1)
> +		return -EINVAL;
> +	parent_name = of_clk_get_parent_name(np, 0);
> +	if (!parent_name)
> +		return -EINVAL;
> +
> +	onecell_data = devm_kzalloc(&pdev->dev, struct_size(onecell_data, hws,
> +							nchannels), GFP_KERNEL);
> +	if (!onecell_data)
> +		return -ENOMEM;
> +	onecell_data->num = nchannels;
> +
> +	tbl = devm_kzalloc(&pdev->dev, sizeof(*tbl), GFP_KERNEL);
> +	if (!tbl)
> +		return -ENOMEM;
> +	nco_compute_tables(tbl);

Technically probe could be called multiple times (possibly even in parallel
I think). Might make sense to make sure these tables are only calculated once
for the entire driver.

> +
> +	for (i = 0; i < nchannels; i++) {
> +		struct nco_channel *chan;
> +
> +		chan = devm_kzalloc(&pdev->dev, sizeof(*chan), GFP_KERNEL);
> +		if (!chan)
> +			return -ENOMEM;
> +		chan->base = regs + NCO_CHANNEL_STRIDE*i;
> +		chan->tbl = tbl;
> +
> +		memset(&init, 0, sizeof(init));
> +		init.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +						"%s-%d", np->name, i);
> +		init.ops = &nco_ops;
> +		init.num_parents = 1;
> +		init.parent_names = &parent_name;
> +		init.flags = 0;
> +
> +		chan->hw.init = &init;
> +		ret = devm_clk_hw_register(&pdev->dev, &chan->hw);
> +		if (ret)
> +			return ret;
> +
> +		onecell_data->hws[i] = &chan->hw;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
> +							onecell_data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

just returning devm_of_clk_add_hw_provider(...); does the same thing here and is 
a bit more concise :-)

> +}
> +
> +static const struct of_device_id apple_nco_ids[] = {
> +	{ .compatible = "apple,nco" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, apple_nco_ids)
> +
> +static struct platform_driver apple_nco_driver = {
> +	.driver = {
> +		.name = "apple-nco",
> +		.of_match_table = apple_nco_ids,
> +	},
> +	.probe = apple_nco_probe,
> +};
> +module_platform_driver(apple_nco_driver);
> +
> +MODULE_AUTHOR("Martin Povišer <povik@protonmail.com>");
> +MODULE_DESCRIPTION("Clock driver for NCO blocks on Apple SoCs");
> +MODULE_LICENSE("GPL v2");
> --
> 2.33.0


Thanks,


Sven

Re: [PATCH 1/2] dt-bindings: clock: Add Apple NCO

[Martin Povišer]

> On 15. 12. 2021, at 9:43, Sven Peter <sven@svenpeter.dev> wrote:
>

> Are there any dependencies between these individual channels?
> Is there some common initialization required for all of them?
>
> From a quick glance and my uninformed opinion it looks like these are
> separate to me. They only all need this LSFR table which could still be
> shared.

That’s right.

>
> Sven

Martin

Re: [PATCH 2/2] clk: clk-apple-nco: Add driver for Apple NCO

[Martin Povišer]

> On 15. 12. 2021, at 10:01, Sven Peter <sven@svenpeter.dev> wrote:
>
> Hi,

Hi Sven!

> Thanks for working on the entire audio stack! I don't know much
> about the clock framework and just have a few nits below.
>
> On Tue, Dec 14, 2021, at 13:02, Martin Povišer wrote:
>> Add a common clock driver for NCO blocks found on Apple SoCs where they
>> are typically the generators of audio clocks.
>>
>> Signed-off-by: Martin Povišer <povik@protonmail.com>
>> ---
>> drivers/clk/Kconfig         |   9 ++
>> drivers/clk/Makefile        |   1 +
>> drivers/clk/clk-apple-nco.c | 299 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 309 insertions(+)
>> create mode 100644 drivers/clk/clk-apple-nco.c
>>

>> diff --git a/drivers/clk/clk-apple-nco.c b/drivers/clk/clk-apple-nco.c
>> new file mode 100644
>> index 000000000000..152901f6a40d
>> --- /dev/null
>> +++ b/drivers/clk/clk-apple-nco.c
>> @@ -0,0 +1,299 @@
>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +/*
>> + * Apple NCO (Numerically Controlled Oscillator) clock driver
>> + *
>> + * Driver for an SoC block found on t8103 (M1) and other Apple chips
>> + *
>> + * Copyright (C) The Asahi Linux Contributors
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/math64.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_clk.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +
>> +#define NCO_CHANNEL_STRIDE	0x4000
>> +
>> +#define REG_CTRL	0
>> +#define CTRL_ENABLE	BIT(31)
>> +#define REG_DIV		4
>> +#define DIV_FINE	GENMASK(1, 0)
>> +#define DIV_COARSE	GENMASK(12, 2)
>> +#define REG_INC1	8
>> +#define REG_INC2	12
>> +#define REG_ACCINIT	16
>> +
>> +struct nco_tables;
>> +
>> +struct nco_channel {
>> +	void __iomem *base;
>> +	struct nco_tables *tbl;
>> +	struct clk_hw hw;
>> +};
>> +
>> +#define to_nco_channel(_hw) container_of(_hw, struct nco_channel, hw)
>> +
>> +#define LFSR_POLY	0xa01
>> +#define LFSR_INIT	0x7ff
>> +#define LFSR_LEN	11
>> +#define LFSR_PERIOD	((1 << LFSR_LEN) - 1)
>> +#define LFSR_TBLSIZE	(1 << LFSR_LEN)
>> +
>> +/* The minimal attainable coarse divisor (first value in table) */
>> +#define COARSE_DIV_OFFSET 2
>> +
>> +struct nco_tables {
>> +	u16 fwd[LFSR_TBLSIZE];
>> +	u16 inv[LFSR_TBLSIZE];
>> +};
>> +
>> +static int nco_enable(struct clk_hw *hw);
>> +static void nco_disable(struct clk_hw *hw);
>> +static int nco_is_enabled(struct clk_hw *hw);
>
> I think you can drop these forward-declarations if you move these
> functions a bit to the top.

I can but then the LFSR pieces (definitions, table preparation,
table usage) would be further apart from each other.

Comparatively there’s not much to see in the disable/enable functions.

>> +
>> +static void nco_compute_tables(struct nco_tables *tbl)
>> +{
>> +	int i;
>> +	u32 state = LFSR_INIT;
>> +
>> +	/*
>> +	 * Go through the states of a galois LFSR and build
>> +	 * a coarse divisor translation table.
>> +	 */
>> +	for (i = LFSR_PERIOD; i > 0; i--) {
>> +		if (state & 1)
>> +			state = (state >> 1) ^ (LFSR_POLY >> 1);
>> +		else
>> +			state = (state >> 1);
>> +		tbl->fwd[i] = state;
>> +		tbl->inv[state] = i;
>> +	}
>> +
>> +	/* Zero value is special-cased */
>> +	tbl->fwd[0] = 0;
>> +	tbl->inv[0] = 0;
>> +}
>> +
>> +static bool nco_div_check(int div)
>> +{
>> +	int coarse = div / 4;
>> +	return coarse >= COARSE_DIV_OFFSET &&
>> +		coarse < COARSE_DIV_OFFSET + LFSR_TBLSIZE;
>> +}
>> +
>> +static u32 nco_div_translate(struct nco_tables *tbl, int div)
>> +{
>> +	int coarse = div / 4;
>> +
>> +	if (WARN_ON(!nco_div_check(div)))
>> +		return 0;
>> +
>> +	return FIELD_PREP(DIV_COARSE, tbl->fwd[coarse - COARSE_DIV_OFFSET]) |
>> +			FIELD_PREP(DIV_FINE, div % 4);
>> +}
>> +
>> +static int nco_div_translate_inv(struct nco_tables *tbl, int regval)
>> +{
>> +	int coarse, fine;
>> +
>> +	coarse = tbl->inv[FIELD_GET(DIV_COARSE, regval)] + COARSE_DIV_OFFSET;
>> +	fine = FIELD_GET(DIV_FINE, regval);
>> +
>> +	return coarse * 4 + fine;
>> +}
>> +
>> +static int nco_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				unsigned long parent_rate)
>> +{
>> +	struct nco_channel *chan = to_nco_channel(hw);
>> +	u32 div;
>> +	s32 inc1, inc2;
>> +	bool was_enabled;
>> +
>> +	was_enabled = nco_is_enabled(hw);
>> +	nco_disable(hw);
>> +
>> +	div = 2 * parent_rate / rate;
>> +	inc1 = 2 * parent_rate - div * rate;
>> +	inc2 = -((s32) (rate - inc1));
>> +
>> +	if (!nco_div_check(div))
>> +		return -EINVAL;
>> +
>> +	div = nco_div_translate(chan->tbl, div);
>
> You end up doing nco_div_check twice here and this is also the only
> place you use nco_div_translate.
> Maybe just drop the check above, remove the WARN_ON in nco_div_translate
> and use if (!div) return -EINVAL; here.

That would work if zero wasn't a valid value for div after lookup.

>
>> +
>> +	writel_relaxed(div,  chan->base + REG_DIV);
>> +	writel_relaxed(inc1, chan->base + REG_INC1);
>> +	writel_relaxed(inc2, chan->base + REG_INC2);
>> +	writel_relaxed(1 << 31, chan->base + REG_ACCINIT);
>
> What does that magic number 1 << 31 represent?

Presumably that’s a neutral initial value for the fractional divider’s
32-bit accumulator.

>
>> +
>> +	if (was_enabled)
>> +		nco_enable(hw);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned long nco_recalc_rate(struct clk_hw *hw,
>> +				unsigned long parent_rate)
>> +{
>> +	struct nco_channel *chan = to_nco_channel(hw);
>> +	u32 div;
>> +	s32 inc1, inc2, incbase;
>> +
>> +	div = nco_div_translate_inv(chan->tbl,
>> +			readl_relaxed(chan->base + REG_DIV));
>> +
>> +	inc1 = readl_relaxed(chan->base + REG_INC1);
>> +	inc2 = readl_relaxed(chan->base + REG_INC2);
>> +
>> +	/*
>> +	 * We don't support wraparound of accumulator
>> +	 * nor the edge case of both increments being zero
>> +	 */
>> +	if (inc1 < 0 || inc2 > 0 || (inc1 == 0 && inc2 == 0))
>> +		return 0;
>> +
>> +	/* Scale both sides of division by incbase to maintain precision */
>> +	incbase = inc1 - inc2;
>> +
>> +	return div_u64(((u64) parent_rate) * 2 * incbase,
>> +			((u64) div) * incbase + inc1);
>> +}
>> +
>> +static long nco_round_rate(struct clk_hw *hw, unsigned long rate,
>> +				unsigned long *parent_rate)
>> +{
>> +	unsigned long lo = *parent_rate / (COARSE_DIV_OFFSET + LFSR_TBLSIZE) + 1;
>> +	unsigned long hi = *parent_rate / COARSE_DIV_OFFSET;
>> +
>> +	return clamp(rate, lo, hi);
>> +}
>> +
>> +static int nco_enable(struct clk_hw *hw)
>> +{
>> +	struct nco_channel *chan = to_nco_channel(hw);
>> +	u32 val;
>> +
>> +	val = readl_relaxed(chan->base + REG_CTRL);
>> +	writel_relaxed(val | CTRL_ENABLE, chan->base + REG_CTRL);
>> +	return 0;
>> +}
>> +
>> +static void nco_disable(struct clk_hw *hw)
>> +{
>> +	struct nco_channel *chan = to_nco_channel(hw);
>> +	u32 val;
>> +
>> +	val = readl_relaxed(chan->base + REG_CTRL);
>> +	writel_relaxed(val & ~CTRL_ENABLE, chan->base + REG_CTRL);
>> +}
>> +
>> +static int nco_is_enabled(struct clk_hw *hw)
>> +{
>> +	struct nco_channel *chan = to_nco_channel(hw);
>> +
>> +	return (readl_relaxed(chan->base + REG_CTRL) & CTRL_ENABLE) != 0;
>> +}
>> +
>> +static const struct clk_ops nco_ops = {
>> +	.set_rate = nco_set_rate,
>> +	.recalc_rate = nco_recalc_rate,
>> +	.round_rate = nco_round_rate,
>> +	.enable = nco_enable,
>> +	.disable = nco_disable,
>> +	.is_enabled = nco_is_enabled,
>> +};
>> +
>> +static int apple_nco_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct clk_init_data init;
>> +	struct clk_hw_onecell_data *onecell_data;
>> +	const char *parent_name;
>> +	void __iomem *regs;
>> +	struct nco_tables *tbl;
>> +	int nchannels;
>> +	int ret, i;
>> +
>> +	ret = of_property_read_u32(np, "apple,nchannels", &nchannels);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "missing or invalid apple,nchannels property\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +
>> +	if (of_clk_get_parent_count(np) != 1)
>> +		return -EINVAL;
>> +	parent_name = of_clk_get_parent_name(np, 0);
>> +	if (!parent_name)
>> +		return -EINVAL;
>> +
>> +	onecell_data = devm_kzalloc(&pdev->dev, struct_size(onecell_data, hws,
>> +							nchannels), GFP_KERNEL);
>> +	if (!onecell_data)
>> +		return -ENOMEM;
>> +	onecell_data->num = nchannels;
>> +
>> +	tbl = devm_kzalloc(&pdev->dev, sizeof(*tbl), GFP_KERNEL);
>> +	if (!tbl)
>> +		return -ENOMEM;
>> +	nco_compute_tables(tbl);
>
> Technically probe could be called multiple times (possibly even in parallel
> I think). Might make sense to make sure these tables are only calculated once
> for the entire driver.

Well not in parallel for the same device I hope! :-p

Sure, the table could be computed once for the entire driver, but computing it
per-device is simpler and doesn’t matter if all SoCs have at most one NCO device
anyway. (But if the binding changes and we start having devices per what we now
call channel then of course it starts to matter.)

>
>> +
>> +	for (i = 0; i < nchannels; i++) {
>> +		struct nco_channel *chan;
>> +
>> +		chan = devm_kzalloc(&pdev->dev, sizeof(*chan), GFP_KERNEL);
>> +		if (!chan)
>> +			return -ENOMEM;
>> +		chan->base = regs + NCO_CHANNEL_STRIDE*i;
>> +		chan->tbl = tbl;
>> +
>> +		memset(&init, 0, sizeof(init));
>> +		init.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>> +						"%s-%d", np->name, i);
>> +		init.ops = &nco_ops;
>> +		init.num_parents = 1;
>> +		init.parent_names = &parent_name;
>> +		init.flags = 0;
>> +
>> +		chan->hw.init = &init;
>> +		ret = devm_clk_hw_register(&pdev->dev, &chan->hw);
>> +		if (ret)
>> +			return ret;
>> +
>> +		onecell_data->hws[i] = &chan->hw;
>> +	}
>> +
>> +	ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
>> +							onecell_data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>
> just returning devm_of_clk_add_hw_provider(...); does the same thing here and is
> a bit more concise :-)
>
>> +}
>> +
>> +static const struct of_device_id apple_nco_ids[] = {
>> +	{ .compatible = "apple,nco" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, apple_nco_ids)
>> +
>> +static struct platform_driver apple_nco_driver = {
>> +	.driver = {
>> +		.name = "apple-nco",
>> +		.of_match_table = apple_nco_ids,
>> +	},
>> +	.probe = apple_nco_probe,
>> +};
>> +module_platform_driver(apple_nco_driver);
>> +
>> +MODULE_AUTHOR("Martin Povišer <povik@protonmail.com>");
>> +MODULE_DESCRIPTION("Clock driver for NCO blocks on Apple SoCs");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.33.0
>
>
> Thanks,
>
>
> Sven

Martin

Re: [PATCH 2/2] clk: clk-apple-nco: Add driver for Apple NCO

[Stephen Boyd]

Quoting Martin Povišer (2021-12-14 04:02:55)
> Add a common clock driver for NCO blocks found on Apple SoCs where they
> are typically the generators of audio clocks.
> 
> Signed-off-by: Martin Povišer <povik@protonmail.com>
> ---
>  drivers/clk/Kconfig         |   9 ++
>  drivers/clk/Makefile        |   1 +
>  drivers/clk/clk-apple-nco.c | 299 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 309 insertions(+)
>  create mode 100644 drivers/clk/clk-apple-nco.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c5b3dc97396a..d2b3d40de29d 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -390,6 +390,15 @@ config COMMON_CLK_K210
>         help
>           Support for the Canaan Kendryte K210 RISC-V SoC clocks.
> 
> +config COMMON_CLK_APPLE_NCO

Please try to keep this sorted by Kconfig name. I know it isn't very
well done right now but it would be better than adding it here at the
end.

> +       bool "Clock driver for Apple SoC NCOs"
> +       depends on ARCH_APPLE || COMPILE_TEST
> +       default ARCH_APPLE
> +       help
> +         This driver supports NCO (Numerically Controlled Oscillator) blocks
> +         found on Apple SoCs such as t8103 (M1). The blocks are typically
> +         generators of audio clocks.
> +
>  source "drivers/clk/actions/Kconfig"
>  source "drivers/clk/analogbits/Kconfig"
>  source "drivers/clk/baikal-t1/Kconfig"
> diff --git a/drivers/clk/clk-apple-nco.c b/drivers/clk/clk-apple-nco.c
> new file mode 100644
> index 000000000000..152901f6a40d
> --- /dev/null
> +++ b/drivers/clk/clk-apple-nco.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple NCO (Numerically Controlled Oscillator) clock driver
> + *
> + * Driver for an SoC block found on t8103 (M1) and other Apple chips
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +#define NCO_CHANNEL_STRIDE     0x4000
> +
> +#define REG_CTRL       0
> +#define CTRL_ENABLE    BIT(31)
> +#define REG_DIV                4
> +#define DIV_FINE       GENMASK(1, 0)
> +#define DIV_COARSE     GENMASK(12, 2)
> +#define REG_INC1       8
> +#define REG_INC2       12
> +#define REG_ACCINIT    16
> +
> +struct nco_tables;
> +
> +struct nco_channel {
> +       void __iomem *base;
> +       struct nco_tables *tbl;
> +       struct clk_hw hw;
> +};
> +
> +#define to_nco_channel(_hw) container_of(_hw, struct nco_channel, hw)
> +
> +#define LFSR_POLY      0xa01
> +#define LFSR_INIT      0x7ff
> +#define LFSR_LEN       11
> +#define LFSR_PERIOD    ((1 << LFSR_LEN) - 1)
> +#define LFSR_TBLSIZE   (1 << LFSR_LEN)
> +
> +/* The minimal attainable coarse divisor (first value in table) */
> +#define COARSE_DIV_OFFSET 2
> +
> +struct nco_tables {
> +       u16 fwd[LFSR_TBLSIZE];
> +       u16 inv[LFSR_TBLSIZE];
> +};
> +
> +static int nco_enable(struct clk_hw *hw);
> +static void nco_disable(struct clk_hw *hw);
> +static int nco_is_enabled(struct clk_hw *hw);
> +
> +static void nco_compute_tables(struct nco_tables *tbl)
> +{
> +       int i;
> +       u32 state = LFSR_INIT;
> +
> +       /*
> +        * Go through the states of a galois LFSR and build
> +        * a coarse divisor translation table.
> +        */
> +       for (i = LFSR_PERIOD; i > 0; i--) {
> +               if (state & 1)
> +                       state = (state >> 1) ^ (LFSR_POLY >> 1);
> +               else
> +                       state = (state >> 1);
> +               tbl->fwd[i] = state;
> +               tbl->inv[state] = i;
> +       }
> +
> +       /* Zero value is special-cased */
> +       tbl->fwd[0] = 0;
> +       tbl->inv[0] = 0;
> +}
> +
> +static bool nco_div_check(int div)

Maybe nco_div_out_of_range()? And then invert the logic below.

> +{
> +       int coarse = div / 4;
> +       return coarse >= COARSE_DIV_OFFSET &&
> +               coarse < COARSE_DIV_OFFSET + LFSR_TBLSIZE;
> +}
> +
> +static u32 nco_div_translate(struct nco_tables *tbl, int div)
> +{
> +       int coarse = div / 4;

Why signed types?

> +
> +       if (WARN_ON(!nco_div_check(div)))
> +               return 0;
> +
> +       return FIELD_PREP(DIV_COARSE, tbl->fwd[coarse - COARSE_DIV_OFFSET]) |
> +                       FIELD_PREP(DIV_FINE, div % 4);
> +}
> +
> +static int nco_div_translate_inv(struct nco_tables *tbl, int regval)
> +{
> +       int coarse, fine;

Why signed types?

> +
> +       coarse = tbl->inv[FIELD_GET(DIV_COARSE, regval)] + COARSE_DIV_OFFSET;
> +       fine = FIELD_GET(DIV_FINE, regval);
> +
> +       return coarse * 4 + fine;
> +}
> +
> +static int nco_set_rate(struct clk_hw *hw, unsigned long rate,
> +                               unsigned long parent_rate)
> +{
> +       struct nco_channel *chan = to_nco_channel(hw);
> +       u32 div;
> +       s32 inc1, inc2;
> +       bool was_enabled;
> +
> +       was_enabled = nco_is_enabled(hw);
> +       nco_disable(hw);

Does anything prevent a clk_enable() call from racing with this?

> +
> +       div = 2 * parent_rate / rate;
> +       inc1 = 2 * parent_rate - div * rate;
> +       inc2 = -((s32) (rate - inc1));
> +
> +       if (!nco_div_check(div))
> +               return -EINVAL;

Did we just fail and leave the clk disabled?

> +
> +       div = nco_div_translate(chan->tbl, div);
> +
> +       writel_relaxed(div,  chan->base + REG_DIV);
> +       writel_relaxed(inc1, chan->base + REG_INC1);
> +       writel_relaxed(inc2, chan->base + REG_INC2);
> +       writel_relaxed(1 << 31, chan->base + REG_ACCINIT);
> +
> +       if (was_enabled)
> +               nco_enable(hw);
> +
> +       return 0;
> +}
> +
> +static unsigned long nco_recalc_rate(struct clk_hw *hw,
> +                               unsigned long parent_rate)
> +{
> +       struct nco_channel *chan = to_nco_channel(hw);
> +       u32 div;
> +       s32 inc1, inc2, incbase;
> +
> +       div = nco_div_translate_inv(chan->tbl,
> +                       readl_relaxed(chan->base + REG_DIV));
> +
> +       inc1 = readl_relaxed(chan->base + REG_INC1);
> +       inc2 = readl_relaxed(chan->base + REG_INC2);
> +
> +       /*
> +        * We don't support wraparound of accumulator
> +        * nor the edge case of both increments being zero
> +        */
> +       if (inc1 < 0 || inc2 > 0 || (inc1 == 0 && inc2 == 0))
> +               return 0;
> +
> +       /* Scale both sides of division by incbase to maintain precision */
> +       incbase = inc1 - inc2;
> +
> +       return div_u64(((u64) parent_rate) * 2 * incbase,
> +                       ((u64) div) * incbase + inc1);
> +}
> +
> +static long nco_round_rate(struct clk_hw *hw, unsigned long rate,
> +                               unsigned long *parent_rate)
> +{
> +       unsigned long lo = *parent_rate / (COARSE_DIV_OFFSET + LFSR_TBLSIZE) + 1;
> +       unsigned long hi = *parent_rate / COARSE_DIV_OFFSET;
> +
> +       return clamp(rate, lo, hi);
> +}
> +
> +static int nco_enable(struct clk_hw *hw)
> +{
> +       struct nco_channel *chan = to_nco_channel(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(chan->base + REG_CTRL);
> +       writel_relaxed(val | CTRL_ENABLE, chan->base + REG_CTRL);

Nitpick, newline here.

> +       return 0;
> +}
> +
> +static void nco_disable(struct clk_hw *hw)
> +{
> +       struct nco_channel *chan = to_nco_channel(hw);
> +       u32 val;
> +
> +       val = readl_relaxed(chan->base + REG_CTRL);
> +       writel_relaxed(val & ~CTRL_ENABLE, chan->base + REG_CTRL);
> +}
> +
> +static int nco_is_enabled(struct clk_hw *hw)
> +{
> +       struct nco_channel *chan = to_nco_channel(hw);
> +
> +       return (readl_relaxed(chan->base + REG_CTRL) & CTRL_ENABLE) != 0;
> +}
> +
> +static const struct clk_ops nco_ops = {
> +       .set_rate = nco_set_rate,
> +       .recalc_rate = nco_recalc_rate,
> +       .round_rate = nco_round_rate,
> +       .enable = nco_enable,
> +       .disable = nco_disable,
> +       .is_enabled = nco_is_enabled,
> +};
> +
> +static int apple_nco_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk_init_data init;
> +       struct clk_hw_onecell_data *onecell_data;
> +       const char *parent_name;
> +       void __iomem *regs;
> +       struct nco_tables *tbl;
> +       int nchannels;
> +       int ret, i;
> +
> +       ret = of_property_read_u32(np, "apple,nchannels", &nchannels);
> +       if (ret) {
> +               dev_err(&pdev->dev, "missing or invalid apple,nchannels property\n");
> +               return -EINVAL;
> +       }
> +
> +       regs = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +
> +       if (of_clk_get_parent_count(np) != 1)
> +               return -EINVAL;
> +       parent_name = of_clk_get_parent_name(np, 0);

Use clk_parent_data instead please.

> +       if (!parent_name)
> +               return -EINVAL;
> +
> +       onecell_data = devm_kzalloc(&pdev->dev, struct_size(onecell_data, hws,
> +                                                       nchannels), GFP_KERNEL);
> +       if (!onecell_data)
> +               return -ENOMEM;
> +       onecell_data->num = nchannels;
> +
> +       tbl = devm_kzalloc(&pdev->dev, sizeof(*tbl), GFP_KERNEL);
> +       if (!tbl)
> +               return -ENOMEM;
> +       nco_compute_tables(tbl);
> +
> +       for (i = 0; i < nchannels; i++) {
> +               struct nco_channel *chan;
> +
> +               chan = devm_kzalloc(&pdev->dev, sizeof(*chan), GFP_KERNEL);
> +               if (!chan)
> +                       return -ENOMEM;
> +               chan->base = regs + NCO_CHANNEL_STRIDE*i;
> +               chan->tbl = tbl;
> +
> +               memset(&init, 0, sizeof(init));
> +               init.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +                                               "%s-%d", np->name, i);
> +               init.ops = &nco_ops;
> +               init.num_parents = 1;
> +               init.parent_names = &parent_name;
> +               init.flags = 0;
> +
> +               chan->hw.init = &init;
> +               ret = devm_clk_hw_register(&pdev->dev, &chan->hw);
> +               if (ret)
> +                       return ret;
> +
> +               onecell_data->hws[i] = &chan->hw;
> +       }
> +
> +       ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
> +                                                       onecell_data);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id apple_nco_ids[] = {
> +       { .compatible = "apple,nco" },
> +       { },

Nitpick, drop the comma so nothing can come after.

> +};
> +MODULE_DEVICE_TABLE(of, apple_nco_ids)
> +
> +static struct platform_driver apple_nco_driver = {
> +       .driver = {
> +               .name = "apple-nco",
> +               .of_match_table = apple_nco_ids,
> +       },
> +       .probe = apple_nco_probe,
> +};
> +module_platform_driver(apple_nco_driver);