Re: [PATCH 1/2] arm64: dts: mediatek: mt8192: Make sure MSDCPLL's rate is 400MHz

[Matthias Brugger]

On 15/06/2023 11:51, TingHan Shen (沈廷翰) wrote:
> Hi AngeloGioacchino,
>
> On Mon, 2023-05-22 at 11:30 +0200, AngeloGioacchino Del Regno wrote:
> > External email : Please do not click links or open attachments until you have verified the sender or the content.
> >
> >
> > Some bootloaders will set MSDCPLL's rate lower than 400MHz: what I have
> > seen is this clock being set at around 384MHz.
> > This is a performance concern (and possibly a stability one, for picky
> > eMMC/SD cards) as the MSDC controller's internal divier will choose a
> > frequency that is lower than expected, in the end causing a difference
> > in the expected mmc/sd device's timings.
> >
> > Make sure that the MSDCPLL frequency is always set to 400MHz to both
> > improve performance and reliability of the sd/mmc storage.
> >
> > Fixes: 5d2b897bc6f5 ("arm64: dts: mediatek: Add mt8192 clock controllers")
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   arch/arm64/boot/dts/mediatek/mt8192.dtsi | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index 5c30caf74026..6fc14004f6fd 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -677,6 +677,8 @@ apmixedsys: syscon@1000c000 {
> >                          compatible = "mediatek,mt8192-apmixedsys", "syscon";
> >                          reg = <0 0x1000c000 0 0x1000>;
> >                          #clock-cells = <1>;
> > +                       assigned-clocks = <&apmixedsys CLK_APMIXED_MSDCPLL>;
> > +                       assigned-clock-rates = <400000000>;
> >                  };
> >
> >                  systimer: timer@10017000 {
> > --
> > 2.40.1
>
> Comment from mtk emmc owner,
>
> "As we all know, the clock has some jitter, when we set MSDCPLL to 400M,
> but the actual measurement is not exactly 200M.
> For eMMC, the spec stipulates that clock cannot exceed 200M.
> If MSDCPLL is set to 400M, the actual measurement may exceed the spec.
> So we set MSDCPLL to 384M in the bootloader stage to avoid exceeding the spec."

Thanks for the feedback. Given that I'm not aware of any regressions that got 
fixed by this commits I will drop this series for now. We can keep on the 
discussion and if needed add them in a later stage.

Regards,
Matthias