diff mbox

r8168 is needed to enter P-state: Package State 6 (pc6)onHaswell hardware

Message ID 20141006221307.GB10936@electric-eye.fr.zoreil.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Francois Romieu Oct. 6, 2014, 10:13 p.m. UTC 
Hayes Wang <hayeswang@realtek.com> :
>  Francois Romieu [mailto:romieu@fr.zoreil.com] 
[...]
> I don't sure if the following information is helpful. Besides, I remember
> the rtl_init_one() would disable it.
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=d64ec841517a25f6d468bde9f67e5b4cffdc67c7
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=4521e1a94279ce610d3f9b7945c17d581f804242

Yes, I did not expect this stuff to stay in geostationary orbit for long :o/

Realtek's r8168 driver defaults to CONFIG_ASPM=1 but I guess some users
need to disable it and there's no known pattern / blacklist, right ?

Ceriel, does the patch below against current kernel make a difference ?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hayes Wang Oct. 7, 2014, 2:50 a.m. UTC | #1 
Francois Romieu [mailto:romieu@fr.zoreil.com] 
> Sent: Tuesday, October 07, 2014 6:13 AM
[...]
> Realtek's r8168 driver defaults to CONFIG_ASPM=1 but I guess 
> some users
> need to disable it and there's no known pattern / blacklist, right ?

When enabling the ASPM, it would influence the thrpughput. It is hard to
choose performance or power saving. Therefore, we reserve the config
to let the user determines it.

 Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ceriel Jacobs Oct. 7, 2014, 10:40 a.m. UTC | #2 
Francois Romieu schreef op 07-10-14 om 00:13:
> Hayes Wang <hayeswang@realtek.com> :
>>   Francois Romieu [mailto:romieu@fr.zoreil.com]
> [...]
>> I don't sure if the following information is helpful. Besides, I remember
>> the rtl_init_one() would disable it.
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=d64ec841517a25f6d468bde9f67e5b4cffdc67c7
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=4521e1a94279ce610d3f9b7945c17d581f804242
>
> Yes, I did not expect this stuff to stay in geostationary orbit for long :o/
>
> Realtek's r8168 driver defaults to CONFIG_ASPM=1

# modinfo r8168 suggests the opposite (ASPM is disabled by default):
version:        8.039.00-NAPI
parm:           aspm:Enable ASPM. (int)

When ASPM would be enabled by default, one would need a boot parameter like:
parm:		aspm:Disable ASPM. (int)

  but I guess some users
> need to disable it and there's no known pattern / blacklist, right ?

I don't want to disable ASPM. In fact the r8168 module I am even running 
with poot params like:
# cat /proc/cmdline
BOOT_IMAGE=/boot/vmlinuz-3.17.0-999-generic root=/dev/sda1 ro 
biosdevname=0 intel_pstate=enable ipv6.disabled=1 debug ignore_loglevel 
panic=10 pcie_aspm.policy=powersave pcie_aspm=force r8168.aspm=1 
r8168.eee_enable=1 oops=panic

>
> Ceriel, does the patch below against current kernel make a difference ?

Francois, what do you mean with "current kernel", the latest Ubuntu 
mainline kernel or something different?

>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 0921302..b4a3881 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -468,6 +468,7 @@ enum rtl8168_registers {
>   #define PWM_EN				(1 << 22)
>   #define RXDV_GATED_EN			(1 << 19)
>   #define EARLY_TALLY_EN			(1 << 16)
> +#define FORCE_CLK			(1 << 15) /* force clock request */
>   };
>
>   enum rtl_register_content {
> @@ -5279,8 +5280,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
>   	rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
>
>   	RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
> -	RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
> +	RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
>   	RTL_W8(MaxTxPacketSize, EarlySize);
> +	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> +	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
>
>   	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
>   	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Oct. 7, 2014, 8:16 p.m. UTC | #3 
Ceriel Jacobs <linux-ide@crashplan.pro> :
> Francois Romieu schreef op 07-10-14 om 00:13:
[...]
> >Realtek's r8168 driver defaults to CONFIG_ASPM=1
> 
> # modinfo r8168 suggests the opposite (ASPM is disabled by default):
> version:        8.039.00-NAPI
> parm:           aspm:Enable ASPM. (int)

$ grep CONFIG_ASPM src/Makefile 
CONFIG_ASPM = y

$ less src/r8168_n.c
[...]
#ifdef CONFIG_ASPM
static int aspm = 1;
#else
static int aspm = 0;
#endif

[...]
> Francois, what do you mean with "current kernel", the latest Ubuntu mainline
> kernel or something different?

Sorry. I meant current mainline as seen from git but anything close like
v3.15.x, v3.16.y or v3.17.k should be fine (patch applies).
Francois Romieu Oct. 7, 2014, 8:17 p.m. UTC | #4 
Hayes Wang <hayeswang@realtek.com> :
[...]
> When enabling the ASPM, it would influence the thrpughput. It is hard to
> choose performance or power saving. Therefore, we reserve the config
> to let the user determines it.

Mmmm...

How do Realtek's devices behave if Config{2, 3} + MISC registers are
configured for ASPM whereas PCI config space registers aren't ?
Hayes Wang Oct. 8, 2014, 2:35 a.m. UTC | #5 
Francois Romieu [mailto:romieu@fr.zoreil.com] 
> Sent: Wednesday, October 08, 2014 4:17 AM
[...]
> > When enabling the ASPM, it would influence the thrpughput. 
> > It is hard to
> > choose performance or power saving. Therefore, we reserve the config
> > to let the user determines it.
> 
> Mmmm...
> 
> How do Realtek's devices behave if Config{2, 3} + MISC registers are
> configured for ASPM whereas PCI config space registers aren't ?

The feature of the ASPM for the nic would be disabled.
ASPM is enabled when both the internal settings and
the PCI config space registers are enabled.
 
Best Regards,
Hayes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ceriel Jacobs Oct. 8, 2014, 11:40 a.m. UTC | #6 
Hayes Wang schreef op 08-10-14 om 04:35:
>   Francois Romieu [mailto:romieu@fr.zoreil.com]
>> Sent: Wednesday, October 08, 2014 4:17 AM
> [...]
>>> When enabling the ASPM, it would influence the thrpughput.
>>> It is hard to
>>> choose performance or power saving. Therefore, we reserve the config
>>> to let the user determines it.
>>
>> Mmmm...
>>
>> How do Realtek's devices behave if Config{2, 3} + MISC registers are
>> configured for ASPM whereas PCI config space registers aren't ?
>
> The feature of the ASPM for the nic would be disabled.
> ASPM is enabled when both the internal settings and
> the PCI config space registers are enabled.

Have in mind a situation where the PCI config space registers are set 
manually (after user login).

At which point in time will be determined whether ASPM for the nic will 
be disabled or enabled? Once, when loading the kernel module? Or more often?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ceriel Jacobs Oct. 8, 2014, 8:29 p.m. UTC | #7 
Sorry for the delay. As newbie I first needed to learn about depmod to 
restore loading the r8169 module instead of r8168.

Now, I am struggling with applying the patch. Even with fuzz 99 applying 
this patch fails for hunk #2:

~/linux-3.13.0# patch -Np1 -F99 -r - --dry-run <<'END'
 > index 0921302..b4a3881 100644
...
 >  rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
 > END
checking file drivers/net/ethernet/realtek/r8169.c
Hunk #1 succeeded at 468 with fuzz 3.
Hunk #2 FAILED at 5280.
1 out of 2 hunks FAILED

~/linux-3.13.0# grep -nA8 'rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 
0x1d8f, ERIAR_EXGMAC)' drivers/net/ethernet/realtek/r8169.c
5274:	rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
5275-
5276-	RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
5277-	RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
5278-	RTL_W8(MaxTxPacketSize, EarlySize);
5279-
5280-	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
5281-	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
5282-

# ls -l $(locate drivers/net/ethernet/realtek/r8169.c)
-rw-r--r-- 1 root root 177933 Aug 12 23:42 
/home/user/src/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c
-rw-r--r-- 1 root root 177933 Aug 24 22:41 
/root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c

# md5sum /root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c
95056b56932b375f8b65a6379009f704 
/root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c


Francois, could you help me apply your patch?


Francois Romieu schreef op 07-10-14 om 00:13:
> Hayes Wang <hayeswang@realtek.com> :
>>   Francois Romieu [mailto:romieu@fr.zoreil.com]
> [...]
>> I don't sure if the following information is helpful. Besides, I remember
>> the rtl_init_one() would disable it.
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=d64ec841517a25f6d468bde9f67e5b4cffdc67c7
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=4521e1a94279ce610d3f9b7945c17d581f804242
>
> Yes, I did not expect this stuff to stay in geostationary orbit for long :o/
>
> Realtek's r8168 driver defaults to CONFIG_ASPM=1 but I guess some users
> need to disable it and there's no known pattern / blacklist, right ?
>
> Ceriel, does the patch below against current kernel make a difference ?
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 0921302..b4a3881 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -468,6 +468,7 @@ enum rtl8168_registers {
>   #define PWM_EN				(1 << 22)
>   #define RXDV_GATED_EN			(1 << 19)
>   #define EARLY_TALLY_EN			(1 << 16)
> +#define FORCE_CLK			(1 << 15) /* force clock request */
>   };
>
>   enum rtl_register_content {
> @@ -5279,8 +5280,10 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
>   	rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
>
>   	RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
> -	RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
> +	RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
>   	RTL_W8(MaxTxPacketSize, EarlySize);
> +	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> +	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
>
>   	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
>   	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Oct. 8, 2014, 10:17 p.m. UTC | #8 
Ceriel Jacobs <linux-ide@crashplan.pro> :
[...]
> Francois, could you help me apply your patch?

$ git describe
v3.13
$ patch -p1 --dry-run < /tmp/plop
checking file drivers/net/ethernet/realtek/r8169.c
Hunk #1 succeeded at 467 (offset -1 lines).
Hunk #2 succeeded at 5275 (offset -5 lines).

$ git rev-list v3.16 -- drivers/net/ethernet/realtek/r8169.c | while read x; do echo $x:$(git cat-file -p $x:drivers/net/ethernet/realtek/r8169.c | md5sum); done | grep 95056b56932b375f8b65a6379009f704

-> oualou

Where does your 3.13 source tree come from ?
Ceriel Jacobs Oct. 8, 2014, 10:46 p.m. UTC | #9 
The 3.13 source was installed using "apt-get source linux-source-3.13.0"

The patch hunk#2 failing issue is something in the "heredoc" notation 
(me trying to apply the patch without storing the patch lines as 
temporary input file). I am now using the temp file without any issue.

The next newly blocking issue is this:

# make -C /lib/modules/$(uname -r)/build M=$(pwd) modules
make: Entering directory `/usr/src/linux-headers-3.17.0-031700rc7-generic'
   CC [M]  /root/linux-3.13.0/drivers/net/ethernet/realtek/8139cp.o
   CC [M]  /root/linux-3.13.0/drivers/net/ethernet/realtek/8139too.o
   CC [M]  /root/linux-3.13.0/drivers/net/ethernet/realtek/atp.o
   CC [M]  /root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.o
/root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c: In function 
'rtl_init_one':
/root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.c:7127:2: error: 
implicit declaration of function 'SET_ETHTOOL_OPS' 
[-Werror=implicit-function-declaration]
   SET_ETHTOOL_OPS(dev, &rtl8169_ethtool_ops);
   ^
cc1: some warnings being treated as errors
make[1]: *** [/root/linux-3.13.0/drivers/net/ethernet/realtek/r8169.o] 
Error 1
make: *** [_module_/root/linux-3.13.0/drivers/net/ethernet/realtek] Error 2
make: Leaving directory `/usr/src/linux-headers-3.17.0-031700rc7-generic'

Any new suggestion how to honour your patch/test request?

Francois Romieu schreef op 09-10-14 om 00:17:
> Ceriel Jacobs <linux-ide@crashplan.pro> :
> [...]
>> Francois, could you help me apply your patch?
>
> $ git describe
> v3.13
> $ patch -p1 --dry-run < /tmp/plop
> checking file drivers/net/ethernet/realtek/r8169.c
> Hunk #1 succeeded at 467 (offset -1 lines).
> Hunk #2 succeeded at 5275 (offset -5 lines).
>
> $ git rev-list v3.16 -- drivers/net/ethernet/realtek/r8169.c | while read x; do echo $x:$(git cat-file -p $x:drivers/net/ethernet/realtek/r8169.c | md5sum); done | grep 95056b56932b375f8b65a6379009f704
>
> -> oualou
>
> Where does your 3.13 source tree come from ?
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Oct. 8, 2014, 11:26 p.m. UTC | #10 
Ceriel Jacobs <linux-ide@crashplan.pro> :
> The 3.13 source was installed using "apt-get source linux-source-3.13.0"
> 
> The patch hunk#2 failing issue is something in the "heredoc" notation (me
> trying to apply the patch without storing the patch lines as temporary input
> file). I am now using the temp file without any issue.
> 
> The next newly blocking issue is this:
> 
> # make -C /lib/modules/$(uname -r)/build M=$(pwd) modules
> make: Entering directory `/usr/src/linux-headers-3.17.0-031700rc7-generic'

You are mixing 3.13 and 3.17. Try 'uname -r' alone. Got it ?

Please get some pristine 3.17 sources - they're available on www.kernel.org -
and try again.
Ceriel Jacobs Oct. 9, 2014, 12:02 p.m. UTC | #11 
Francois Romieu schreef op 09-10-14 om 01:26:
 > You are mixing 3.13 and 3.17. Try 'uname -r' alone. Got it ?
I think I have got it.

Francois Romieu schreef op 07-10-14 om 00:13:
 > Ceriel, does the patch below against current kernel make a difference?

Old r8169 "powertop --auto-tune && powertop" result:
C2 (pc2)   26.1%    |                     |
C3 (pc3)   73.6%    | C3 (cc3)    1.6%    | C3-HSW      1.6%  108.0 ms
C6 (pc6)    0.0%    | C6 (cc6)   98.3%    | C6-HSW     98.3%  194.9 ms
---
C2 (pc2)    0.0%    |                     |
C3 (pc3)   99.4%    | C3 (cc3)    0.0%    | C3-HSW      0.0%    0.0 ms
C6 (pc6)    0.0%    | C6 (cc6)   99.6%    | C6-HSW     99.6%  237.4 ms
---

New r8169 "powertop" result (even without --auto-tune):
C2 (pc2)    0.0%    |                     |
C3 (pc3)    9.9%    | C3 (cc3)    0.7%    | C3-HSW      0.7%   16.4 ms
C6 (pc6)   89.9%    | C6 (cc6)   99.2%    | C6-HSW     99.2%  223.2 ms
---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Oct. 9, 2014, 10:14 p.m. UTC | #12 
Ceriel Jacobs <linux-ide@crashplan.pro> :
> Francois Romieu schreef op 07-10-14 om 00:13:
> > Ceriel, does the patch below against current kernel make a difference?
[...]
> New r8169 "powertop" result (even without --auto-tune):
> C2 (pc2)    0.0%    |                     |
> C3 (pc3)    9.9%    | C3 (cc3)    0.7%    | C3-HSW      0.7%   16.4 ms
> C6 (pc6)   89.9%    | C6 (cc6)   99.2%    | C6-HSW     99.2%  223.2 ms
> ---

Fine (almost: I hope that ASPM was enabled from bios or during boot
behind your back).

Remember your "lspci -nnkvv -s 03:00.0" ?

03:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168] (rev 11)
[...]
        Capabilities: [70] Express (v2) Endpoint, MSI 01
                DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
                        ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
                DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
                        RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
                        MaxPayload 128 bytes, MaxReadReq 4096 bytes
                DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
                LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 <64us
                   	ClockPM+ Surprise- LLActRep- BwNot-
                LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-

It should now look like:
[...]
                LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+

Let's temporarily disable it and see if powertop notices a difference.

<full disclosure>

"Capabilities: [70]" above gives you the offset of the relevant registers:
# lspci -xxx -s 03:00.0
[...]
70: .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
^^ -> "[70]"

You are interested in the Link Control register, aka PCI_EXP_LNKCTL in
/usr/include/pci/header.h (devel part of pciutils) or kernel's
include/uapi/linux/pci_regs.h. It's 16 bytes further, thus
[...]
70: .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
80: 42 .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
    ^^

0x42 matches "LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+
ExtSynch-" built from above. There may be differences but the 3 lower
weight binary digits in 0x42 encode ASPM control (0=nada, 1=L0, 2=L1,
see PCI_EXP_LNKCTL_ASPxyz in include/uapi/linux/pci_regs.h). Mask these
out (0x42 & ~0x03) and feed the resulting value back into the Link
Control register:

# setpci -s 03:00.0 CAP_EXP+10.b=0x40

(CAP_EXP is pciutils's alias for the PCI Express capability block, see
PCI_CAP_ID_EXP in kernel's include/uapi/linux/pci_regs.h)

If you are not too sure about the 0x40 value, you can retrieve it with
lspci and an unpatched r8169 driver.

</full disclosure>

If I have understood Hayes correctly and he got my question right, lspci
should now tell that ASPM is disabled. C6 should not be reached anymore.

ASPM could thus be enabled unconditionally at the driver level, then
controled through the PCI config registers. Kernel r8169 driver would
thus protect polar bears as Realtek's own r8168 driver already does.

I can't exclude that it will fail miserably in a firework of smelly
smoke though.
Ceriel Jacobs Oct. 10, 2014, 11:09 a.m. UTC | #13 
ASPM is enabled in the BIOS as far as possible.
ASPM is also enabled using kernel parameters:
1. pcie_aspm.policy=powersave
2. pcie_aspm=force

Despite the result for 03:00.0 (and 2 other PCIe devices) is:
LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk+


I have filed a bug report for that at:
https://bugzilla.kernel.org/show_bug.cgi?id=85621


In my testing before, I did manually enable L0s L1 ASPM after login 
prompt using:
# setpci -s 03:00.0 0x80.B=0x43 (also for 3 other ASPM PCIe devices)

=============================
Now back to your Realtek test request
=============================
For your information: I am running all testing remote via SSH over the 
Realtek NIc.

=======
L0s and L1 = PC6
=======
# setpci -s 03:00.0 0x80.B=0x43
# lspci -vvvv -s 03:00.0 | grep LnkCtl:
		LnkCtl:	ASPM L0s L1 Enabled; RCB 64 bytes Disabled- ...

PC6 reached in 1 powertop screen update (5 second interval)
C3 (pc3)   21.0%    | C3 (cc3)    0.0%    | C3-HSW      0.0%    0.0 ms
C6 (pc6)   76.0%    | C6 (cc6)   99.9%    | C6-HSW     99.9%  198.1 ms

=======
L1 only = no PC6
=======
# setpci -s 03:00.0 0x80.B=0x42
		LnkCtl:	ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+

C3 (pc3)   99.8%    | C3 (cc3)    0.0%    | C3-HSW      0.0%    0.0 ms
C6 (pc6)    0.0%    | C6 (cc6)   99.9%    | C6-HSW    100.0%  206.4 ms

=======
L0s only = PC6 !!
=======
# setpci -s 03:00.0 0x80.B=0x41
		LnkCtl:	ASPM L0s Enabled; RCB 64 bytes Disabled- ...

C3 (pc3)   10.6%    | C3 (cc3)    0.0%    | C3-HSW      0.0%    0.0 ms
C6 (pc6)   89.1%    | C6 (cc6)   99.9%    | C6-HSW     99.9%  210.7 ms

=======
Disabled = no PC6
=======
# setpci -s 03:00.0 0x80.B=0x40
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- CommClk+

C3 (pc3)   99.8%    | C3 (cc3)    0.0%    | C3-HSW      0.0%    0.0 ms
C6 (pc6)    0.0%    | C6 (cc6)   99.9%    | C6-HSW     99.9%  230.1 ms

Now I am wondering whether it is by design or a bug that PC6 is not 
reached when only L1 ASPM is activated

An applause for:
1. saving "polar bears",
2. letting me test a little more,
3. realtime behavior changing (no module unloading and loading to active 
ASPM, just set the config space registers).

Francois Romieu schreef op 10-10-14 om 00:14:
> Ceriel Jacobs <linux-ide@crashplan.pro> :
>> Francois Romieu schreef op 07-10-14 om 00:13:
>>> Ceriel, does the patch below against current kernel make a difference?
> [...]
>> New r8169 "powertop" result (even without --auto-tune):
>> C2 (pc2)    0.0%    |                     |
>> C3 (pc3)    9.9%    | C3 (cc3)    0.7%    | C3-HSW      0.7%   16.4 ms
>> C6 (pc6)   89.9%    | C6 (cc6)   99.2%    | C6-HSW     99.2%  223.2 ms
>> ---
>
> Fine (almost: I hope that ASPM was enabled from bios or during boot
> behind your back).
>
> Remember your "lspci -nnkvv -s 03:00.0" ?
>
> 03:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168] (rev 11)
> [...]
>          Capabilities: [70] Express (v2) Endpoint, MSI 01
>                  DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
>                          ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
>                  DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
>                          RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
>                          MaxPayload 128 bytes, MaxReadReq 4096 bytes
>                  DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
>                  LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 <64us
>                     	ClockPM+ Surprise- LLActRep- BwNot-
>                  LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+
>                          ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
>
> It should now look like:
> [...]
>                  LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+
>
> Let's temporarily disable it and see if powertop notices a difference.
>
> <full disclosure>
>
> "Capabilities: [70]" above gives you the offset of the relevant registers:
> # lspci -xxx -s 03:00.0
> [...]
> 70: .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
> ^^ -> "[70]"
>
> You are interested in the Link Control register, aka PCI_EXP_LNKCTL in
> /usr/include/pci/header.h (devel part of pciutils) or kernel's
> include/uapi/linux/pci_regs.h. It's 16 bytes further, thus
> [...]
> 70: .. .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
> 80: 42 .. .. .. .. .. .. .. .. .. .. .. .. .. .. ..
>      ^^
>
> 0x42 matches "LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+
> ExtSynch-" built from above. There may be differences but the 3 lower
> weight binary digits in 0x42 encode ASPM control (0=nada, 1=L0, 2=L1,
> see PCI_EXP_LNKCTL_ASPxyz in include/uapi/linux/pci_regs.h). Mask these
> out (0x42 & ~0x03) and feed the resulting value back into the Link
> Control register:
>
> # setpci -s 03:00.0 CAP_EXP+10.b=0x40
>
> (CAP_EXP is pciutils's alias for the PCI Express capability block, see
> PCI_CAP_ID_EXP in kernel's include/uapi/linux/pci_regs.h)
>
> If you are not too sure about the 0x40 value, you can retrieve it with
> lspci and an unpatched r8169 driver.
>
> </full disclosure>
>
> If I have understood Hayes correctly and he got my question right, lspci
> should now tell that ASPM is disabled. C6 should not be reached anymore.
>
> ASPM could thus be enabled unconditionally at the driver level, then
> controled through the PCI config registers. Kernel r8169 driver would
> thus protect polar bears as Realtek's own r8168 driver already does.
>
> I can't exclude that it will fail miserably in a firework of smelly
> smoke though.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0921302..b4a3881 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -468,6 +468,7 @@  enum rtl8168_registers {
 #define PWM_EN				(1 << 22)
 #define RXDV_GATED_EN			(1 << 19)
 #define EARLY_TALLY_EN			(1 << 16)
+#define FORCE_CLK			(1 << 15) /* force clock request */
 };
 
 enum rtl_register_content {
@@ -5279,8 +5280,10 @@  static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
 	rtl_eri_write(tp, 0x2f8, ERIAR_MASK_0011, 0x1d8f, ERIAR_EXGMAC);
 
 	RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
-	RTL_W32(MISC, RTL_R32(MISC) & ~RXDV_GATED_EN);
+	RTL_W32(MISC, (RTL_R32(MISC) | FORCE_CLK) & ~RXDV_GATED_EN);
 	RTL_W8(MaxTxPacketSize, EarlySize);
+	RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+	RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
 
 	rtl_eri_write(tp, 0xc0, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);
 	rtl_eri_write(tp, 0xb8, ERIAR_MASK_0011, 0x0000, ERIAR_EXGMAC);