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

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

[Stefan Wahren]

Hi,

we planned to submit our custom i.MX6ULL board [1] to mainline after 
release of Linux 6.3-rc1, but the recent enet refclock changes breaks 
our Ethernet phy:

[    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)

...

[   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: 
phy_poll_reset failed: -110
[   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy

I narrow down the PHY issue to this first bad commit:

5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")

The clock issues seems to be cause by the following commit. If i revert 
5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.

Maybe this helps for further analyse:

--- clk_summary.good    2023-03-05 22:41:52.607392458 +0100
+++ clk_summary.bad    2023-03-05 22:41:52.587391697 +0100
@@ -46,12 +46,13 @@
               usbphy2                  0        0        0 
480000000          0     0  50000         N
      pll6                              1        1        0 
500000000          0     0  50000         Y
         pll6_bypass                    1        1        0 
500000000          0     0  50000         Y
-          pll6_enet                   2        2        0 
500000000          0     0  50000         Y
-             enet_ptp_ref             1        1        0 
25000000          0     0  50000         Y
-                enet_ptp              1        1        0 
25000000          0     0  50000         Y
+          pll6_enet                   1        1        0 
500000000          0     0  50000         Y
+             enet_ptp_ref             0        0        0 
25000000          0     0  50000         Y
+                enet_ptp              0        0        0 
25000000          0     0  50000         N
               enet2_ref                0        0        0 
50000000          0     0  50000         Y
-                enet_ref_125m         0        0        0 
50000000          0     0  50000         N
-             enet_ref                 3        3        0 
50000000          0     0  50000         Y
+                enet2_ref_125m        0        0        0 
50000000          0     0  50000         N
+             enet1_ref                1        1        0 
50000000          0     0  50000         Y
+                enet1_ref_125m        0        0        0 
50000000          0     0  50000         N
      pll5                              0        0        0 
296600000          0     0  50000         Y
         pll5_bypass                    0        0        0 
296600000          0     0  50000         Y
            pll5_video                  0        0        0 
296600000          0     0  50000         N
@@ -137,19 +138,19 @@
                  step                  0        0        0 
528000000          0     0  50000         Y
               periph_pre               1        1        0 
528000000          0     0  50000         Y
                  periph                3        3        0 
528000000          0     0  50000         Y
-                   ahb                9        9        0 
132000000          0     0  50000         Y
+                   ahb                8        8        0 
132000000          0     0  50000         Y
                        sdma            4        1        0 
132000000          0     0  50000         Y
                        rom             1        1        0 
132000000          0     0  50000         Y
                        esai_mem        0        0        0 
132000000          0     0  50000         N
                        esai_ipg        0        0        0 
132000000          0     0  50000         N
                        aips_tz3        1        1        0 
132000000          0     0  50000         Y
-                      enet_ahb        1        1        0 
132000000          0     0  50000         Y
+                      enet_ahb        0        0        0 
132000000          0     0  50000         N
                        dcp             1        1        0 
132000000          0     0  50000         Y
                        asrc_mem        0        0        0 
132000000          0     0  50000         N
                        asrc_ipg        0        0        0 
132000000          0     0  50000         N
                        aips_tz2        1        1        0 
132000000          0     0  50000         Y
                        aips_tz1        1        1        0 
132000000          0     0  50000         Y
-                      ipg            15       13        0 
66000000          0     0  50000         Y
+                      ipg            14       12        0 
66000000          0     0  50000         Y
                           wdog3        0        0        0 
66000000          0     0  50000         N
                           uart8_ipg       0        0        0 
66000000          0     0  50000         N
                           usboh3       0        0        0 
66000000          0     0  50000         N
@@ -180,7 +181,7 @@
                           uart2_ipg       0        0        0 
66000000          0     0  50000         N
                           can2_ipg       0        0        0 
66000000          0     0  50000         N
                           can1_ipg       0        0        0 
66000000          0     0  50000         N
-                         enet         1        1        0 
66000000          0     0  50000         Y
+                         enet         0        0        0 
66000000          0     0  50000         N
                     axi_sel            1        1        0 
528000000          0     0  50000         Y
                        axi_podf        2        2        0 
264000000          0     0  50000         Y
                           axi          1        1        0 
264000000          0     0  50000         Y

[1] - 
https://github.com/chargebyte/linux/commit/5f47f1b029d34391a4b24d4a30105f517126363e#diff-a3def31334aa0f3b641ed5b21a60b462a9cbdda064e7276770be22d9860f2ae7R137

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

[Oleksij Rempel]

Hi Stefan,

On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
> Hi,
> 
> we planned to submit our custom i.MX6ULL board [1] to mainline after release
> of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
> phy:
> 
> [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
> 
> ...
> 
> [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
> failed: -110
> [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
> 
> I narrow down the PHY issue to this first bad commit:
> 
> 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
> 
> The clock issues seems to be cause by the following commit. If i revert
> 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.

It looks like in your kernel version are some missing patches. Can you please
rebase your patches on top of this branch:
https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next

and please rename IMX6UL_CLK_ENET_REF to IMX6UL_CLK_ENET1_REF_SEL in
your dtsi.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Stefan Wahren]

Hi Oleksij,

Am 06.03.23 um 06:25 schrieb Oleksij Rempel:
> Hi Stefan,
>
> On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
>> Hi,
>>
>> we planned to submit our custom i.MX6ULL board [1] to mainline after release
>> of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
>> phy:
>>
>> [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
>>
>> ...
>>
>> [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
>> failed: -110
>> [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
>>
>> I narrow down the PHY issue to this first bad commit:
>>
>> 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
>>
>> The clock issues seems to be cause by the following commit. If i revert
>> 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.
> It looks like in your kernel version are some missing patches. Can you please
> rebase your patches on top of this branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next

thanks for your fast reply. But i rebased my patches against Linux 
v6.3-rc1 since this was released yesterday and should contain all 
patches from Shawn. I also changed the clockref in my DTSI file:

https://github.com/chargebyte/linux/commits/v6.3-tarragon-v3

Now the PHY issue disappeared and ethernet is working, but the

imx:clk-gpr-mux: failed to get parent (-EINVAL)

is still there.

> and please rename IMX6UL_CLK_ENET_REF to IMX6UL_CLK_ENET1_REF_SEL in
> your dtsi.

Yes, this seems to be the issue in my case.

Does this mean a Linux 6.3 kernel doesn't work with a i.MX6ULL Linux 6.2 
devicetree?
So there is no fallback?

What about these other dtsi in Linux 6.3rc-1?

$ grep IMX6UL_CLK_ENET_REF *
imx6ul-14x14-evk.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
..
imx6ul-kontron-bl-common.dtsi:            clocks = <&clks 
IMX6UL_CLK_ENET_REF>;
imx6ul-kontron-sl-common.dtsi:            clocks = <&clks 
IMX6UL_CLK_ENET_REF>;
imx6ull-dhcom-picoitx.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
imx6ull-dhcom-som.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
imx6ull-jozacp.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
imx6ull-myir-mys-6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
imx6ul-phytec-phycore-som.dtsi:            clocks = <&clks 
IMX6UL_CLK_ENET_REF>;
mba6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;

>
> Regards,
> Oleksij

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

[Oleksij Rempel]

On Mon, Mar 06, 2023 at 10:13:57AM +0100, Stefan Wahren wrote:
> Hi Oleksij,
> 
> Am 06.03.23 um 06:25 schrieb Oleksij Rempel:
> > Hi Stefan,
> > 
> > On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
> > > Hi,
> > > 
> > > we planned to submit our custom i.MX6ULL board [1] to mainline after release
> > > of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
> > > phy:
> > > 
> > > [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
> > > 
> > > ...
> > > 
> > > [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
> > > failed: -110
> > > [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
> > > 
> > > I narrow down the PHY issue to this first bad commit:
> > > 
> > > 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
> > > 
> > > The clock issues seems to be cause by the following commit. If i revert
> > > 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.
> > It looks like in your kernel version are some missing patches. Can you please
> > rebase your patches on top of this branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next
> 
> thanks for your fast reply. But i rebased my patches against Linux v6.3-rc1
> since this was released yesterday and should contain all patches from Shawn.

No, it is not. Related DTS changes are not included in to v6.3-rc1.

> I also changed the clockref in my DTSI file:
> 
> https://github.com/chargebyte/linux/commits/v6.3-tarragon-v3
> 
> Now the PHY issue disappeared and ethernet is working, but the
> 
> imx:clk-gpr-mux: failed to get parent (-EINVAL)

I need to take a look at it. It should not be critical.

> 
> is still there.
> 
> > and please rename IMX6UL_CLK_ENET_REF to IMX6UL_CLK_ENET1_REF_SEL in
> > your dtsi.
> 
> Yes, this seems to be the issue in my case.
> 
> Does this mean a Linux 6.3 kernel doesn't work with a i.MX6ULL Linux 6.2
> devicetree?

If I see it correctly. Since you do not have patch [1] related clock is not
enabled by the fec controller. Since this PHY is not addressable without
running rmii clock, the PHY can't be probed.

> So there is no fallback?

With [1] it should not be needed.

> 
> What about these other dtsi in Linux 6.3rc-1?
> 
> $ grep IMX6UL_CLK_ENET_REF *
> imx6ul-14x14-evk.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> ..
> imx6ul-kontron-bl-common.dtsi:            clocks = <&clks
> IMX6UL_CLK_ENET_REF>;
> imx6ul-kontron-sl-common.dtsi:            clocks = <&clks
> IMX6UL_CLK_ENET_REF>;
> imx6ull-dhcom-picoitx.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> imx6ull-dhcom-som.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> imx6ull-jozacp.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> imx6ull-myir-mys-6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> imx6ul-phytec-phycore-som.dtsi:            clocks = <&clks
> IMX6UL_CLK_ENET_REF>;
> mba6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;

It is nice to convert all of them to proper clock. But all of them are
expected to work with [1].

Can you please confirm it? Revert yourdtsi back to IMX6UL_CLK_ENET_REF
and include [1]?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/commit/?h=for-next&id=8940c105273fcde00a60023f68f8a5b75e1df0cc

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Stefan Wahren]

Hi Oleksij,

Am 06.03.23 um 10:47 schrieb Oleksij Rempel:
> On Mon, Mar 06, 2023 at 10:13:57AM +0100, Stefan Wahren wrote:
>> Hi Oleksij,
>>
>> Am 06.03.23 um 06:25 schrieb Oleksij Rempel:
>>> Hi Stefan,
>>>
>>> On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
>>>> Hi,
>>>>
>>>> we planned to submit our custom i.MX6ULL board [1] to mainline after release
>>>> of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
>>>> phy:
>>>>
>>>> [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
>>>>
>>>> ...
>>>>
>>>> [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
>>>> failed: -110
>>>> [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
>>>>
>>>> I narrow down the PHY issue to this first bad commit:
>>>>
>>>> 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
>>>>
>>>> The clock issues seems to be cause by the following commit. If i revert
>>>> 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.
>>> It looks like in your kernel version are some missing patches. Can you please
>>> rebase your patches on top of this branch:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next
>> thanks for your fast reply. But i rebased my patches against Linux v6.3-rc1
>> since this was released yesterday and should contain all patches from Shawn.
> No, it is not. Related DTS changes are not included in to v6.3-rc1.

Sorry, i didn't noticed that Shawn already rebased his for-next changes 
on top of v6.3-rc1.

So, the problem is that your clk changes has been applied for 6.3, but 
the necessary arm changes will land in 6.4? :-(

>
>> I also changed the clockref in my DTSI file:
>>
>> https://github.com/chargebyte/linux/commits/v6.3-tarragon-v3
>>
>> Now the PHY issue disappeared and ethernet is working, but the
>>
>> imx:clk-gpr-mux: failed to get parent (-EINVAL)
> I need to take a look at it. It should not be critical.

I prepared a patch [1] to improve the debugging here:

[    0.000000] Entry 262144 != val 0
[    0.000000] Entry 16384 != val 0
[    0.000000] imx:clk-gpr-mux: val 0, num_parents 2
[    0.000000] imx:clk-gpr-mux: failed to get parent of enet2_ref_sel 
(-EINVAL)

It seems that val 0 is unexpected for the driver. Maybe it's worth to 
mention that we use an older U-Boot [2]. But Linux should make any 
assumptions here.

>
>> is still there.
>>
>>> and please rename IMX6UL_CLK_ENET_REF to IMX6UL_CLK_ENET1_REF_SEL in
>>> your dtsi.
>> Yes, this seems to be the issue in my case.
>>
>> Does this mean a Linux 6.3 kernel doesn't work with a i.MX6ULL Linux 6.2
>> devicetree?
> If I see it correctly. Since you do not have patch [1] related clock is not
> enabled by the fec controller. Since this PHY is not addressable without
> running rmii clock, the PHY can't be probed.
>
>> So there is no fallback?
> With [1] it should not be needed.
Maybe this patch comes too late (Linux 6.4) for some boards.
>
>> What about these other dtsi in Linux 6.3rc-1?
>>
>> $ grep IMX6UL_CLK_ENET_REF *
>> imx6ul-14x14-evk.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> ..
>> imx6ul-kontron-bl-common.dtsi:            clocks = <&clks
>> IMX6UL_CLK_ENET_REF>;
>> imx6ul-kontron-sl-common.dtsi:            clocks = <&clks
>> IMX6UL_CLK_ENET_REF>;
>> imx6ull-dhcom-picoitx.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> imx6ull-dhcom-som.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> imx6ull-jozacp.dts:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> imx6ull-myir-mys-6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
>> imx6ul-phytec-phycore-som.dtsi:            clocks = <&clks
>> IMX6UL_CLK_ENET_REF>;
>> mba6ulx.dtsi:            clocks = <&clks IMX6UL_CLK_ENET_REF>;
> It is nice to convert all of them to proper clock. But all of them are
> expected to work with [1].
>
> Can you please confirm it? Revert yourdtsi back to IMX6UL_CLK_ENET_REF
> and include [1]?

I rebased all changes on top of Shawn's branch and reverted to 
IMX6UL_CLK_ENET_REF [3]. So yes, i confirm that Ethernet works in this case.

Best regards

[1] - 
https://github.com/chargebyte/linux/commit/74a883d3ca4960a7d178d4a184daf9856600ca14
[2] - https://github.com/chargebyte/U-Boot/tree/v2020.04-in-tech
[3] - https://github.com/chargebyte/linux/tree/v6.4-shawnguo-tarragon

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/commit/?h=for-next&id=8940c105273fcde00a60023f68f8a5b75e1df0cc
>
> Regards,
> Oleksij

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

[Oleksij Rempel]

On Mon, Mar 06, 2023 at 02:33:24PM +0100, Stefan Wahren wrote:
> Hi Oleksij,
> 
> Am 06.03.23 um 10:47 schrieb Oleksij Rempel:
> > On Mon, Mar 06, 2023 at 10:13:57AM +0100, Stefan Wahren wrote:
> > > Hi Oleksij,
> > > 
> > > Am 06.03.23 um 06:25 schrieb Oleksij Rempel:
> > > > Hi Stefan,
> > > > 
> > > > On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
> > > > > Hi,
> > > > > 
> > > > > we planned to submit our custom i.MX6ULL board [1] to mainline after release
> > > > > of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
> > > > > phy:
> > > > > 
> > > > > [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
> > > > > 
> > > > > ...
> > > > > 
> > > > > [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
> > > > > failed: -110
> > > > > [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
> > > > > 
> > > > > I narrow down the PHY issue to this first bad commit:
> > > > > 
> > > > > 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
> > > > > 
> > > > > The clock issues seems to be cause by the following commit. If i revert
> > > > > 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.
> > > > It looks like in your kernel version are some missing patches. Can you please
> > > > rebase your patches on top of this branch:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next
> > > thanks for your fast reply. But i rebased my patches against Linux v6.3-rc1
> > > since this was released yesterday and should contain all patches from Shawn.
> > No, it is not. Related DTS changes are not included in to v6.3-rc1.
> 
> Sorry, i didn't noticed that Shawn already rebased his for-next changes on
> top of v6.3-rc1.
> 
> So, the problem is that your clk changes has been applied for 6.3, but the
> necessary arm changes will land in 6.4? :-(

I hope it will go as fixes to 6.3-rcX. Shawn?

> > > I also changed the clockref in my DTSI file:
> > > 
> > > https://github.com/chargebyte/linux/commits/v6.3-tarragon-v3
> > > 
> > > Now the PHY issue disappeared and ethernet is working, but the
> > > 
> > > imx:clk-gpr-mux: failed to get parent (-EINVAL)
> > I need to take a look at it. It should not be critical.
> 
> I prepared a patch [1] to improve the debugging here:
> 
> [    0.000000] Entry 262144 != val 0
> [    0.000000] Entry 16384 != val 0
> [    0.000000] imx:clk-gpr-mux: val 0, num_parents 2
> [    0.000000] imx:clk-gpr-mux: failed to get parent of enet2_ref_sel
> (-EINVAL)
> 
> It seems that val 0 is unexpected for the driver. Maybe it's worth to
> mention that we use an older U-Boot [2]. But Linux should make any
> assumptions here.

There are two configuration bits per Ethernet interface:
- BIT(17) ENET1_TX_CLK_DIR
- BIT(13) ENET1_CLK_SEL

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

Probably, the way to go is do register dummy parents for not supported
modes. It would silent the kernel. Other ideas?

> > Can you please confirm it? Revert yourdtsi back to IMX6UL_CLK_ENET_REF
> > and include [1]?
> 
> I rebased all changes on top of Shawn's branch and reverted to
> IMX6UL_CLK_ENET_REF [3]. So yes, i confirm that Ethernet works in this case.

Thx! So, there should be no regressions if this patch will to as fix for
6.3-rcX. Except of kernel warning wrong parent configuration.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Stefan Wahren]

Hi Oleksij,

Am 06.03.23 um 15:02 schrieb Oleksij Rempel:
> On Mon, Mar 06, 2023 at 02:33:24PM +0100, Stefan Wahren wrote:
>> Hi Oleksij,
>>
>> Am 06.03.23 um 10:47 schrieb Oleksij Rempel:
>>> On Mon, Mar 06, 2023 at 10:13:57AM +0100, Stefan Wahren wrote:
>>>> Hi Oleksij,
>>>>
>>>> Am 06.03.23 um 06:25 schrieb Oleksij Rempel:
>>>>> Hi Stefan,
>>>>>
>>>>> On Sun, Mar 05, 2023 at 11:16:17PM +0100, Stefan Wahren wrote:
>>>>>> Hi,
>>>>>>
>>>>>> we planned to submit our custom i.MX6ULL board [1] to mainline after release
>>>>>> of Linux 6.3-rc1, but the recent enet refclock changes breaks our Ethernet
>>>>>> phy:
>>>>>>
>>>>>> [    0.000000] imx:clk-gpr-mux: failed to get parent (-EINVAL)
>>>>>>
>>>>>> ...
>>>>>>
>>>>>> [   18.574595] SMSC LAN8710/LAN8720 2188000.ethernet-1:00: phy_poll_reset
>>>>>> failed: -110
>>>>>> [   18.581064] fec 2188000.ethernet eth0: Unable to connect to phy
>>>>>>
>>>>>> I narrow down the PHY issue to this first bad commit:
>>>>>>
>>>>>> 5f82bfced611 ("clk: imx6ul: fix enet1 gate configuration")
>>>>>>
>>>>>> The clock issues seems to be cause by the following commit. If i revert
>>>>>> 5f82bfced611 and 4e197ee880c24 or use Linux 6.2 everything is fine.
>>>>> It looks like in your kernel version are some missing patches. Can you please
>>>>> rebase your patches on top of this branch:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next
>>>> thanks for your fast reply. But i rebased my patches against Linux v6.3-rc1
>>>> since this was released yesterday and should contain all patches from Shawn.
>>> No, it is not. Related DTS changes are not included in to v6.3-rc1.
>> Sorry, i didn't noticed that Shawn already rebased his for-next changes on
>> top of v6.3-rc1.
>>
>> So, the problem is that your clk changes has been applied for 6.3, but the
>> necessary arm changes will land in 6.4? :-(
> I hope it will go as fixes to 6.3-rcX. Shawn?
>
>>>> I also changed the clockref in my DTSI file:
>>>>
>>>> https://github.com/chargebyte/linux/commits/v6.3-tarragon-v3
>>>>
>>>> Now the PHY issue disappeared and ethernet is working, but the
>>>>
>>>> imx:clk-gpr-mux: failed to get parent (-EINVAL)
>>> I need to take a look at it. It should not be critical.
>> I prepared a patch [1] to improve the debugging here:
>>
>> [    0.000000] Entry 262144 != val 0
>> [    0.000000] Entry 16384 != val 0
>> [    0.000000] imx:clk-gpr-mux: val 0, num_parents 2
>> [    0.000000] imx:clk-gpr-mux: failed to get parent of enet2_ref_sel
>> (-EINVAL)
>>
>> It seems that val 0 is unexpected for the driver. Maybe it's worth to
>> mention that we use an older U-Boot [2]. But Linux should make any
>> assumptions here.
> There are two configuration bits per Ethernet interface:
> - BIT(17) ENET1_TX_CLK_DIR
> - BIT(13) ENET1_CLK_SEL

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.

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

Best regards

>
>>> Can you please confirm it? Revert yourdtsi back to IMX6UL_CLK_ENET_REF
>>> and include [1]?
>> I rebased all changes on top of Shawn's branch and reverted to
>> IMX6UL_CLK_ENET_REF [3]. So yes, i confirm that Ethernet works in this case.
> Thx! So, there should be no regressions if this patch will to as fix for
> 6.3-rcX. Except of kernel warning wrong parent configuration.
>
> Regards,
> Oleksij

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

[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;
 
Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[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

Thanks
Stefan

>   
> Regards,
> Oleksij

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