[PATCH v3 0/4] clk: introduce TC9564 clock support

[PATCH v3 0/4] clk: introduce TC9564 clock support

[Alex Elder]

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/20261002181849.790275-1-elder@riscstar.com/
    The series (with that patch) is available in this git branch:
      https://github.com/alexelder/linux/tree/outgoing/clock-v3

Between version 2 and version 3:
- 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

- 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 its parent clock from devicetree
- Added Brian's Reviewed-by tag to the second patch

- A new 25 MHz fixed clock is defined in a new patch, and is later used
  by the clock controller


Version 2 is available here:
  https://lore.kernel.org/lkml/20261005230927.2000398-1-elder@riscstar.com/

Between version 1 and version 2:
  - Previously, this driver also included reset functionality.  That
    is now in a separate reset (only) driver, which will be sent for
    review separately
  - Dropped the "config syscon" devicetree binding
  - Reworded the clock binding description to focus on hardware only
  - Clock IDs are now consecutive (no more commented-out values)
  - Get the definition of of_device_id from <linux/device_id/of.h>
    rather than <linux/platform_device.h>
  - Dropped a comma that followed a ternminating empty array entry
  - Config option COMMON_CLK_TC9564 now selects MFD_SYSCON
  - Clarified in config help text that the reset and stmmac drivers
    are the "others" that use the config syscon
  - Moved the clock devicetree node to be a peer (rather than a child) of
    the syscon node.

Version 1 is available here:
  https://lore.kernel.org/lkml/20260918165234.687224-1-elder@riscstar.com/

Alex Elder (4):
  dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
  clk: toshiba: introduce a TC9564 SoC clock driver
  arm64: dts: qcom: define a fixed 25 MHz clock
  arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller

 .../bindings/clock/toshiba,tc9564-clock.yaml  |  65 +++++
 MAINTAINERS                                   |   8 +
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts  |  22 +-
 drivers/clk/Kconfig                           |  12 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-tc9564.c                      | 268 ++++++++++++++++++
 include/dt-bindings/clock/toshiba,tc9564.h    |  34 +++
 7 files changed, 409 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
 create mode 100644 drivers/clk/clk-tc9564.c
 create mode 100644 include/dt-bindings/clock/toshiba,tc9564.h


base-commit: dc44eb73f8da11f478815bc29b6509a6a4c5c35f
-- 
2.53.0

[PATCH v3 1/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml

[Alex Elder]

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

Co-developed-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
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

diff --git a/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml b/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
new file mode 100644
index 0000000000000..24e07d4aead32
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
@@ -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 <elder@riscstar.com>
+  - Daniel Thompson <daniel@riscstar.com>
+
+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:
+  - |
+    tc9564_xo_board: clock-25000000 {
+        compatible = "fixed-clock";
+        clock-frequency = <25000000>;
+        #clock-cells = <0>;
+    };
+
+    syscon@0 {
+        compatible = "syscon", "simple-mfd";
+        reg = <0x0 0x2000>;
+
+        clock-controller {
+            compatible = "toshiba,tc9564-clock";
+            clocks = <&tc9564_xo_board>;
+            #clock-cells = <1>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 77bd64bae919f..b19c62eabaa7e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <elder@kernel.org>
+M:	Daniel Thompson <danielt@kernel.org>
+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 <elder@kernel.org>
 M:	Daniel Thompson <danielt@kernel.org>
diff --git a/include/dt-bindings/clock/toshiba,tc9564.h b/include/dt-bindings/clock/toshiba,tc9564.h
new file mode 100644
index 0000000000000..2106e4199ed45
--- /dev/null
+++ b/include/dt-bindings/clock/toshiba,tc9564.h
@@ -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__*/
-- 
2.53.0

Re: [PATCH v3 1/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml

[Conor Dooley]

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

On Mon, Oct 12, 2026 at 10:24:04AM -0500, Alex Elder wrote:
> Define the binding for the clock controller functionality present in
> the Toshiba TC9564 SoC.
> 
> Co-developed-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> 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
> 
> diff --git a/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml b/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
> new file mode 100644
> index 0000000000000..24e07d4aead32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml
> @@ -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 <elder@riscstar.com>
> +  - Daniel Thompson <daniel@riscstar.com>
> +
> +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:
> +  - |
> +    tc9564_xo_board: clock-25000000 {
> +        compatible = "fixed-clock";
> +        clock-frequency = <25000000>;
> +        #clock-cells = <0>;
> +    };
> +
> +    syscon@0 {
> +        compatible = "syscon", "simple-mfd";
> +        reg = <0x0 0x2000>;

Same comments apply here as did on the reset, so I won't bother
discussing it in two places at once.

I'm going to mark this CR so that the lads don't need to look at it,
given that you need to resubmit anyway for the driver side.
pw-bot: changes-requested

Thanks,
Conor.

> +
> +        clock-controller {
> +            compatible = "toshiba,tc9564-clock";
> +            clocks = <&tc9564_xo_board>;
> +            #clock-cells = <1>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77bd64bae919f..b19c62eabaa7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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 <elder@kernel.org>
> +M:	Daniel Thompson <danielt@kernel.org>
> +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 <elder@kernel.org>
>  M:	Daniel Thompson <danielt@kernel.org>
> diff --git a/include/dt-bindings/clock/toshiba,tc9564.h b/include/dt-bindings/clock/toshiba,tc9564.h
> new file mode 100644
> index 0000000000000..2106e4199ed45
> --- /dev/null
> +++ b/include/dt-bindings/clock/toshiba,tc9564.h
> @@ -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__*/
> -- 
> 2.53.0
> 

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

[PATCH v3 2/4] clk: toshiba: introduce a TC9564 SoC clock driver

[Alex Elder]

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 <masneyb@kernel.org>
Co-developed-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
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

diff --git a/MAINTAINERS b/MAINTAINERS
index b19c62eabaa7e..20e076fc6221c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28171,6 +28171,7 @@ M:	Alex Elder <elder@kernel.org>
 M:	Daniel Thompson <danielt@kernel.org>
 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
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 37a8e35d41783..928fff40597ee 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -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.
 
+config COMMON_CLK_TC9564
+	tristate "Toshiba TC9564 clock support"
+	depends on TC9564_PCI
+	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
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 52d138d4ffc88..a11a8326d79cb 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -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
diff --git a/drivers/clk/clk-tc9564.c b/drivers/clk/clk-tc9564.c
new file mode 100644
index 0000000000000..09a4f6a144fd9
--- /dev/null
+++ b/drivers/clk/clk-tc9564.c
@@ -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;
+	u32 mask;
+};
+
+struct tc9564_clock {
+	struct clk_hw hw;
+	u32 which;
+	u32 offset;		/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */
+	u32 mask;		/* Zero means undefined clock */
+};
+
+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)		\
+	[CLOCK_##_name] = {				\
+		.name	= #_name,			\
+		.offset	= CLK_CTRL ## _reg ## _OFFSET,	\
+		.mask	= BIT(_bit),			\
+	}
+
+/*
+ * 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), */
+	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),
+
+	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),
+	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),
+	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;
+	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);
+		return -ENXIO;
+	}
+
+	return regmap_update_bits(clocks->regmap, offset, mask,
+				  enable ? mask : 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)
+{
+	(void)tc9564_clk_manage(hw, false);
+}
+
+static const struct clk_ops tc9564_clk_ops = {
+	.enable = tc9564_clk_enable,
+	.disable = tc9564_clk_disable,
+};
+
+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)
+		return &clocks->clocks[i].hw;
+
+	dev_err(clocks->dev, "invalid index %u\n", i);
+
+	return ERR_PTR(-EINVAL);
+}
+
+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++) {
+		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;
+
+		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);
+		}
+
+		clock->which = i;
+		clock->offset = clock_init->offset;
+		clock->mask = clock_init->mask;
+	}
+
+	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))
+		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");
+
+	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;
+}
+
+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);
+}
+
+static const struct of_device_id tc9564_clk_ids[] = {
+	{ .compatible = "toshiba,tc9564-clock" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tc9564_clk_ids);
+
+static struct platform_driver tc9564_clk_driver = {
+	.probe = tc9564_clk_probe,
+	.remove = tc9564_clk_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = tc9564_clk_ids,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+module_platform_driver(tc9564_clk_driver);
+
+MODULE_DESCRIPTION("Toshiba TC9564 Clock Driver");
+MODULE_LICENSE("GPL");
-- 
2.53.0

Re: [PATCH v3 2/4] clk: toshiba: introduce a TC9564 SoC clock driver

[Alex Elder]

On 10/12/26 10:24 AM, Alex Elder wrote:
> +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))
> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
> +
> +	clk = devm_clk_get(dev, NULL);

This should be devm_clk_get_enabled().

(It still works this way, given it's an always-on fixed clock.)

					-Alex

> +	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;
> +}

Re: [PATCH v3 2/4] clk: toshiba: introduce a TC9564 SoC clock driver

[Jerome Brunet]

> 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 <masneyb@kernel.org>
> Co-developed-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> 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
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 746fc8918714..25443a2de1f7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -28178,6 +28178,7 @@ M:	Alex Elder <elder@kernel.org>
>  M:	Daniel Thompson <danielt@kernel.org>
>  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
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 37a8e35d4178..928fff40597e 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -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.
>  
> +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.

> +	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
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 52d138d4ffc8..a11a8326d79c 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -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
> diff --git a/drivers/clk/clk-tc9564.c b/drivers/clk/clk-tc9564.c
> new file mode 100644
> index 000000000000..09a4f6a144fd
> --- /dev/null
> +++ b/drivers/clk/clk-tc9564.c
> @@ -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;
> +	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 ?

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

Is there any such undefined clock ?

> +};
> +
> +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)		\
> +	[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. 

> +
> +/*
> + * 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 ?

> +	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),
> +
> +	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 ...

> +	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

> +	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;
> +	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

> +		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.

> +}
> +
> +static int tc9564_clk_enable(struct clk_hw *hw)
> +{
> +	return tc9564_clk_manage(hw, true);
> +}
> +
> +static void tc9564_clk_disable(struct clk_hw *hw)
> +{
> +	(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.

> +};
> +
> +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)
> +		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 ?

> +
> +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++) {
> +		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.

> +
> +		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);
> +		}
> +
> +		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

> +	}
> +
> +	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))
> +		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

> +
> +	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.

> +
> +	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.

-- 
Jerome

Re: [PATCH v3 2/4] clk: toshiba: introduce a TC9564 SoC clock driver

[Alex Elder]

On 10/12/26 2:10 PM, Jerome Brunet wrote:
>> 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.

Thanks a lot for this review.  I respond to all of your
comments below.

>> Reviewed-by: Brian Masney <masneyb@kernel.org>
>> Co-developed-by: Daniel Thompson <daniel@riscstar.com>
>> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> 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
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 746fc8918714..25443a2de1f7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -28178,6 +28178,7 @@ M:	Alex Elder <elder@kernel.org>
>>   M:	Daniel Thompson <danielt@kernel.org>
>>   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
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 37a8e35d4178..928fff40597e 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -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.
>>   
>> +config COMMON_CLK_TC9564
>> +	tristate "Toshiba TC9564 clock support"
>> +	depends on TC9564_PCI
> 
> Consider "depends on TC9564_PCI || COMPILE_TEST".

Yes.  I normally prefer doing that, but haven't actually tried
to cross-compile this code on other target architectures yet
and didn't want to encounter "unrelated" errors.

I will do so before I send the next version so I can add
COMPILE_TEST.

> This driver gets little build coverage otherwise and it has no
> compile-time dependency on the PCI driver.
> 
>> +	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
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 52d138d4ffc8..a11a8326d79c 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -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
>> diff --git a/drivers/clk/clk-tc9564.c b/drivers/clk/clk-tc9564.c
>> new file mode 100644
>> index 000000000000..09a4f6a144fd
>> --- /dev/null
>> +++ b/drivers/clk/clk-tc9564.c
>> @@ -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;
>> +	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 ?

If you mean the "which" field, it is mainly used for
that purpose, yes.  It's also used in a warning message,
but that only occurs if it's a bad value, which I don't
think will ever happen.

If we just trust that clk_hw_get callback function won't
get called with a bad clock, the "which" field can probably
be eliminated, and the result will be a little simpler.

If you were referring to something other than "which" please
say so.

>> +	u32 offset;		/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */
>> +	u32 mask;		/* Zero means undefined clock */
> 
> Is there any such undefined clock ?

There isn't any more.  There was at one time, because the
list of clock IDs was enumerated, and two were skipped.
Krzysztof said to eliminate the hole in that range.  So
this comment can go away now.

>> +};
>> +
>> +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)		\
>> +	[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.

You're right.  I added a parent in devicetree, but did not
have these clocks point back to it.

We have this set of clocks with fixed frequencies, but no
definitive definition of how they're derived.  So the best
I can do is have them all refer back to the single 25 MHz
input clock.
> 
>> +
>> +/*
>> + * 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.

OK.  My main goal was to document those bit numbers.

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

That's a good idea (and it documents the bits).
> None of the consumer will ever need those clocks ?

To the best of my knowledge, no.

>> +	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),
>> +
>> +	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()?

> 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.
>> +	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
> 
>> +	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;
>> +	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.

Wow, you're right, and it's used twice in that dev_err().
I'll fix this.

> 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
> 
>> +		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.

OK, that sounds much better.

> 
>> +}
>> +
>> +static int tc9564_clk_enable(struct clk_hw *hw)
>> +{
>> +	return tc9564_clk_manage(hw, true);
>> +}
>> +
>> +static void tc9564_clk_disable(struct clk_hw *hw)
>> +{
>> +	(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.

OK, that's easy.

> 
>> +};
>> +
>> +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)
>> +		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 ?

I'll look into that.

> 
>> +
>> +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++) {
>> +		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;

Yes, understood.  I neglected to do this.

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


> 
>> +
>> +		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);
>> +		}
>> +
>> +		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

Yes, you're right.  I'll fully initialize the clock structure
before registering.

> 
>> +	}
>> +
>> +	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))
>> +		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

It will once I have the clocks get properly parented.

>> +
>> +	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.

> 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.

Yes, understood.  This will be done in the proper order.

> 
>> +
>> +	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.

Exactly (as I responded to the Sashiko message that said roughly
the same thing).

This was a great review.  I really appreciate it.

					-Alex

Re: [PATCH v3 2/4] clk: toshiba: introduce a TC9564 SoC clock driver

[Jerome Brunet]

On lun. 12 oct. 2026 at 14:59, Alex Elder <elder@riscstar.com> wrote:

>>> +	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

>
> Looking at __clk_core_init(), should I define a
> recalc_rate() callback that just returns the
> equivalent of get_rate()?
>
>> 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.

>>> +	TC9564_CLOCK_INIT0(MAC0_ALL, 31),

[...]

>>> +
>>> +	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.

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

>
>>> +
>>> +	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.

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.

-- 
Jerome

[PATCH v3 3/4] arm64: dts: qcom: define a fixed 25 MHz clock

[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 <daniel@riscstar.com>
Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
v3: - New patch in this version of the series

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

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index 0e3bbdaf4661f..b8d290eed2f38 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -891,6 +891,12 @@ pcie@0,0 {
 		pinctrl-0 = <&tc9563_resx_n>;
 		pinctrl-names = "default";
 
+		clock-25000000 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <25000000>;
+		};
+
 		pcie1_switch0_dsp1: pcie@1,0 {
 			reg = <0x20800 0x0 0x0 0x0 0x0>;
 			#address-cells = <3>;
-- 
2.53.0

Re: [PATCH v3 3/4] arm64: dts: qcom: define a fixed 25 MHz clock

[Konrad Dybcio]

On 10/12/26 5:24 PM, Alex Elder wrote:
> 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 <daniel@riscstar.com>
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v3: - New patch in this version of the series
> 
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index 0e3bbdaf4661f..b8d290eed2f38 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -891,6 +891,12 @@ pcie@0,0 {
>  		pinctrl-0 = <&tc9563_resx_n>;
>  		pinctrl-names = "default";
>  
> +		clock-25000000 {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <25000000>;
> +		};

This and patch 4 should be squashed

Konrad

Re: [PATCH v3 3/4] arm64: dts: qcom: define a fixed 25 MHz clock

[Alex Elder]

On 10/12/26 11:02 AM, Konrad Dybcio wrote:
> On 10/12/26 5:24 PM, Alex Elder wrote:
>> 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 <daniel@riscstar.com>
>> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v3: - New patch in this version of the series
>>
>>   arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> index 0e3bbdaf4661f..b8d290eed2f38 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
>> @@ -891,6 +891,12 @@ pcie@0,0 {
>>   		pinctrl-0 = <&tc9563_resx_n>;
>>   		pinctrl-names = "default";
>>   
>> +		clock-25000000 {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-frequency = <25000000>;
>> +		};
> 
> This and patch 4 should be squashed

OK, will do.  Thanks for the review.	-Alex

> 
> Konrad

[PATCH v3 4/4] arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller

[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 <daniel@riscstar.com>
Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
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(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index b8d290eed2f38..e7601daf1bc5a 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -9,6 +9,7 @@
 #define PM7250B_SID 8
 #define PM7250B_SID1 9
 
+#include <dt-bindings/clock/toshiba,tc9564.h>
 #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 {
 	};
 };
 
+&pcie_s1b5d0f0_bar4_bus {
+	syscon@0 {
+		compatible = "syscon", "simple-mfd";
+		reg = <0x0 0x2000>;
+
+		tc9564_clock0: clock-controller {
+			compatible = "toshiba,tc9564-clock";
+			clocks = <&tc9564_xo_board>;
+			#clock-cells = <1>;
+		};
+	};
+};
+
 &pm7325_gpios {
 	kypd_vol_up_n: kypd-vol-up-n-state {
 		pins = "gpio6";
-- 
2.53.0

Re: [PATCH v3 4/4] arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller

[Konrad Dybcio]

On 10/12/26 5:24 PM, Alex Elder wrote:
> 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 <daniel@riscstar.com>
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---

[...]

> +&pcie_s1b5d0f0_bar4_bus {
> +	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

Re: [PATCH v3 4/4] arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller

[Alex Elder]

On 10/12/26 11:02 AM, Konrad Dybcio wrote:
> On 10/12/26 5:24 PM, Alex Elder wrote:
>> 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 <daniel@riscstar.com>
>> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
> 
> [...]
> 
>> +&pcie_s1b5d0f0_bar4_bus {
>> +	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

OK.  I was just trying to avoid very deep indentation.  I
saw this practice in at least one other place in Qualcomm
devicetree files, but I'll do as you say in version 4 of
this series.

Thank you.

					-Alex
> 
> Konrad

Re: [PATCH v3 0/4] clk: introduce TC9564 clock support

[Jerome Brunet]

> 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/20261002181849.790275-1-elder@riscstar.com/

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

-- 
Jerome

Re: [PATCH v3 0/4] clk: introduce TC9564 clock support

[Alex Elder]

On 10/12/26 2:10 PM, Jerome Brunet wrote:
>> 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/20261002181849.790275-1-elder@riscstar.com/
> 
> 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.

Yes.  I considered dropping the DT change for this reason, but
on v2 I didn't say I would do this, and thought it would be best
to not change this.

Here I'll be clear:  I will send v4, and in v4, I will *not*
include the DTS file patch at the end.
> 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.

OK.

> 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.

I understand.  Thanks for your feedback.

					-Alex

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