[PATCH V3 13/14] ARM: dts: stm32: Adjust PLL4 settings on AV96

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex at denx.de>
> Sent: jeudi 2 avril 2020 15:05
> 
> On 4/2/20 9:44 AM, Gerald BAEZA wrote:
> > Hi Marek
> >
> >> From: Marek Vasut <marex at denx.de>
> >> Sent: mercredi 1 avril 2020 20:49
> >>
> >> On 4/1/20 6:49 PM, Gerald BAEZA wrote:
> >>> Hi Marek and Patrick
> >>
> >> Hi,
> >>
> >>>> From: Marek Vasut <marex at denx.de>
> >>>> Sent: mercredi 1 avril 2020 13:09
> >>>>
> >>>> On 4/1/20 12:24 PM, Patrick DELAUNAY wrote:
> >>>>> Hi Gerald and Manivannan,
> >>>>
> >>>> Hi,
> >>>>
> >>>>>> From: Marek Vasut <marex at denx.de>
> >>>>>> Sent: mardi 31 mars 2020 19:52
> >>>>>>
> >>>>>> The PLL4 is supplying SDMMC12, SDMMC3 and SPDIF with 120 MHz
> and
> >>>>>> FDCAN with 96 MHz. This isn't good for the SDMMC interfaces,
> >>>>>> which can not easily divide the clock down to e.g. 50 MHz for
> >>>>>> high speed SD and eMMC devices, so those devices end up running
> >>>>>> at 30 MHz as that is
> >>>> 120 MHz / 4.
> >>>>>> Adjust the PLL4 settings such that both PLL4P and PLL4R run at
> >>>>>> 100 MHz instead, which is easy to divide to 50MHz for optimal
> >>>>>> operation of both SD and eMMC, SPDIF clock are not that much
> >>>>>> slower and
> >> FDCAN
> >>>>>> is
> >>>> also unaffected.
> >>>
> >>> As far as I see, SPDIF is not supported on Avenger board so we don't
> >>> care
> >> for this one.
> >>> FDCAN can be supported via the expansion connector, so better to
> >>> keep it
> >> high (and < 100 MHz).
> >>
> >> Why < 100 MHz and not <= 100 MHz ?
> >
> > You're right, it is <= 100 MHz and it is important to be precise since
> > you were exactly on 100 MHz :)
> >
> >>> Be careful because the LTDC pixel clock also comes from the PLL4Q
> >>> and it is
> >> used for the HDMI on Avenger.
> >>> The pixel clock is very constraint and I am surprised by the initial
> >>> 40 MHz
> >> configuration (that becomes 50 MHz with your patch).
> >>
> >> Is that a problem that the LTDC pixel clock are 50 MHz ? The kernel
> >> will reconfigure them anyway, so the 50 MHz is not the final value.
> >
> > The kernel set_rate() changes the PLL output divisor, so it will indeed be able to
> switch back to (600/15=) 40 MHz from an initial (600/12=) 50 MHz.
> >
> >>> I would recommend to align the Avenger configuration to ST boards
> >>> one,
> >> that is the best compromise found so far (99 MHz for SDMMC and 74.250
> >> MHz for HDMI):
> >>
> >> Why is this better than 100/50/100 ?
> >>
> >>> 	https://wiki.st.com/stm32mpu/wiki/STM32MP15_clock_tree
> >>> 	/* VCO = 594.0 MHz => P = 99, Q = 74, R = 74 */
> >>> 	pll4: st,pll at 3 {
> >>> 		cfg = < 3 98 5 7 7 PQR(1,1,1) >;
> >>> 		u-boot,dm-pre-reloc;
> >>> 	};
> >> [...]
> >
> > The simplest explanation I found is here:
> > https://timetoexplore.net/blog/video-timings-vga-720p-1080p
> > (you can also look for "74.25" in the HDMI specification but there is
> > more text to read)
> >
> > So 74.250 MHz is the needed pixel clock for 720p resolution in HDMI, that is
> quite common, so this frequency is wished for interoperability with a maximum of
> TVs.
> > This frequency cannot be reached from the initial or your proposed PLL4
> configuration, that is why I proposed to change this and align on ST board clock
> tree.
> >
> > For the other displays we are supporting on ST boards, the kernel is tuning
> (down) this 74.250 MHz frequency via the set_rate (that changes the PLL output
> divisor).
> 
> The article you found is focused on FPGA, where the FPGA is connected directly
> to the HDMI TMDS signals without transceiver, so the FPGA has to generate very
> precise clock. I don't think this is our case here.
> 
> The ADV7513 datasheet says nothing about such specific clock rate, it only
> mentions maximum clock per resolution to be 82.5 MHz and that it can do pixel
> repeat to meet the timing requirements.
> 
> (Note to self, PLL4R should run at 99 MHz too, to run FDCAN fast enough.)

Without more requirement on the clock tree, the new PLL4 is acceptable.

Applied to u-boot-stm/master, thanks!

Regards

Patrick