| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3fhnjybnrrixse5h6epfp2aj7pk3uo6fk34gvsqkku6sofir6r@t3ofcy5xure6>
Date: Wed, 18 Dec 2024 17:03:01 +0800
From: Inochi Amaoto <inochiama@...il.com>
To: Yixun Lan <dlan@...too.org>, Haylen Chu <heylenay@....org>
Cc: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Haylen Chu <heylenay@...look.com>, linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Inochi Amaoto <inochiama@...look.com>, Chen Wang <unicornxdotw@...mail.com>,
Jisheng Zhang <jszhang@...nel.org>, Inochi Amaoto <inochiama@...il.com>
Subject: Re: [PATCH v3 3/3] clk: spacemit: Add clock support for Spacemit K1
SoC
On Wed, Dec 18, 2024 at 03:27:27PM +0800, Yixun Lan wrote:
> On 14:47 Sun 08 Dec , Inochi Amaoto wrote:
> > On Tue, Nov 26, 2024 at 02:31:28PM +0000, Haylen Chu wrote:
> > > The clock tree of K1 SoC contains three main types of clock hardware
> > > (PLL/DDN/MIX) and is managed by several independent controllers in
> > > different SoC parts (APBC, APBS and etc.), thus different compatible
> > > strings are added to distinguish them.
> > >
> > > Some controllers may share IO region with reset controller and other low
> > > speed peripherals like watchdog, so all register operations are done
> > > through regmap to avoid competition.
> > >
> > > Signed-off-by: Haylen Chu <heylenay@....org>
> > > ---
> > > drivers/clk/Kconfig | 1 +
> > > drivers/clk/Makefile | 1 +
> > > drivers/clk/spacemit/Kconfig | 20 +
> > > drivers/clk/spacemit/Makefile | 5 +
> > > drivers/clk/spacemit/ccu-k1.c | 1747 +++++++++++++++++++++++++++++
> > > drivers/clk/spacemit/ccu_common.h | 62 +
> > > drivers/clk/spacemit/ccu_ddn.c | 146 +++
> > > drivers/clk/spacemit/ccu_ddn.h | 85 ++
> > > drivers/clk/spacemit/ccu_mix.c | 296 +++++
> > > drivers/clk/spacemit/ccu_mix.h | 336 ++++++
> > > drivers/clk/spacemit/ccu_pll.c | 198 ++++
> > > drivers/clk/spacemit/ccu_pll.h | 80 ++
> ....snip
> > > diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
> > > new file mode 100644
> > > index 000000000000..de343405fcc7
> > > --- /dev/null
> > > +++ b/drivers/clk/spacemit/ccu_mix.c
> > > @@ -0,0 +1,296 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Spacemit clock type mix(div/mux/gate/factor)
> > > + *
> > > + * Copyright (c) 2024 SpacemiT Technology Co. Ltd
> > > + * Copyright (c) 2024 Haylen Chu <heylenay@...look.com>
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +
> > > +#include "ccu_mix.h"
> > > +
> > > +#define MIX_TIMEOUT 10000
> > > +
> > > +#define mix_hwparam_in_sel(c) \
> > > + ((c)->reg_type == CLK_DIV_TYPE_2REG_NOFC_V3 || \
> > > + (c)->reg_type == CLK_DIV_TYPE_2REG_FC_V4)
> > > +
> >
> > > +static void ccu_mix_disable(struct clk_hw *hw)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_gate_config *gate = mix->gate;
> > > +
> > > + if (!gate)
> > > + return;
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_update(sel, common, gate->gate_mask, gate->val_disable);
> > > + else
> > > + ccu_update(ctrl, common, gate->gate_mask, gate->val_disable);
> > > +}
> > > +
> > > +static int ccu_mix_enable(struct clk_hw *hw)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_gate_config *gate = mix->gate;
> > > + u32 val_enable, mask;
> > > + u32 tmp;
> > > +
> > > + if (!gate)
> > > + return 0;
> > > +
> > > + val_enable = gate->val_enable;
> > > + mask = gate->gate_mask;
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_update(sel, common, mask, val_enable);
> > > + else
> > > + ccu_update(ctrl, common, mask, val_enable);
> > > +
> > > + if (common->reg_type == CLK_DIV_TYPE_2REG_NOFC_V3 ||
> > > + common->reg_type == CLK_DIV_TYPE_2REG_FC_V4)
> > > + return ccu_poll(sel, common, tmp, (tmp & mask) == val_enable,
> > > + 10, MIX_TIMEOUT);
> > > + else
> > > + return ccu_poll(ctrl, common, tmp, (tmp & mask) == val_enable,
> > > + 10, MIX_TIMEOUT);
> > > +}
> > > +
> > > +static int ccu_mix_is_enabled(struct clk_hw *hw)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_gate_config *gate = mix->gate;
> > > + u32 tmp;
> > > +
> > > + if (!gate)
> > > + return 1;
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_read(sel, common, &tmp);
> > > + else
> > > + ccu_read(ctrl, common, &tmp);
> > > +
> > > + return (tmp & gate->gate_mask) == gate->val_enable;
> > > +}
> > > +
> > > +static unsigned long ccu_mix_recalc_rate(struct clk_hw *hw,
> > > + unsigned long parent_rate)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_div_config *div = mix->div;
> > > + unsigned long val;
> > > + u32 reg;
> > > +
> > > + if (!div) {
> > > + if (mix->factor)
> > > + return parent_rate * mix->factor->mul / mix->factor->div;
> > > +
> > > + return parent_rate;
> > > + }
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_read(sel, common, ®);
> > > + else
> > > + ccu_read(ctrl, common, ®);
> > > +
> > > + val = reg >> div->shift;
> > > + val &= (1 << div->width) - 1;
> > > +
> > > + val = divider_recalc_rate(hw, parent_rate, val, div->table,
> > > + div->flags, div->width);
> > > +
> > > + return val;
> > > +}
> >
> > I think you should distinguish these muxs, it is not good to
> > use mix_hwparam_in_sel everywhere. There are two types of mux.
> >
> > > +
> > > +
> >
> > Double empty line here, you should run checkpatch.
> >
> > > +static int ccu_mix_trigger_fc(struct clk_hw *hw)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + unsigned int val = 0;
> > > + int ret = 0;
> > > +
> > > + if (common->reg_type == CLK_DIV_TYPE_1REG_FC_V2 ||
> > > + common->reg_type == CLK_DIV_TYPE_2REG_FC_V4 ||
> > > + common->reg_type == CLK_DIV_TYPE_1REG_FC_DIV_V5 ||
> > > + common->reg_type == CLK_DIV_TYPE_1REG_FC_MUX_V6) {
> > > + ccu_update(ctrl, common, common->fc, common->fc);
> > > +
> > > + ret = ccu_poll(ctrl, common, val, !(val & common->fc),
> > > + 5, MIX_TIMEOUT);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int ccu_mix_determine_rate(struct clk_hw *hw,
> > > + struct clk_rate_request *req)
> > > +{
> > > + return 0;
> > > +}
> > > +
> >
> > Why a empty determine_rate function?
> >
> > > +static unsigned long
> > > +ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate, u32 *mux_val,
> > > + u32 *div_val)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_div_config *div = mix->div ? mix->div : NULL;
> > > + struct clk_hw *parent;
> > > + unsigned long parent_rate = 0, best_rate = 0;
> > > + u32 i, j, div_max;
> > > +
> > > + for (i = 0; i < common->num_parents; i++) {
> > > + parent = clk_hw_get_parent_by_index(hw, i);
> > > + if (!parent)
> > > + continue;
> > > +
> > > + parent_rate = clk_hw_get_rate(parent);
> > > +
> > > + if (div)
> > > + div_max = 1 << div->width;
> > > + else
> > > + div_max = 1;
> > > +
> > > + for (j = 1; j <= div_max; j++) {
> > > + if (abs(parent_rate/j - rate) < abs(best_rate - rate)) {
> > > + best_rate = DIV_ROUND_UP_ULL(parent_rate, j);
> > > + *mux_val = i;
> > > + *div_val = j - 1;
> > > + }
> > > + }
> > > + }
> > > +
> > > + return best_rate;
> > > +}
> > > +
> > > +static int ccu_mix_set_rate(struct clk_hw *hw, unsigned long rate,
> > > + unsigned long parent_rate)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_div_config *div = mix->div;
> > > + struct ccu_mux_config *mux = mix->mux;
> > > + u32 cur_mux, cur_div, mux_val = 0, div_val = 0;
> > > + unsigned long best_rate = 0;
> > > + int ret = 0, tmp = 0;
> > > +
> > > + if (!div && !mux)
> > > + return 0;
> > > +
> > > + best_rate = ccu_mix_calc_best_rate(hw, rate, &mux_val, &div_val);
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_read(sel, common, &tmp);
> > > + else
> > > + ccu_read(ctrl, common, &tmp);
> > > +
> > > + if (mux) {
> > > + cur_mux = tmp >> mux->shift;
> > > + cur_mux &= (1 << mux->width) - 1;
> > > +
> > > + if (cur_mux != mux_val)
> > > + clk_hw_set_parent(hw, clk_hw_get_parent_by_index(hw, mux_val));
> > > + }
> > > +
> > > + if (div) {
> > > + cur_div = tmp >> div->shift;
> > > + cur_div &= (1 << div->width) - 1;
> > > +
> > > + if (cur_div == div_val)
> > > + return 0;
> > > + } else {
> > > + return 0;
> > > + }
> > > +
> > > + tmp = GENMASK(div->width + div->shift - 1, div->shift);
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_update(sel, common, tmp, div_val << div->shift);
> > > + else
> > > + ccu_update(ctrl, common, tmp, div_val << div->shift);
> > > +
> > > + if (common->reg_type == CLK_DIV_TYPE_1REG_FC_V2 ||
> > > + common->reg_type == CLK_DIV_TYPE_2REG_FC_V4 ||
> > > + common->reg_type == CLK_DIV_TYPE_1REG_FC_DIV_V5)
> > > + ret = ccu_mix_trigger_fc(hw);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static u8 ccu_mix_get_parent(struct clk_hw *hw)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_mux_config *mux = mix->mux;
> > > + u32 reg;
> > > + u8 parent;
> > > +
> > > + if (!mux)
> > > + return 0;
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_read(sel, common, ®);
> > > + else
> > > + ccu_read(ctrl, common, ®);
> > > +
> > > + parent = reg >> mux->shift;
> > > + parent &= (1 << mux->width) - 1;
> > > +
> > > + if (mux->table) {
> > > + int num_parents = clk_hw_get_num_parents(&common->hw);
> > > + int i;
> > > +
> > > + for (i = 0; i < num_parents; i++)
> > > + if (mux->table[i] == parent)
> > > + return i;
> > > + }
> > > +
> > > + return parent;
> > > +}
> > > +
> > > +static int ccu_mix_set_parent(struct clk_hw *hw, u8 index)
> > > +{
> > > + struct ccu_mix *mix = hw_to_ccu_mix(hw);
> > > + struct ccu_common *common = &mix->common;
> > > + struct ccu_mux_config *mux = mix->mux;
> > > + int ret = 0;
> > > + u32 mask;
> > > +
> > > + if (!mux)
> > > + return 0;
> > > +
> > > + if (mux->table)
> > > + index = mux->table[index];
> > > +
> > > + mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> > > +
> > > + if (mix_hwparam_in_sel(common))
> > > + ccu_update(sel, common, mask, index << mux->shift);
> > > + else
> > > + ccu_update(ctrl, common, mask, index << mux->shift);
> > > +
> > > + if (common->reg_type == CLK_DIV_TYPE_1REG_FC_V2 ||
> > > + common->reg_type == CLK_DIV_TYPE_2REG_FC_V4 ||
> > > + common->reg_type == CLK_DIV_TYPE_1REG_FC_MUX_V6)
> > > + ret = ccu_mix_trigger_fc(hw);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +const struct clk_ops spacemit_ccu_mix_ops = {
> > > + .disable = ccu_mix_disable,
> > > + .enable = ccu_mix_enable,
> > > + .is_enabled = ccu_mix_is_enabled,
> > > + .get_parent = ccu_mix_get_parent,
> > > + .set_parent = ccu_mix_set_parent,
> > > + .determine_rate = ccu_mix_determine_rate,
> > > + .recalc_rate = ccu_mix_recalc_rate,
> > > + .set_rate = ccu_mix_set_rate,
> > > +};
> >
> > I think you should separate the clock into different type and
> > use pre-defined function to simplify, but not use a unified,
> > complex and hard to read structure to represent all kinds of
> > clocks.
> >
> agree
>
> I'd not object to use composite clock, it simplify the implementation,
> but, in this version, there is lots of "if" conditions which make it
> hard to review and maintain..
>
> would it possibile to use more basic clk prototype? or just the composite
> model[1] or something similar as owl-composite.c [2] which has more fine
> composite prototype (instead of bundling all in one)
>
> drivers/clk/clk-composite.c [1]
> drivers/clk/actions/owl-composite.c [2]
>
This depends on what you clock is. I guest this mix has a all in one
register to perform mix/div/gate. If so, it is better to provide multiple
clk_ops to adopt the real one. Otherwise, it will be better to use
different clk type to represent the real clock.
Regards,
Inochi
Powered by blists - more mailing lists