Message-ID: <20230801174347.whzhqwzh7vdjqeap@mercury.elektranox.org>
Date:   Tue, 1 Aug 2023 19:43:47 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Elaine Zhang <zhangqing@...k-chips.com>
Cc:     mturquette@...libre.com, sboyd@...nel.org,
        kever.yang@...k-chips.com, heiko@...ech.de,
        linux-clk@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-kernel@...r.kernel.org, huangtao@...k-chips.com
Subject: Re: [PATCH v2 1/3] clk: rockchip: add support for gate link

Hi,

On Tue, Aug 01, 2023 at 03:43:55PM +0800, Elaine Zhang wrote:
> Recent Rockchip SoCs have a new hardware block called Native Interface
> Unit (NIU), which gates clocks to devices behind them. These effectively
> need two parent clocks.
> Use GATE_LINK to handle this.
> 
> Signed-off-by: Elaine Zhang <zhangqing@...k-chips.com>
> ---

This looks better than the current upstream solution of just
marking all linked clocks as criticial. I do have a few comments,
though.

>  drivers/clk/rockchip/Makefile        |   1 +
>  drivers/clk/rockchip/clk-gate-link.c | 189 +++++++++++++++++++++++++++
>  drivers/clk/rockchip/clk.c           |   7 +
>  drivers/clk/rockchip/clk.h           |  22 ++++
>  4 files changed, 219 insertions(+)
>  create mode 100644 drivers/clk/rockchip/clk-gate-link.c
> 
> diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
> index 36894f6a7022..87cc39d54f72 100644
> --- a/drivers/clk/rockchip/Makefile
> +++ b/drivers/clk/rockchip/Makefile
> @@ -13,6 +13,7 @@ clk-rockchip-y += clk-inverter.o
>  clk-rockchip-y += clk-mmc-phase.o
>  clk-rockchip-y += clk-muxgrf.o
>  clk-rockchip-y += clk-ddr.o
> +clk-rockchip-y += clk-gate-link.o
>  clk-rockchip-$(CONFIG_RESET_CONTROLLER) += softrst.o
>  
>  obj-$(CONFIG_CLK_PX30)          += clk-px30.o
> diff --git a/drivers/clk/rockchip/clk-gate-link.c b/drivers/clk/rockchip/clk-gate-link.c
> new file mode 100644
> index 000000000000..06d25f00ec0c
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-gate-link.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Fuzhou Rockchip Electronics Co., Ltd
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/string.h>
> +#include "clk.h"
> +
> +/**
> + * struct clk_gate_link - gating link clock
> + *
> + * @hw:		handle between common and hardware-specific interfaces
> + * @reg:	register controlling gate
> + * @bit_idx:	single bit controlling gate
> + * @flags:	hardware-specific flags
> + * @lock:	register lock
> + * @link:	links clk
> + */
> +struct clk_gate_link {
> +	struct clk_hw hw;
> +	void __iomem	*reg;
> +	u8		bit_idx;
> +	u8		flags;
> +	spinlock_t	*lock;
> +	struct clk	*link;
> +};
> +
> +#define to_clk_gate_link(_hw) container_of(_hw, struct clk_gate_link, hw)
> +
> +static inline u32 clk_gate_readl(struct clk_gate_link *gate)
> +{
> +	if (gate->flags & CLK_GATE_BIG_ENDIAN)
> +		return ioread32be(gate->reg);
> +
> +	return readl(gate->reg);
> +}
> +
> +static inline void clk_gate_writel(struct clk_gate_link *gate, u32 val)
> +{
> +	if (gate->flags & CLK_GATE_BIG_ENDIAN)
> +		iowrite32be(val, gate->reg);
> +	else
> +		writel(val, gate->reg);
> +}

Rockchip platform has no clocks using CLK_GATE_BIG_ENDIAN flag, so
you can just use readl/writel. But by reusing clk-gate's function
(see next comment) you don't need to directly read/write registers
anyways.

> +
> +/*
> + * It works on following logic:
> + *
> + * For enabling clock, enable = 1
> + *	set2dis = 1	-> clear bit	-> set = 0
> + *	set2dis = 0	-> set bit	-> set = 1
> + *
> + * For disabling clock, enable = 0
> + *	set2dis = 1	-> set bit	-> set = 1
> + *	set2dis = 0	-> clear bit	-> set = 0
> + *
> + * So, result is always: enable xor set2dis.
> + */
> +static void clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> +	struct clk_gate_link *gate = to_clk_gate_link(hw);
> +	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	set ^= enable;
> +
> +	if (gate->lock)
> +		spin_lock_irqsave(gate->lock, flags);
> +	else
> +		__acquire(gate->lock);
> +
> +	if (gate->flags & CLK_GATE_HIWORD_MASK) {
> +		reg = BIT(gate->bit_idx + 16);
> +		if (set)
> +			reg |= BIT(gate->bit_idx);
> +	} else {
> +		reg = clk_gate_readl(gate);
> +
> +		if (set)
> +			reg |= BIT(gate->bit_idx);
> +		else
> +			reg &= ~BIT(gate->bit_idx);
> +	}
> +
> +	clk_gate_writel(gate, reg);
> +
> +	if (gate->lock)
> +		spin_unlock_irqrestore(gate->lock, flags);
> +	else
> +		__release(gate->lock);
> +}

By changing "struct clk_gate_link" to look like this:

struct clk_gate_link {
    struct clk_gate gate;
    struct clk	*link;
}

It can be casted to be a normal struct clk_gate, which
means you can reuse clk_gate_endisable() (needs to be
exported) and clk_gate_is_enabled().

> +static int clk_gate_link_enable(struct clk_hw *hw)
> +{
> +	struct clk_gate_link *gate = to_clk_gate_link(hw);
> +
> +	clk_gate_endisable(hw, 1);
> +	clk_enable(gate->link);
> +
> +	return 0;
> +}
> +
> +static void clk_gate_link_disable(struct clk_hw *hw)
> +{
> +	struct clk_gate_link *gate = to_clk_gate_link(hw);
> +
> +	clk_gate_endisable(hw, 0);
> +	clk_disable(gate->link);
> +}
> +
> +static int clk_gate_link_is_enabled(struct clk_hw *hw)
> +{
> +	u32 reg;
> +	struct clk_gate_link *gate = to_clk_gate_link(hw);
> +
> +	reg = clk_gate_readl(gate);
> +
> +	/* if a set bit disables this clk, flip it before masking */
> +	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> +		reg ^= BIT(gate->bit_idx);
> +
> +	reg &= BIT(gate->bit_idx);
> +
> +	return reg ? 1 : 0;
> +}
> +
> +const struct clk_ops clk_gate_link_ops = {
> +	.enable = clk_gate_link_enable,
> +	.disable = clk_gate_link_disable,
> +	.is_enabled = clk_gate_link_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(clk_gate_link_ops);

I don't think they need to be exported?

> +
> +struct clk *rockchip_clk_register_gate_link(struct rockchip_clk_provider *ctx,
> +					    const char *name, const char *parent_name,
> +					    unsigned int link_id, u8 flags,
> +					    void __iomem *gate_offset, u8 gate_shift,
> +					    u8 gate_flags, spinlock_t *lock)
> +{
> +	struct clk_gate_link *gate;
> +	struct clk_init_data init = {};
> +	struct clk **clks;
> +	struct clk *clk_link;
> +
> +	if (gate_flags & CLK_GATE_HIWORD_MASK) {
> +		if (gate_shift > 15) {
> +			pr_err("gate bit exceeds LOWORD field\n");
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	/* allocate the gate */
> +	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clks = ctx->clk_data.clks;
> +	gate->link = clks[link_id];
> +
> +	init.name = name;
> +	init.ops = &clk_gate_link_ops;
> +	init.flags = flags;
> +	init.parent_names = parent_name ? &parent_name : NULL;
> +	init.num_parents = 1;
> +
> +	/* struct clk_gate assignments */
> +	gate->reg = gate_offset;
> +	gate->bit_idx = gate_shift;
> +	gate->flags = gate_flags;
> +	gate->lock = lock;
> +	gate->hw.init = &init;
> +
> +	clk_link = clk_register(NULL, &gate->hw);
> +	if (IS_ERR(clk_link)) {
> +		kfree(gate);
> +		pr_err("%s clk_register field\n", name);
> +		return ERR_CAST(clk_link);
> +	}
> +	clk_prepare(gate->link);

So the linked clock will always be prepared? Can't we just extend clk_gate_link_ops with

.prepare = clk_gate_link_prepare,
.unprepare = clk_gate_link_unprepare,

int clk_gate_link_prepare(clk_gate_link_prepare) {
    struct clk_gate_link *gate = to_clk_gate_link(hw);
    return clk_prepare(gate->link);
}

void clk_gate_link_unprepare(clk_gate_link_prepare) {
    struct clk_gate_link *gate = to_clk_gate_link(hw);
    clk_unprepare(gate->link);
}

Greetings,

-- Sebastian

> +
> +	return clk_link;
> +}
> +EXPORT_SYMBOL_GPL(rockchip_clk_register_gate_link);
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 4059d9365ae6..d981ef6c5487 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -509,6 +509,13 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx,
>  				ctx->reg_base + list->gate_offset,
>  				list->gate_shift, list->gate_flags, &ctx->lock);
>  			break;
> +
> +		case branch_gate_link:
> +			clk = rockchip_clk_register_gate_link(ctx, list->name,
> +				list->parent_names[0], list->link_id, flags,
> +				ctx->reg_base + list->gate_offset,
> +				list->gate_shift, list->gate_flags, &ctx->lock);
> +			break;
>  		case branch_composite:
>  			clk = rockchip_clk_register_branch(list->name,
>  				list->parent_names, list->num_parents,
> diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
> index 758ebaf2236b..b49e3cede33a 100644
> --- a/drivers/clk/rockchip/clk.h
> +++ b/drivers/clk/rockchip/clk.h
> @@ -517,6 +517,7 @@ enum rockchip_clk_branch_type {
>  	branch_divider,
>  	branch_fraction_divider,
>  	branch_gate,
> +	branch_gate_link,
>  	branch_mmc,
>  	branch_inverter,
>  	branch_factor,
> @@ -529,6 +530,7 @@ struct rockchip_clk_branch {
>  	enum rockchip_clk_branch_type	branch_type;
>  	const char			*name;
>  	const char			*const *parent_names;
> +	unsigned int			link_id;
>  	u8				num_parents;
>  	unsigned long			flags;
>  	int				muxdiv_offset;
> @@ -842,6 +844,20 @@ struct rockchip_clk_branch {
>  		.gate_flags	= gf,				\
>  	}
>  
> +#define GATE_LINK(_id, cname, pname, _linkid, f, o, b, gf) \
> +	{							\
> +		.id		= _id,				\
> +		.branch_type	= branch_gate_link,		\
> +		.name		= cname,			\
> +		.parent_names	= (const char *[]){ pname },	\
> +		.num_parents	= 1,				\
> +		.link_id	= _linkid,			\
> +		.flags		= f,				\
> +		.gate_offset	= o,				\
> +		.gate_shift	= b,				\
> +		.gate_flags	= gf,				\
> +	}
> +
>  #define MMC(_id, cname, pname, offset, shift)			\
>  	{							\
>  		.id		= _id,				\
> @@ -1002,6 +1018,12 @@ struct clk *rockchip_clk_register_halfdiv(const char *name,
>  					  unsigned long flags,
>  					  spinlock_t *lock);
>  
> +struct clk *rockchip_clk_register_gate_link(struct rockchip_clk_provider *ctx,
> +					    const char *name, const char *parent_name,
> +					    unsigned int link_id, u8 flags,
> +					    void __iomem *gate_offset, u8 gate_shift,
> +					    u8 gate_flags, spinlock_t *lock);
> +
>  #ifdef CONFIG_RESET_CONTROLLER
>  void rockchip_register_softrst_lut(struct device_node *np,
>  				   const int *lookup_table,
> -- 
> 2.17.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists