clk: introduce TC9564 clock support

Define the binding for the clock controller functionality present in the Toshiba TC9564 SoC.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3: - What was formerly described as a "clock" node is now "clock-controller"
- The clock-controller DTS node, which dropped its reg property in v2, is now a sub-device of the config syscon
- The config syscon is now (also) compatible with "simple-mfd"
- The clock-controller DTS node no longer defines a toshiba,config-syscon property, and the config syscon no longer requires a label
- The clock-controller now requires a (parent) clock property
- The header file is no longer included in the binding example
- The binding example now includes the definition of fixed 25 MHz clock

 .../bindings/clock/toshiba,tc9564-clock.yaml  | 65 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 include/dt-bindings/clock/toshiba,tc9564.h    | 34 ++++++++++
 3 files changed, 106 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
 create mode 100644 include/dt-bindings/clock/toshiba,tc9564.h

Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +48 -0

new file mode 100644
index 0000000000000..24e07d4aead32
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/toshiba,tc9564-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba TC9564 Clock Controller
+
+maintainers:
+  - Alex Elder <[email protected]>
+  - Daniel Thompson <[email protected]>
+
+description:
+  The Toshiba TC9564 is an SoC accessed by a host system through the
+  upstream PCIe port on the PCIe switch it implements.  The switch
+  includes an embedded PCIe endpoint that provides access to various
+  SoC peripherals (including a clock controller) via its BARs.
+
+  A total of 21 clocks are implemented, though two of these are not
+  controllable.  Access to the clock controller relies on PCIe being
+  functional, so the PCIe clock is assumed to be always on.  Similarly,
+  the PCIe controller relies on I2C, so the I2C clock is also assumed
+  to be always on.
+
+  Clock ids are defined in <dt-bindings/clock/toshiba,tc9564.h>.
+
+  The clock controller uses registers within the same page as a
+  reset controller, so both devices are defined as children of a
+  "config" syscon.
+
+properties:
+  compatible:
+    const: toshiba,tc9564-clock
+
+  clocks:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+required:
+  - compatible
+  - clocks
+  - "#clock-cells"
+
+unevaluatedProperties: false
+
+examples:

Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +7 -0

+
+        clock-controller {
+            compatible = "toshiba,tc9564-clock";
+            clocks = <&tc9564_xo_board>;
+            #clock-cells = <1>;
+        };
+    };

MAINTAINERS +7 -0

index 77bd64bae919f..b19c62eabaa7e 100644
@@ -28166,6 +28166,13 @@ F:	Documentation/devicetree/bindings/media/i2c/toshiba,tc358743.yaml
 F:	drivers/media/i2c/tc358743*
 F:	include/media/i2c/tc358743.h
 
+TOSHIBA TC9564 CLOCK DRIVER
+M:	Alex Elder <[email protected]>
+M:	Daniel Thompson <[email protected]>
+S:	Maintained
+F:	Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
+F:	include/dt-bindings/clock/toshiba,tc9564.h
+
 TOSHIBA TC9564 PCI DRIVER
 M:	Alex Elder <[email protected]>
 M:	Daniel Thompson <[email protected]>

include/dt-bindings/clock/toshiba,tc9564.h new file +34 -0

new file mode 100644
index 0000000000000..2106e4199ed45
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+/*
+ * Copyright (C) 2026 by RISCstar Solutions Corporation.  All rights reserved.
+ */
+
+#ifndef __CLOCK_TOSHIBA_TC9564_H__
+#define __CLOCK_TOSHIBA_TC9564_H__
+
+/* Clock IDs */
+
+#define CLOCK_MCU		0
+#define CLOCK_INTC		1
+#define CLOCK_SRAM		2
+#define CLOCK_UART		3
+#define CLOCK_MSIGEN		4
+#define CLOCK_PLL		5
+#define CLOCK_SGMII		6
+#define CLOCK_REFCLKO		7
+
+#define CLOCK_MAC0_TX		8
+#define CLOCK_MAC0_RX		9
+#define CLOCK_MAC0_125M		10
+#define CLOCK_MAC0_312_5M	11
+#define CLOCK_MAC0_ALL		12
+
+#define CLOCK_MAC1_TX		13
+#define CLOCK_MAC1_RX		14
+#define CLOCK_MAC1_RMII		15
+#define CLOCK_MAC1_125M		16
+#define CLOCK_MAC1_312_5M	17
+#define CLOCK_MAC1_ALL		18
+
+#endif /* __CLOCK_TOSHIBA_TC9564_H__*/

Signature · 1 line

2.53.0

drivers/clk/clk-tc9564.c new file +16 -0

+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to get input clock\n");
+
+	clocks = tc9564_clk_init(dev);
+	if (IS_ERR(clocks))
+		return dev_err_probe(dev, PTR_ERR(clocks),
+				     "failed to initialize clocks\n");
+
+	/* Force all clocks to be initially disabled */
+	tc9564_clock_disable_all(clocks);
+
+	platform_set_drvdata(pdev, clocks);
+
+	return 0;
+}

Define a new platform driver that manages clocks implemented by the Toshiba TC9564 SoC. There are 21 clocks that can be enabled and disabled. Two of these are reserved, so only 19 can be managed.

Two registers manage the state of the clocks. The registers are accessed via a regmap supplied by a system controller, because the region of memory accessed is shared with other drivers.

Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason the PCIe clock cannot be manipulated by this driver (it is assumed to be enabled). Similarly, control is not available for the I2C clock, because the PCIe subsystem on the TC9564 relies on I2C.

Reviewed-by Brian Masney <[email protected]>

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3: - The clock driver now gets its regmap by calling syscon_node_to_regmap()
on its device parent's devicetree node
- The clock driver now must get its parent clock from devicetree
- Added Brian's Reviewed-by tag to the second patch

 MAINTAINERS              |   1 +
 drivers/clk/Kconfig      |  12 ++
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-tc9564.c | 268 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 282 insertions(+)
 create mode 100644 drivers/clk/clk-tc9564.c

MAINTAINERS +1 -0

index 746fc8918714..25443a2de1f7 100644
@@ -28178,6 +28178,7 @@ M:	Alex Elder <[email protected]>
 M:	Daniel Thompson <[email protected]>
 S:	Maintained
 F:	Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
+F:	drivers/clk/clk-tc9564.c
 F:	include/dt-bindings/clock/toshiba,tc9564.h
 
 TOSHIBA TC9564 PCI DRIVER

drivers/clk/Kconfig +0 -0

index 37a8e35d4178..928fff40597e 100644

drivers/clk/Kconfig +9 -0

+	select MFD_SYSCON
+	default TC9564_PCI
+	help
+	  This enables support for the clock controller embedded in the
+	  Toshiba TC9564 SoC.  The state of each clock is controlled via
+	  MMIO to a region managed by a system controller; this ensures
+	  access to the region is coordinated between this and the reset
+	  and XGMAC (stmmac) drivers.
+
 config CLK_TWL
 	tristate "Clock driver for the TWL PMIC family"
 	depends on TWL4030_CORE

drivers/clk/Makefile +1 -0

index 52d138d4ffc8..a11a8326d79c 100644
@@ -111,6 +111,7 @@ obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_COMMON_CLK_SP7021)		+= clk-sp7021.o
 obj-$(CONFIG_COMMON_CLK_STM32F)		+= clk-stm32f4.o
 obj-$(CONFIG_COMMON_CLK_STM32H7)	+= clk-stm32h7.o
+obj-$(CONFIG_COMMON_CLK_TC9564)		+= clk-tc9564.o
 obj-$(CONFIG_COMMON_CLK_TPS68470)      += clk-tps68470.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
 obj-$(CONFIG_CLK_TWL)			+= clk-twl.o

drivers/clk/clk-tc9564.c new file +20 -0

new file mode 100644
index 000000000000..09a4f6a144fd
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (C) 2026 by RISCstar Solutions Corporation.  All rights reserved.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device-id/of.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string_choices.h>
+
+#include <dt-bindings/clock/toshiba,tc9564.h>
+
+#define CLK_CTRL0_OFFSET	0x1004
+#define CLK_CTRL1_OFFSET	0x100c

drivers/clk/clk-tc9564.c new file +8 -0

+};
+
+struct tc9564_clocks {
+	struct device *dev;
+	struct regmap *regmap;
+	size_t clock_count;
+	struct tc9564_clock clocks[] __counted_by(clock_count);
+};

drivers/clk/clk-tc9564.c new file +21 -0

+	TC9564_CLOCK_INIT1(MAC1_ALL, 31),
+};
+#define TC9564_CLOCK_COUNT	ARRAY_SIZE(tc9564_clock_init)
+
+static const struct tc9564_clock *hw_to_tc9564_clock(struct clk_hw *hw)
+{
+	return container_of_const(hw, struct tc9564_clock, hw);
+}
+
+static const struct tc9564_clocks *
+tc9564_clock_to_clocks(const struct tc9564_clock *clock)
+{
+	u32 which = clock->which;
+
+	if (which >= TC9564_CLOCK_COUNT)
+		return ERR_PTR(-ENXIO);
+
+	return container_of_const(clock, struct tc9564_clocks, clocks[which]);
+}
+
+static int tc9564_clk_manage(struct clk_hw *hw, bool enable)

drivers/clk/clk-tc9564.c new file +5 -0

+}
+
+static int tc9564_clk_enable(struct clk_hw *hw)
+{
+	return tc9564_clk_manage(hw, true);

drivers/clk/clk-tc9564.c new file +16 -0

+};
+
+static void tc9564_clock_disable_all(struct tc9564_clocks *clocks)
+{
+	for (u32 i = 0; i < clocks->clock_count; i++) {
+		const struct tc9564_clock *clock = &clocks->clocks[i];
+
+		if (clock->mask)
+			regmap_update_bits(clocks->regmap, clock->offset,
+					   clock->mask, 0);
+	}
+}
+
+static struct clk_hw *tc9564_clk_hw_get(struct of_phandle_args *clkspec,
+					void *data)
+{

drivers/clk/clk-tc9564.c new file +24 -0

+
+static struct tc9564_clocks *tc9564_clk_init(struct device *dev)
+{
+	struct tc9564_clocks *clocks;
+	struct regmap *regmap;
+	size_t clocks_size;
+	int ret;
+
+	regmap = syscon_node_to_regmap(dev_of_node(dev->parent));
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to get config regmap\n");
+		return ERR_CAST(regmap);
+	}
+
+	clocks_size = struct_size(clocks, clocks, TC9564_CLOCK_COUNT);
+	clocks = devm_kzalloc(dev, clocks_size, GFP_KERNEL);
+	if (!clocks)
+		return ERR_PTR(-ENOMEM);
+
+	clocks->dev = dev;
+	clocks->regmap = regmap;
+	clocks->clock_count = TC9564_CLOCK_COUNT;
+
+	for (u32 i = 0; i < TC9564_CLOCK_COUNT; i++) {

drivers/clk/clk-tc9564.c new file +4 -0

+
+		clock = &clocks->clocks[i];
+		clock->hw.init = &init;
+

drivers/clk/clk-tc9564.c new file +14 -0

+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, tc9564_clk_hw_get, clocks);
+	if (ret) {
+		dev_err(dev, "failed to add clock hardware provider\n");
+		return ERR_PTR(ret);
+	}
+
+	return clocks;
+}
+
+static int tc9564_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;

Quoted 8 lines

Define a new platform driver that manages clocks implemented by the Toshiba TC9564 SoC. There are 21 clocks that can be enabled and disabled. Two of these are reserved, so only 19 can be managed.

Two registers manage the state of the clocks. The registers are accessed via a regmap supplied by a system controller, because the region of memory accessed is shared with other drivers.

Quoted 5 lines

Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason the PCIe clock cannot be manipulated by this driver (it is assumed to be enabled). Similarly, control is not available for the I2C clock, because the PCIe subsystem on the TC9564 relies on I2C.

Quoted 37 lines

Reviewed-by Brian Masney <[email protected]>

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3: - The clock driver now gets its regmap by calling syscon_node_to_regmap()
on its device parent's devicetree node
- The clock driver now must get its parent clock from devicetree
- Added Brian's Reviewed-by tag to the second patch

  MAINTAINERS              |   1 +
  drivers/clk/Kconfig      |  12 ++
  drivers/clk/Makefile     |   1 +
  drivers/clk/clk-tc9564.c | 268 +++++++++++++++++++++++++++++++++++++++
  4 files changed, 282 insertions(+)

create mode 100644 drivers/clk/clk-tc9564.c

MAINTAINERS +1 -0

index 746fc8918714..25443a2de1f7 100644
@@ -28178,6 +28178,7 @@ M:	Alex Elder <[email protected]>
  M:	Daniel Thompson <[email protected]>
  S:	Maintained
  F:	Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
+F:	drivers/clk/clk-tc9564.c
  F:	include/dt-bindings/clock/toshiba,tc9564.h
 
  TOSHIBA TC9564 PCI DRIVER

drivers/clk/Kconfig +0 -0

index 37a8e35d4178..928fff40597e 100644
@@ -293,6 +293,18 @@ config COMMON_CLK_S2MPS11
  	  clock. These multi-function devices have two (S2MPS14) or three
  	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
 

drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 298

Quoted 3 lines

drivers/clk/Kconfig +3 -0

+config COMMON_CLK_TC9564
+	tristate "Toshiba TC9564 clock support"
+	depends on TC9564_PCI

Consider "depends on TC9564_PCI || COMPILE_TEST".

This driver gets little build coverage otherwise and it has no compile-time dependency on the PCI driver.

Quoted 54 lines

drivers/clk/Kconfig +9 -0

+	select MFD_SYSCON
+	default TC9564_PCI
+	help
+	  This enables support for the clock controller embedded in the
+	  Toshiba TC9564 SoC.  The state of each clock is controlled via
+	  MMIO to a region managed by a system controller; this ensures
+	  access to the region is coordinated between this and the reset
+	  and XGMAC (stmmac) drivers.
+
  config CLK_TWL
  	tristate "Clock driver for the TWL PMIC family"
  	depends on TWL4030_CORE

drivers/clk/Makefile +1 -0

index 52d138d4ffc8..a11a8326d79c 100644
@@ -111,6 +111,7 @@ obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
  obj-$(CONFIG_COMMON_CLK_SP7021)		+= clk-sp7021.o
  obj-$(CONFIG_COMMON_CLK_STM32F)		+= clk-stm32f4.o
  obj-$(CONFIG_COMMON_CLK_STM32H7)	+= clk-stm32h7.o
+obj-$(CONFIG_COMMON_CLK_TC9564)		+= clk-tc9564.o
  obj-$(CONFIG_COMMON_CLK_TPS68470)      += clk-tps68470.o
  obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
  obj-$(CONFIG_CLK_TWL)			+= clk-twl.o

drivers/clk/clk-tc9564.c new file +24 -0

new file mode 100644
index 000000000000..09a4f6a144fd
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (C) 2026 by RISCstar Solutions Corporation.  All rights reserved.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device-id/of.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string_choices.h>
+
+#include <dt-bindings/clock/toshiba,tc9564.h>
+
+#define CLK_CTRL0_OFFSET	0x1004
+#define CLK_CTRL1_OFFSET	0x100c
+
+struct tc9564_clock_init {
+	const char *name;	/* NULL means unused entry */
+	u32 offset;

drivers/clk/clk-tc9564.c line 30

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+	u32 mask;
+};
+
+struct tc9564_clock {
+	struct clk_hw hw;
+	u32 which;

Seems like this is only used to get back to the regmap, isn't it ?

drivers/clk/clk-tc9564.c line 32

Quoted 2 lines

drivers/clk/clk-tc9564.c new file +2 -0

+	u32 offset;		/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */
+	u32 mask;		/* Zero means undefined clock */

Is there any such undefined clock ?

Quoted 13 lines

drivers/clk/clk-tc9564.c new file +13 -0

+};
+
+struct tc9564_clocks {
+	struct device *dev;
+	struct regmap *regmap;
+	size_t clock_count;
+	struct tc9564_clock clocks[] __counted_by(clock_count);
+};
+
+#define TC9564_CLOCK_INIT0(_name, _bit)	__TC9564_CLOCK_INIT(_name, 0, _bit)
+#define TC9564_CLOCK_INIT1(_name, _bit)	__TC9564_CLOCK_INIT(_name, 1, _bit)
+
+#define __TC9564_CLOCK_INIT(_name, _reg, _bit)		\

drivers/clk/clk-tc9564.c line 50

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+	[CLOCK_##_name] = {				\
+		.name	= #_name,			\
+		.offset	= CLK_CTRL ## _reg ## _OFFSET,	\
+		.mask	= BIT(_bit),			\
+	}

Those clock still have no parent, so my previous comment regarding unparented clock remains unaddressed.

drivers/clk/clk-tc9564.c line 60

Quoted 11 lines

drivers/clk/clk-tc9564.c new file +10 -0

+
+/*
+ * The PCIe and I2C clocks are controllable, but we depend on PCIe
+ * and that relies on I2C, so we don't allow them to be disabled.
+ */
+static const struct tc9564_clock_init tc9564_clock_init[] = {
+	TC9564_CLOCK_INIT0(MCU, 0),
+	TC9564_CLOCK_INIT0(INTC, 4),
+	/* TC9564_CLOCK_INIT0(PCIE, 9), */
+	/* TC9564_CLOCK_INIT0(I2C, 12), */

Please don't leave commented-out code. The comment above already explains why these two clocks are not exposed.

BTW, the comment points more toward CLK_IS_CRITICAL, rather than removing the clocks.

None of the consumer will ever need those clocks ?

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+	TC9564_CLOCK_INIT0(SRAM, 13),
+	TC9564_CLOCK_INIT0(UART, 16),
+	TC9564_CLOCK_INIT0(MSIGEN, 18),
+	TC9564_CLOCK_INIT0(PLL, 24),
+	TC9564_CLOCK_INIT0(SGMII, 25),
+	TC9564_CLOCK_INIT0(REFCLKO, 26),

drivers/clk/clk-tc9564.c line 71

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+
+	TC9564_CLOCK_INIT0(MAC0_TX, 7),
+	TC9564_CLOCK_INIT0(MAC0_RX, 14),
+	TC9564_CLOCK_INIT0(MAC0_125M, 29),
+	TC9564_CLOCK_INIT0(MAC0_312_5M, 30),

Are those really just providing the oscillator parent ? That is what they will appear to do without a proper recalc_rate()

Their names suggest that maybe, it is not the osc coming out of them ...

drivers/clk/clk-tc9564.c line 78

Quoted 7 lines

drivers/clk/clk-tc9564.c new file +7 -0

+	TC9564_CLOCK_INIT0(MAC0_ALL, 31),
+
+	TC9564_CLOCK_INIT1(MAC1_TX, 7),
+	TC9564_CLOCK_INIT1(MAC1_RX, 14),
+	TC9564_CLOCK_INIT1(MAC1_RMII, 15),
+	TC9564_CLOCK_INIT1(MAC1_125M, 29),
+	TC9564_CLOCK_INIT1(MAC1_312_5M, 30),

same here

Quoted 25 lines

drivers/clk/clk-tc9564.c new file +25 -0

+	TC9564_CLOCK_INIT1(MAC1_ALL, 31),
+};
+#define TC9564_CLOCK_COUNT	ARRAY_SIZE(tc9564_clock_init)
+
+static const struct tc9564_clock *hw_to_tc9564_clock(struct clk_hw *hw)
+{
+	return container_of_const(hw, struct tc9564_clock, hw);
+}
+
+static const struct tc9564_clocks *
+tc9564_clock_to_clocks(const struct tc9564_clock *clock)
+{
+	u32 which = clock->which;
+
+	if (which >= TC9564_CLOCK_COUNT)
+		return ERR_PTR(-ENXIO);
+
+	return container_of_const(clock, struct tc9564_clocks, clocks[which]);
+}
+
+static int tc9564_clk_manage(struct clk_hw *hw, bool enable)
+{
+	const struct tc9564_clock *clock = hw_to_tc9564_clock(hw);
+	const struct tc9564_clocks *clocks;
+	u32 offset = clock->offset;

drivers/clk/clk-tc9564.c line 109

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+	u32 mask = clock->mask;
+
+	clocks = tc9564_clock_to_clocks(clock);
+	if (IS_ERR(clocks) || !mask) {
+		dev_err(clocks->dev, "invalid clock (%s id %u)\n",
+			str_enable_disable(enable), clock->which);

clocks is an error pointer at this point so this will oops.

Good thing is that this is dead code because it manifest only if there a hole in your table, which do not have ATM. Drop everything related to this hole managment that you did not test ... you'll introduce it when you need it and test it

drivers/clk/clk-tc9564.c line 114

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+		return -ENXIO;
+	}
+
+	return regmap_update_bits(clocks->regmap, offset, mask,
+				  enable ? mask : 0);

The dance around clocks and 'which' is only to get to this regmap, right ?

You could just use clk_hw_get_dev() and use the drvdata to get back to it. That would be a lot simpler.

Quoted 10 lines

drivers/clk/clk-tc9564.c new file +9 -0

+}
+
+static int tc9564_clk_enable(struct clk_hw *hw)
+{
+	return tc9564_clk_manage(hw, true);
+}
+
+static void tc9564_clk_disable(struct clk_hw *hw)
+{

drivers/clk/clk-tc9564.c line 129

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+	(void)tc9564_clk_manage(hw, false);
+}
+
+static const struct clk_ops tc9564_clk_ops = {
+	.enable = tc9564_clk_enable,
+	.disable = tc9564_clk_disable,

Please implement is_enabled(). This helps disabling unused clocks.

Quoted 21 lines

drivers/clk/clk-tc9564.c new file +20 -0

+};
+
+static void tc9564_clock_disable_all(struct tc9564_clocks *clocks)
+{
+	for (u32 i = 0; i < clocks->clock_count; i++) {
+		const struct tc9564_clock *clock = &clocks->clocks[i];
+
+		if (clock->mask)
+			regmap_update_bits(clocks->regmap, clock->offset,
+					   clock->mask, 0);
+	}
+}
+
+static struct clk_hw *tc9564_clk_hw_get(struct of_phandle_args *clkspec,
+					void *data)
+{
+	struct tc9564_clocks *clocks = data;
+	u32 i = clkspec->args[0];
+
+	if (i < clocks->clock_count)

drivers/clk/clk-tc9564.c line 155

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+		return &clocks->clocks[i].hw;
+
+	dev_err(clocks->dev, "invalid index %u\n", i);
+
+	return ERR_PTR(-EINVAL);
+}

clk_hw_onecell_data and register it with of_clk_hw_onecell_get() maybe ?

Quoted 25 lines

drivers/clk/clk-tc9564.c new file +24 -0

+
+static struct tc9564_clocks *tc9564_clk_init(struct device *dev)
+{
+	struct tc9564_clocks *clocks;
+	struct regmap *regmap;
+	size_t clocks_size;
+	int ret;
+
+	regmap = syscon_node_to_regmap(dev_of_node(dev->parent));
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to get config regmap\n");
+		return ERR_CAST(regmap);
+	}
+
+	clocks_size = struct_size(clocks, clocks, TC9564_CLOCK_COUNT);
+	clocks = devm_kzalloc(dev, clocks_size, GFP_KERNEL);
+	if (!clocks)
+		return ERR_PTR(-ENOMEM);
+
+	clocks->dev = dev;
+	clocks->regmap = regmap;
+	clocks->clock_count = TC9564_CLOCK_COUNT;
+
+	for (u32 i = 0; i < TC9564_CLOCK_COUNT; i++) {

drivers/clk/clk-tc9564.c line 189

Quoted 10 lines

drivers/clk/clk-tc9564.c new file +10 -0

+		const struct tc9564_clock_init *clock_init;
+		struct clk_init_data init = { };
+		struct tc9564_clock *clock;
+
+		clock_init = &tc9564_clock_init[i];
+		if (!clock_init->name)
+			continue;
+
+		init.name = clock_init->name;
+		init.ops = &tc9564_clk_ops;

No parent is declared. The input clock from the binding should be the parent of every gate:

	init.parent_data = &(const struct clk_parent_data){ .index = 0 };
	init.num_parents = 1;

Without a parent, these clocks are orphans with rate 0, and enabling one never enables the input if it needs to be.

Quoted 10 lines

drivers/clk/clk-tc9564.c new file +10 -0

+
+		clock = &clocks->clocks[i];
+		clock->hw.init = &init;
+
+		ret = devm_clk_hw_register(dev, &clock->hw);
+		if (ret) {
+			dev_err(dev, "failed to register clock \"%s\"\n",
+				init.name);
+			return ERR_PTR(ret);
+		}

drivers/clk/clk-tc9564.c line 203

Quoted 4 lines

drivers/clk/clk-tc9564.c new file +4 -0

+
+		clock->which = i;
+		clock->offset = clock_init->offset;
+		clock->mask = clock_init->mask;

These fields are set after the clock is registered. Please fill in the clock completely before calling devm_clk_hw_register(). Ops may be called as soon as the clock is registered

Quoted 19 lines

drivers/clk/clk-tc9564.c new file +18 -0

+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, tc9564_clk_hw_get, clocks);
+	if (ret) {
+		dev_err(dev, "failed to add clock hardware provider\n");
+		return ERR_PTR(ret);
+	}
+
+	return clocks;
+}
+
+static int tc9564_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tc9564_clocks *clocks;
+	struct clk *clk;
+
+	if (!dev_of_node(dev))

drivers/clk/clk-tc9564.c line 227

Quoted 6 lines

drivers/clk/clk-tc9564.c new file +6 -0

+		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to get input clock\n");

Nothing is using this. This is not how you set the parent. See above

drivers/clk/clk-tc9564.c line 235

Quoted 8 lines

drivers/clk/clk-tc9564.c new file +8 -0

+
+	clocks = tc9564_clk_init(dev);
+	if (IS_ERR(clocks))
+		return dev_err_probe(dev, PTR_ERR(clocks),
+				     "failed to initialize clocks\n");
+
+	/* Force all clocks to be initially disabled */
+	tc9564_clock_disable_all(clocks);

Why is this needed ? In general a provider should never act the provided clock on its own. Let the framework deal with the used clocks

At this point you've already register the provider so you may be turning off a clock on a consumer already using it and also mess with the enable count.

drivers/clk/clk-tc9564.c line 247

Quoted 13 lines

drivers/clk/clk-tc9564.c new file +12 -0

+
+	platform_set_drvdata(pdev, clocks);
+
+	return 0;
+}
+
+static void tc9564_clk_remove(struct platform_device *pdev)
+{
+	struct tc9564_clocks *clocks = platform_get_drvdata(pdev);
+
+	/* Leave all clocks disabled when done */
+	tc9564_clock_disable_all(clocks);

Same type of problem here, there may be still active consumer at this point. Please drop .remove() entirely.

Quoted 7 lines

drivers/clk/clk-tc9564.c line 71

Quoted 4 lines

drivers/clk/clk-tc9564.c new file +4 -0

+	TC9564_CLOCK_INIT0(MAC0_TX, 7),
+	TC9564_CLOCK_INIT0(MAC0_RX, 14),
+	TC9564_CLOCK_INIT0(MAC0_125M, 29),
+	TC9564_CLOCK_INIT0(MAC0_312_5M, 30),

Are those really just providing the oscillator parent ? That is what they will appear to do without a proper recalc_rate()

They are fixed clocks that can be gated, but their rate cannot be changed.

Looking at __clk_core_init(), should I define a recalc_rate() callback that just returns the equivalent of get_rate()?

Quoted 2 lines

Their names suggest that maybe, it is not the osc coming out of them ...

We have requested a clock tree that explains exactly how these clock rates are derived from the 25 MHz oscillator. They're all fixed rates, and although we could easily speculate how they're derived, we don't have the documentation.

Quoted 1 line

Quoted 1 line

drivers/clk/clk-tc9564.c new file +1 -0

+	TC9564_CLOCK_INIT0(MAC0_ALL, 31),

Quoted 7 lines

drivers/clk/clk-tc9564.c line 227

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to get input clock\n");

Nothing is using this. This is not how you set the parent. See above

It will once I have the clocks get properly parented.

Quoted 12 lines

drivers/clk/clk-tc9564.c line 235

Quoted 9 lines

drivers/clk/clk-tc9564.c new file +8 -0

+
+	clocks = tc9564_clk_init(dev);
+	if (IS_ERR(clocks))
+		return dev_err_probe(dev, PTR_ERR(clocks),
+				     "failed to initialize clocks\n");
+
+	/* Force all clocks to be initially disabled */
+	tc9564_clock_disable_all(clocks);

Why is this needed ? In general a provider should never act the provided clock on its own. Let the framework deal with the used clocks

The intent was to start with everything disabled--a known state. Sashiko pointed out this races with a user that could get the clock right away, so I intend to do this by initially disabling all the clocks before they're all registered.

Quoted 10 lines · Alex Elder

Quoted 7 lines

drivers/clk/clk-tc9564.c line 71

Quoted 4 lines

drivers/clk/clk-tc9564.c new file +4 -0

+	TC9564_CLOCK_INIT0(MAC0_TX, 7),
+	TC9564_CLOCK_INIT0(MAC0_RX, 14),
+	TC9564_CLOCK_INIT0(MAC0_125M, 29),
+	TC9564_CLOCK_INIT0(MAC0_312_5M, 30),

Are those really just providing the oscillator parent ? That is what they will appear to do without a proper recalc_rate()

They are fixed clocks that can be gated, but their rate cannot be changed.

It is not a problem if the rate cannot be changed but still a consumer could call clk_get_rate() and change its behavior based on the result, so it is better if it does reflect the reality to some extent.

Just add fixed_factor clock elements on the path of clocks where do know the output does not match the input osc

Quoted 4 lines

Looking at __clk_core_init(), should I define a recalc_rate() callback that just returns the equivalent of get_rate()?

Quoted 8 lines

Quoted 2 lines

Their names suggest that maybe, it is not the osc coming out of them ...

We have requested a clock tree that explains exactly how these clock rates are derived from the 25 MHz oscillator. They're all fixed rates, and although we could easily speculate how they're derived, we don't have the documentation.

If you know the input and output, you have enough to figure out a fixed factor. If later on someone gets more information than you, they'll just modify the clock tree.

If you no idea what the ouput rate is, at least clearly say so in the comment so a future user or contributor can easily understand the situation.

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/clk/clk-tc9564.c new file +1 -0

+	TC9564_CLOCK_INIT0(MAC0_ALL, 31),

[...]

Quoted 8 lines

Quoted 7 lines

drivers/clk/clk-tc9564.c line 227

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to get input clock\n");

Nothing is using this. This is not how you set the parent. See above

Quoted 1 line

It will once I have the clocks get properly parented.

No it won't. You don't need a consumer clock reference. just drop it.

Quoted 12 lines

Quoted 11 lines

drivers/clk/clk-tc9564.c line 235

Quoted 8 lines

drivers/clk/clk-tc9564.c new file +8 -0

+
+	clocks = tc9564_clk_init(dev);
+	if (IS_ERR(clocks))
+		return dev_err_probe(dev, PTR_ERR(clocks),
+				     "failed to initialize clocks\n");
+
+	/* Force all clocks to be initially disabled */
+	tc9564_clock_disable_all(clocks);

Why is this needed ? In general a provider should never act the provided clock on its own. Let the framework deal with the used clocks

Quoted 5 lines

The intent was to start with everything disabled--a known state. Sashiko pointed out this races with a user that could get the clock right away, so I intend to do this by initially disabling all the clocks before they're all registered.

We do not do that because (in general) there maybe devices already using a clock provided by your device and setup at a prior stage (like the bootloader). Doing so would break them.

Leave this alone, the framework will disable the unused clock for you once the init is done and most devices have probed.

Quoted 18 lines · Jerome Brunet

Quoted 10 lines · Alex Elder

Quoted 7 lines

drivers/clk/clk-tc9564.c line 71

Quoted 4 lines

drivers/clk/clk-tc9564.c new file +4 -0

+	TC9564_CLOCK_INIT0(MAC0_TX, 7),
+	TC9564_CLOCK_INIT0(MAC0_RX, 14),
+	TC9564_CLOCK_INIT0(MAC0_125M, 29),
+	TC9564_CLOCK_INIT0(MAC0_312_5M, 30),

Are those really just providing the oscillator parent ? That is what they will appear to do without a proper recalc_rate()

They are fixed clocks that can be gated, but their rate cannot be changed.

It is not a problem if the rate cannot be changed but still a consumer could call clk_get_rate() and change its behavior based on the result, so it is better if it does reflect the reality to some extent.

Quoted 2 lines

Just add fixed_factor clock elements on the path of clocks where do know the output does not match the input osc

OK. And because I'm not sure whether it matches reality, I'll do so in the code, and not expose these in devicetree. (If this is not correct, please say so.)

Quoted 13 lines

Quoted 4 lines

Looking at __clk_core_init(), should I define a recalc_rate() callback that just returns the equivalent of get_rate()?

Quoted 8 lines

Quoted 2 lines

Their names suggest that maybe, it is not the osc coming out of them ...

We have requested a clock tree that explains exactly how these clock rates are derived from the 25 MHz oscillator. They're all fixed rates, and although we could easily speculate how they're derived, we don't have the documentation.

Quoted 3 lines

If you know the input and output, you have enough to figure out a fixed factor. If later on someone gets more information than you, they'll just modify the clock tree.

OK.

Quoted 3 lines

If you no idea what the ouput rate is, at least clearly say so in the comment so a future user or contributor can easily understand the situation.

I'm pretty sure I know the output rate of all clocks, but if not I'll make sure there is such a comment.

Quoted 13 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/clk/clk-tc9564.c new file +1 -0

+	TC9564_CLOCK_INIT0(MAC0_ALL, 31),

[...]

Quoted 8 lines

Quoted 7 lines

drivers/clk/clk-tc9564.c line 227

Quoted 5 lines

drivers/clk/clk-tc9564.c new file +5 -0

+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to get input clock\n");

Nothing is using this. This is not how you set the parent. See above

Quoted 3 lines

Quoted 1 line

It will once I have the clocks get properly parented.

No it won't. You don't need a consumer clock reference. just drop it.

OK, yes, the parent(s) will be specified in the init data. Sorry for this, and thank you for pointing it out.

Quoted 18 lines

Quoted 12 lines

Quoted 11 lines

drivers/clk/clk-tc9564.c line 235

Quoted 8 lines

drivers/clk/clk-tc9564.c new file +8 -0

+
+	clocks = tc9564_clk_init(dev);
+	if (IS_ERR(clocks))
+		return dev_err_probe(dev, PTR_ERR(clocks),
+				     "failed to initialize clocks\n");
+
+	/* Force all clocks to be initially disabled */
+	tc9564_clock_disable_all(clocks);

Why is this needed ? In general a provider should never act the provided clock on its own. Let the framework deal with the used clocks

Quoted 5 lines

The intent was to start with everything disabled--a known state. Sashiko pointed out this races with a user that could get the clock right away, so I intend to do this by initially disabling all the clocks before they're all registered.

Quoted 6 lines

We do not do that because (in general) there maybe devices already using a clock provided by your device and setup at a prior stage (like the bootloader). Doing so would break them.

Leave this alone, the framework will disable the unused clock for you once the init is done and most devices have probed.

OK, I'll drop this. Thank you very much for your review.

-Alex

The TC9564 SoC has a single 25 MHz fixed-rate input clock. Add it to the definition associated with the chip.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3: - New patch in this version of the series

 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +0 -0

index 0e3bbdaf4661f..b8d290eed2f38 100644

Quoted 20 lines · Alex Elder

The TC9564 SoC has a single 25 MHz fixed-rate input clock. Add it to the definition associated with the chip.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3: - New patch in this version of the series

  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 6 ++++++
  1 file changed, 6 insertions(+)

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +0 -0

index 0e3bbdaf4661f..b8d290eed2f38 100644
@@ -891,6 +891,12 @@ pcie@0,0 {
  		pinctrl-0 = <&tc9563_resx_n>;
  		pinctrl-names = "default";
 

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts pcie@0,0 { · line 898

Quoted 5 lines

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +5 -0

+		clock-25000000 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <25000000>;
+		};

This and patch 4 should be squashed

Konrad

The clock controller within the TC9564 SoC is accessed via a PCI endpoint bus through BAR 4 of the first embedded PCIe function. A syscon node is created to provide access to a range of memory that is used by the clock controller and shared with other devices.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---

Quoted 9 lines · Alex Elder

The clock controller within the TC9564 SoC is accessed via a PCI endpoint bus through BAR 4 of the first embedded PCIe function. A syscon node is created to provide access to a range of memory that is used by the clock controller and shared with other devices.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---

[...]

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts pci-ep-bus@4 { · line 1009

Quoted 11 lines

+&pcie_s1b5d0f0_bar4_bus {

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +10 -0

+	syscon@0 {
+		compatible = "syscon", "simple-mfd";
+		reg = <0x0 0x2000>;
+
+		tc9564_clock0: clock-controller {
+			compatible = "toshiba,tc9564-clock";
+			clocks = <&tc9564_xo_board>;
+			#clock-cells = <1>;
+		};
+	};

It's the same file - just put the node under the ep-bus definition

Konrad

The clock controller within the TC9564 SoC is accessed via a PCI endpoint bus through BAR 4 of the first embedded PCIe function. A syscon node is created to provide access to a range of memory that is used by the clock controller and shared with other devices.

Co-developed-by Daniel Thompson <[email protected]>

Signed-off-by Daniel Thompson <[email protected]>

Signed-off-by Alex Elder <[email protected]>

---
v3:
- What was formerly described as a "clock" node is now "clock-controller"
- Label for the 25 MHz input clock is added (new in this version)
- The clock-controller node is now a sub-device of the config syscon
- The config syscon is now (also) compatible with "simple-mfd"
- The clock-controller DTS node no longer defines a toshiba,config-syscon property, and the config syscon no longer requires a label
- The clock-controller now requires a (parent) clock property, which refers to the new 25 MHz clock

 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +0 -0

index b8d290eed2f38..e7601daf1bc5a 100644

arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +2 -2

 #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
 #include <dt-bindings/iio/qcom,spmi-adc7-pm7325.h>
 #include <dt-bindings/leds/common.h>

@@ -891,7 +892,7 @@ pcie@0,0 {
 		pinctrl-0 = <&tc9563_resx_n>;
 		pinctrl-names = "default";
 
-		clock-25000000 {
+		tc9564_xo_board: clock-25000000 {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <25000000>;

@@ -967,7 +968,7 @@ dev@0,0 {
 				#size-cells = <2>;
 				ranges;	/* This will be updated dynamically */
 
-				pci-ep-bus@4 {
+				pcie_s1b5d0f0_bar4_bus: pci-ep-bus@4 {
 					compatible = "simple-bus";
 					#address-cells = <1>;
 					#size-cells = <1>;

@@ -995,6 +996,19 @@ pci-ep-bus@4 {
 	};
 };

This series adds support for the clock controller present within the Toshiba TC9564 SoC.

This SoC attaches to a host system via PCIe. It includes a PCIe switch with three downstream ports. To one of these ports an embedded PCIe endpoint is attached, and it implements two PCI functions. BAR 4 for both PCI functions provides access to the registers that enable or disable a set of clocks.

Both TC9564 PCI functions can provide PCI endpoint buses via devicetree. Each PCI endpoint bus provides access to one of the function's BARs. Subordinate devices (like the clock controller) can then be defined as nodes accessible on the endpoint bus.

Only one of the two PCI functions is permitted to define the clock device node.

-Alex

NOTE:
This series is built upon linux-next, with this patch applied:
https://lore.kernel.org/lkml/[email protected]/

Quoted 17 lines

This series adds support for the clock controller present within the Toshiba TC9564 SoC.

This SoC attaches to a host system via PCIe. It includes a PCIe switch with three downstream ports. To one of these ports an embedded PCIe endpoint is attached, and it implements two PCI functions. BAR 4 for both PCI functions provides access to the registers that enable or disable a set of clocks.

Both TC9564 PCI functions can provide PCI endpoint buses via devicetree. Each PCI endpoint bus provides access to one of the function's BARs. Subordinate devices (like the clock controller) can then be defined as nodes accessible on the endpoint bus.

Only one of the two PCI functions is permitted to define the clock device node.

Quoted 5 lines

-Alex

NOTE:
This series is built upon linux-next, with this patch applied:
https://lore.kernel.org/lkml/[email protected]/

next AND a patch on top ?? you are not making this easy and this is mostly to accommodate the DT change that could have been sent separately.

Basing on linux-next means there are other deps and you just did not explain. This makes it very difficult for me to apply your series if I wanted to.

Next time around, please send a series on the clk list with only the 2 first change. That way we do not have to cherry pick, we can use your cover letter for the merge commit, which makes it easier to provide an immutable branch to another subsystem, if need be.

*Clearly* list you dependencies if any. Please consider using b4 to list the pre-requisite.

Thanks