[PATCH 0/2] MT8192/95: Set correct MSDCPLL rate

[PATCH 0/2] MT8192/95: Set correct MSDCPLL rate

[AngeloGioacchino Del Regno]

This series improves both stability/reliability and performance for
eMMC and SD cards on MT8192 and MT8195, where the PLL may be set at
a sub-optimal rate from the bootloader.

This was tested on MT8192 Asurada Spherion and MT8195 Cherry Tomato
Chromebooks.

AngeloGioacchino Del Regno (2):
  arm64: dts: mediatek: mt8192: Make sure MSDCPLL's rate is 400MHz
  arm64: dts: mediatek: mt8195: Make sure MSDCPLL's rate is 400MHz

 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 2 ++
 arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.40.1

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

[AngeloGioacchino Del Regno]

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@collabora.com>
---
 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

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

[TingHan Shen (沈廷翰)]

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@collabora.com>
> ---
>  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."

-- 
Best regards,
TingHan

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@collabora.com>
>> ---
>>   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

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

[AngeloGioacchino Del Regno]

Il 15/06/23 13:16, Matthias Brugger ha scritto:
> 
> 
> 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@collabora.com>
>>> ---
>>>   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 comment,
I haven't seen any issue with this commit, if not a slight performance improvement,
on MT8192 and MT8195, but if there's risk to overclock the card, then it's not ok.

In any case, what is the expected jitter percentage?
eMMC/SD cards do expect jitter by spec anyway.

Thanks,
Angelo


> 
> 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
> 

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

[AngeloGioacchino Del Regno]

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: 37f2582883be ("arm64: dts: Add mediatek SoC mt8195 and evaluation board")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
index a44aae4ab953..daac8e050ce7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
@@ -852,6 +852,8 @@ apmixedsys: syscon@1000c000 {
 			compatible = "mediatek,mt8195-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

Re: [PATCH 0/2] MT8192/95: Set correct MSDCPLL rate

[Matthias Brugger]

On 22/05/2023 11:30, AngeloGioacchino Del Regno wrote:
> This series improves both stability/reliability and performance for
> eMMC and SD cards on MT8192 and MT8195, where the PLL may be set at
> a sub-optimal rate from the bootloader.
> 
> This was tested on MT8192 Asurada Spherion and MT8195 Cherry Tomato
> Chromebooks.
> 
> AngeloGioacchino Del Regno (2):
>    arm64: dts: mediatek: mt8192: Make sure MSDCPLL's rate is 400MHz
>    arm64: dts: mediatek: mt8195: Make sure MSDCPLL's rate is 400MHz
> 
>   arch/arm64/boot/dts/mediatek/mt8192.dtsi | 2 ++
>   arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 ++
>   2 files changed, 4 insertions(+)
> 

Whole series applied, thanks a lot!
Matthias