Re: [PATCH] clk: sprd: Composite driver support offset config

From: Chunyan Zhang
Date: Fri Aug 25 2023 - 02:27:20 EST

On Sat, 5 Aug 2023 at 15:07, Zhifeng Tang <zhifeng.tang@xxxxxxxxxx> wrote:
>
> The composite interface support the offset configuration,
> which is userd to support mux and div in different registers.

Please fix the typo "userd".

Also, you should describe in more detail the reason why we need this
change. Is it because the divider has different addresses from mux for
one composite clk?

>
> Signed-off-by: Zhifeng Tang <zhifeng.tang@xxxxxxxxxx>
> ---
> drivers/clk/sprd/composite.h | 38 +++++++++++++++++++++++++-----------
> drivers/clk/sprd/div.c | 6 +++---
> drivers/clk/sprd/div.h | 17 +++++++++++-----
> 3 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clk/sprd/composite.h b/drivers/clk/sprd/composite.h
> index adbabbe596b7..74765584021d 100644
> --- a/drivers/clk/sprd/composite.h
> +++ b/drivers/clk/sprd/composite.h
> @@ -19,24 +19,24 @@ struct sprd_comp {
> };
>
> #define SPRD_COMP_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, _table, \
> - _mshift, _mwidth, _dshift, _dwidth, \
> - _flags, _fn) \
> + _mshift, _mwidth, _doffset, _dshift, \
> + _dwidth, _flags, _fn) \
> struct sprd_comp _struct = { \
> .mux = _SPRD_MUX_CLK(_mshift, _mwidth, _table), \
> - .div = _SPRD_DIV_CLK(_dshift, _dwidth), \
> + .div = _SPRD_DIV_CLK(_doffset, _dshift, _dwidth), \
> .common = { \
> .regmap = NULL, \
> .reg = _reg, \
> .hw.init = _fn(_name, _parent, \
> &sprd_comp_ops, _flags), \
> - } \
> + } \
> }
>
> #define SPRD_COMP_CLK_TABLE(_struct, _name, _parent, _reg, _table, \
> _mshift, _mwidth, _dshift, _dwidth, _flags) \
> SPRD_COMP_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, _table, \
> - _mshift, _mwidth, _dshift, _dwidth, \
> - _flags, CLK_HW_INIT_PARENTS)
> + _mshift, _mwidth, 0x0, _dshift, \
> + _dwidth, _flags, CLK_HW_INIT_PARENTS)
>
> #define SPRD_COMP_CLK(_struct, _name, _parent, _reg, _mshift, \
> _mwidth, _dshift, _dwidth, _flags) \
> @@ -47,14 +47,30 @@ struct sprd_comp {
> _mshift, _mwidth, _dshift, \
> _dwidth, _flags) \
> SPRD_COMP_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, _table, \
> - _mshift, _mwidth, _dshift, _dwidth, \
> - _flags, CLK_HW_INIT_PARENTS_DATA)
> + _mshift, _mwidth, 0x0, _dshift, \
> + _dwidth, _flags, \
> + CLK_HW_INIT_PARENTS_DATA)
>
> #define SPRD_COMP_CLK_DATA(_struct, _name, _parent, _reg, _mshift, \
> _mwidth, _dshift, _dwidth, _flags) \
> - SPRD_COMP_CLK_DATA_TABLE(_struct, _name, _parent, _reg, NULL, \
> - _mshift, _mwidth, _dshift, _dwidth, \
> - _flags)
> + SPRD_COMP_CLK_DATA_TABLE(_struct, _name, _parent, _reg, NULL, \
> + _mshift, _mwidth, _dshift, \
> + _dwidth, _flags)

It seems that nothing changes here? Please don't make unrelated changes.

> +
> +#define SPRD_COMP_CLK_DATA_TABLE_OFFSET(_struct, _name, _parent, _reg, \
> + _table, _mshift, _mwidth, \
> + _dshift, _dwidth, _flags) \
> + SPRD_COMP_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, _table, \
> + _mshift, _mwidth, 0x4, _dshift, \

I suggest not using hard code here.

Also I think all _OFFSET() should have an offset parameter, that will
be more flexible and logical.

> + _dwidth, _flags, \
> + CLK_HW_INIT_PARENTS_DATA)
> +
> +#define SPRD_COMP_CLK_DATA_OFFSET(_struct, _name, _parent, _reg, \
> + _mshift, _mwidth, _dshift, \
> + _dwidth, _flags) \
> + SPRD_COMP_CLK_DATA_TABLE_OFFSET(_struct, _name, _parent, _reg, \
> + NULL, _mshift, _mwidth, \
> + _dshift, _dwidth, _flags)
>
> static inline struct sprd_comp *hw_to_sprd_comp(const struct clk_hw *hw)
> {
> diff --git a/drivers/clk/sprd/div.c b/drivers/clk/sprd/div.c
> index c7261630cab4..0fb41d653f1e 100644
> --- a/drivers/clk/sprd/div.c
> +++ b/drivers/clk/sprd/div.c
> @@ -25,7 +25,7 @@ unsigned long sprd_div_helper_recalc_rate(struct sprd_clk_common *common,
> unsigned long val;
> unsigned int reg;
>
> - regmap_read(common->regmap, common->reg, &reg);
> + regmap_read(common->regmap, common->reg - div->offset, &reg);

Generally offset should be an increase based on reg, but it is converse here.

> val = reg >> div->shift;
> val &= (1 << div->width) - 1;
>
> @@ -53,10 +53,10 @@ int sprd_div_helper_set_rate(const struct sprd_clk_common *common,
> val = divider_get_val(rate, parent_rate, NULL,
> div->width, 0);
>
> - regmap_read(common->regmap, common->reg, &reg);
> + regmap_read(common->regmap, common->reg - div->offset, &reg);
> reg &= ~GENMASK(div->width + div->shift - 1, div->shift);
>
> - regmap_write(common->regmap, common->reg,
> + regmap_write(common->regmap, common->reg - div->offset,
> reg | (val << div->shift));
>
> return 0;
> diff --git a/drivers/clk/sprd/div.h b/drivers/clk/sprd/div.h
> index f5d614b3dcf1..db6b62ee6a8d 100644
> --- a/drivers/clk/sprd/div.h
> +++ b/drivers/clk/sprd/div.h
> @@ -20,12 +20,14 @@
> * classes.
> */
> struct sprd_div_internal {
> + u32 offset;

I suggest changing the type to 'signed' int, which would cover the
cases no matter whether the divider register offset is up or down
based on the mux register address. That would be more flexible.

Thanks,
Chunyan

> u8 shift;
> u8 width;
> };
>
> -#define _SPRD_DIV_CLK(_shift, _width) \
> +#define _SPRD_DIV_CLK(_offset, _shift, _width) \
> { \
> + .offset = _offset, \
> .shift = _shift, \
> .width = _width, \
> }
> @@ -35,10 +37,10 @@ struct sprd_div {
> struct sprd_clk_common common;
> };
>
> -#define SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, \
> +#define SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, _offset, \
> _shift, _width, _flags, _fn) \
> struct sprd_div _struct = { \
> - .div = _SPRD_DIV_CLK(_shift, _width), \
> + .div = _SPRD_DIV_CLK(_offset, _shift, _width), \
> .common = { \
> .regmap = NULL, \
> .reg = _reg, \
> @@ -49,12 +51,17 @@ struct sprd_div {
>
> #define SPRD_DIV_CLK(_struct, _name, _parent, _reg, \
> _shift, _width, _flags) \
> - SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, \
> + SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, 0x0, \
> _shift, _width, _flags, CLK_HW_INIT)
>
> +#define SPRD_DIV_CLK_FW_NAME(_struct, _name, _parent, _reg, \
> + _shift, _width, _flags) \
> + SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, 0x0, \
> + _shift, _width, _flags, CLK_HW_INIT_FW_NAME)
> +
> #define SPRD_DIV_CLK_HW(_struct, _name, _parent, _reg, \
> _shift, _width, _flags) \
> - SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, \
> + SPRD_DIV_CLK_HW_INIT_FN(_struct, _name, _parent, _reg, 0x0, \
> _shift, _width, _flags, CLK_HW_INIT_HW)
>
> static inline struct sprd_div *hw_to_sprd_div(const struct clk_hw *hw)
> --
> 2.17.1
>