RE: [PATCH net-next] r8169: add module param for control of ASPM disable

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Friday, February 2, 2018 7:27 AM
> To: Hau <h...@realtek.com>
> Cc: net...@vger.kernel.org; nic_swsd <nic_s...@realtek.com>; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH net-next] r8169: add module param for control of ASPM
> disable
> 
> Chunhao Lin <h...@realtek.com> :
> [...]
> > @@ -5878,6 +5881,20 @@ static void rtl_pcie_state_l2l3_enable(struct
> rtl8169_private *tp, bool enable)
> > RTL_W8(Config3, data);
> >  }
> >
> > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private
> *tp,
> > +   bool enable)
> > +{
> > +   void __iomem *ioaddr = tp->mmio_addr;
> > +
> > +   if (enable) {
> > +   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> > +   } else {
> > +   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
> > +   }
> > +}
> 
> s/enable(..., false)/disable()/
> 
> static void rtl_hw_internal_aspm_clkreq_enable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en); }
> 
> static void rtl_hw_internal_aspm_clkreq_disable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en); }
> 
> If you really want to factor something out, you may use helpers that set or
> clear bits according to the 3-uple (tp, register, bits) but foo_enable(..., 
> false)
> is pointlessly convoluted.
> 
> --
> Ueimor
> 
> --Please consider the environment before printing this e-mail.

This two bits are related to hardware ASPM function, so I put them together 
into one function.
I could use helper to set these bits, if doing this can make the patch more 
readable for kernel team.

Thanks for inform me that net-next is closed. I will resubmit this patch when 
net-next open.

Thanks.

--Please consider the environment before printing this e-mail.

RE: [PATCH net-next] r8169: add module param for control of ASPM disable

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Friday, February 2, 2018 7:27 AM
> To: Hau 
> Cc: net...@vger.kernel.org; nic_swsd ; linux-
> ker...@vger.kernel.org
> Subject: Re: [PATCH net-next] r8169: add module param for control of ASPM
> disable
> 
> Chunhao Lin  :
> [...]
> > @@ -5878,6 +5881,20 @@ static void rtl_pcie_state_l2l3_enable(struct
> rtl8169_private *tp, bool enable)
> > RTL_W8(Config3, data);
> >  }
> >
> > +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private
> *tp,
> > +   bool enable)
> > +{
> > +   void __iomem *ioaddr = tp->mmio_addr;
> > +
> > +   if (enable) {
> > +   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> > +   } else {
> > +   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
> > +   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
> > +   }
> > +}
> 
> s/enable(..., false)/disable()/
> 
> static void rtl_hw_internal_aspm_clkreq_enable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) | ASPM_en); }
> 
> static void rtl_hw_internal_aspm_clkreq_disable(truct rtl8169_private *tp) {
>   void __iomem *ioaddr = tp->mmio_addr;
> 
>   RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
>   RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en); }
> 
> If you really want to factor something out, you may use helpers that set or
> clear bits according to the 3-uple (tp, register, bits) but foo_enable(..., 
> false)
> is pointlessly convoluted.
> 
> --
> Ueimor
> 
> --Please consider the environment before printing this e-mail.

This two bits are related to hardware ASPM function, so I put them together 
into one function.
I could use helper to set these bits, if doing this can make the patch more 
readable for kernel team.

Thanks for inform me that net-next is closed. I will resubmit this patch when 
net-next open.

Thanks.

--Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Friday, February 2, 2018 10:03 AM
> To: Hau <h...@realtek.com>
> Cc: nic_swsd <nic_s...@realtek.com>; net...@vger.kernel.org; Linux
> Kernel <linux-kernel@vger.kernel.org>; Linux Upstreaming Team
> <li...@endlessm.com>
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Tue, Jan 30, 2018 at 8:07 PM, Chris Chiu <c...@endlessm.com> wrote:
> > On Mon, Jan 29, 2018 at 11:24 PM, Hau <h...@realtek.com> wrote:
> >> Hi Chris,
> >>
> >> Could you test following patch?
> >>
> >>  DECLARE_RTL_COND(rtl_ocp_tx_cond)
> >>  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> -   return RTL_R8(IBISR0) & 0x02;
> >> +   return RTL_R8(IBISR0) & 0x20;
> >>  }
> >>
> >>  static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
> >> -   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
> >> +   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
> >> RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
> >> RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);  }
> >>
> >> Thanks.
> >>
> >
> > Yes. It completes the initialization in 70 ms. So it means the
> > rtl_ocp_tx_cond are waiting for incorrect register bit? Can you help work
> out a patch for this?
> >
> > Chris
> >
> >
> 
> Gentle ping,
> cheers.
> 
> Chris
> 

I have submitted the patch to kernel team.
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=086ca23d03c0d2f4088f472386778d293e15c5f6


--Please consider the environment before printing this e-mail.

> >>> -Original Message-
> >>> From: Chris Chiu [mailto:c...@endlessm.com]
> >>> Sent: Monday, January 29, 2018 6:12 PM
> >>> To: nic_swsd <nic_s...@realtek.com>; net...@vger.kernel.org; Linux
> >>> Kernel <linux-kernel@vger.kernel.org>; Linux Upstreaming Team
> >>> <li...@endlessm.com>
> >>> Subject: Re: r8169 take too long to complete driver initialization
> >>>
> >>> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu <c...@endlessm.com> wrote:
> >>> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu <c...@endlessm.com>
> wrote:
> >>> >> Hi,
> >>> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >>> >> r8169 driver. The dmseg
> >>> >>
> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >>> >> shows it's still in msleep() within a mutex lock.
> >>> >> After looking into the code, it's caused by the
> >>> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >>> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver
> >>> >> is loaded.
> >>> >>
> >>> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>> >>
> >>> >> But it takes > 100 seconds to get the following messages
> >>> >>
> >>> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >>> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >>> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >>> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >>> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >>> >> bytes, tx checksumming: ko]
> >>> >>
> >>> >> So any trial to suspend the machine during this period would
> >>> >> always get device/resource busy message then abort. Is this
> >>> >> rtl_ocp_tx_cond necessary? Because the ethernet is still working
> >>> >> and I don't see any problem. I don't know it should be considered
> >>> >> normal or not. Please let me know if any more information
> >>> >> required. Thanks
> >>> >>
> >>> >> Chris
> >>> >
> >>> > gentle ping,
> >>> >
> >>> > cheers.
> >>>
> >>> Hi,
> >>> Just found a r8168 driver which seems to be authrized by realtek
> >>> for cross comparison. I tried applying the patch to latest 4.15
> >>> kernel and the driver done it's initialization in faily short time. The 
> >>> patch
> file is here
> >>> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> >>>
> >>> In mainline r8169.c, the IBISR0 register need to be polled in
> >>> the rtl8168ep_stop_cmac().
> >>> In the patch file, there's also the same IBISR0 polling code in
> >>> Dash2DisableTx(), but it's been bypassed since the chipset maches
> >>> HW_DASH_SUPPORT_TYPE_2.
> >>> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> >>> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> >>> RTL8168EP/8111EP in the rtl_chip_info[].
> >>>
> >>> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> >>> seems share the same config. Can anyone clarify if the
> >>> rtl_ocp_tx_cond() really necessary for 8168EP/8111EP?
> >>> Or we can just ignore the condition check for
> RTL_GIGA_MAC_VER_49/50/51?
> >>>
> >>> Chris
> >>>
> >>> --Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Friday, February 2, 2018 10:03 AM
> To: Hau 
> Cc: nic_swsd ; net...@vger.kernel.org; Linux
> Kernel ; Linux Upstreaming Team
> 
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Tue, Jan 30, 2018 at 8:07 PM, Chris Chiu  wrote:
> > On Mon, Jan 29, 2018 at 11:24 PM, Hau  wrote:
> >> Hi Chris,
> >>
> >> Could you test following patch?
> >>
> >>  DECLARE_RTL_COND(rtl_ocp_tx_cond)
> >>  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> -   return RTL_R8(IBISR0) & 0x02;
> >> +   return RTL_R8(IBISR0) & 0x20;
> >>  }
> >>
> >>  static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)  {
> >> void __iomem *ioaddr = tp->mmio_addr;
> >>
> >> RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
> >> -   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
> >> +   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
> >> RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
> >> RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);  }
> >>
> >> Thanks.
> >>
> >
> > Yes. It completes the initialization in 70 ms. So it means the
> > rtl_ocp_tx_cond are waiting for incorrect register bit? Can you help work
> out a patch for this?
> >
> > Chris
> >
> >
> 
> Gentle ping,
> cheers.
> 
> Chris
> 

I have submitted the patch to kernel team.
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=086ca23d03c0d2f4088f472386778d293e15c5f6


--Please consider the environment before printing this e-mail.

> >>> -Original Message-
> >>> From: Chris Chiu [mailto:c...@endlessm.com]
> >>> Sent: Monday, January 29, 2018 6:12 PM
> >>> To: nic_swsd ; net...@vger.kernel.org; Linux
> >>> Kernel ; Linux Upstreaming Team
> >>> 
> >>> Subject: Re: r8169 take too long to complete driver initialization
> >>>
> >>> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu  wrote:
> >>> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu 
> wrote:
> >>> >> Hi,
> >>> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >>> >> r8169 driver. The dmseg
> >>> >>
> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >>> >> shows it's still in msleep() within a mutex lock.
> >>> >> After looking into the code, it's caused by the
> >>> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >>> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver
> >>> >> is loaded.
> >>> >>
> >>> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>> >>
> >>> >> But it takes > 100 seconds to get the following messages
> >>> >>
> >>> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >>> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >>> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >>> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >>> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >>> >> bytes, tx checksumming: ko]
> >>> >>
> >>> >> So any trial to suspend the machine during this period would
> >>> >> always get device/resource busy message then abort. Is this
> >>> >> rtl_ocp_tx_cond necessary? Because the ethernet is still working
> >>> >> and I don't see any problem. I don't know it should be considered
> >>> >> normal or not. Please let me know if any more information
> >>> >> required. Thanks
> >>> >>
> >>> >> Chris
> >>> >
> >>> > gentle ping,
> >>> >
> >>> > cheers.
> >>>
> >>> Hi,
> >>> Just found a r8168 driver which seems to be authrized by realtek
> >>> for cross comparison. I tried applying the patch to latest 4.15
> >>> kernel and the driver done it's initialization in faily short time. The 
> >>> patch
> file is here
> >>> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> >>>
> >>> In mainline r8169.c, the IBISR0 register need to be polled in
> >>> the rtl8168ep_stop_cmac().
> >>> In the patch file, there's also the same IBISR0 polling code in
> >>> Dash2DisableTx(), but it's been bypassed since the chipset maches
> >>> HW_DASH_SUPPORT_TYPE_2.
> >>> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> >>> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> >>> RTL8168EP/8111EP in the rtl_chip_info[].
> >>>
> >>> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> >>> seems share the same config. Can anyone clarify if the
> >>> rtl_ocp_tx_cond() really necessary for 8168EP/8111EP?
> >>> Or we can just ignore the condition check for
> RTL_GIGA_MAC_VER_49/50/51?
> >>>
> >>> Chris
> >>>
> >>> --Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

Hi Chris,

Could you test following patch?

 DECLARE_RTL_COND(rtl_ocp_tx_cond)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
-   return RTL_R8(IBISR0) & 0x02;
+   return RTL_R8(IBISR0) & 0x20;
 }
 
 static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
-   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
+   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);
 }

Thanks.

--Please consider the environment before printing this e-mail.

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Monday, January 29, 2018 6:12 PM
> To: nic_swsd ; net...@vger.kernel.org; Linux
> Kernel ; Linux Upstreaming Team
> 
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu  wrote:
> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu  wrote:
> >> Hi,
> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >> r8169 driver. The dmseg
> >> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >> shows it's still in msleep() within a mutex lock.
> >> After looking into the code, it's caused by the
> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver is
> >> loaded.
> >>
> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>
> >> But it takes > 100 seconds to get the following messages
> >>
> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >> bytes, tx checksumming: ko]
> >>
> >> So any trial to suspend the machine during this period would always
> >> get device/resource busy message then abort. Is this  rtl_ocp_tx_cond
> >> necessary? Because the ethernet is still working and I don't see any
> >> problem. I don't know it should be considered normal or not. Please
> >> let me know if any more information required. Thanks
> >>
> >> Chris
> >
> > gentle ping,
> >
> > cheers.
> 
> Hi,
> Just found a r8168 driver which seems to be authrized by realtek for cross
> comparison. I tried applying the patch to latest 4.15 kernel and the driver
> done it's initialization in faily short time. The patch file is here
> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> 
> In mainline r8169.c, the IBISR0 register need to be polled in the
> rtl8168ep_stop_cmac().
> In the patch file, there's also the same IBISR0 polling code in
> Dash2DisableTx(), but it's been bypassed since the chipset maches
> HW_DASH_SUPPORT_TYPE_2.
> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> RTL8168EP/8111EP in the rtl_chip_info[].
> 
> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> seems share the same config. Can anyone clarify if the rtl_ocp_tx_cond()
> really necessary for 8168EP/8111EP?
> Or we can just ignore the condition check for RTL_GIGA_MAC_VER_49/50/51?
> 
> Chris
> 
> --Please consider the environment before printing this e-mail.

RE: r8169 take too long to complete driver initialization

[Hau]

Hi Chris,

Could you test following patch?

 DECLARE_RTL_COND(rtl_ocp_tx_cond)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
-   return RTL_R8(IBISR0) & 0x02;
+   return RTL_R8(IBISR0) & 0x20;
 }
 
 static void rtl8168ep_stop_cmac(struct rtl8169_private *tp)
 {
void __iomem *ioaddr = tp->mmio_addr;
 
RTL_W8(IBCR2, RTL_R8(IBCR2) & ~0x01);
-   rtl_msleep_loop_wait_low(tp, _ocp_tx_cond, 50, 2000);
+   rtl_msleep_loop_wait_high(tp, _ocp_tx_cond, 50, 2000);
RTL_W8(IBISR0, RTL_R8(IBISR0) | 0x20);
RTL_W8(IBCR0, RTL_R8(IBCR0) & ~0x01);
 }

Thanks.

--Please consider the environment before printing this e-mail.

> -Original Message-
> From: Chris Chiu [mailto:c...@endlessm.com]
> Sent: Monday, January 29, 2018 6:12 PM
> To: nic_swsd ; net...@vger.kernel.org; Linux
> Kernel ; Linux Upstreaming Team
> 
> Subject: Re: r8169 take too long to complete driver initialization
> 
> On Fri, Jan 5, 2018 at 10:17 AM, Chris Chiu  wrote:
> > On Wed, Dec 20, 2017 at 4:41 PM, Chris Chiu  wrote:
> >> Hi,
> >> We've hit a suspend/resume issue on a Acer desktop caused by
> >> r8169 driver. The dmseg
> >> https://gist.github.com/mschiu77/b741849b5070281daaead8dfee312d1a
> >> shows it's still in msleep() within a mutex lock.
> >> After looking into the code, it's caused by the
> >> rtl8168ep_stop_cmac() which is waiting 100 seconds for
> >> rtl_ocp_tx_cond. The following dmesg states that the r8169 driver is
> >> loaded.
> >>
> >> [   20.270526] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
> >>
> >> But it takes > 100 seconds to get the following messages
> >>
> >> [  140.400223] r8169 :02:00.0 (unnamed net_device)
> >> (uninitialized): rtl_ocp_tx_cond == 1 (loop: 2000, delay: 50).
> >> [  140.413294] r8169 :02:00.0 eth0: RTL8168ep/8111ep at
> >> 0xb16c80db1000, f8:0f:41:ea:74:0d, XID 10200800 IRQ 46 [
> >> 140.413297] r8169 :02:00.0 eth0: jumbo features [frames: 9200
> >> bytes, tx checksumming: ko]
> >>
> >> So any trial to suspend the machine during this period would always
> >> get device/resource busy message then abort. Is this  rtl_ocp_tx_cond
> >> necessary? Because the ethernet is still working and I don't see any
> >> problem. I don't know it should be considered normal or not. Please
> >> let me know if any more information required. Thanks
> >>
> >> Chris
> >
> > gentle ping,
> >
> > cheers.
> 
> Hi,
> Just found a r8168 driver which seems to be authrized by realtek for cross
> comparison. I tried applying the patch to latest 4.15 kernel and the driver
> done it's initialization in faily short time. The patch file is here
> https://gist.github.com/mschiu77/fcf406e64a1a437f46cf2be643f1057d.
> 
> In mainline r8169.c, the IBISR0 register need to be polled in the
> rtl8168ep_stop_cmac().
> In the patch file, there's also the same IBISR0 polling code in
> Dash2DisableTx(), but it's been bypassed since the chipset maches
> HW_DASH_SUPPORT_TYPE_2.
> Per the rtl_chip_info[] in r8168_n.c, CFG_METHOD_23/27/28 are
> HW_DASH_SUPPORT_TYPE_2, and they happens to be the only 3 named
> RTL8168EP/8111EP in the rtl_chip_info[].
> 
> To find the same matches in r8169.c, RTL_GIGA_MAC_VER_49/50/51
> seems share the same config. Can anyone clarify if the rtl_ocp_tx_cond()
> really necessary for 8168EP/8111EP?
> Or we can just ignore the condition check for RTL_GIGA_MAC_VER_49/50/51?
> 
> Chris
> 
> --Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> Nit: you may directly use "struct device *d = >pci_dev->dev;"
> 

I will do that on my next version patch.

Thanks.
--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> Nit: you may directly use "struct device *d = >pci_dev->dev;"
> 

I will do that on my next version patch.

Thanks.
--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> > @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device
> *dev, struct ethtool_wolinfo *wol)
> > tp->features |= RTL_FEATURE_WOL;
> > else
> > tp->features &= ~RTL_FEATURE_WOL;
> > -   __rtl8169_set_wol(tp, wol->wolopts);
> > +   if (pm_runtime_active(>dev))
> > +   __rtl8169_set_wol(tp, wol->wolopts);
> > +   else
> > +   tp->saved_wolopts = wol->wolopts;
> >
> > rtl_unlock_work(tp);
> >
> > device_set_wakeup_enable(>pci_dev->dev, wol->wolopts);
> >
> > +   pm_runtime_put_noidle(>dev);
> > +
> > return 0;
> 
> Either the driver resumes the device so that it can perform requested
> operation or it signals .set_wol failure when the device is suspended.
> 
> If the driver does something else, "spam removal" translates to "silent
> failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in 
rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to " 
tp->saved_wolopts " in runtime suspend state and set this to this 
"wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

Thanks.

--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.

[Hau]

[...]
> > @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device
> *dev, struct ethtool_wolinfo *wol)
> > tp->features |= RTL_FEATURE_WOL;
> > else
> > tp->features &= ~RTL_FEATURE_WOL;
> > -   __rtl8169_set_wol(tp, wol->wolopts);
> > +   if (pm_runtime_active(>dev))
> > +   __rtl8169_set_wol(tp, wol->wolopts);
> > +   else
> > +   tp->saved_wolopts = wol->wolopts;
> >
> > rtl_unlock_work(tp);
> >
> > device_set_wakeup_enable(>pci_dev->dev, wol->wolopts);
> >
> > +   pm_runtime_put_noidle(>dev);
> > +
> > return 0;
> 
> Either the driver resumes the device so that it can perform requested
> operation or it signals .set_wol failure when the device is suspended.
> 
> If the driver does something else, "spam removal" translates to "silent
> failure".

Because "tp->saved_wolopts" will be used to set hardware wol capability in 
rtl8169_runtime_resume().  So I prefer to keep "wol->wolopts" to " 
tp->saved_wolopts " in runtime suspend state and set this to this 
"wol->wolopts" to hardware in in rtl8169_runtime_resume(). 

Thanks.

--Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:Remove unnecessary phy reset for pcie nic when setting link spped.

[Hau]

[...] 
> Can you clarify:
> - actually this patch does not care about the link at all. So when there's
>   link no phy reset is needed either, right ?
> - does "this" in "to do this" means that
>   1. phy reset prevents phy from auto speed down
>   2. avoiding phy reset prevents phy from auto speed down
>   I would expect 1. from the rtl_wol_pll_power_down + rtl_speed_down +
>   rtl8169_set_speed combo (i.e. we want the driver to allow auto speed
> down)
>   but it's a bit ambiguous.

Unless pcie nic has bug, pcie nic does not need to reset phy to let phy link on.

There is a counter for phy speed down. If phy is in link down state, this 
counter will start to count down. When it count to 0, phy will speed down. 
Reset phy will reset this counter and prevent phy from speed down.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:Remove unnecessary phy reset for pcie nic when setting link spped.

[Hau]

[...] 
> Can you clarify:
> - actually this patch does not care about the link at all. So when there's
>   link no phy reset is needed either, right ?
> - does "this" in "to do this" means that
>   1. phy reset prevents phy from auto speed down
>   2. avoiding phy reset prevents phy from auto speed down
>   I would expect 1. from the rtl_wol_pll_power_down + rtl_speed_down +
>   rtl8169_set_speed combo (i.e. we want the driver to allow auto speed
> down)
>   but it's a bit ambiguous.

Unless pcie nic has bug, pcie nic does not need to reset phy to let phy link on.

There is a counter for phy speed down. If phy is in link down state, this 
counter will start to count down. When it count to 0, phy will speed down. 
Reset phy will reset this counter and prevent phy from speed down.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net 0/3] r8169:issues fix.

[Hau]

> I don't agree with changes #1 and #2.
> 
> If you are going to go to a model where every single configuration operation
> is recorded in software and performed at resume time, then really do it and
> fix it in the whole driver.  As currently coded you are leaving lots of known
> bugs in the driver.
> 
> #2 is even a worse situation.  If you are going to handle things this way you
> must sync the counters when the suspend happens, so that the statistics get
> call receives up to date values.

Thanks for your advice. I will send #3 patch first. Then modify #1 and #2 patch 
and resend these two patches.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net 0/3] r8169:issues fix.

[Hau]

> I don't agree with changes #1 and #2.
> 
> If you are going to go to a model where every single configuration operation
> is recorded in software and performed at resume time, then really do it and
> fix it in the whole driver.  As currently coded you are leaving lots of known
> bugs in the driver.
> 
> #2 is even a worse situation.  If you are going to handle things this way you
> must sync the counters when the suspend happens, so that the statistics get
> call receives up to date values.

Thanks for your advice. I will send #3 patch first. Then modify #1 and #2 patch 
and resend these two patches.

 --Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Hau]

 > Instead of taking the device out of suspended mode to perform the required
> action, the driver is moving to a model where 1) said action may be
> scheduled to a later time - or result from past time work - and 2) rpm handler
> must handle a lot of pm unrelated work.
> 
> rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed yet
> (what about the .set_xyz handlers ?).
> 
> I can't help thinking that the driver should return to a state where it 
> stupidly
> does what it is asked to. No software caching, plain device access, resume
> when needed, suspend as "suspend" instead of suspend as "anticipate
> whatever may happen to avoid waking up".
>

This rpm related patches just the workaround for the issues reported by end 
users.  As you say, the Linux kernel should handle these events when driver is 
in runtime suspend state.
 
--Please consider the environment before printing this e-mail.

RE: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.

[Hau]

 > Instead of taking the device out of suspended mode to perform the required
> action, the driver is moving to a model where 1) said action may be
> scheduled to a later time - or result from past time work - and 2) rpm handler
> must handle a lot of pm unrelated work.
> 
> rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed yet
> (what about the .set_xyz handlers ?).
> 
> I can't help thinking that the driver should return to a state where it 
> stupidly
> does what it is asked to. No software caching, plain device access, resume
> when needed, suspend as "suspend" instead of suspend as "anticipate
> whatever may happen to avoid waking up".
>

This rpm related patches just the workaround for the issues reported by end 
users.  As you say, the Linux kernel should handle these events when driver is 
in runtime suspend state.
 
--Please consider the environment before printing this e-mail.

RE: [PATCH net v4] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Fine with me.
> 
> Is there any chance for the set of chipset dependent registers that are safe 
> to
> be read when in D3 state to be documented ?
> 

I think except registers in PCI configuration, all other registers should be 
read in D0 state.

--Please consider the environment before printing this e-mail.

RE: [PATCH net v4] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Fine with me.
> 
> Is there any chance for the set of chipset dependent registers that are safe 
> to
> be read when in D3 state to be documented ?
> 

I think except registers in PCI configuration, all other registers should be 
read in D0 state.

--Please consider the environment before printing this e-mail.

RE: [PATCH net v2] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Nits:
> 
> - the tp->TxDescArray test provides the required synchronization: see
>   rtl8169_{open/close} and their pm_runtime_{get / put}.
> 
> - ioaddr is not really needed : tp->mmio_addr appears only once and it does
>   not mess the 72..80 cols limit.
> 
> - even if the device can only be automatically runtime suspended some time
>   after a link down event, you may address davem's point regarding stats
>   reliability and move rtl8169_rx_missed + rtl8169_update_counters after
>   rtl8169_net_suspend.

I will submit the new patch according to your advice.

Thanks.

Please consider the environment before printing this e-mail.

RE: [PATCH net v2] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Nits:
> 
> - the tp->TxDescArray test provides the required synchronization: see
>   rtl8169_{open/close} and their pm_runtime_{get / put}.
> 
> - ioaddr is not really needed : tp->mmio_addr appears only once and it does
>   not mess the 72..80 cols limit.
> 
> - even if the device can only be automatically runtime suspended some time
>   after a link down event, you may address davem's point regarding stats
>   reliability and move rtl8169_rx_missed + rtl8169_update_counters after
>   rtl8169_net_suspend.

I will submit the new patch according to your advice.

Thanks.

Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Chunhao Lin  :
> [...]
> > I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
> > this issue.
> 
> Would you consider taking the device out of suspended mode during
> rtl8169_get_stats64 to prevent outdated stats ?
> 

When in runtime suspend, it mean there is no traffic on NIC, so I did not wake 
the device during rtl8169_get_stats64().
Maybe we can update tally counter before entering runtime suspend mode.

--Please consider the environment before printing this e-mail.

RE: [PATCH net] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.

[Hau]

> Chunhao Lin  :
> [...]
> > I add checking driver's pm runtime status in rtl8169_get_stats64() to fix
> > this issue.
> 
> Would you consider taking the device out of suspended mode during
> rtl8169_get_stats64 to prevent outdated stats ?
> 

When in runtime suspend, it mean there is no traffic on NIC, so I did not wake 
the device during rtl8169_get_stats64().
Maybe we can update tally counter before entering runtime suspend mode.

--Please consider the environment before printing this e-mail.

RE: [PATCH net-next 07/11] r8169:update rtl8168f pcie ephy parameter

[Hau]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Friday, January 09, 2015 12:09 PM
> To: david.lai...@aculab.com
> Cc: Hau; net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 07/11] r8169:update rtl8168f pcie ephy
> parameter
> 
> From: David Laight 
> Date: Wed, 7 Jan 2015 16:45:58 +
> 
> > From: Chunhao Lin
> >> @@ -5852,7 +5852,9 @@ static void rtl_hw_start_8168f_1(struct
> rtl8169_private *tp)
> >>{ 0x06, 0x00c0, 0x0020 },
> >>{ 0x08, 0x0001, 0x0002 },
> >>{ 0x09, 0x, 0x0080 },
> >> -  { 0x19, 0x, 0x0224 }
> >> +  { 0x19, 0x, 0x0224 },
> >> +  { 0x00, 0x, 0x0008 },
> >> +  { 0x0c, 0x3df0, 0x0200 }
> >
> > I can't help feeling these lines all require short comments.
> 
> Agreed.
> 
> And this goes for some of the other patches that look like this too.
>
I will merge the patches and update again.

Thanks.
 
--Please consider the environment before printing this e-mail.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next 07/11] r8169:update rtl8168f pcie ephy parameter

[Hau]

 -Original Message-
 From: David Miller [mailto:da...@davemloft.net]
 Sent: Friday, January 09, 2015 12:09 PM
 To: david.lai...@aculab.com
 Cc: Hau; net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH net-next 07/11] r8169:update rtl8168f pcie ephy
 parameter
 
 From: David Laight david.lai...@aculab.com
 Date: Wed, 7 Jan 2015 16:45:58 +
 
  From: Chunhao Lin
  @@ -5852,7 +5852,9 @@ static void rtl_hw_start_8168f_1(struct
 rtl8169_private *tp)
 { 0x06, 0x00c0, 0x0020 },
 { 0x08, 0x0001, 0x0002 },
 { 0x09, 0x, 0x0080 },
  -  { 0x19, 0x, 0x0224 }
  +  { 0x19, 0x, 0x0224 },
  +  { 0x00, 0x, 0x0008 },
  +  { 0x0c, 0x3df0, 0x0200 }
 
  I can't help feeling these lines all require short comments.
 
 Agreed.
 
 And this goes for some of the other patches that look like this too.

I will merge the patches and update again.

Thanks.
 
--Please consider the environment before printing this e-mail.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next 0/9] r8169:update hardware ephy parameter

[Hau]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Wednesday, December 10, 2014 7:36 AM
> To: Hau
> Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 0/9] r8169:update hardware ephy parameter
> 
> From: Chunhao Lin 
> Date: Wed, 10 Dec 2014 00:45:54 +0800
> 
> > Update hardware ephy parameter to improve pcie compatibility.
> 
> This really doesn't tell me anything, I really dislike patch series like this 
> one.
> 
> All of the programming is magic values to magic offsets.
> 
> You aren't even trying to describe in the commit log message exactly what
> kind of settings are being changed, and exactly how those changes achieve
> the stated goal.
> 
> Furthermore, the commit description makes no sense at all to me.
> 
> How can programming the ethernet MAC PHY have any influence on PCI-E
> bus compatability?  Or are you programming the PCI bus interface's PHY?
> 
> In what way are you adjusting which settings and in what way do those
> adjustments help improve PCI-E bus behavior?
> 
> You absolutely must describe exactly what the new programming is actually
> doing, precisely, and in detail.  I want to know if some kind of timings are
> being adjusted, and in what way.
> Are some fifo limits being changes?  If so, in what way, and why does that
> help.
> 
> You have to describe what you are doing. Short and non-informative commit
> log messages alongside random changes to undocumented magic constant
> registers is simply unacceptable.
> 

These series patch is an alignment with our latest hardware pcie ephy 
parameters. I will try to explain more on my next patch.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next 0/9] r8169:update hardware ephy parameter

[Hau]

 From: David Miller [mailto:da...@davemloft.net]
 Sent: Wednesday, December 10, 2014 7:36 AM
 To: Hau
 Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH net-next 0/9] r8169:update hardware ephy parameter
 
 From: Chunhao Lin h...@realtek.com
 Date: Wed, 10 Dec 2014 00:45:54 +0800
 
  Update hardware ephy parameter to improve pcie compatibility.
 
 This really doesn't tell me anything, I really dislike patch series like this 
 one.
 
 All of the programming is magic values to magic offsets.
 
 You aren't even trying to describe in the commit log message exactly what
 kind of settings are being changed, and exactly how those changes achieve
 the stated goal.
 
 Furthermore, the commit description makes no sense at all to me.
 
 How can programming the ethernet MAC PHY have any influence on PCI-E
 bus compatability?  Or are you programming the PCI bus interface's PHY?
 
 In what way are you adjusting which settings and in what way do those
 adjustments help improve PCI-E bus behavior?
 
 You absolutely must describe exactly what the new programming is actually
 doing, precisely, and in detail.  I want to know if some kind of timings are
 being adjusted, and in what way.
 Are some fifo limits being changes?  If so, in what way, and why does that
 help.
 
 You have to describe what you are doing. Short and non-informative commit
 log messages alongside random changes to undocumented magic constant
 registers is simply unacceptable.
 

These series patch is an alignment with our latest hardware pcie ephy 
parameters. I will try to explain more on my next patch.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next] r8169:add support for RTL8168EP

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Tuesday, October 07, 2014 6:13 AM
> To: Hau
> Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
> 
> Hau  :
> [...]
> > Do you mean I should collect similar hardware parameters setting into
> > one function ? or I should set hardware parameters according to
> > hardware feature support version?
> 
> static void r8168dp_ocp_write(...)
> [...]
> 
> static void r8168ep_ocp_write(...)
> [...]
> 
> static void ocp_write(...)
> {
>   switch (...
>   case ...
>   r8168dp_ocp_write
> 
>   case ...
>   r8168ep_ocp_write
> [...]
> 
> static void rtl8168dp_driver_start(...)
> [...]
> 
> static void rtl8168ep_driver_start(...)
> [...]
> 
> etc.
> 
> Nothing more. At some point the helpers themselves may turn into data
> struct members. Things don't need to be immediately right - if ever.
> However you really want to avoid unrelated changes in your patches:
> shuffling code and changing features at the same time hurts reviews, late
> regression hunts, backports, etc.
> 
> --
> Ueimor
> 

Thanks, I will do that on later patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next] r8169:add support for RTL8168EP

[Hau]

> -Original Message-
> From: Francois Romieu [mailto:rom...@fr.zoreil.com]
> Sent: Saturday, October 04, 2014 4:33 AM
> To: Hau
> Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
> 
> Chun-Hao Lin  :
> [...]
> > diff --git a/drivers/net/ethernet/realtek/r8169.c
> > b/drivers/net/ethernet/realtek/r8169.c
> > index 54476ba..3efdf4d 100644
> > --- a/drivers/net/ethernet/realtek/r8169.c
> > +++ b/drivers/net/ethernet/realtek/r8169.c
> [...]
> > @@ -1276,6 +1273,52 @@ static void rtl_w0w1_eri(struct rtl8169_private
> *tp, int addr, u32 mask, u32 p,
> > rtl_eri_write(tp, addr, mask, (val & ~m) | p, type);  }
> >
> > +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) {
> > +   void __iomem *ioaddr = tp->mmio_addr;
> > +
> > +   switch (tp->mac_version) {
> > +   case RTL_GIGA_MAC_VER_27:
> > +   case RTL_GIGA_MAC_VER_28:
> > +   case RTL_GIGA_MAC_VER_31:
> > +   RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> > +   return rtl_udelay_loop_wait_high(tp, _ocpar_cond, 100,
> 20)
> > +   ? RTL_R32(OCPDR) : ~0;
> 
> '?' should be on the previous line. There isn't more room than needed but it's
> still ok.
> 
> [...]
> > +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) {
> [...]
> > +   rtl_w0w1_phy(tp, 0x14, 0x7C00, ~0x7Cff);
> 
> Nit:  rtl_w0w1_phy(tp, 0x14, 0x7c00, ~0x7cff);
> 
> [...]
> + rtl_writephy(tp, 0x1f, 0x0bC8);
> 
> Nit:  rtl_writephy(tp, 0x1f, 0x0bc8);
> 
> Tangent: please find feature wise identical code below without moves.
> I have avoided introducing more helpers that may have kept the code more
> balanced for 8168dp vs 8168ep as it wasn't the point and it is still possible 
> to
> change the code from there.
> 
> It provides a different review experience. Feel free to give it a thought.

Do you mean I should collect similar hardware parameters setting into one 
function?
or
I should set hardware parameters according to hardware feature support version?
For example,
If mac_version == RTL8168DP
dash_ver = 1
else If mac_version == RTL8168DP
dash_ver = 2

if dashver == 1
...
else if dashver == 2
...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next] r8169:add support for RTL8168EP

[Hau]

 -Original Message-
 From: Francois Romieu [mailto:rom...@fr.zoreil.com]
 Sent: Tuesday, October 07, 2014 6:13 AM
 To: Hau
 Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
 
 Hau h...@realtek.com :
 [...]
  Do you mean I should collect similar hardware parameters setting into
  one function ? or I should set hardware parameters according to
  hardware feature support version?
 
 static void r8168dp_ocp_write(...)
 [...]
 
 static void r8168ep_ocp_write(...)
 [...]
 
 static void ocp_write(...)
 {
   switch (...
   case ...
   r8168dp_ocp_write
 
   case ...
   r8168ep_ocp_write
 [...]
 
 static void rtl8168dp_driver_start(...)
 [...]
 
 static void rtl8168ep_driver_start(...)
 [...]
 
 etc.
 
 Nothing more. At some point the helpers themselves may turn into data
 struct members. Things don't need to be immediately right - if ever.
 However you really want to avoid unrelated changes in your patches:
 shuffling code and changing features at the same time hurts reviews, late
 regression hunts, backports, etc.
 
 --
 Ueimor
 

Thanks, I will do that on later patch.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next] r8169:add support for RTL8168EP

[Hau]

 -Original Message-
 From: Francois Romieu [mailto:rom...@fr.zoreil.com]
 Sent: Saturday, October 04, 2014 4:33 AM
 To: Hau
 Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
 
 Chun-Hao Lin h...@realtek.com :
 [...]
  diff --git a/drivers/net/ethernet/realtek/r8169.c
  b/drivers/net/ethernet/realtek/r8169.c
  index 54476ba..3efdf4d 100644
  --- a/drivers/net/ethernet/realtek/r8169.c
  +++ b/drivers/net/ethernet/realtek/r8169.c
 [...]
  @@ -1276,6 +1273,52 @@ static void rtl_w0w1_eri(struct rtl8169_private
 *tp, int addr, u32 mask, u32 p,
  rtl_eri_write(tp, addr, mask, (val  ~m) | p, type);  }
 
  +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) {
  +   void __iomem *ioaddr = tp-mmio_addr;
  +
  +   switch (tp-mac_version) {
  +   case RTL_GIGA_MAC_VER_27:
  +   case RTL_GIGA_MAC_VER_28:
  +   case RTL_GIGA_MAC_VER_31:
  +   RTL_W32(OCPAR, ((u32)mask  0x0f)  12 | (reg  0x0fff));
  +   return rtl_udelay_loop_wait_high(tp, rtl_ocpar_cond, 100,
 20)
  +   ? RTL_R32(OCPDR) : ~0;
 
 '?' should be on the previous line. There isn't more room than needed but it's
 still ok.
 
 [...]
  +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) {
 [...]
  +   rtl_w0w1_phy(tp, 0x14, 0x7C00, ~0x7Cff);
 
 Nit:  rtl_w0w1_phy(tp, 0x14, 0x7c00, ~0x7cff);
 
 [...]
 + rtl_writephy(tp, 0x1f, 0x0bC8);
 
 Nit:  rtl_writephy(tp, 0x1f, 0x0bc8);
 
 Tangent: please find feature wise identical code below without moves.
 I have avoided introducing more helpers that may have kept the code more
 balanced for 8168dp vs 8168ep as it wasn't the point and it is still possible 
 to
 change the code from there.
 
 It provides a different review experience. Feel free to give it a thought.

Do you mean I should collect similar hardware parameters setting into one 
function?
or
I should set hardware parameters according to hardware feature support version?
For example,
If mac_version == RTL8168DP
dash_ver = 1
else If mac_version == RTL8168DP
dash_ver = 2

if dashver == 1
...
else if dashver == 2
...

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next 01/10] r8169:change uppercase numbertolowercase nubmer

[Hau]

I will do that next time.

Thanks.

-Original Message-
From: David Miller [mailto:da...@davemloft.net] 
Sent: Thursday, October 02, 2014 3:35 AM
To: Hau
Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net-next 01/10] r8169:change uppercase number 
tolowercase nubmer

From: Chun-Hao Lin 
Date: Wed, 1 Oct 2014 23:17:12 +0800

> Signed-off-by: Chun-Hao Lin 

Series applied, and I fixed the typo in the Subject line here.

But you _(REALLY)_ need to provide a [PATCH net-next 00/xx] posting at the 
beginning of the patch series which gives an overview of what the patch series 
does at a high level, and why.

This also allows me to have a sensible post to reply to when I just need to say 
what I've done with the entire series rather than providing comments on a 
specific patch.

Thanks.

--Please consider the environment before printing this e-mail.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 net-next 01/10] r8169:change uppercase numbertolowercase nubmer

[Hau]

I will do that next time.

Thanks.

-Original Message-
From: David Miller [mailto:da...@davemloft.net] 
Sent: Thursday, October 02, 2014 3:35 AM
To: Hau
Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net-next 01/10] r8169:change uppercase number 
tolowercase nubmer

From: Chun-Hao Lin h...@realtek.com
Date: Wed, 1 Oct 2014 23:17:12 +0800

 Signed-off-by: Chun-Hao Lin h...@realtek.com

Series applied, and I fixed the typo in the Subject line here.

But you _(REALLY)_ need to provide a [PATCH net-next 00/xx] posting at the 
beginning of the patch series which gives an overview of what the patch series 
does at a high level, and why.

This also allows me to have a sensible post to reply to when I just need to say 
what I've done with the entire series rather than providing comments on a 
specific patch.

Thanks.

--Please consider the environment before printing this e-mail.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] r8169:add support for RTL8168EP

[Hau]

I will spilt this patch and submit again.


Thanks,
Hau

-Original Message-
From: Francois Romieu [mailto:rom...@fr.zoreil.com] 
Sent: Sunday, September 28, 2014 4:39 AM
To: Hau
Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] r8169:add support for RTL8168EP

Chun-Hao Lin  :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
> 
> This patch add support for this chip.

It does more than that.

Did Hayes review it ?

> Signed-off-by: Chun-Hao Lin 
> ---
>  drivers/net/ethernet/realtek/r8169.c | 611 
> +--
>  1 file changed, 514 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c 
> b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
>   RTL_GIGA_MAC_VER_46,
>   RTL_GIGA_MAC_VER_47,
>   RTL_GIGA_MAC_VER_48,
> + RTL_GIGA_MAC_VER_49,
> + RTL_GIGA_MAC_VER_50,
> + RTL_GIGA_MAC_VER_51,
>   RTL_GIGA_MAC_NONE   = 0xff,
>  };
>  
> @@ -302,6 +305,15 @@ static const struct {
>   [RTL_GIGA_MAC_VER_48] =
>   _R("RTL8107e",  RTL_TD_1, FIRMWARE_8107E_2,
>   JUMBO_1K, false),
> + [RTL_GIGA_MAC_VER_49] =
> + _R("RTL8168ep/8111ep",  RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_50] =
> + _R("RTL8168ep/8111ep",  RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_51] =
> + _R("RTL8168ep/8111ep",  RTL_TD_1, NULL,
> + JUMBO_9K, false),
>  };
>  #undef _R
>  
> @@ -400,6 +412,10 @@ enum rtl_registers {
>   FuncEvent   = 0xf0,
>   FuncEventMask   = 0xf4,
>   FuncPresetState = 0xf8,
> + IBCR0   = 0xf8,
> + IBCR2   = 0xf9,
> + IBIMR0  = 0xfa,
> + IBISR0  = 0xfb,
>   FuncForceEvent  = 0xfc,
>  };
>  
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
>  #define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_MSIX   (0x01 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_ASF(0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB(0x02 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_MASK_SHIFT 12
>  #define ERIAR_MASK_0001  (0x1 << ERIAR_MASK_SHIFT)
>  #define ERIAR_MASK_0011  (0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = {   
> \
>   \
>  static bool name ## _check(struct rtl8169_private *tp)
>  
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate patch. 
You are adding extra noise and thus making this stuff harder to review than it 
should be.

> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - return rtl_udelay_loop_wait_high(tp, _ocpar_cond, 100, 20) ?
> - RTL_R32(OCPDR) : ~0;
> -}

(this one is modified)

> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, 
> u32 data) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPDR, data);
> - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - rtl_udelay_loop_wait_low(tp, _ocpar_cond, 100, 20);
> -}

(this one is modified)

> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate patch.

> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W8(ERIDR, cmd);
> - RTL_W32(ERIAR, 0x800010e8);
> - msleep(2);
> -
> - if (!rtl_udelay_loop_wait_low(tp, _eriar_cond, 100, 5))
> - return;
> -
> - ocp_write(tp, 0x1, 0x30, 0x0001);
> -}

This one is modified but it is

RE: [PATCH net-next] r8169:add support for RTL8168EP

[Hau]

I will spilt this patch and submit again.


Thanks,
Hau

-Original Message-
From: Francois Romieu [mailto:rom...@fr.zoreil.com] 
Sent: Sunday, September 28, 2014 4:39 AM
To: Hau
Cc: net...@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] r8169:add support for RTL8168EP

Chun-Hao Lin h...@realtek.com :
 RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
 It is a successor chip of RTL8168DP.
 
 This patch add support for this chip.

It does more than that.

Did Hayes review it ?

 Signed-off-by: Chun-Hao Lin h...@realtek.com
 ---
  drivers/net/ethernet/realtek/r8169.c | 611 
 +--
  1 file changed, 514 insertions(+), 97 deletions(-)
 
 diff --git a/drivers/net/ethernet/realtek/r8169.c 
 b/drivers/net/ethernet/realtek/r8169.c
 index 1d81238..0ead9a7 100644
 --- a/drivers/net/ethernet/realtek/r8169.c
 +++ b/drivers/net/ethernet/realtek/r8169.c
 @@ -155,6 +155,9 @@ enum mac_version {
   RTL_GIGA_MAC_VER_46,
   RTL_GIGA_MAC_VER_47,
   RTL_GIGA_MAC_VER_48,
 + RTL_GIGA_MAC_VER_49,
 + RTL_GIGA_MAC_VER_50,
 + RTL_GIGA_MAC_VER_51,
   RTL_GIGA_MAC_NONE   = 0xff,
  };
  
 @@ -302,6 +305,15 @@ static const struct {
   [RTL_GIGA_MAC_VER_48] =
   _R(RTL8107e,  RTL_TD_1, FIRMWARE_8107E_2,
   JUMBO_1K, false),
 + [RTL_GIGA_MAC_VER_49] =
 + _R(RTL8168ep/8111ep,  RTL_TD_1, NULL,
 + JUMBO_9K, false),
 + [RTL_GIGA_MAC_VER_50] =
 + _R(RTL8168ep/8111ep,  RTL_TD_1, NULL,
 + JUMBO_9K, false),
 + [RTL_GIGA_MAC_VER_51] =
 + _R(RTL8168ep/8111ep,  RTL_TD_1, NULL,
 + JUMBO_9K, false),
  };
  #undef _R
  
 @@ -400,6 +412,10 @@ enum rtl_registers {
   FuncEvent   = 0xf0,
   FuncEventMask   = 0xf4,
   FuncPresetState = 0xf8,
 + IBCR0   = 0xf8,
 + IBCR2   = 0xf9,
 + IBIMR0  = 0xfa,
 + IBISR0  = 0xfb,
   FuncForceEvent  = 0xfc,
  };
  
 @@ -467,6 +483,7 @@ enum rtl8168_registers {
  #define ERIAR_EXGMAC (0x00  ERIAR_TYPE_SHIFT)
  #define ERIAR_MSIX   (0x01  ERIAR_TYPE_SHIFT)
  #define ERIAR_ASF(0x02  ERIAR_TYPE_SHIFT)
 +#define ERIAR_OOB(0x02  ERIAR_TYPE_SHIFT)
  #define ERIAR_MASK_SHIFT 12
  #define ERIAR_MASK_0001  (0x1  ERIAR_MASK_SHIFT)
  #define ERIAR_MASK_0011  (0x3  ERIAR_MASK_SHIFT)
 @@ -935,93 +952,10 @@ static const struct rtl_cond name = {   
 \
   \
  static bool name ## _check(struct rtl8169_private *tp)
  
 -DECLARE_RTL_COND(rtl_ocpar_cond)
 -{
 - void __iomem *ioaddr = tp-mmio_addr;
 -
 - return RTL_R32(OCPAR)  OCPAR_FLAG;
 -}

Feel free to move this function around but please do it in a separate patch. 
You are adding extra noise and thus making this stuff harder to review than it 
should be.

 -
 -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) -{
 - void __iomem *ioaddr = tp-mmio_addr;
 -
 - RTL_W32(OCPAR, ((u32)mask  0x0f)  12 | (reg  0x0fff));
 -
 - return rtl_udelay_loop_wait_high(tp, rtl_ocpar_cond, 100, 20) ?
 - RTL_R32(OCPDR) : ~0;
 -}

(this one is modified)

 -
 -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, 
 u32 data) -{
 - void __iomem *ioaddr = tp-mmio_addr;
 -
 - RTL_W32(OCPDR, data);
 - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask  0x0f)  12 | (reg  0x0fff));
 -
 - rtl_udelay_loop_wait_low(tp, rtl_ocpar_cond, 100, 20);
 -}

(this one is modified)

 -
 -DECLARE_RTL_COND(rtl_eriar_cond)
 -{
 - void __iomem *ioaddr = tp-mmio_addr;
 -
 - return RTL_R32(ERIAR)  ERIAR_FLAG;
 -}

Feel free to move this function around but please do it in a separate patch.

 -
 -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd) -{
 - void __iomem *ioaddr = tp-mmio_addr;
 -
 - RTL_W8(ERIDR, cmd);
 - RTL_W32(ERIAR, 0x800010e8);
 - msleep(2);
 -
 - if (!rtl_udelay_loop_wait_low(tp, rtl_eriar_cond, 100, 5))
 - return;
 -
 - ocp_write(tp, 0x1, 0x30, 0x0001);
 -}

This one is modified but it is also modified for the existing 8168DP.

Mantra: please do it in a separate patch.

 -
  #define OOB_CMD_RESET0x00
  #define OOB_CMD_DRIVER_START 0x05
  #define OOB_CMD_DRIVER_STOP  0x06
  
 -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp) -{
 - return (tp-mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
 -}
 -
 -DECLARE_RTL_COND(rtl_ocp_read_cond)
 -{
 - u16 reg;
 -
 - reg = rtl8168_get_ocp_reg(tp);
 -
 - return ocp_read(tp, 0x0f, reg)  0x0800;
 -}

(this one is simply moved around)

 -
 -static void rtl8168_driver_start

Memory maps

[Fu-hau Hsu]

Dear freinds:

 I have following questions about memory maps. I appreciate any
suggestion.

 Q. (1)When a process is running, how can I get the range of data, stack,
   and code segments, say the stack segment is from address 0x. to
   0x. so do data segments and code segments?
   PS: Under ELF format, there are several seperaed code and data
   segments, but the process control table has only one pair of
   pointers for each, Are the pointers still useful?

(2) /proc/*/maps will show us those info, but how does it get these
   info? 


FuHau 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Memory maps

[Fu-hau Hsu]

Dear freinds:

 I have following questions about memory maps. I appreciate any
suggestion.

 Q. (1)When a process is running, how can I get the range of data, stack,
   and code segments, say the stack segment is from address 0x. to
   0x. so do data segments and code segments?
   PS: Under ELF format, there are several seperaed code and data
   segments, but the process control table has only one pair of
   pointers for each, Are the pointers still useful?

(2) /proc/*/maps will show us those info, but how does it get these
   info? 


FuHau 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

code region

[Fu-hau Hsu]

Dear friends:

 I have a question about memory protection. I appreciate any suggestion. 
 Thank you so much.

Given a virtual address, how can we know whether this address contains
an executable code? If there is a method that can be used to answer
the above question, is there any exception for this method?

PS:
(a)Could we get the result by checking the VM_EXECUTABLE attribute of
the vm_flags of the vm_area_struct for the memory area that contains
that address? If yes, does this apply to x86 architecture?

(b) Could information in vm_page_prot of vm_area_struct or in
struct mem_map_t help? If yes, which attribute and how?

  Best Regards,

FuHau   

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

code region

[Fu-hau Hsu]

Dear friends:

 I have a question about memory protection. I appreciate any suggestion. 
 Thank you so much.

Given a virtual address, how can we know whether this address contains
an executable code? If there is a method that can be used to answer
the above question, is there any exception for this method?

PS:
(a)Could we get the result by checking the VM_EXECUTABLE attribute of
the vm_flags of the vm_area_struct for the memory area that contains
that address? If yes, does this apply to x86 architecture?

(b) Could information in vm_page_prot of vm_area_struct or in
struct mem_map_t help? If yes, which attribute and how?

  Best Regards,

FuHau   

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/