Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver

[Jerome Brunet <jbrunet@baylibre.com>]

On mar. 22 sept. 2026 at 08:33, Alex Elder <elder@riscstar.com> wrote:

> On 9/21/26 5:59 PM, Brian Masney wrote:
>> Hi Alex,
>> 
>> On Fri, Sep 18, 2026 at 11:52:32AM -0500, Alex Elder wrote:
>>> Define a new platform driver that manages clock and reset signals
>>> within the TC9564 SoC.  There are 21 clocks, which can only be
>>> enabled and disabled, as well as 13 reset signals.
>>>
>>> Two registers manage the state of the clocks and two others manage
>>> the state of the resets.  The registers are accessed via a regmap
>>> supplied by a system controller, which coordinates access to a
>>> region of memory that will be shared with another driver.
>>>
>>> Access to the memory region is provided via a BAR on a PCIe endpoint
>>> function embedded in the TC9564 SoC.  For that reason, neither the
>>> PCIe clock nor PCIe reset can be manipulated by this driver (they
>>> are assumed always on and deasserted, respectively).
>>>
>>> Similarly, control is not available for the I2C clock and reset,
>>> because the PCIe subsystem on the TC9564 relies on I2C
>>>
>>> Co-developed-by: Daniel Thompson <daniel@riscstar.com>
>>> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>> ---
>>>   MAINTAINERS              |   1 +
>>>   drivers/clk/Kconfig      |  11 ++
>>>   drivers/clk/Makefile     |   1 +
>>>   drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 379 insertions(+)
>>>   create mode 100644 drivers/clk/clk-tc9564.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 66d0e7e65adcb..39346a5cd9a7f 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -27675,6 +27675,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 f9592fd9ec2bb..50efa10d48450 100644
>>> --- a/drivers/clk/Kconfig
>>> +++ b/drivers/clk/Kconfig
>>> @@ -292,6 +292,17 @@ 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 RESET_CONTROLLER
>
> Thank you.  The reset and clock drivers were previously separate
> and the reset only became available if RESET_CONTROLLER was enabled.
> Combining them means I need this.  I will add it.

Why did you combine them ?

it would be a lot better if the reset were handled in
drivers/reset rather than in clock. There has already been some work to
move reset from clock back to reset. This often involve auxiliary drivers.

>
> I also need to select MFD_SYSCON to get syscon_node_to_regmap()
> (or perhaps something similar, depending on the answers to my
> questions about the proper way to define the syscon).
>> 
>>> +	default m
>> 
>> default n
>
> This depends on TC9564_PCI, and if TC9564_PCI is enabled I would
> like this to (automatically) be available (at least as a module).
>
> Why do you recommend "n"?
>
>>> +	help
>>> +	  This enables support for the clock and reset controller embedded
>>> +	  in the Toshiba TC9564 (and Qualcomm QPS615) SoC.  The state of
>>> +	  clock and reset lines is controlled by MMIO to a region managed
>>> +	  by a system controller; this ensures access to the region is
>>> +	  coordinated between this and other drivers.
>> 
>> Rather than 'other drivers', outline specifically which other drivers.
>
> I was intentionally vague at this point because the one other driver
> has not yet gone out for upstream review for this iteration of the
> code.  But I do agree with you, so I'll change this to say:
>
>     ... between this and the XGMAC (stmmac) driver.
>
> Then it will be correct without modification once that driver goes
> out for review.  Is that better, or do you suggest something else?
>
>>> +
>>>   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 b18af485d7f03..a98ea8ff92066 100644
>>> --- a/drivers/clk/Makefile
>>> +++ b/drivers/clk/Makefile
>>> @@ -110,6 +110,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..4f900bc8994a7
>>> --- /dev/null
>>> +++ b/drivers/clk/clk-tc9564.c
>>> @@ -0,0 +1,366 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/*
>>> + * Copyright (C) 2026 by RISCstar Solutions Corporation.  All rights reserved.
>>> + */
>>> +
>>> +#include <linux/bits.h>
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/mod_devicetable.h>
>> 
>> Uwe already pointed out removing this.
>
> Yes, done.
>
>> 
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/reset-controller.h>
>>> +
>>> +#include <dt-bindings/clock/toshiba,tc9564.h>
>>> +
>>> +#define CLK_CTRL0_OFFSET	0x1004
>>> +#define RST_CTRL0_OFFSET	0x1008
>>> +#define CLK_CTRL1_OFFSET	0x100c
>>> +#define RST_CTRL1_OFFSET	0x1010
>>> +
>>> +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_reset {
>>> +	u32 offset;		/* RST_CTRL0_OFFSET or RST_CTRL1_OFFSET */
>>> +	u32 mask;		/* Zero means undefined reset */
>>> +};
>>> +
>>> +struct tc9564_clocks {
>>> +	struct device *dev;
>>> +	struct regmap *regmap;
>>> +	struct reset_controller_dev rcdev;
>>> +	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),			\
>>> +	}
>>> +
>>> +#define TC9564_RESET_INIT0(_name, _bit)	__TC9564_RESET_INIT(_name, 0, _bit)
>>> +#define TC9564_RESET_INIT1(_name, _bit)	__TC9564_RESET_INIT(_name, 1, _bit)
>>> +
>>> +#define __TC9564_RESET_INIT(_name, _reg, _bit)		\
>>> +	[RESET_##_name] = {				\
>>> +		.offset	= RST_CTRL ## _reg ## _OFFSET,	\
>>> +		.mask = BIT(_bit),			\
>>> +	}
>>> +
>>> +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), */
>> 
>> A comment would be useful here to outline why these are commented out.
>> Is the PCIE one comment out because of what's outlined in the commit
>> message?
>
> Yes, that is why.  We are downstream of PCIe, and PCIe in this
> case depends on I2C.  So we don't want to mess with these two
> clocks or their reset counterparts.
>
> I only provide this for the benefit of documentation.  If someone
> happens to see bit 9 set in this register, it means the PCIe clock
> is enabled.
>
> Anyway, others have suggested simply removing these comments.
> So I'll do what you suggest, but I'm interested to know whether
> you would favor just deleting them instead.
>> 
>>> +	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),
>
> . . .
>
>>> +static const struct reset_control_ops tc9564_reset_control_ops = {
>>> +	.assert		= tc9564_reset_assert,
>>> +	.deassert	= tc9564_reset_deassert,
>> 
>> I'd rather not have the extra spaces because sometimes these get out of
>> line with new members that need more space. Also tc9564_clk_ops above
>> doesn't have the extra spaces.
>
> That's OK with me.  I normally align assignments like this for
> readability, but it's obviously an arbitrary stylistic choice.
> I'll just use one space, no alignment (just as the clk_ops do
> above).  I'll do the same in the platform driver structure
> assignments.
>> 
>>> +};
>>> +
>>> +static void tc9564_reset_assert_all(struct tc9564_clocks *clocks)
>>> +{
>>> +	for (u32 id = 0; id < TC9564_RESET_COUNT; id++)
>>> +		if (tc9564_reset[id].mask)
>>> +			tc9564_reset_manage(&clocks->rcdev, id, true);
>>> +}
>>> +
>>> +static int tc9564_reset_init(struct tc9564_clocks *clocks)
>>> +{
>>> +	struct reset_controller_dev *rcdev = &clocks->rcdev;
>>> +
>>> +	rcdev->ops = &tc9564_reset_control_ops;
>>> +	rcdev->owner = THIS_MODULE;
>>> +	rcdev->dev = clocks->dev;
>>> +	rcdev->of_node = dev_of_node(clocks->dev);
>>> +	rcdev->nr_resets = TC9564_RESET_COUNT;
>>> +
>>> +	return devm_reset_controller_register(clocks->dev, rcdev);
>>> +}
>>> +
>>> +static int tc9564_clk_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct tc9564_clocks *clocks;
>>> +	int ret;
>>> +
>>> +	if (!dev_of_node(dev))
>>> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
>>> +
>>> +	clocks = tc9564_clk_init(dev);
>>> +	if (IS_ERR(clocks))
>>> +		return dev_err_probe(dev, PTR_ERR(clocks),
>>> +				     "failed to initialize clocks\n");
>>> +
>>> +	ret = tc9564_reset_init(clocks);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret, "failed to initialize resets\n");
>>> +
>>> +	/* Force all resets to be initially asserted */
>>> +	tc9564_reset_assert_all(clocks);
>>> +
>>> +	/* 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 resets to be deasserted when done */
>>> +	tc9564_reset_assert_all(clocks);
>> 
>> The comment says deasserted but the code calls assert_all.
>
> Oops.  Will fix.  Thank you very much for your review Brian.
>
> 					-Alex
>
>> 
>> Brian
>> 
>> 
>>> +
>>> +	/* 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 and Reset Driver");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 2.53.0
>>>
>> 
>

-- 
Jerome