| Message ID | 20231218-pinephone-pll-fixes-v1-4-e238b6ed6dc1@oltmanns.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Pinephone video out fixes (flipping between two frames) | expand |
Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): > The Allwinner A64 manual lists the following constraint for the > PLL-VIDEO0 clock: 8 <= N/M <= 25 > > Use this constraint. > > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > --- > drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > index c034ac027d1c..75d839da446c 100644 > --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", > BIT(28), /* lock */ > CLK_SET_RATE_UNGATE); > > -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", > +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, > + "pll-video0", > "osc24M", 0x010, > 192000000, /* Minimum rate */ > 1008000000, /* Maximum rate */ > @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid > 297000000, /* frac rate 1 */ > BIT(31), /* gate */ > BIT(28), /* lock */ > - CLK_SET_RATE_UNGATE); > + CLK_SET_RATE_UNGATE, > + CCU_FEATURE_FRACTIONAL | > + CCU_FEATURE_CLOSEST_RATE, Above flags are unrelated change, put them in new patch if needed. Best regards, Jernej > + 8, 25); /* min/max nm ratio */ > > static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", > "osc24M", 0x018, > >
On 2023-12-19 at 17:54:19 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): >> The Allwinner A64 manual lists the following constraint for the >> PLL-VIDEO0 clock: 8 <= N/M <= 25 >> >> Use this constraint. >> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> >> --- >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> index c034ac027d1c..75d839da446c 100644 >> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", >> BIT(28), /* lock */ >> CLK_SET_RATE_UNGATE); >> >> -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", >> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, >> + "pll-video0", >> "osc24M", 0x010, >> 192000000, /* Minimum rate */ >> 1008000000, /* Maximum rate */ >> @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid >> 297000000, /* frac rate 1 */ >> BIT(31), /* gate */ >> BIT(28), /* lock */ >> - CLK_SET_RATE_UNGATE); >> + CLK_SET_RATE_UNGATE, >> + CCU_FEATURE_FRACTIONAL | >> + CCU_FEATURE_CLOSEST_RATE, > > Above flags are unrelated change, put them in new patch if needed. You might notice that I am using a new macro for initializing the pll_video0_clk struct: New: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO Old: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST Setting the two CCU_FEATURE flags is part of the old initialization macro. I'll add SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_NM_RATIO_CLOSEST which hopefully resolves the confusion. Thanks, Frank > > Best regards, > Jernej > >> + 8, 25); /* min/max nm ratio */ >> >> static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", >> "osc24M", 0x018, >> >>
Dne sreda, 20. december 2023 ob 08:09:28 CET je Frank Oltmanns napisal(a): > > On 2023-12-19 at 17:54:19 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): > >> The Allwinner A64 manual lists the following constraint for the > >> PLL-VIDEO0 clock: 8 <= N/M <= 25 > >> > >> Use this constraint. > >> > >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > >> --- > >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> index c034ac027d1c..75d839da446c 100644 > >> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", > >> BIT(28), /* lock */ > >> CLK_SET_RATE_UNGATE); > >> > >> -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", > >> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, > >> + "pll-video0", > >> "osc24M", 0x010, > >> 192000000, /* Minimum rate */ > >> 1008000000, /* Maximum rate */ > >> @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid > >> 297000000, /* frac rate 1 */ > >> BIT(31), /* gate */ > >> BIT(28), /* lock */ > >> - CLK_SET_RATE_UNGATE); > >> + CLK_SET_RATE_UNGATE, > >> + CCU_FEATURE_FRACTIONAL | > >> + CCU_FEATURE_CLOSEST_RATE, > > > > Above flags are unrelated change, put them in new patch if needed. > > You might notice that I am using a new macro for initializing the > pll_video0_clk struct: > New: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO > Old: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST > > Setting the two CCU_FEATURE flags is part of the old initialization > macro. > > I'll add SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_NM_RATIO_CLOSEST which > hopefully resolves the confusion. I'm in doubt if we need so many macros. How many users of these macro we'll have? I see that R40 SoC would also need same ratio limits, but other that that, none? Best regards, Jernej > > Thanks, > Frank > > > > > Best regards, > > Jernej > > > >> + 8, 25); /* min/max nm ratio */ > >> > >> static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", > >> "osc24M", 0x018, > >> > >> >
On 2023-12-20 at 16:12:42 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > Dne sreda, 20. december 2023 ob 08:09:28 CET je Frank Oltmanns napisal(a): >> >> On 2023-12-19 at 17:54:19 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: >> > Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): >> >> The Allwinner A64 manual lists the following constraint for the >> >> PLL-VIDEO0 clock: 8 <= N/M <= 25 >> >> >> >> Use this constraint. >> >> >> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> >> >> --- >> >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> >> index c034ac027d1c..75d839da446c 100644 >> >> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> >> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> >> @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", >> >> BIT(28), /* lock */ >> >> CLK_SET_RATE_UNGATE); >> >> >> >> -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", >> >> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, >> >> + "pll-video0", >> >> "osc24M", 0x010, >> >> 192000000, /* Minimum rate */ >> >> 1008000000, /* Maximum rate */ >> >> @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid >> >> 297000000, /* frac rate 1 */ >> >> BIT(31), /* gate */ >> >> BIT(28), /* lock */ >> >> - CLK_SET_RATE_UNGATE); >> >> + CLK_SET_RATE_UNGATE, >> >> + CCU_FEATURE_FRACTIONAL | >> >> + CCU_FEATURE_CLOSEST_RATE, >> > >> > Above flags are unrelated change, put them in new patch if needed. >> >> You might notice that I am using a new macro for initializing the >> pll_video0_clk struct: >> New: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO >> Old: SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST >> >> Setting the two CCU_FEATURE flags is part of the old initialization >> macro. >> >> I'll add SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_NM_RATIO_CLOSEST which >> hopefully resolves the confusion. > > I'm in doubt if we need so many macros. How many users of these macro we'll have? > I see that R40 SoC would also need same ratio limits, but other that that, none? Ok, IIUC no additional macro and we keep this part of the patch as is. Best regards, Frank > > Best regards, > Jernej > >> >> Thanks, >> Frank >> >> > >> > Best regards, >> > Jernej >> > >> >> + 8, 25); /* min/max nm ratio */ >> >> >> >> static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", >> >> "osc24M", 0x018, >> >> >> >> >>
On 2023-12-19 at 17:54:19 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): >> The Allwinner A64 manual lists the following constraint for the >> PLL-VIDEO0 clock: 8 <= N/M <= 25 >> >> Use this constraint. >> >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> >> --- >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> index c034ac027d1c..75d839da446c 100644 >> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c >> @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", >> BIT(28), /* lock */ >> CLK_SET_RATE_UNGATE); >> >> -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", >> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, >> + "pll-video0", >> "osc24M", 0x010, >> 192000000, /* Minimum rate */ >> 1008000000, /* Maximum rate */ I just realized that adding the whole ratio limits for ccu_nm is superfluous as you could just as well express them in for of a minimum and maximum range: Since 8 <= N/M <= 25 and parent_rate = 24 MHz, therefore 192 MHz <= rate <= 600 MHz. These absolute limits are also listed in Allwinner's A64 manual. BUT, here the upper limit was raised to 1008 MHz: 5de39acaf34604bd04834f092479cf4dcc946dd "clk: sunxi-ng: a64: Add max. rate constraint to video PLL" With this upper limit the ratio limitation is effectively: 8 <= N/M <= 42 Icenowy Zheng (added to CC) had the reasonable explanation that this was used in the BSP kernel, so we should probably stick to that and ditch the two PLL-VIDEO0 related patches. What are your thoughts on that? >> @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid >> 297000000, /* frac rate 1 */ >> BIT(31), /* gate */ >> BIT(28), /* lock */ >> - CLK_SET_RATE_UNGATE); >> + CLK_SET_RATE_UNGATE, >> + CCU_FEATURE_FRACTIONAL | >> + CCU_FEATURE_CLOSEST_RATE, > > Above flags are unrelated change, put them in new patch if needed. > > Best regards, > Jernej > >> + 8, 25); /* min/max nm ratio */ >> >> static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", >> "osc24M", 0x018, >> >>
Dne nedelja, 31. december 2023 ob 10:10:40 CET je Frank Oltmanns napisal(a): > > On 2023-12-19 at 17:54:19 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Dne ponedeljek, 18. december 2023 ob 14:35:22 CET je Frank Oltmanns napisal(a): > >> The Allwinner A64 manual lists the following constraint for the > >> PLL-VIDEO0 clock: 8 <= N/M <= 25 > >> > >> Use this constraint. > >> > >> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> > >> --- > >> drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> index c034ac027d1c..75d839da446c 100644 > >> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c > >> @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", > >> BIT(28), /* lock */ > >> CLK_SET_RATE_UNGATE); > >> > >> -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", > >> +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, > >> + "pll-video0", > >> "osc24M", 0x010, > >> 192000000, /* Minimum rate */ > >> 1008000000, /* Maximum rate */ > > I just realized that adding the whole ratio limits for ccu_nm is > superfluous as you could just as well express them in for of a minimum > and maximum range: > Since 8 <= N/M <= 25 and parent_rate = 24 MHz, therefore > 192 MHz <= rate <= 600 MHz. Good point! > > These absolute limits are also listed in Allwinner's A64 manual. > > BUT, here the upper limit was raised to 1008 MHz: > 5de39acaf34604bd04834f092479cf4dcc946dd "clk: sunxi-ng: a64: Add max. > rate constraint to video PLL" > > With this upper limit the ratio limitation is effectively: > 8 <= N/M <= 42 > > Icenowy Zheng (added to CC) had the reasonable explanation that this was > used in the BSP kernel, so we should probably stick to that and ditch > the two PLL-VIDEO0 related patches. What are your thoughts on that? Ok, it seems that these patches are really superfluous. Remove them for v2. Best regards, Jernej > > >> @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid > >> 297000000, /* frac rate 1 */ > >> BIT(31), /* gate */ > >> BIT(28), /* lock */ > >> - CLK_SET_RATE_UNGATE); > >> + CLK_SET_RATE_UNGATE, > >> + CCU_FEATURE_FRACTIONAL | > >> + CCU_FEATURE_CLOSEST_RATE, > > > > Above flags are unrelated change, put them in new patch if needed. > > > > Best regards, > > Jernej > > > >> + 8, 25); /* min/max nm ratio */ > >> > >> static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", > >> "osc24M", 0x018, > >> > >> >
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c index c034ac027d1c..75d839da446c 100644 --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c @@ -68,7 +68,8 @@ static SUNXI_CCU_NM_WITH_SDM_GATE_LOCK(pll_audio_base_clk, "pll-audio-base", BIT(28), /* lock */ CLK_SET_RATE_UNGATE); -static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-video0", +static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_FEAT_NM_RATIO(pll_video0_clk, + "pll-video0", "osc24M", 0x010, 192000000, /* Minimum rate */ 1008000000, /* Maximum rate */ @@ -80,7 +81,10 @@ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK_MIN_MAX_CLOSEST(pll_video0_clk, "pll-vid 297000000, /* frac rate 1 */ BIT(31), /* gate */ BIT(28), /* lock */ - CLK_SET_RATE_UNGATE); + CLK_SET_RATE_UNGATE, + CCU_FEATURE_FRACTIONAL | + CCU_FEATURE_CLOSEST_RATE, + 8, 25); /* min/max nm ratio */ static SUNXI_CCU_NM_WITH_FRAC_GATE_LOCK(pll_ve_clk, "pll-ve", "osc24M", 0x018,
The Allwinner A64 manual lists the following constraint for the PLL-VIDEO0 clock: 8 <= N/M <= 25 Use this constraint. Signed-off-by: Frank Oltmanns <frank@oltmanns.dev> --- drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)