Re: [PATCH 4/4] reset: toshiba: introduce a TC9564 SoC reset driver

[Alex Elder <elder@riscstar.com>]

On 11/4/26 3:49 AM, Philipp Zabel wrote:
> On Di, 2026-11-03 at 18:43 -0600, Alex Elder wrote:
>> Define a new platform driver that manages reset signals implemented by
>> the Toshiba TC9564 SoC.  There are 13 reset signals available, but
>> currently only the resets that are (or will soon be) used are defined
>> to be controllable.
>>
>> Two registers manage the state of the resets.  The registers are
>> accessed via a regmap supplied by a system controller.  Access to
>> the memory region is provided via a BAR on a PCIe endpoint function
>> embedded in the 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>
>> ---
>>   MAINTAINERS                  |   7 ++
>>   drivers/reset/Kconfig        |  12 ++++
>>   drivers/reset/Makefile       |   1 +
>>   drivers/reset/reset-tc9564.c | 135 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 155 insertions(+)
>>   create mode 100644 drivers/reset/reset-tc9564.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ee01aed56508d..8389b11fe6e1a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -28355,6 +28355,13 @@ F:	Documentation/devicetree/bindings/soc/toshiba/toshiba,tc9564-config-syscon.ya
>>   F:	drivers/soc/toshiba/tc9564-syscon.c
>>   F:	include/dt-bindings/soc/toshiba,tc9564.h
>>   
>> +TOSHIBA TC9564 RESET DRIVER
>> +M:	Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
>> +M:	Alex Elder <elder@kernel.org>
>> +M:	Daniel Thompson <danielt@kernel.org>
>> +S:	Maintained
>> +F:	drivers/reset/reset-tc9564.c
>> +
>>   TOSHIBA WMI HOTKEYS DRIVER
>>   M:	Azael Avalos <coproscefalo@gmail.com>
>>   L:	platform-driver-x86@vger.kernel.org
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index eda9c698a95f3..8c1b08f4eebe6 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -358,6 +358,18 @@ config RESET_SUNXI
>>   	help
>>   	  This enables the reset driver for Allwinner SoCs.
>>   
>> +config RESET_TC9564
>> +	tristate "Toshiba TC9564 reset support"
>> +	depends on SOC_TOSHIBA_TC9564_SYSCON
>> +	select MFD_SYSCON
>> +	default SOC_TOSHIBA_TC9564_SYSCON
>> +	help
>> +	  This enables support for the reset controller embedded in the
>> +	  Toshiba TC9564 SoC.  The state of reset lines is controlled
>> +	  via MMIO to a region managed by a system controller; this
>> +	  ensures access to the region is coordinated between this and
>> +	  the clock and XGMAC (stmmac) drivers.
> 
> Drop the second sentence, explaining driver details to a user that just
> needs to decide whether to enable the driver or not is not helpful.

OK.  I think I originally had something to the effect of
using a regmap to ensure multiple drivers could access
the space safely.  Then someone asked "which drivers" so
I was more explicit.

Anyway, I'll just drop the sentence, as you suggest.  I
will also drop a similar sentence in the clock Kconfig
file description for the COMMON_CLK_TC9564 option.

>>   config RESET_TENSTORRENT_ATLANTIS
>>   	tristate "Tenstorrent atlantis reset driver"
>>   	depends on ARCH_TENSTORRENT || COMPILE_TEST
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 489fafd6bbe3f..6ddded26309ef 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_RESET_SKY1_AUDSS) += reset-sky1-audss.o
>>   obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
>>   obj-$(CONFIG_RESET_SUNPLUS) += reset-sunplus.o
>>   obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
>> +obj-$(CONFIG_RESET_TC9564) += reset-tc9564.o
>>   obj-$(CONFIG_RESET_TENSTORRENT_ATLANTIS) += reset-tenstorrent-atlantis.o
>>   obj-$(CONFIG_RESET_TH1520) += reset-th1520.o
>>   obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
>> diff --git a/drivers/reset/reset-tc9564.c b/drivers/reset/reset-tc9564.c
>> new file mode 100644
>> index 0000000000000..fcd58d5f1962f
>> --- /dev/null
>> +++ b/drivers/reset/reset-tc9564.c
>> @@ -0,0 +1,135 @@
> [...]
>> +static void reset_manage(struct reset_controller_dev *rcdev,
>> +			 unsigned long id, bool assert)
>> +{
>> +	const struct tc9564_reset *reset = &tc9564_reset[id];
>> +	struct regmap *regmap = dev_get_drvdata(rcdev->dev);
>> +	u32 mask = reset->mask;
>> +
>> +	regmap_update_bits(regmap, reset->offset, mask, assert ? mask : 0);
> 
> Just propagating the error value ...
> 
>> +}
>> +
>> +static int reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	reset_manage(rcdev, id, true);
>> +
>> +	return 0;
> 
> ... makes this even shorter.

Yes I see what you mean.  Just because we don't *have to* check
for errors doesn't mean we must ignore the return value.

> 
>> +}
>> +
>> +static int reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	reset_manage(rcdev, id, false);
>> +
>> +	return 0;
>> +}
>> +
>> +static int reset_status(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	const struct tc9564_reset *reset = &tc9564_reset[id];
>> +	struct regmap *regmap = dev_get_drvdata(rcdev->dev);
>> +	u32 val;
>> +
>> +	regmap_read(regmap, reset->offset, &val);
>> +
>> +	return val & reset->mask ? 1 : 0;
> 
> Consider using regmap_test_bits() here.

I will.

>> +}
>> +
>> +static const struct reset_control_ops reset_control_ops = {
>> +	.assert = reset_assert,
>> +	.deassert = reset_deassert,
>> +	.status = reset_status,
>> +};
>> +
>> +static int tc9564_reset_probe(struct platform_device *pdev)
>> +{
>> +	struct reset_controller_dev *rcdev;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *parent_np;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	parent_np = dev_of_node(dev->parent);
>> +	if (!parent_np)
>> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
>> +
>> +	/* Note: MMIO regmap (no error checking needed when used) */
> 
> I suggest you just add the unneeded error checks and drop this comment.

Yup, agreed.

>> +	regmap = device_node_to_regmap(parent_np);
> 
> Can't you just use "dev_get_regmap(dev->parent, NULL)" ?

I just tried what you suggest, and it returns a null
pointer rather than the desired regmap.

device_node_to_regmap() searches the syscon_list for an
entry that has the provided node pointer, and if found,
returns that system controller's regmap.  That does find
a syscon, and returns its regmap.

of_syscon_register() seems to create a syscon but does
*not* register any device resources (devres) on any device.

So I think when dev_get_regmap() simply scans the parent
device for the latest regmap resource, it finds none.

I'm happy to discuss this further, but for now I will
not be switching over to dev_get_regmap().


Thank you for the quick review.

					-Alex

> regards
> Philipp