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@xxxxxxxxxxxxxxxxxxxx>
Nothing left I'd insist to be changed, so:
Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxx>
> + *
> + * 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
