Message-ID: <20220711150850.mn63zaky6gh24c6i@mobilestation>
Date:   Mon, 11 Jul 2022 18:08:50 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-clk@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 5/7] clk: baikal-t1: Move reset-controls code into a
 dedicated module

On Mon, Jul 11, 2022 at 03:23:07PM +0200, Philipp Zabel wrote:
> 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@...kalelectronics.ru>
> 

> Nothing left I'd insist to be changed, so:
> 
> Reviewed-by: Philipp Zabel <p.zabel@...gutronix.de>

Thanks.

> 
> 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@...kalelectronics.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.

This is a matter of the fields usage. If they were used in simple
arithmetic statements, then that would have been a good reason to use the
simple arithmetic types thus saving several bytes of data memory. But this
case is different:
1. Both of these fields are directly passed to the regmap-methods, which
accept "unsigned int" arguments. So if we have "unsigned int" types there
won't be needed any implicit and explicit type casts.
2. Since they are used directly in the regmap-methods having additional
arithmetics in there will needlessly complicate the code.

So to speak having "unsigned int" types and ready-to-use mask in this case
provides a better readable code. Changing just type of the base field
won't give any benefit due to the structure padding. Thus the most optimal
construction is to keep the structure as is.

> 
> > 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.

Thanks for the suggestion. In this case I'd rather have a macro for the
code unification with the rest of the container-of wrappers defined in
ccu-div.h and ccu-pll.h.

> 
> > 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.

Got it. Thanks.

> 
> > -	}
> > -
> > -	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.

Got it. I'll fix it in v7.

-Sergey

> 
> regards
> Philipp

Powered by blists - more mailing lists