[PATCH] mmc: meson_gx_mmc: change a clock phase to stable value

[Neil Armstrong]

On 10/11/2020 09:36, Jaehoon Chung wrote:
> On 11/10/20 5:05 PM, Neil Armstrong wrote:
>> On 09/11/2020 23:19, Jaehoon Chung wrote:
>>> On 11/9/20 11:23 PM, Neil Armstrong wrote:
>>>> On 09/11/2020 15:10, Mark Kettenis wrote:
>>>>>> From: Neil Armstrong <narmstrong at baylibre.com>
>>>>>> Date: Mon, 9 Nov 2020 14:37:09 +0100
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 09/11/2020 04:12, Jaehoon Chung wrote:
>>>>>>> Core clock phase value is changed from 180' to 270'.
>>>>>>> It's more stable than before.
>>>>>>> - Odroidn-N2/C4 : Working fine with 52MHz
>>>>>>> - VIM3 : Working fine with 52MHz
>>>>>>>
>>>>>>> Before this patch, Odroid-C4 doesn't work fine with 52MHz.
>>>>>>>
>>>>>>> Signed-off-by: Jaehoon Chung <jh80.chung at samsung.com>
>>>>>>> ---
>>>>>>>  drivers/mmc/meson_gx_mmc.c | 14 ++++++++++----
>>>>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c
>>>>>>> index 719dd1e5e570..7c60e0566560 100644
>>>>>>> --- a/drivers/mmc/meson_gx_mmc.c
>>>>>>> +++ b/drivers/mmc/meson_gx_mmc.c
>>>>>>> @@ -52,10 +52,16 @@ static void meson_mmc_config_clock(struct mmc *mmc)
>>>>>>>  	}
>>>>>>>  	clk_div = DIV_ROUND_UP(clk, mmc->clock);
>>>>>>>  
>>>>>>> -	/* 180 phase core clock */
>>>>>>> -	meson_mmc_clk |= CLK_CO_PHASE_180;
>>>>>>> -
>>>>>>> -	/* 180 phase tx clock */
>>>>>>> +	/*
>>>>>>> +	 * Clock Phase needs to set a proper value.
>>>>>>> +	 * It can be changed to other value.
>>>>>>> +	 * Because CORE : 270' Phase and TX : 0' Phase are stable,
>>>>>>> +	 * set to them by default.
>>>>>>> +	 */
>>>>>>> +	/* Core Clock Phase */
>>>>>>> +	meson_mmc_clk |= CLK_CO_PHASE_270;
>>>>>>> +
>>>>>>> +	/* TX Clock Phase */
>>>>>>>  	meson_mmc_clk |= CLK_TX_PHASE_000;
>>>>>>>  
>>>>>>>  	/* clock settings */
>>>>>>>
>>>>>>
>>>>>> The previous values were aligned on the Linux driver, which is functional.
>>>>>
>>>>> Actually the Linux driver isn't really functional; the 52 MHz
>>>>> high-speed mode doesn't work.  But since HS200 does work in Linux,
>>>>> probably nobody noticed this.
>>>>>
>>>>> That said, I'm not confident a single clock phase setting will work
>>>>> across all Amlogic SoCs and across different boards.  Maybe we need
>>>>> something in the device tree such that we can control the values on a
>>>>> per-board level.
>>>>>
>>>>
>>>> So this is specific to SM1 SoCs then, because others families doesn't have such issues
>>>> at 52MHz.
>>>
>>> I don't have much knowledge of SM1 SoCs. But how did its phase value select them on Linux driver?
>>>
>>>>
>>>> So the Linux must be fixes, including the bindings to introduce a new compatible, then
>>>> ported to U-Boot.
>>>>
>>>> So in the meantime, it's right to limit to 26MHz on SM1 in U-boot until we have all this
>>>> clarified.
>>> If you want to limit to 26MHz, I don't have any objection about your opinion.
>>> But I wonder how to clarify all. And I also wonder that values what is used on Linux kernel are really right.
>>
>> OK, I'm not a MMC & SDCard expert on Amlogic, all the work was done in Linux.
>>
>> I just want to keep a working and stable SDCard & eMMC support for the 7 SoCs Families.
>>
>> Could you rewrite my "[PATCH 2/3] mmc: meson-gx: limit max frequency on SM1 SoCs" by instead changing the
>> clock phase only for SM1 ?
>> I'll then apply it with the 2 other patches and push then for the current development cycle.
> 
> Ok. I will rewrite with you patch. Is it ok that i send the patch on Tomorrow?

Agreed

> 
>>
>> The driver is functional for all the other SoCs, and I want to keep it stable unless extensively tested.
> 
> Agreed. If i can test all Amlogic SoCs, I want to do that..But it's impossible to me.
> In future, i hope that it will be fixed with generic approach.

Sure, thanks for your work & testing !

Neil

> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> Neil
>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>> Neil
>>>>
>>>
>>
>>
>