RE: [PATCH V3 13/14] ARM: dts: stm32: Adjust PLL4 settings on AV96
Hi Marek
> From: Marek Vasut
> 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
> >> 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
> >>>> 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@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).
Best regards
Gérald
RE: [PATCH V3 13/14] ARM: dts: stm32: Adjust PLL4 settings on AV96
Hi Marek and Patrick
> From: Marek Vasut
> 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
> >> 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).
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).
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):
https://wiki.st.com/stm32mpu/wiki/STM32MP15_clock_tree
/* VCO = 594.0 MHz => P = 99, Q = 74, R = 74 */
pll4: st,pll@3 {
cfg = < 3 98 5 7 7 PQR(1,1,1) >;
u-boot,dm-pre-reloc;
};
> >> Reviewed-by: Patrice Chotard
> >> Reviewed-by: Patrick Delaunay
> >> Signed-off-by: Marek Vasut
> >> Cc: Manivannan Sadhasivam
> >> Cc: Patrick Delaunay
> >> Cc: Patrice Chotard
> >> ---
> >> V2: Move this patch before the split of AV96 into SoM and carrier
> >> V3: No change
> >> ---
> >
> > This patch update the PLL4 frequency used on AV96 board, with
> > different of reference clock tree used on ST board, this new setting
> > allow to optimize the SDMMC frequency (50MHz vs 30Mz).
> >
> > I don't know why the previous PLL4 frequency was chosen as a
> > compromise on reference clock-tree (PLL4 is used by mostly all the
> > peripheral, with display and audio requirements).
> >
> > Can you cross check the proposed clock tree and ack this patch if
> > these ST requirements are not applicable on AV96 board.
> >
> > Anyway the code is correct.
>
> Likely because these PLL settings are being copied from reference platform
> to other platforms etc.
As far as I remember, we never had this configuration for PLL4 on ST boards, so
the copy certainly comes from somewhere else.
> But I did notice one odd thing, which is when running the SD1 in SDR104, the
> read data transfers can be unstable, which I suspect is because the bus runs
> at actual 100 MHz instead of some 60 MHz. I need to look at that with a
> scope, so that's to be checked. For now I turned the SDR104 off.
