Re: [PATCH v6 5/7] clk: baikal-t1: Move reset-controls code into a dedicated module

[Philipp Zabel <p.zabel@pengutronix.de>]

On Fri, Jul 08, 2022 at 10:27:23PM +0300, Serge Semin wrote:
> Before adding the directly controlled resets support it's reasonable to
> move the existing resets control functionality into a dedicated object for
> the sake of the CCU dividers clock driver simplification. After the new
> functionality was added clk-ccu-div.c would have got to a mixture of the
> weakly dependent clocks and resets methods. Splitting the methods up into
> the two objects will make the code easier to read and maintain. It shall
> also improve the code scalability (though hopefully we won't need this
> part that much in the future).
> 
> The reset control functionality is now implemented in the framework of a
> single unit since splitting it up doesn't make much sense due to
> relatively simple reset operations. The ccu-rst.c has been designed to be
> looking like ccu-div.c or ccu-pll.c with two globally available methods
> for the sake of the code unification and better code readability.
> 
> This commit doesn't provide any change in the CCU reset implementation
> semantics. As before the driver will support the trigger-like CCU resets
> only, which are responsible for the AXI-bus, APB-bus and SATA-ref blocks
> reset. The assert/de-assert-capable reset controls support will be added
> in the next commit.
> 
> Note the CCU Clock dividers and resets functionality split up was possible
> due to not having any side-effects (at least we didn't found ones) of the
> regmap-based concurrent access of the common CCU dividers/reset CSRs.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Nothing left I'd insist to be changed, so:

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Just a few nitpicks below:

> 
> ---
> 
> Changelog v4:
> - Completely split CCU Dividers and Resets functionality. (@Stephen)
> 
> Changelog v6:
> - Combine the reset-related code into a single file. (@Philipp)
> - Refactor the code to support the linear reset IDs only. (@Philipp)
> - Drop CCU_DIV_RST_MAP() macro. It's no longer used.
> ---
>  drivers/clk/baikal-t1/Kconfig       |  12 ++-
>  drivers/clk/baikal-t1/Makefile      |   1 +
>  drivers/clk/baikal-t1/ccu-div.c     |  19 ----
>  drivers/clk/baikal-t1/ccu-div.h     |   4 +-
>  drivers/clk/baikal-t1/ccu-rst.c     | 151 ++++++++++++++++++++++++++++
>  drivers/clk/baikal-t1/ccu-rst.h     |  57 +++++++++++
>  drivers/clk/baikal-t1/clk-ccu-div.c |  92 ++---------------
>  7 files changed, 231 insertions(+), 105 deletions(-)
>  create mode 100644 drivers/clk/baikal-t1/ccu-rst.c
>  create mode 100644 drivers/clk/baikal-t1/ccu-rst.h
> 
[...]
> diff --git a/drivers/clk/baikal-t1/ccu-rst.c b/drivers/clk/baikal-t1/ccu-rst.c
> new file mode 100644
> index 000000000000..8fd40810d24e
> --- /dev/null
> +++ b/drivers/clk/baikal-t1/ccu-rst.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 BAIKAL ELECTRONICS, JSC
> + *
> + * Authors:
> + *   Serge Semin <Sergey.Semin@baikalelectronics.ru>
> + *
> + * Baikal-T1 CCU Resets interface driver
> + */
> +
> +#define pr_fmt(fmt) "bt1-ccu-rst: " fmt
> +
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/reset/bt1-ccu.h>
> +
> +#include "ccu-rst.h"
> +
> +#define CCU_AXI_MAIN_BASE		0x030
> +#define CCU_AXI_DDR_BASE		0x034
> +#define CCU_AXI_SATA_BASE		0x038
> +#define CCU_AXI_GMAC0_BASE		0x03C
> +#define CCU_AXI_GMAC1_BASE		0x040
> +#define CCU_AXI_XGMAC_BASE		0x044
> +#define CCU_AXI_PCIE_M_BASE		0x048
> +#define CCU_AXI_PCIE_S_BASE		0x04C
> +#define CCU_AXI_USB_BASE		0x050
> +#define CCU_AXI_HWA_BASE		0x054
> +#define CCU_AXI_SRAM_BASE		0x058
> +
> +#define CCU_SYS_SATA_REF_BASE		0x060
> +#define CCU_SYS_APB_BASE		0x064
> +
> +#define CCU_RST_DELAY_US		1
> +
> +#define CCU_RST_TRIG(_base, _ofs)		\
> +	{					\
> +		.base = _base,			\
> +		.mask = BIT(_ofs),		\
> +	}
> +
> +struct ccu_rst_info {
> +	unsigned int base;
> +	unsigned int mask;
> +};
[...]

This could be compacted by making the base offset u16 and - if there are
no resets that require toggling two bits at once - by storing an u8 bit
offset instead of the mask.

> diff --git a/drivers/clk/baikal-t1/ccu-rst.h b/drivers/clk/baikal-t1/ccu-rst.h
> new file mode 100644
> index 000000000000..68214d777465
> --- /dev/null
> +++ b/drivers/clk/baikal-t1/ccu-rst.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 BAIKAL ELECTRONICS, JSC
> + *
> + * Baikal-T1 CCU Resets interface driver
> + */
> +#ifndef __CLK_BT1_CCU_RST_H__
> +#define __CLK_BT1_CCU_RST_H__
> +
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +struct ccu_rst_info;
> +
> +/*
> + * struct ccu_rst_init_data - CCU Resets initialization data
> + * @sys_regs: Baikal-T1 System Controller registers map.
> + * @np: Pointer to the node with the System CCU block.
> + */
> +struct ccu_rst_init_data {
> +	struct regmap *sys_regs;
> +	struct device_node *np;
> +};
> +
> +/*
> + * struct ccu_rst - CCU Reset descriptor
> + * @rcdev: Reset controller descriptor.
> + * @sys_regs: Baikal-T1 System Controller registers map.
> + * @rsts_info: Reset flag info (base address and mask).
> + */
> +struct ccu_rst {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *sys_regs;
> +	const struct ccu_rst_info *rsts_info;
> +};
> +#define to_ccu_rst(_rcdev) container_of(_rcdev, struct ccu_rst, rcdev)

I'd make this a static inline function.

> diff --git a/drivers/clk/baikal-t1/clk-ccu-div.c b/drivers/clk/baikal-t1/clk-ccu-div.c
> index 90f4fda406ee..278aa38d767e 100644
> --- a/drivers/clk/baikal-t1/clk-ccu-div.c
> +++ b/drivers/clk/baikal-t1/clk-ccu-div.c
[...]
> @@ -274,42 +241,6 @@ static struct ccu_div *ccu_div_find_desc(struct ccu_div_data *data,
>  	return ERR_PTR(-EINVAL);
>  }
>  
> -static int ccu_div_reset(struct reset_controller_dev *rcdev,
> -			 unsigned long rst_id)
> -{
> -	struct ccu_div_data *data = to_ccu_div_data(rcdev);
> -	const struct ccu_div_rst_map *map;
> -	struct ccu_div *div;
> -	int idx, ret;
> -
> -	for (idx = 0, map = data->rst_map; idx < data->rst_num; ++idx, ++map) {
> -		if (map->rst_id == rst_id)
> -			break;
> -	}
> -	if (idx == data->rst_num) {
> -		pr_err("Invalid reset ID %lu specified\n", rst_id);
> -		return -EINVAL;
> -	}
> -
> -	div = ccu_div_find_desc(data, map->clk_id);
> -	if (IS_ERR(div)) {
> -		pr_err("Invalid clock ID %d in mapping\n", map->clk_id);
> -		return PTR_ERR(div);
> -	}
> -
> -	ret = ccu_div_reset_domain(div);
> -	if (ret) {
> -		pr_err("Reset isn't supported by divider %s\n",
> -			clk_hw_get_name(ccu_div_get_clk_hw(div)));
                       ^
This should be aligned to the parenthesis, see checkpatch.pl --strict.

> -	}
> -
> -	return ret;
> -}
> -
> -static const struct reset_control_ops ccu_div_rst_ops = {
> -	.reset = ccu_div_reset,
> -};
> -
>  static struct ccu_div_data *ccu_div_create_data(struct device_node *np)
>  {
>  	struct ccu_div_data *data;
> @@ -323,13 +254,9 @@ static struct ccu_div_data *ccu_div_create_data(struct device_node *np)
>  	if (of_device_is_compatible(np, "baikal,bt1-ccu-axi")) {
>  		data->divs_num = ARRAY_SIZE(axi_info);
>  		data->divs_info = axi_info;
> -		data->rst_num = ARRAY_SIZE(axi_rst_map);
> -		data->rst_map = axi_rst_map;
>  	} else if (of_device_is_compatible(np, "baikal,bt1-ccu-sys")) {
>  		data->divs_num = ARRAY_SIZE(sys_info);
>  		data->divs_info = sys_info;
> -		data->rst_num = ARRAY_SIZE(sys_rst_map);
> -		data->rst_map = sys_rst_map;
>  	} else {
>  		pr_err("Incompatible DT node '%s' specified\n",
>  			of_node_full_name(np));
                       ^
Same as above.

regards
Philipp