| Message ID | 20231218-pinephone-pll-fixes-v1-1-e238b6ed6dc1@oltmanns.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Pinephone video out fixes (flipping between two frames) | expand |
On 2023-12-18 at 14:35:19 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote: > The Allwinner A64 manual lists the following constraints for the > PLL-MIPI clock: > - M/N >= 3 > - (PLL_VIDEO0)/M >= 24MHz > > The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for > these constraints. > > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > --- > drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++ > drivers/clk/sunxi-ng/ccu_nkm.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c > index eed64547ad42..2af5c1ebd527 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkm.c > +++ b/drivers/clk/sunxi-ng/ccu_nkm.c > @@ -16,6 +16,20 @@ struct _ccu_nkm { > unsigned long m, min_m, max_m; > }; > > +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent, > + unsigned long n, unsigned long m) > +{ > + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common); > + > + if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n)) > + return false; > + > + if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m)) > + return false; > + > + return true; > +} > + > static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common, > struct clk_hw *parent_hw, > unsigned long *parent, unsigned long rate, > @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common > > tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k)); > > + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m)) > + continue; > + > tmp_rate = tmp_parent * _n * _k / _m; > > if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) || > @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate, > for (_k = nkm->min_k; _k <= nkm->max_k; _k++) { > for (_n = nkm->min_n; _n <= nkm->max_n; _n++) { > for (_m = nkm->min_m; _m <= nkm->max_m; _m++) { > + if ((common->reg == 0x040) && (_m > 3 * _n)) > + break; > + > + if ((common->reg == 0x040) && (parent < 24000000 * _m)) > + continue; > + This, of course, is rubbish and should be this instead: + if (!ccu_nkm_is_valid_rate(common, parent, _n, _m)) + continue; + I'll submit a V2 after receiving some feedback. > unsigned long tmp_rate; > > tmp_rate = parent * _n * _k / _m; > diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h > index 6601defb3f38..d3d3eaf55faf 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkm.h > +++ b/drivers/clk/sunxi-ng/ccu_nkm.h > @@ -16,6 +16,12 @@ > * struct ccu_nkm - Definition of an N-K-M clock > * > * Clocks based on the formula parent * N * K / M > + * > + * @max_mn_ratio: Maximum value for M / N. > + * @parent_wo_nk: The minimum rate the parent must provide after applying the divisor, > + * but without applying the multipliers, i.e. the contstraint > + * (parent rate)/M >= parent_wo_nk > + * must be fulfilled. > */ > struct ccu_nkm { > u32 enable; > @@ -27,6 +33,8 @@ struct ccu_nkm { > struct ccu_mux_internal mux; > > unsigned int fixed_post_div; > + unsigned long max_mn_ratio; > + unsigned long parent_wo_nk; > > struct ccu_common common; > };
Hi Frank! Dne ponedeljek, 18. december 2023 ob 14:35:19 CET je Frank Oltmanns napisal(a): > The Allwinner A64 manual lists the following constraints for the > PLL-MIPI clock: > - M/N >= 3 This should be "<=" > - (PLL_VIDEO0)/M >= 24MHz > > The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for > these constraints. > > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > --- > drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++ > drivers/clk/sunxi-ng/ccu_nkm.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c > index eed64547ad42..2af5c1ebd527 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkm.c > +++ b/drivers/clk/sunxi-ng/ccu_nkm.c > @@ -16,6 +16,20 @@ struct _ccu_nkm { > unsigned long m, min_m, max_m; > }; > > +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent, > + unsigned long n, unsigned long m) > +{ > + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common); > + > + if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n)) > + return false; > + > + if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m)) > + return false; > + > + return true; > +} > + > static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common, > struct clk_hw *parent_hw, > unsigned long *parent, unsigned long rate, > @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common > > tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k)); > > + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m)) > + continue; > + > tmp_rate = tmp_parent * _n * _k / _m; > > if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) || > @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate, > for (_k = nkm->min_k; _k <= nkm->max_k; _k++) { > for (_n = nkm->min_n; _n <= nkm->max_n; _n++) { > for (_m = nkm->min_m; _m <= nkm->max_m; _m++) { > + if ((common->reg == 0x040) && (_m > 3 * _n)) > + break; > + > + if ((common->reg == 0x040) && (parent < 24000000 * _m)) > + continue; > + You already figured this part. > unsigned long tmp_rate; > > tmp_rate = parent * _n * _k / _m; > diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h > index 6601defb3f38..d3d3eaf55faf 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkm.h > +++ b/drivers/clk/sunxi-ng/ccu_nkm.h > @@ -16,6 +16,12 @@ > * struct ccu_nkm - Definition of an N-K-M clock > * > * Clocks based on the formula parent * N * K / M > + * > + * @max_mn_ratio: Maximum value for M / N. > + * @parent_wo_nk: The minimum rate the parent must provide after applying the divisor, > + * but without applying the multipliers, i.e. the contstraint > + * (parent rate)/M >= parent_wo_nk > + * must be fulfilled. > */ > struct ccu_nkm { > u32 enable; > @@ -27,6 +33,8 @@ struct ccu_nkm { > struct ccu_mux_internal mux; > > unsigned int fixed_post_div; > + unsigned long max_mn_ratio; > + unsigned long parent_wo_nk; What about max_m_n_ratio and max_parent_m_ratio, to be consistent? This should also allow to simplify description. Best regards, Jernej > > struct ccu_common common; > }; > >
Hi Jernej! On 2023-12-19 at 17:46:08 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > Hi Frank! > > Dne ponedeljek, 18. december 2023 ob 14:35:19 CET je Frank Oltmanns napisal(a): >> The Allwinner A64 manual lists the following constraints for the >> PLL-MIPI clock: >> - M/N >= 3 > > This should be "<=" Yes, good catch! I will fix it in V2. > >> - (PLL_VIDEO0)/M >= 24MHz >> >> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for >> these constraints. >> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> >> --- >> drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++ >> drivers/clk/sunxi-ng/ccu_nkm.h | 8 ++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c >> index eed64547ad42..2af5c1ebd527 100644 >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c >> @@ -16,6 +16,20 @@ struct _ccu_nkm { >> unsigned long m, min_m, max_m; >> }; >> >> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent, >> + unsigned long n, unsigned long m) >> +{ >> + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common); >> + >> + if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n)) >> + return false; >> + >> + if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m)) >> + return false; >> + >> + return true; >> +} >> + >> static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common, >> struct clk_hw *parent_hw, >> unsigned long *parent, unsigned long rate, >> @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common >> >> tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k)); >> >> + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m)) >> + continue; >> + >> tmp_rate = tmp_parent * _n * _k / _m; >> >> if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) || >> @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate, >> for (_k = nkm->min_k; _k <= nkm->max_k; _k++) { >> for (_n = nkm->min_n; _n <= nkm->max_n; _n++) { >> for (_m = nkm->min_m; _m <= nkm->max_m; _m++) { >> + if ((common->reg == 0x040) && (_m > 3 * _n)) >> + break; >> + >> + if ((common->reg == 0x040) && (parent < 24000000 * _m)) >> + continue; >> + > > You already figured this part. > >> unsigned long tmp_rate; >> >> tmp_rate = parent * _n * _k / _m; >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h >> index 6601defb3f38..d3d3eaf55faf 100644 >> --- a/drivers/clk/sunxi-ng/ccu_nkm.h >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h >> @@ -16,6 +16,12 @@ >> * struct ccu_nkm - Definition of an N-K-M clock >> * >> * Clocks based on the formula parent * N * K / M >> + * >> + * @max_mn_ratio: Maximum value for M / N. >> + * @parent_wo_nk: The minimum rate the parent must provide after applying the divisor, >> + * but without applying the multipliers, i.e. the contstraint >> + * (parent rate)/M >= parent_wo_nk >> + * must be fulfilled. >> */ >> struct ccu_nkm { >> u32 enable; >> @@ -27,6 +33,8 @@ struct ccu_nkm { >> struct ccu_mux_internal mux; >> >> unsigned int fixed_post_div; >> + unsigned long max_mn_ratio; >> + unsigned long parent_wo_nk; > > What about max_m_n_ratio and max_parent_m_ratio, to be consistent? This > should also allow to simplify description. Jernej, thank you so much! This is brilliant! I was racking my brain for a good name but failed. Now, that I see your proposal, I don't know why I hadn't come up with it. It's the obvious choice. I'd say with the new names we should be able to get rid of the comments describing the new struct members (also in ccu_nm.h). What are your thoughts on that? Best regards, Frank > > Best regards, > Jernej > >> >> struct ccu_common common; >> }; >> >>
Dne sreda, 20. december 2023 ob 07:58:07 CET je Frank Oltmanns napisal(a): > Hi Jernej! > > On 2023-12-19 at 17:46:08 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Hi Frank! > > > > Dne ponedeljek, 18. december 2023 ob 14:35:19 CET je Frank Oltmanns napisal(a): > >> The Allwinner A64 manual lists the following constraints for the > >> PLL-MIPI clock: > >> - M/N >= 3 > > > > This should be "<=" > > Yes, good catch! I will fix it in V2. > > > > >> - (PLL_VIDEO0)/M >= 24MHz > >> > >> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for > >> these constraints. > >> > >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > >> --- > >> drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++ > >> drivers/clk/sunxi-ng/ccu_nkm.h | 8 ++++++++ > >> 2 files changed, 31 insertions(+) > >> > >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c > >> index eed64547ad42..2af5c1ebd527 100644 > >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c > >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c > >> @@ -16,6 +16,20 @@ struct _ccu_nkm { > >> unsigned long m, min_m, max_m; > >> }; > >> > >> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent, > >> + unsigned long n, unsigned long m) > >> +{ > >> + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common); > >> + > >> + if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n)) > >> + return false; > >> + > >> + if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m)) > >> + return false; > >> + > >> + return true; > >> +} > >> + > >> static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common, > >> struct clk_hw *parent_hw, > >> unsigned long *parent, unsigned long rate, > >> @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common > >> > >> tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k)); > >> > >> + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m)) > >> + continue; > >> + > >> tmp_rate = tmp_parent * _n * _k / _m; > >> > >> if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) || > >> @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate, > >> for (_k = nkm->min_k; _k <= nkm->max_k; _k++) { > >> for (_n = nkm->min_n; _n <= nkm->max_n; _n++) { > >> for (_m = nkm->min_m; _m <= nkm->max_m; _m++) { > >> + if ((common->reg == 0x040) && (_m > 3 * _n)) > >> + break; > >> + > >> + if ((common->reg == 0x040) && (parent < 24000000 * _m)) > >> + continue; > >> + > > > > You already figured this part. > > > >> unsigned long tmp_rate; > >> > >> tmp_rate = parent * _n * _k / _m; > >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h > >> index 6601defb3f38..d3d3eaf55faf 100644 > >> --- a/drivers/clk/sunxi-ng/ccu_nkm.h > >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h > >> @@ -16,6 +16,12 @@ > >> * struct ccu_nkm - Definition of an N-K-M clock > >> * > >> * Clocks based on the formula parent * N * K / M > >> + * > >> + * @max_mn_ratio: Maximum value for M / N. > >> + * @parent_wo_nk: The minimum rate the parent must provide after applying the divisor, > >> + * but without applying the multipliers, i.e. the contstraint > >> + * (parent rate)/M >= parent_wo_nk > >> + * must be fulfilled. > >> */ > >> struct ccu_nkm { > >> u32 enable; > >> @@ -27,6 +33,8 @@ struct ccu_nkm { > >> struct ccu_mux_internal mux; > >> > >> unsigned int fixed_post_div; > >> + unsigned long max_mn_ratio; > >> + unsigned long parent_wo_nk; > > > > What about max_m_n_ratio and max_parent_m_ratio, to be consistent? This > > should also allow to simplify description. > > Jernej, thank you so much! This is brilliant! I was racking my brain for > a good name but failed. Now, that I see your proposal, I don't know why > I hadn't come up with it. It's the obvious choice. > > I'd say with the new names we should be able to get rid of the comments > describing the new struct members (also in ccu_nm.h). What are your > thoughts on that? Ah, I missed that only new ones are documented. Yeah, you can skip it. Best regards, Jernej > > Best regards, > Frank > > > > > Best regards, > > Jernej > > > >> > >> struct ccu_common common; > >> }; > >> > >> >
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c index eed64547ad42..2af5c1ebd527 100644 --- a/drivers/clk/sunxi-ng/ccu_nkm.c +++ b/drivers/clk/sunxi-ng/ccu_nkm.c @@ -16,6 +16,20 @@ struct _ccu_nkm { unsigned long m, min_m, max_m; }; +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent, + unsigned long n, unsigned long m) +{ + struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common); + + if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n)) + return false; + + if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m)) + return false; + + return true; +} + static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common, struct clk_hw *parent_hw, unsigned long *parent, unsigned long rate, @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k)); + if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m)) + continue; + tmp_rate = tmp_parent * _n * _k / _m; if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) || @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate, for (_k = nkm->min_k; _k <= nkm->max_k; _k++) { for (_n = nkm->min_n; _n <= nkm->max_n; _n++) { for (_m = nkm->min_m; _m <= nkm->max_m; _m++) { + if ((common->reg == 0x040) && (_m > 3 * _n)) + break; + + if ((common->reg == 0x040) && (parent < 24000000 * _m)) + continue; + unsigned long tmp_rate; tmp_rate = parent * _n * _k / _m; diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h index 6601defb3f38..d3d3eaf55faf 100644 --- a/drivers/clk/sunxi-ng/ccu_nkm.h +++ b/drivers/clk/sunxi-ng/ccu_nkm.h @@ -16,6 +16,12 @@ * struct ccu_nkm - Definition of an N-K-M clock * * Clocks based on the formula parent * N * K / M + * + * @max_mn_ratio: Maximum value for M / N. + * @parent_wo_nk: The minimum rate the parent must provide after applying the divisor, + * but without applying the multipliers, i.e. the contstraint + * (parent rate)/M >= parent_wo_nk + * must be fulfilled. */ struct ccu_nkm { u32 enable; @@ -27,6 +33,8 @@ struct ccu_nkm { struct ccu_mux_internal mux; unsigned int fixed_post_div; + unsigned long max_mn_ratio; + unsigned long parent_wo_nk; struct ccu_common common; };
The Allwinner A64 manual lists the following constraints for the PLL-MIPI clock: - M/N >= 3 - (PLL_VIDEO0)/M >= 24MHz The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for these constraints. Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> --- drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++ drivers/clk/sunxi-ng/ccu_nkm.h | 8 ++++++++ 2 files changed, 31 insertions(+)