mbox series

[v7,0/5] clk: add driver for the SiFive FU740

Message ID 20201209094916.17383-1-zong.li@sifive.com (mailing list archive)
Headers show
Series clk: add driver for the SiFive FU740 | expand

Message

Zong Li Dec. 9, 2020, 9:49 a.m. UTC
Add a driver for the SiFive FU740 PRCI IP block, which handles more
clocks than FU540. These patches also refactor the original
implementation by spliting the dependent-code of fu540 and fu740
respectively.

We also add a separate patch for DT binding documentation of FU740 PRCI:
https://patchwork.kernel.org/project/linux-riscv/patch/20201126030043.67390-1-zong.li@sifive.com/

Changed in v7:
 - Pick changes in v5 back up into this patch series.

Changed in v6:
 - Modify the patch "Add clock enable and disable ops"
   by Pragnesh. The changes as follows:
   - Remove spin lock in enable and disable functions
   - Call enable_bypass() before PLL output disable
 - Rebase code to Linux v5.10-rc7

Changed in v5:
 - Fix copyright format
 - Add a link of documentation in commit message
 - Modify build dependency for sifive-prci.c
 - Add enable and disable functions by Pragnesh Patel

Changed in v4:
 - Fix the wrong enable bit field shift for FU540 and FU740.

Changed in v3:
 - Fix the wrong enable bit field shift for FU740.

Changed in v2:
 - Remove the macro definition for __prci_clock_array.
 - Indicate the functional changes in commit message.
 - Using option -M and -C to create patches.
 - Rebase code to kernel v5.10-rc3.

Pragnesh Patel (1):
  clk: sifive: Add clock enable and disable ops

Zong Li (4):
  clk: sifive: Extract prci core to common base
  clk: sifive: Use common name for prci configuration
  clk: sifive: Add a driver for the SiFive FU740 PRCI IP block
  clk: sifive: Fix the wrong bit field shift

 arch/riscv/Kconfig.socs                       |   2 +-
 drivers/clk/sifive/Kconfig                    |   8 +-
 drivers/clk/sifive/Makefile                   |   2 +-
 drivers/clk/sifive/fu540-prci.c               | 598 +-----------------
 drivers/clk/sifive/fu540-prci.h               |  21 +
 drivers/clk/sifive/fu740-prci.c               | 120 ++++
 drivers/clk/sifive/fu740-prci.h               |  21 +
 drivers/clk/sifive/sifive-prci.c              | 574 +++++++++++++++++
 drivers/clk/sifive/sifive-prci.h              | 299 +++++++++
 include/dt-bindings/clock/sifive-fu740-prci.h |  23 +
 10 files changed, 1091 insertions(+), 577 deletions(-)
 create mode 100644 drivers/clk/sifive/fu540-prci.h
 create mode 100644 drivers/clk/sifive/fu740-prci.c
 create mode 100644 drivers/clk/sifive/fu740-prci.h
 create mode 100644 drivers/clk/sifive/sifive-prci.c
 create mode 100644 drivers/clk/sifive/sifive-prci.h
 create mode 100644 include/dt-bindings/clock/sifive-fu740-prci.h

Comments

Andreas Schwab March 16, 2021, 7:45 p.m. UTC | #1
On Dez 09 2020, Zong Li wrote:

> Add a driver for the SiFive FU740 PRCI IP block, which handles more
> clocks than FU540. These patches also refactor the original
> implementation by spliting the dependent-code of fu540 and fu740
> respectively.

That breaks ethernet on the fu540.

Andreas.
Zong Li March 18, 2021, 2:07 a.m. UTC | #2
On Wed, Mar 17, 2021 at 3:45 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 09 2020, Zong Li wrote:
>
> > Add a driver for the SiFive FU740 PRCI IP block, which handles more
> > clocks than FU540. These patches also refactor the original
> > implementation by spliting the dependent-code of fu540 and fu740
> > respectively.
>
> That breaks ethernet on the fu540.
>

I would check that, thanks for the report.

> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Zong Li March 19, 2021, 8:20 a.m. UTC | #3
On Thu, Mar 18, 2021 at 10:07 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Wed, Mar 17, 2021 at 3:45 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >
> > On Dez 09 2020, Zong Li wrote:
> >
> > > Add a driver for the SiFive FU740 PRCI IP block, which handles more
> > > clocks than FU540. These patches also refactor the original
> > > implementation by spliting the dependent-code of fu540 and fu740
> > > respectively.
> >
> > That breaks ethernet on the fu540.
> >
>
> I would check that, thanks for the report.
>

Hi Andreas,

Could you please point me out how to test the ethernet from your side?
I had tried to quick test by using iperf and wget, the ethernet seems
to work fine to me.

Thanks.

> > Andreas.
> >
> > --
> > Andreas Schwab, schwab@linux-m68k.org
> > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> > "And now for something completely different."
Yixun Lan March 19, 2021, 8:44 a.m. UTC | #4
HI Zong, Andreas:

On Fri, Mar 19, 2021 at 8:21 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Thu, Mar 18, 2021 at 10:07 AM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Wed, Mar 17, 2021 at 3:45 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > >
> > > On Dez 09 2020, Zong Li wrote:
> > >
> > > > Add a driver for the SiFive FU740 PRCI IP block, which handles more
> > > > clocks than FU540. These patches also refactor the original
> > > > implementation by spliting the dependent-code of fu540 and fu740
> > > > respectively.
> > >
> > > That breaks ethernet on the fu540.
> > >
> >
> > I would check that, thanks for the report.
> >
>
> Hi Andreas,
>
> Could you please point me out how to test the ethernet from your side?
> I had tried to quick test by using iperf and wget, the ethernet seems
> to work fine to me.
>
I will give it a shot during this weekend, since I'm facing the same issue..

Yixun Lan
Andreas Schwab March 19, 2021, 8:49 a.m. UTC | #5
On Mär 19 2021, Zong Li wrote:

> Could you please point me out how to test the ethernet from your side?

Please use
<https://github.com/openSUSE/kernel-source/blob/stable/config/riscv64/default>.

Andreas.
Andreas Schwab March 24, 2021, 10:36 a.m. UTC | #6
Were you able to reproduce the problem?

Andreas.
Zong Li March 25, 2021, 3:21 a.m. UTC | #7
On Wed, Mar 24, 2021 at 6:36 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> Were you able to reproduce the problem?
>

Hi Andreas,

Sorry, I'm not available past few days, I'm just coming back, I would
take a look at this again. Could you also let me know which bootloader
you used (FSBL or U-boot-SPL)? Thanks.

> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Andreas Schwab March 25, 2021, 9:22 a.m. UTC | #8
On Mär 25 2021, Zong Li wrote:

> take a look at this again. Could you also let me know which bootloader
> you used (FSBL or U-boot-SPL)? Thanks.

U-Boot SPL

Please try this image:

http://download.opensuse.org/ports/riscv/tumbleweed/images/openSUSE-Tumbleweed-RISC-V-JeOS-hifiveunleashed.riscv64.raw.xz

Andreas.
Zong Li March 26, 2021, 9:10 a.m. UTC | #9
On Thu, Mar 25, 2021 at 5:22 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 25 2021, Zong Li wrote:
>
> > take a look at this again. Could you also let me know which bootloader
> > you used (FSBL or U-boot-SPL)? Thanks.
>
> U-Boot SPL
>
> Please try this image:
>
> http://download.opensuse.org/ports/riscv/tumbleweed/images/openSUSE-Tumbleweed-RISC-V-JeOS-hifiveunleashed.riscv64.raw.xz
>

Hi Andreas,

The following is the result of the test so far. I would continue to
see what happened there.

1. Boot on openSUSE-Tumbleweed-RISC-V-JeOS-hifiveunleashed.riscv64.raw.xz
w/ plugging ethernet cable
  - It seems that I encountered a different situation with you, my
system hung up and I didn't see the boot message you mentioned yet.

[  OK  ] Finished Generate issue file for login session.
[  OK  ] Finished Apply settings from /etc/sysconfig/keyboard.
[  OK  ] Started User Login Management.
[  *** ] (3 of 3) A start job is running for…upplicant service (58s / 1min 51s)
[**    ] (3 of 3) A start job is running for…cant service (1min 28s / 1min 51s)
[   ***] (2 of 3) A start job is running for…cant service (1min 58s / 3min 21s)
[   ***] (1 of 3) A start job is running for…cant service (2min 28s / 3min 21s)
[**    ] (3 of 3) A start job is running for…cant service (2min 58s / 3min 21s)
[ ***  ] (2 of 3) A start job is running for…cant service (3min 28s / 4min 51s)
[     *] (1 of 3) A start job is running for…cant service (3min 58s / 4min 51s)
[ ***  ] (3 of 3) A start job is running for…cant service (4min 28s / 4min 51s)
[***   ] (2 of 3) A start job is running for…cant service (4min 59s / 6min 22s)
[    **] (1 of 3) A start job is running for…cant service (5min 29s / 6min 22s)
[  *** ] (3 of 3) A start job is running for…cant service (5min 59s / 6min 22s)
[*     ] (2 of 3) A start job is running for…cant service (6min 29s / 7min 52s)
[  *** ] (1 of 3) A start job is running for…cant service (6min 59s / 7min 52s)
[    **] (3 of 3) A start job is running for…cant service (7min 29s / 7min 52s)
[FAILED] Failed to start wicked AutoIPv4 supplicant service.
See 'systemctl status wickedd-auto4.service' for details.
[FAILED] Failed to start wicked DHCPv4 supplicant service.
See 'systemctl status wickedd-dhcp4.service' for details.
[FAILED] Failed to start wicked DHCPv6 supplicant service.
See 'systemctl status wickedd-dhcp6.service' for details.
         Starting wicked network management service daemon...
[    **] A start job is running for wicked n…rvice daemon (7min 59s / 9min 22s)
[***   ] A start job is running for wicked n…rvice daemon (8min 29s / 9min 22s)
[  603.364988] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0
nice=0 stuck for 36s!
[***   ] A start job is running for wicked n…rvice daemon (8min 59s / 9min 22s)
[  633.444986] BUG: workqueue lockup - pool cpus=1 node=0 flags=0x0
nice=0 stuck for 66s!
         Stopping Flush Journal to Persistent Storage...
[  OK  ] Stopped Flush Journal to Persistent Storage.
[  OK  ] Stopped Journal Service.


2. Boot on kernel image which built by opensuse defconfig with
changing CONFIG_MACB to y instead of m
 - Although I got some problem for mounting the root filesystem on
this image now, but I didn't hang up at the message you mentioned, I
could go through after macb driver initialization

[    2.350309] libphy: Fixed MDIO Bus: probed
[    2.354476] macb 10090000.ethernet: Registered clk switch
'sifive-gemgxl-mgmt'
[    2.358752] macb 10090000.ethernet: GEM doesn't support hardware ptp.
[    2.361464] libphy: MACB_mii_bus: probed
[    2.366289] macb 10090000.ethernet eth0: Cadence GEM rev 0x10070109
at 0x10090000 irq 16 (70:b3:d5:92:f2:6c)
[    2.375570] e1000e: Intel(R) PRO/1000 Network Driver
[    2.380323] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[    2.386338] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
...
[    2.687447] Waiting for root device /dev/mmcblk0p4...

3. I check the patch set of supporting fu740, it shouldn't impact
fu540, I'm going to dump and comparing the prci content and give more
testing.


> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Andreas Schwab March 26, 2021, 9:23 a.m. UTC | #10
On Mär 26 2021, Zong Li wrote:

> 1. Boot on openSUSE-Tumbleweed-RISC-V-JeOS-hifiveunleashed.riscv64.raw.xz
> w/ plugging ethernet cable
>   - It seems that I encountered a different situation with you, my
> system hung up and I didn't see the boot message you mentioned yet.

That's exactly the issue.  The process is stuck in D.

Andreas.
Zong Li March 29, 2021, 10:18 a.m. UTC | #11
On Fri, Mar 26, 2021 at 5:24 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 26 2021, Zong Li wrote:
>
> > 1. Boot on openSUSE-Tumbleweed-RISC-V-JeOS-hifiveunleashed.riscv64.raw.xz
> > w/ plugging ethernet cable
> >   - It seems that I encountered a different situation with you, my
> > system hung up and I didn't see the boot message you mentioned yet.
>
> That's exactly the issue.  The process is stuck in D.
>

Yes, I could get the network problem by using the defconfig you
provided, the system hung up when executing 'ifconfig' immediately
after installing macb driver module, the network can work by only
reverting the commit 732374a0b440d9a79c8412f318a25cd37ba6f4e2. But the
network is fine by using the mainline's defconfig, this is a little
bit weird, I will check that and try to find the difference.

> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Andreas Schwab March 29, 2021, 10:37 a.m. UTC | #12
On Mär 29 2021, Zong Li wrote:

> Yes, I could get the network problem by using the defconfig you
> provided, the system hung up when executing 'ifconfig' immediately
> after installing macb driver module, the network can work by only
> reverting the commit 732374a0b440d9a79c8412f318a25cd37ba6f4e2. But the
> network is fine by using the mainline's defconfig, this is a little
> bit weird, I will check that and try to find the difference.

My guess would be that it is an init dependency problem between the phy
driver and the clock driver, which causes the clock to be enabled too
late.

Andreas.
Zong Li March 31, 2021, 8:11 a.m. UTC | #13
On Mon, Mar 29, 2021 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 29 2021, Zong Li wrote:
>
> > Yes, I could get the network problem by using the defconfig you
> > provided, the system hung up when executing 'ifconfig' immediately
> > after installing macb driver module, the network can work by only
> > reverting the commit 732374a0b440d9a79c8412f318a25cd37ba6f4e2. But the
> > network is fine by using the mainline's defconfig, this is a little
> > bit weird, I will check that and try to find the difference.
>
> My guess would be that it is an init dependency problem between the phy
> driver and the clock driver, which causes the clock to be enabled too
> late.
>

I found that the gemgxlpll was disabled immediately by power
management after macb driver install. The mainline's defconfig doesn't
enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
enables CONFIG_PM, and the patch
732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
callback functions, so the gemgxlpll PLL, I have no idea why power
management disable it, I would keep trace it.

By the way, I tried to disable CONFIG_PM on oenpsuse defconfig, the
system didn't hang anymore, on the contrary, I enable CONFIG_PM on
mainline's defconfig, I expect that the system would hang up as well,
unfortunately, I cannot boot successfully by just enabling CONFIG_PM
easily.


> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Andreas Schwab April 12, 2021, 11:31 a.m. UTC | #14
On Mär 31 2021, Zong Li wrote:

> I found that the gemgxlpll was disabled immediately by power
> management after macb driver install. The mainline's defconfig doesn't
> enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> enables CONFIG_PM, and the patch
> 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> callback functions, so the gemgxlpll PLL, I have no idea why power
> management disable it, I would keep trace it.

Does that mean that CONFIG_PM also affects the FU740?

Andreas.
Zong Li April 14, 2021, 2:13 p.m. UTC | #15
On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 31 2021, Zong Li wrote:
>
> > I found that the gemgxlpll was disabled immediately by power
> > management after macb driver install. The mainline's defconfig doesn't
> > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > enables CONFIG_PM, and the patch
> > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > callback functions, so the gemgxlpll PLL, I have no idea why power
> > management disable it, I would keep trace it.
>
> Does that mean that CONFIG_PM also affects the FU740?
>

Yes, we got the same problem on the FU740. We are checking the issue.

> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
Yixun Lan May 11, 2021, 8:53 a.m. UTC | #16
On Wed, Apr 14, 2021 at 2:25 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >
> > On Mär 31 2021, Zong Li wrote:
> >
> > > I found that the gemgxlpll was disabled immediately by power
> > > management after macb driver install. The mainline's defconfig doesn't
> > > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > > enables CONFIG_PM, and the patch
> > > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > > callback functions, so the gemgxlpll PLL, I have no idea why power
> > > management disable it, I would keep trace it.
> >
> > Does that mean that CONFIG_PM also affects the FU740?
> >
>
> Yes, we got the same problem on the FU740. We are checking the issue.
>
Just a mild ping, any progress regarding this issue?

Yxun
Zong Li May 19, 2021, 3:53 p.m. UTC | #17
On Tue, May 11, 2021 at 4:57 PM Yixun Lan <yixun.lan@gmail.com> wrote:
>
> On Wed, Apr 14, 2021 at 2:25 PM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > >
> > > On Mär 31 2021, Zong Li wrote:
> > >
> > > > I found that the gemgxlpll was disabled immediately by power
> > > > management after macb driver install. The mainline's defconfig doesn't
> > > > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > > > enables CONFIG_PM, and the patch
> > > > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > > > callback functions, so the gemgxlpll PLL, I have no idea why power
> > > > management disable it, I would keep trace it.
> > >
> > > Does that mean that CONFIG_PM also affects the FU740?
> > >
> >
> > Yes, we got the same problem on the FU740. We are checking the issue.
> >
> Just a mild ping, any progress regarding this issue?

Currently, if runtime power management is enabled, macb driver would
go to sleep at the end of macb_probe, then the gigabit ethernet PLL
would be disabled.  During this period of time, the system would hang
up if we try to access GEMGXL control registers, it means that we
can't access GEMGXL control registers before the gigabit ethernet PLL
is resumed again. There are some cases, for example, if we execute the
'ifconfig' command, it would eventually go to the macb_get_status to
access GEMGXL control registers and cause the system to hang up. Give
more example here, if we execute 'ip link set lo up & ip addr add
127.0.0.1/8 dev lo', it would cause the system to hang up, because
these commands would try to query the interfaces and eventually go to
macb_get_status as well. However, if we can resume the gigabit
ethernet PLL first, such as 'ip link set eth0 up' or 'udhcpc', then
everything goes well. I'm trying to figure out if there are some hooks
that we can check the PLL status in the macb driver before it actually
touches the control registers. If anyone has an idea about that,
please feel free to point it out to me, thanks.


>
> Yxun
Geert Uytterhoeven May 19, 2021, 6:17 p.m. UTC | #18
Hi Zong,

On Wed, May 19, 2021 at 5:55 PM Zong Li <zong.li@sifive.com> wrote:
> On Tue, May 11, 2021 at 4:57 PM Yixun Lan <yixun.lan@gmail.com> wrote:
> > On Wed, Apr 14, 2021 at 2:25 PM Zong Li <zong.li@sifive.com> wrote:
> > > On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > > On Mär 31 2021, Zong Li wrote:
> > > > > I found that the gemgxlpll was disabled immediately by power
> > > > > management after macb driver install. The mainline's defconfig doesn't
> > > > > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > > > > enables CONFIG_PM, and the patch
> > > > > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > > > > callback functions, so the gemgxlpll PLL, I have no idea why power
> > > > > management disable it, I would keep trace it.
> > > >
> > > > Does that mean that CONFIG_PM also affects the FU740?
> > >
> > > Yes, we got the same problem on the FU740. We are checking the issue.
> > >
> > Just a mild ping, any progress regarding this issue?
>
> Currently, if runtime power management is enabled, macb driver would
> go to sleep at the end of macb_probe, then the gigabit ethernet PLL
> would be disabled.  During this period of time, the system would hang
> up if we try to access GEMGXL control registers, it means that we
> can't access GEMGXL control registers before the gigabit ethernet PLL
> is resumed again. There are some cases, for example, if we execute the

Sounds familiar.

> 'ifconfig' command, it would eventually go to the macb_get_status to

Do you mean mac_get_stats()? macb_get_status() does not exist.

> access GEMGXL control registers and cause the system to hang up. Give
> more example here, if we execute 'ip link set lo up & ip addr add
> 127.0.0.1/8 dev lo', it would cause the system to hang up, because
> these commands would try to query the interfaces and eventually go to
> macb_get_status as well. However, if we can resume the gigabit
> ethernet PLL first, such as 'ip link set eth0 up' or 'udhcpc', then
> everything goes well. I'm trying to figure out if there are some hooks
> that we can check the PLL status in the macb driver before it actually
> touches the control registers. If anyone has an idea about that,
> please feel free to point it out to me, thanks.

And you cannot call pm_runtime_get_sync(), as this is called from
atomic contect. Other drivers avoid accessing the registers while
the device is not up, cfr. e.g. commit 7fa2955ff70ce453 ("sh_eth:
Fix sleeping function called from invalid context").

Gr{oetje,eeting}s,

                        Geert
Zong Li May 21, 2021, 10:34 a.m. UTC | #19
On Thu, May 20, 2021 at 2:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Zong,
>
> On Wed, May 19, 2021 at 5:55 PM Zong Li <zong.li@sifive.com> wrote:
> > On Tue, May 11, 2021 at 4:57 PM Yixun Lan <yixun.lan@gmail.com> wrote:
> > > On Wed, Apr 14, 2021 at 2:25 PM Zong Li <zong.li@sifive.com> wrote:
> > > > On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > > > On Mär 31 2021, Zong Li wrote:
> > > > > > I found that the gemgxlpll was disabled immediately by power
> > > > > > management after macb driver install. The mainline's defconfig doesn't
> > > > > > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > > > > > enables CONFIG_PM, and the patch
> > > > > > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > > > > > callback functions, so the gemgxlpll PLL, I have no idea why power
> > > > > > management disable it, I would keep trace it.
> > > > >
> > > > > Does that mean that CONFIG_PM also affects the FU740?
> > > >
> > > > Yes, we got the same problem on the FU740. We are checking the issue.
> > > >
> > > Just a mild ping, any progress regarding this issue?
> >
> > Currently, if runtime power management is enabled, macb driver would
> > go to sleep at the end of macb_probe, then the gigabit ethernet PLL
> > would be disabled.  During this period of time, the system would hang
> > up if we try to access GEMGXL control registers, it means that we
> > can't access GEMGXL control registers before the gigabit ethernet PLL
> > is resumed again. There are some cases, for example, if we execute the
>
> Sounds familiar.
>
> > 'ifconfig' command, it would eventually go to the macb_get_status to
>
> Do you mean mac_get_stats()? macb_get_status() does not exist.

Sorry for the typo, it should be macb_get_stats.

>
> > access GEMGXL control registers and cause the system to hang up. Give
> > more example here, if we execute 'ip link set lo up & ip addr add
> > 127.0.0.1/8 dev lo', it would cause the system to hang up, because
> > these commands would try to query the interfaces and eventually go to
> > macb_get_status as well. However, if we can resume the gigabit
> > ethernet PLL first, such as 'ip link set eth0 up' or 'udhcpc', then
> > everything goes well. I'm trying to figure out if there are some hooks
> > that we can check the PLL status in the macb driver before it actually
> > touches the control registers. If anyone has an idea about that,
> > please feel free to point it out to me, thanks.
>
> And you cannot call pm_runtime_get_sync(), as this is called from
> atomic contect. Other drivers avoid accessing the registers while
> the device is not up, cfr. e.g. commit 7fa2955ff70ce453 ("sh_eth:
> Fix sleeping function called from invalid context").
>

Thanks for your help. I have done the similar modification by
following the patch you provided. I also verified the bug that we use
pm_runtime_get_sync there. I'm going to post the fix patch by adding
the is_opened flag.

Thanks again.

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Zong Li May 21, 2021, 12:52 p.m. UTC | #20
On Fri, May 21, 2021 at 6:34 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Thu, May 20, 2021 at 2:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Zong,
> >
> > On Wed, May 19, 2021 at 5:55 PM Zong Li <zong.li@sifive.com> wrote:
> > > On Tue, May 11, 2021 at 4:57 PM Yixun Lan <yixun.lan@gmail.com> wrote:
> > > > On Wed, Apr 14, 2021 at 2:25 PM Zong Li <zong.li@sifive.com> wrote:
> > > > > On Mon, Apr 12, 2021 at 7:31 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > > > > > On Mär 31 2021, Zong Li wrote:
> > > > > > > I found that the gemgxlpll was disabled immediately by power
> > > > > > > management after macb driver install. The mainline's defconfig doesn't
> > > > > > > enable CONFIG_PM, so the network is fine on it. The opensuse defconfig
> > > > > > > enables CONFIG_PM, and the patch
> > > > > > > 732374a0b440d9a79c8412f318a25cd37ba6f4e2 added the enable/disable
> > > > > > > callback functions, so the gemgxlpll PLL, I have no idea why power
> > > > > > > management disable it, I would keep trace it.
> > > > > >
> > > > > > Does that mean that CONFIG_PM also affects the FU740?
> > > > >
> > > > > Yes, we got the same problem on the FU740. We are checking the issue.
> > > > >
> > > > Just a mild ping, any progress regarding this issue?
> > >
> > > Currently, if runtime power management is enabled, macb driver would
> > > go to sleep at the end of macb_probe, then the gigabit ethernet PLL
> > > would be disabled.  During this period of time, the system would hang
> > > up if we try to access GEMGXL control registers, it means that we
> > > can't access GEMGXL control registers before the gigabit ethernet PLL
> > > is resumed again. There are some cases, for example, if we execute the
> >
> > Sounds familiar.
> >
> > > 'ifconfig' command, it would eventually go to the macb_get_status to
> >
> > Do you mean mac_get_stats()? macb_get_status() does not exist.
>
> Sorry for the typo, it should be macb_get_stats.
>
> >
> > > access GEMGXL control registers and cause the system to hang up. Give
> > > more example here, if we execute 'ip link set lo up & ip addr add
> > > 127.0.0.1/8 dev lo', it would cause the system to hang up, because
> > > these commands would try to query the interfaces and eventually go to
> > > macb_get_status as well. However, if we can resume the gigabit
> > > ethernet PLL first, such as 'ip link set eth0 up' or 'udhcpc', then
> > > everything goes well. I'm trying to figure out if there are some hooks
> > > that we can check the PLL status in the macb driver before it actually
> > > touches the control registers. If anyone has an idea about that,
> > > please feel free to point it out to me, thanks.
> >
> > And you cannot call pm_runtime_get_sync(), as this is called from
> > atomic contect. Other drivers avoid accessing the registers while
> > the device is not up, cfr. e.g. commit 7fa2955ff70ce453 ("sh_eth:
> > Fix sleeping function called from invalid context").
> >
>
> Thanks for your help. I have done the similar modification by
> following the patch you provided. I also verified the bug that we use
> pm_runtime_get_sync there. I'm going to post the fix patch by adding
> the is_opened flag.
>
> Thanks again.

I have posted a patch to fix this problem. Many thanks to all in this thread.

https://lore.kernel.org/netdev/20210521124859.101012-1-zong.li@sifive.com/T/#u

>
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds