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

[Stefan Wahren <stefan.wahren@i2se.com>]

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

Thanks
Stefan

>   
> Regards,
> Oleksij