[PATCH 18/18] [HACK, RFC] clk: rk3568: do not divide dclk_vop0

[Sascha Hauer]

On Wed, Dec 08, 2021 at 05:51:43PM +0100, Heiko Stübner wrote:
> Hi Sascha,
> 
> Am Mittwoch, 8. Dezember 2021, 16:12:30 CET schrieb Sascha Hauer:
> > On the rk3568 we have this (simplified) situation:
> > 
> >  .--------.     .-----.    .---------.
> > -| hpll   |--.--| /n  |----|dclk_vop0|-
> >  `--------´  |  `-----´    `---------´
> >              |  .-----.    .---------.
> >              `--| /m  |----|dclk_vop1|-
> >              |  `-----´    `---------´
> >              |             .---------.
> >              `-------------|hdmi_ref |-
> >                            `---------´
> > 
> > hpll is the PLL that drives the HDMI reference clock and the pixel
> > clocks for the different CRTCs (dclk_vop0/1). Between the pixel clocks
> > and the hpll there are programmable dividers whereas the HDMI reference
> > clock is directly connected to the hpll.
> > 
> > For the HDMI output to work the pixel clock must be the same as the HDMI
> > reference clock, hence the dividers must be programmed to 1. Normally a
> > rate change on dclk_vop0/1 propagates through to the hpll and the clock
> > framework picks a suitable combination of hpll and divider settings. by
> > accident it picks a divider setting of 1 for the standard 1080p case,
> > but other divider settings for most other resolutions leaving the HDMI
> > port non working.
> > 
> > This patch is not a solution, it merely puts the finger in the wound. We
> > leave out the divider for the composite clock for dclk_vop0 which then
> > leaves the divider at the bootloader default setting of 1. I assume
> > the divider is disturbing only for the HDMI case, but needed for other
> > outputs. Any thoughts how this can be handled?
> 
> I'm not even sure if/how the common clock framework keeps track of
> diverging wishes to parent-rates :-) .

I don't think the common clock framework tries to keep track of that.

> 
> But I do see two direct issues in the _existing_ code.
> 
> dclk_vop0/1 uses CLK_SET_RATE_PARENT so is allowed to change
> the rates of its parent clock(s).
> 
> Its parent clocks are not only hpll but can also be vpll, gpll and cpll.
> So this can cause even more mayhem, if the ccf for example decides
> to select the gpll and then change its rate,which may result in a lot
> of peripherals getting their rates changed under them ;-) .

Right, we can only allow the CLK_SET_RATE_PARENT parent flag on the dclk
clocks when the parent is HPLL. Since we can't be sure that HPLL is the
parent we have to remove the flag.

> 
> On the other hand I see in the clock driver that hdmi-ref is not allowed
> to change its parent rate, so can only select between hpll and hpll_ph0
> (1/2 the rate?).
> 
> So I guess, one way could be:
> - add CLK_SET_RATE_PARENT to the hdmi-ref clock
> - drop CLK_SET_RATE_PARENT from the dclks
> - make sure hdmi-clock is set before the dclk

That solves it for the HDMI case. I can imagine that for a LVDS user the
CLK_SET_RATE_PARENT flag on the dclks is quite handy to get a PLL
frequency suitable for the display. Otherwise he would have to set a
suitable PLL frequency using assigned-clock-rates in the device tree.
That's still possible so this might be a good compromise.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |