Re: [PATCH v2 2/2] reset: canaan: add reset driver for Kendryte K230

[Chen Wang <unicorn_wang@outlook.com>]

Hi, Junhui,


On 2025/4/20 1:09, Junhui Liu wrote:
> Add support for the resets on Canaan Kendryte K230 SoC. The driver
> support CPU0, CPU1, L2 cache flush, hardware auto clear and software
> clear resets.
>
> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
>
> ---
> The reset management module in the K230 SoC also provides reset time
> control registers. For RST_TYPE_CPU0, RST_TYPE_CPU1 and RST_TYPE_SW_DONE,
> the time period when reset is applyed/removed but the clock is stopped
> can be set up to 15*0.25 = 3.75 us. For some RST_TYPE_HW_DONE cases, the
> time period can be set up to 255*0.25 = 63.75 us. For RST_TYPE_FLUSH,
> the reset bit will automatically cleared by hardware when flush done.
>
> Although the current reset driver does not support configuration of
> reset time registers, delay has been added to the assert, deassert and
> reset functions to accommodate the longest reset time.
>
> Besides, although some reset types have done bits, the reference manual
> does not explicitly indicate whether the hardware removes reset or the
> clock stop time period has passed when done bits toggle. Therefore, I
> think it is a safer way to keep delay for reset types with done bits.
>
> link: https://kendryte-download.canaan-creative.com/developer/k230/HDK/K230%E7%A1%AC%E4%BB%B6%E6%96%87%E6%A1%A3/K230_Technical_Reference_Manual_V0.3.1_20241118.pdf
Regards these notes, please move it to the beginning of source code, see 
example: drivers/pwm/pwm-sophgo-sg2042.c
> ---
>   drivers/reset/Kconfig      |   9 ++
>   drivers/reset/Makefile     |   1 +
>   drivers/reset/reset-k230.c | 355 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 365 insertions(+)
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 99f6f9784e6865faddf8621ccfca095778c4dc47..248138ffba3bfbf859c74ba1aed7ba2f72819f7a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -140,6 +140,15 @@ config RESET_K210
>   	  Say Y if you want to control reset signals provided by this
>   	  controller.
>   
> +config RESET_K230
> +	tristate "Reset controller driver for Canaan Kendryte K230 SoC"
> +	depends on ARCH_CANAAN || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Support for the Canaan Kendryte K230 RISC-V SoC reset controller.
> +	  Say Y if you want to control reset signals provided by this
> +	  controller.
> +
>   config RESET_LANTIQ
>   	bool "Lantiq XWAY Reset Driver" if COMPILE_TEST
>   	default SOC_TYPE_XWAY
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 31f9904d13f9c3a107fc1ee1ec9f9baba016d101..13fe94531bea1eb91268b1804e1321b167815a4b 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>   obj-$(CONFIG_RESET_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.o
>   obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
>   obj-$(CONFIG_RESET_K210) += reset-k210.o
> +obj-$(CONFIG_RESET_K230) += reset-k230.o
>   obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
>   obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>   obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
> diff --git a/drivers/reset/reset-k230.c b/drivers/reset/reset-k230.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..492d2274893675b0ff1967426c8fa9e75aed1791
> --- /dev/null
> +++ b/drivers/reset/reset-k230.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2016-2017 Linaro Ltd.
Linaro? Are you sure?
> + * Copyright (C) 2022-2024 Canaan Bright Sight Co., Ltd
> + * Copyright (C) 2024-2025 Junhui Liu <junhui.liu@pigmoral.tech>
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spinlock.h>
> +
> +#include <dt-bindings/reset/canaan,k230-rst.h>
> +
> +/**
> + * enum k230_rst_type - K230 reset types
> + * @RST_TYPE_CPU0: Reset type for CPU0
> + *	Automatically clears, has write enable and done bit, active high
> + * @RST_TYPE_CPU1: Reset type for CPU1
> + *	Manually clears, has write enable and done bit, active high
> + * @RST_TYPE_FLUSH: Reset type for CPU L2 cache flush
> + *	Automatically clears, has write enable, no done bit, active high
> + * @RST_TYPE_HW_DONE: Reset type for hardware auto clear
> + *	Automatically clears, no write enable, has done bit, active high
> + * @RST_TYPE_SW_DONE: Reset type for software manual clear
> + *	Manually clears, no write enable and done bit,
> + *	active high if ID is RST_SPI2AXI, otherwise active low
> + */
> +enum k230_rst_type {
> +	RST_TYPE_CPU0,
> +	RST_TYPE_CPU1,
> +	RST_TYPE_FLUSH,
> +	RST_TYPE_HW_DONE,
> +	RST_TYPE_SW_DONE,
> +};
> +
> +struct k230_rst_map {
> +	u32			offset;
> +	enum k230_rst_type	type;
> +	u32			done;
> +	u32			reset;
> +};
> +
> +struct k230_rst {
> +	struct reset_controller_dev	rcdev;
> +	struct device			*dev;

Why don't usercdev.dev to store the dev, so we can save a pointer.

> +	void __iomem			*base;
> +	spinlock_t			lock;
> +};

[......]

Thanks,

Chen