imx6ul: Recent enet refclock changes breaks custom i.mx6ull board

[Stefan Wahren]

Am 08.03.23 um 16:11 schrieb Stefan Wahren:
> Hi Oleksij,
>
> Am 07.03.23 um 07:06 schrieb Oleksij Rempel:
>> Hi Stefan,
>>
>> On Mon, Mar 06, 2023 at 04:50:18PM +0100, Stefan Wahren wrote:
>>> Did you noticed that the error is caused for enet2_ref_sel?
>>>
>>> On our board variants master/slave/slaveXT only ENET1 is used, so 
>>> ENET2 is
>>> kept to the defaults (ENET2_TX_CLK_DIR = 0, ENET2_CLK_SEL = 0) and the
>>> bootloader won't touch those bits.
>> Ok, i see. It makes sense.
>>
>>>> With this bits we have following variants:
>>>> 1. internal clock source with output on ENET1_TX_CLK
>>>> 2. internal clock source without output on ENET1_TX_CLK. Are there any
>>>>      use cases need to support this mode?
>>> After reading the reference manual, this mode refers to 
>>> ENET1_TX_CLK_DIR =
>>> 0, ENET1_CLK_SEL = 0. Is my understanding correct?
>>>> 3. external clock source without output on ENET1_TX_CLK
>>>> 4. external clock source with output on ENET1_TX_CLK, well 
>>>> ENET1_TX_CLK
>>>>      is input it can't be out put on this case.
>>>>
>>>> Current kernel supports modes 1 and 3. For mode 2 I do not have a use
>>>> case and mode 4 make no sense.
>>>>
>>>> In your case, the boot loader configures clocks to mode 2 which is not
>>>> correct for this HW. It should be mode 1.
>>> As written above the bootloader doesn't touch this. It's the reset 
>>> default
>>> according to the reference manual. So i consider mode 2 as disabled 
>>> clock,
>>> which is the right mode for boards without using this particular 
>>> Ethernet
>>> interface. For EMC reasons we don't want to enable ENET1 and ENET2 
>>> clock
>>> output unconditionally.
>>>> Probably, the way to go is do register dummy parents for not supported
>>>> modes. It would silent the kernel. Other ideas?
>>> Sorry, i don't have no idea how to properly achieve this.
>> can you please test this patch:
>>
>> diff --git a/drivers/clk/imx/clk-imx6ul.c b/drivers/clk/imx/clk-imx6ul.c
>> index 2836adb817b7..e3696a88b5a3 100644
>> --- a/drivers/clk/imx/clk-imx6ul.c
>> +++ b/drivers/clk/imx/clk-imx6ul.c
>> @@ -95,14 +95,16 @@ static const struct clk_div_table 
>> video_div_table[] = {
>>       { }
>>   };
>>   -static const char * enet1_ref_sels[] = { "enet1_ref_125m", 
>> "enet1_ref_pad", };
>> +static const char * enet1_ref_sels[] = { "enet1_ref_125m", 
>> "enet1_ref_pad", "dummy", "dummy"};
>>   static const u32 enet1_ref_sels_table[] = { 
>> IMX6UL_GPR1_ENET1_TX_CLK_DIR,
>> -                        IMX6UL_GPR1_ENET1_CLK_SEL };
>> +                        IMX6UL_GPR1_ENET1_CLK_SEL, 0,
>> +                        IMX6UL_GPR1_ENET1_TX_CLK_DIR | 
>> IMX6UL_GPR1_ENET1_CLK_SEL };
>>   static const u32 enet1_ref_sels_table_mask = 
>> IMX6UL_GPR1_ENET1_TX_CLK_DIR |
>>                            IMX6UL_GPR1_ENET1_CLK_SEL;
>> -static const char * enet2_ref_sels[] = { "enet2_ref_125m", 
>> "enet2_ref_pad", };
>> +static const char * enet2_ref_sels[] = { "enet2_ref_125m", 
>> "enet2_ref_pad", "dummy", "dummy"};
>>   static const u32 enet2_ref_sels_table[] = { 
>> IMX6UL_GPR1_ENET2_TX_CLK_DIR,
>> -                        IMX6UL_GPR1_ENET2_CLK_SEL };
>> +                        IMX6UL_GPR1_ENET2_CLK_SEL, 0,
>> +                        IMX6UL_GPR1_ENET2_TX_CLK_DIR | 
>> IMX6UL_GPR1_ENET2_CLK_SEL };
>>   static const u32 enet2_ref_sels_table_mask = 
>> IMX6UL_GPR1_ENET2_TX_CLK_DIR |
>>                            IMX6UL_GPR1_ENET2_CLK_SEL;
>
> i successful tested this patch on top of Shawn's for-next branch. The 
> error message went away.
>
> Just 2 ideas for a proper patch:
>
> - short explaining comment in clk-imx6ul about the dummies
>
> - instead of "dummy" for both interfaces, i suggest something like 
> "enet1_ref_dummy" which makes investigation at 
> /sys/kernel/debug/clk/clk_summary easier
please ignore the second point, because all the other clocks already 
uses "dummy"
> Thanks
> Stefan
>
>>   Regards,
>> Oleksij