Hi Serge,
On Fr, 2022-06-24 at 17:18 +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).
>
> As it was done for the CCU PLLs and Dividers the reset control
> functionality in its turn has been split up into two sub-modules:
> hw-interface and generic reset device description. This commit doesn't
> provide any change in the CCU reset module semantics. As before it
> supports 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>
>
> ---
>
> Changelog v4:
> - Completely split CCU Dividers and Resets functionality. (@Stephen)
> ---
> 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 | 43 +++++
> drivers/clk/baikal-t1/ccu-rst.h | 67 ++++++++
> drivers/clk/baikal-t1/clk-ccu-div.c | 101 +++---------
> drivers/clk/baikal-t1/clk-ccu-rst.c | 236 ++++++++++++++++++++++++++++
What is the reason for separating ccu-rst.c and clk-ccu-rst.c?
I expect implementing the reset ops and registering the reset
controller in the same compilation unit would be easier.
> diff --git a/drivers/clk/baikal-t1/ccu-rst.c b/drivers/clk/baikal-t1/ccu-rst.c
> new file mode 100644
> index 000000000000..b355bf0b399a
> --- /dev/null
> +++ b/drivers/clk/baikal-t1/ccu-rst.c
> @@ -0,0 +1,43 @@
> +// 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/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/printk.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include "ccu-rst.h"
> +
> +#define CCU_RST_DELAY_US 1
> +
> +static int ccu_rst_reset(struct reset_controller_dev *rcdev, unsigned long idx)
> +{
> + struct ccu_rst *rst;
> +
> + rst = ccu_rst_get_desc(rcdev, idx);
> + if (IS_ERR(rst)) {
> + pr_err("Invalid reset index %lu specified\n", idx);
> + return PTR_ERR(rst);
> + }
I don't think this is necessary, see my comments below. Since the reset
ids are contiguous, just setting nr_resets and using the default
.of_xlate should be enough to make sure this is never called with an
invalid id.
> +
> + regmap_update_bits(rst->sys_regs, rst->reg_ctl, rst->mask, rst->mask);
I would expect this to get sys_regs from data, which can be obtained
from rcdev via container_of. The reg_ctl and mask could then be
obtained from the ccu_rst_info array, data->rsts_info[idx].
> +
> + /* The next delay must be enough to cover all the resets. */
> + udelay(CCU_RST_DELAY_US);
> +
> + return 0;
> +}
[...]
> +
> +const struct reset_control_ops ccu_rst_ops = {
With ops and controller registration in the same .c file this could be
static.
> + .reset = ccu_rst_reset,
> +};
> diff --git a/drivers/clk/baikal-t1/ccu-rst.h b/drivers/clk/baikal-t1/ccu-rst.h
> new file mode 100644
> index 000000000000..d03bae4b7a05
> --- /dev/null
> +++ b/drivers/clk/baikal-t1/ccu-rst.h
> @@ -0,0 +1,67 @@
> +/* 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>
> +
> +struct ccu_rst_data;
> +
> +/*
> + * 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
> + * @id: Reset identifier.
> + * @reg_ctl: Reset control register base address.
> + * @sys_regs: Baikal-T1 System Controller registers map.
> + * @mask: Reset bitmask (normally it's just a single bit flag).
> + */
> +struct ccu_rst {
> + unsigned int id;
I'm not convinced this structure is necessary.
It is just a copy of struct ccu_rst_info, but with an added regmap
pointer per entry, which seems excessive since the regmap is the same
for all resets.
[...]
> diff --git a/drivers/clk/baikal-t1/clk-ccu-rst.c b/drivers/clk/baikal-t1/clk-ccu-rst.c
> new file mode 100644
> index 000000000000..b10857f48b8b
> --- /dev/null
> +++ b/drivers/clk/baikal-t1/clk-ccu-rst.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 BAIKAL ELECTRONICS, JSC
> + *
> + * Authors:
> + * Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> + *
> + * Baikal-T1 CCU Resets domain driver
> + */
> +#define pr_fmt(fmt) "bt1-ccu-rst: " fmt
> +
> +#include <linux/bits.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_TRIG(_id, _base, _ofs) \
> + { \
> + .id = _id, \
I think the _id parameter and id field could be dropped.
> + .base = _base, \
> + .mask = BIT(_ofs), \
> + }
> +
> +struct ccu_rst_info {
> + unsigned int id;
This could be dropped.
> + unsigned int base;
> + unsigned int mask;
Are there actually resets that require setting/clearing multiple bits,
or is this theoretical?
> +};
> +
> +struct ccu_rst_data {
> + struct device_node *np;
This is already in rcdev.of_node, no need to carry a copy.
> + struct regmap *sys_regs;
> +
> + unsigned int rsts_num;
Same as above, this is already in rcdev.nr_resets.
> + const struct ccu_rst_info *rsts_info;
> + struct ccu_rst *rsts;
This is not neccessary if you use sys_regs and rsts_info in the reset
ops.
> +
> + struct reset_controller_dev rcdev;
> +};
> +#define to_ccu_rst_data(_rcdev) container_of(_rcdev, struct ccu_rst_data, rcdev)
> +
> +/*
> + * Each AXI-bus clock divider is equipped with the corresponding clock-consumer
> + * domain reset (it's self-deasserted reset control).
> + */
> +static const struct ccu_rst_info axi_rst_info[] = {
> + CCU_RST_TRIG(CCU_AXI_MAIN_RST, CCU_AXI_MAIN_BASE, 1),
This could be:
[CCU_AXI_MAIN_RST] = CCU_RST_TRIG(CCU_AXI_MAIN_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_DDR_RST, CCU_AXI_DDR_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_SATA_RST, CCU_AXI_SATA_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_GMAC0_RST, CCU_AXI_GMAC0_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_GMAC1_RST, CCU_AXI_GMAC1_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_XGMAC_RST, CCU_AXI_XGMAC_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_PCIE_M_RST, CCU_AXI_PCIE_M_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_PCIE_S_RST, CCU_AXI_PCIE_S_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_USB_RST, CCU_AXI_USB_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_HWA_RST, CCU_AXI_HWA_BASE, 1),
> + CCU_RST_TRIG(CCU_AXI_SRAM_RST, CCU_AXI_SRAM_BASE, 1),
> +};
> +
> +/*
> + * SATA reference clock domain and APB-bus domain are connected with the
> + * sefl-deasserted reset control, which can be activated via the corresponding
> + * clock divider register. DDR and PCIe sub-domains can be reset with directly
> + * controlled reset signals. Resetting the DDR controller though won't end up
> + * well while the Linux kernel is working.
> + */
> +static const struct ccu_rst_info sys_rst_info[] = {
> + CCU_RST_TRIG(CCU_SYS_SATA_REF_RST, CCU_SYS_SATA_REF_BASE, 1),
Same as above.
> + CCU_RST_TRIG(CCU_SYS_APB_RST, CCU_SYS_APB_BASE, 1),
> +};
> +
> +struct ccu_rst *ccu_rst_get_desc(struct reset_controller_dev *rcdev, unsigned long idx)
> +{
> + struct ccu_rst_data *data = to_ccu_rst_data(rcdev);
> +
> + if (idx >= data->rsts_num)
> + return ERR_PTR(-EINVAL);
> +
> + return &data->rsts[idx];
> +}
This is not necessary if you just use the reset id as an index into the
ccu_rst_info array.
> +
> +static int ccu_rst_of_idx_get(struct reset_controller_dev *rcdev,
> + const struct of_phandle_args *rstspec)
> +{
> + struct ccu_rst_data *data = to_ccu_rst_data(rcdev);
> + unsigned int id, idx;
> +
> + id = rstspec->args[0];
> + for (idx = 0; idx < data->rsts_num; ++idx) {
> + if (data->rsts[idx].id == id)
> + break;
> + }
> + if (idx == data->rsts_num) {
> + pr_err("Invalid reset ID %u specified\n", id);
> + return -EINVAL;
> + }
> +
> + return idx;
> +}
Unless I'm mistaken, id == idx for all resets, and this is not
necessary at all. You should be able to use the default .of_xlate.
> +
> +static struct ccu_rst_data *ccu_rst_create_data(const struct ccu_rst_init_data *rst_init)
> +{
> + struct ccu_rst_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + data->np = rst_init->np;
No reason to store data->np only to copy it to data->rcdev.of_node
later.
> + data->sys_regs = rst_init->sys_regs;
> + if (of_device_is_compatible(data->np, "baikal,bt1-ccu-axi")) {
> + data->rsts_num = ARRAY_SIZE(axi_rst_info);
You could store the number of resets directly into
data->rcdev.nr_resets.
> + data->rsts_info = axi_rst_info;
> + } else if (of_device_is_compatible(data->np, "baikal,bt1-ccu-sys")) {
> + data->rsts_num = ARRAY_SIZE(sys_rst_info);
> + data->rsts_info = sys_rst_info;
> + } else {
> + pr_err("Incompatible DT node '%s' specified\n",
> + of_node_full_name(data->np));
> + ret = -EINVAL;
> + goto err_kfree_data;
> + }
> +
> + data->rsts = kcalloc(data->rsts_num, sizeof(*data->rsts), GFP_KERNEL);
> + if (!data->rsts) {
> + ret = -ENOMEM;
> + goto err_kfree_data;
> + }
I think data->rsts is not required.
> +
> + return data;
> +
> +err_kfree_data:
> + kfree(data);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static void ccu_rst_free_data(struct ccu_rst_data *data)
> +{
> + kfree(data->rsts);
>
Not necessary.
> + kfree(data);
> +}
I would fold this into ccu_rst_hw_unregister().
> +
> +static void ccu_rst_init_desc(struct ccu_rst_data *data)
> +{
> + struct ccu_rst *rst = data->rsts;
> + unsigned int idx;
> +
> + for (idx = 0; idx < data->rsts_num; ++idx, ++rst) {
> + const struct ccu_rst_info *info = &data->rsts_info[idx];
> +
> + rst->id = info->id;
> + rst->type = info->type;
> + rst->reg_ctl = info->base;
> + rst->sys_regs = data->sys_regs;
> + rst->mask = info->mask;
> + }
> +}
Not necessary.
> +static int ccu_rst_dev_register(struct ccu_rst_data *data)
> +{
> + int ret;
> +
> + data->rcdev.ops = &ccu_rst_ops;
> + data->rcdev.of_node = data->np;
> + data->rcdev.nr_resets = data->rsts_num;
> + data->rcdev.of_reset_n_cells = 1;
> + data->rcdev.of_xlate = ccu_rst_of_idx_get;
> +
> + ret = reset_controller_register(&data->rcdev);
> + if (ret) {
> + pr_err("Couldn't register '%s' reset controller\n",
> + of_node_full_name(data->np));
> + }
> +
> + return ret;
> +}
> +
> +static void ccu_rst_dev_unregister(struct ccu_rst_data *data)
> +{
> + reset_controller_unregister(&data->rcdev);
> +}
I would fold this into ccu_rst_hw_unregister().
> +struct ccu_rst_data *ccu_rst_hw_register(const struct ccu_rst_init_data *rst_init)
> +{
> + struct ccu_rst_data *data;
> + int ret;
> +
> + data = ccu_rst_create_data(rst_init);
> + if (IS_ERR(data))
> + return data;
> +
> + ccu_rst_init_desc(data);
Not necessary.
> +
> + ret = ccu_rst_dev_register(data);
> + if (ret)
> + goto err_free_data;
> +
> + return data;
> +
> +err_free_data:
> + ccu_rst_free_data(data);
> +
> + return ERR_PTR(ret);
> +}
> +
> +void ccu_rst_hw_unregister(struct ccu_rst_data *data)
> +{
> + ccu_rst_dev_unregister(data);
> +
> + ccu_rst_free_data(data);
> +}
To me it looks like you could avoid a few unnecessary complications and
copied data if you merged ccu-rst.c and clk-ccu-rst.c and made use of
the contiguous reset ids instead of the custom of_xlate and the copied
ccu_rst descriptors.
regards
Philipp
