| Message ID | 20241001042401.31903-7-ziyao@disroot.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support clock and reset unit of Rockchip RK3528 | expand |
Hi, Am Dienstag, 1. Oktober 2024, 06:23:59 CEST schrieb Yao Zi: > This clock type is similar to GATE, but doesn't allow rate setting, > which presents on RK3528 platform. this definitly needs more explanation in the commit message. I.e. regular individual gates always set the CLK_SET_RATE_PARENT flag because of course the gates themselfs cannot influence the rate. But in general, I'm also not convinced yet. Yes if some driver tries to change the rate on those, it may affect the parent rate, but that is also true for the other individual gates. So what makes aclk_emmc (as GATE_NO_SET_RATE) more special than "hclk_emmc" (as regular GATE). [Same for the other clocks of course] . So this either needs more explanation, or for the sake of simplicity use regular GATE for now for those and we revisit when it becomes necessary. Heiko > Signed-off-by: Yao Zi <ziyao@disroot.org> > --- > drivers/clk/rockchip/clk.c | 8 ++++++++ > drivers/clk/rockchip/clk.h | 14 ++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c > index 73d2cbdc716b..7d233770e68b 100644 > --- a/drivers/clk/rockchip/clk.c > +++ b/drivers/clk/rockchip/clk.c > @@ -521,6 +521,14 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx, > case branch_gate: > flags |= CLK_SET_RATE_PARENT; > > + clk = clk_register_gate(NULL, list->name, > + list->parent_names[0], flags, > + ctx->reg_base + list->gate_offset, > + list->gate_shift, list->gate_flags, &ctx->lock); > + break; > + case branch_gate_no_set_rate: > + flags &= ~CLK_SET_RATE_PARENT; > + > clk = clk_register_gate(NULL, list->name, > list->parent_names[0], flags, > ctx->reg_base + list->gate_offset, > diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h > index 1efc5c3a1e77..360d16402fe5 100644 > --- a/drivers/clk/rockchip/clk.h > +++ b/drivers/clk/rockchip/clk.h > @@ -519,6 +519,7 @@ enum rockchip_clk_branch_type { > branch_divider, > branch_fraction_divider, > branch_gate, > + branch_gate_no_set_rate, > branch_mmc, > branch_inverter, > branch_factor, > @@ -844,6 +845,19 @@ struct rockchip_clk_branch { > .gate_flags = gf, \ > } > > +#define GATE_NO_SET_RATE(_id, cname, pname, f, o, b, gf) \ > + { \ > + .id = _id, \ > + .branch_type = branch_gate_no_set_rate, \ > + .name = cname, \ > + .parent_names = (const char *[]){ pname }, \ > + .num_parents = 1, \ > + .flags = f, \ > + .gate_offset = o, \ > + .gate_shift = b, \ > + .gate_flags = gf, \ > + } > + > #define MMC(_id, cname, pname, offset, shift) \ > { \ > .id = _id, \ >
On Wed, Oct 02, 2024 at 10:08:36AM +0200, Heiko Stübner wrote: > Hi, > > Am Dienstag, 1. Oktober 2024, 06:23:59 CEST schrieb Yao Zi: > > This clock type is similar to GATE, but doesn't allow rate setting, > > which presents on RK3528 platform. > > this definitly needs more explanation in the commit message. > > I.e. regular individual gates always set the CLK_SET_RATE_PARENT flag > because of course the gates themselfs cannot influence the rate. > > > But in general, I'm also not convinced yet. Yes if some driver tries to > change the rate on those, it may affect the parent rate, but that is also > true for the other individual gates. > > So what makes aclk_emmc (as GATE_NO_SET_RATE) more special than > "hclk_emmc" (as regular GATE). [Same for the other clocks of course] . > > > So this either needs more explanation, or for the sake of simplicity > use regular GATE for now for those and we revisit when it becomes > necessary. I agree that more digging is needed for GATE_NO_SET_RATE. If no obvious reason for adding a clock type could be found, will convert these clocks into general GATEs and give it a try. Cheers, Yao Zi
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c index 73d2cbdc716b..7d233770e68b 100644 --- a/drivers/clk/rockchip/clk.c +++ b/drivers/clk/rockchip/clk.c @@ -521,6 +521,14 @@ void rockchip_clk_register_branches(struct rockchip_clk_provider *ctx, case branch_gate: flags |= CLK_SET_RATE_PARENT; + clk = clk_register_gate(NULL, list->name, + list->parent_names[0], flags, + ctx->reg_base + list->gate_offset, + list->gate_shift, list->gate_flags, &ctx->lock); + break; + case branch_gate_no_set_rate: + flags &= ~CLK_SET_RATE_PARENT; + clk = clk_register_gate(NULL, list->name, list->parent_names[0], flags, ctx->reg_base + list->gate_offset, diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h index 1efc5c3a1e77..360d16402fe5 100644 --- a/drivers/clk/rockchip/clk.h +++ b/drivers/clk/rockchip/clk.h @@ -519,6 +519,7 @@ enum rockchip_clk_branch_type { branch_divider, branch_fraction_divider, branch_gate, + branch_gate_no_set_rate, branch_mmc, branch_inverter, branch_factor, @@ -844,6 +845,19 @@ struct rockchip_clk_branch { .gate_flags = gf, \ } +#define GATE_NO_SET_RATE(_id, cname, pname, f, o, b, gf) \ + { \ + .id = _id, \ + .branch_type = branch_gate_no_set_rate, \ + .name = cname, \ + .parent_names = (const char *[]){ pname }, \ + .num_parents = 1, \ + .flags = f, \ + .gate_offset = o, \ + .gate_shift = b, \ + .gate_flags = gf, \ + } + #define MMC(_id, cname, pname, offset, shift) \ { \ .id = _id, \
This clock type is similar to GATE, but doesn't allow rate setting, which presents on RK3528 platform. Signed-off-by: Yao Zi <ziyao@disroot.org> --- drivers/clk/rockchip/clk.c | 8 ++++++++ drivers/clk/rockchip/clk.h | 14 ++++++++++++++ 2 files changed, 22 insertions(+)