linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Francois Romieu <romieu@fr.zoreil.com>
To: hayeswang <hayeswang@realtek.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 1/2] r8169: enable ALDPS for power saving
Date: Tue, 23 Oct 2012 21:31:46 +0200	[thread overview]
Message-ID: <20121023193146.GA10463@electric-eye.fr.zoreil.com> (raw)
In-Reply-To: <D81B36B929404E28AE7007B7E21B9D46@realtek.com.tw>

hayeswang <hayeswang@realtek.com> :
[...]
> The flag is determined if the firmware is loaded. It doesn't mean to enable
> ALDPS. Besides ALDPS, the firmware includes the other features about power
> saving, performance, hw behavior, and so on. Thus, I think it  is the extended
> feature. Any suggestion?

RTL_FEATURE_FW_LOADED

[...]
> I don't sure if the firmware code still exists and works after hibernation. I
> clear the flag for safe. It would be set again after loading firmware. It is
> used to make sure to enable ALDPS with firmware.

I don't understand if you want to avoid a coding bug or a hardware problem.

The code only enables ALDPS in the hw_phy_config helpers. Said helpers are
the only places where firmware loading is requested. Loading the firmware
always set RTL_FEATURE_XYZ.

Whatever you are trying to achieve, the change in rtl8169_net_suspend
seem rather useless. Please explain.

(if you want to enforce that ALDPS does nothing when it is not called
right after firmware loading, the ALDPS enable method itself is imho the
natural place to reset the bit).

[...]
> > After this patch the driver would look like:
> > 1. disable ALDPS before setting firmware (unmodified by patch)
> >    RTL_GIGA_MAC_VER_29 "RTL8105e"
> >    RTL_GIGA_MAC_VER_30 "RTL8105e"
> >    RTL_GIGA_MAC_VER_37 "RTL8402"
> >    RTL_GIGA_MAC_VER_39 "RTL8106e"
> > 
> > 2. apply_firmware (unmodified by patch)
> >    RTL_GIGA_MAC_VER_25 "RTL8168d/8111d"
> >    RTL_GIGA_MAC_VER_26 "RTL8168d/8111d"
> >    RTL_GIGA_MAC_VER_29 "RTL8105e"
> >    RTL_GIGA_MAC_VER_30 "RTL8105e"
> >    RTL_GIGA_MAC_VER_32 "RTL8168e/8111e"
> >    RTL_GIGA_MAC_VER_33 "RTL8168e/8111e"
> >    RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl"
> >    RTL_GIGA_MAC_VER_35 "RTL8168f/8111f"
> >    RTL_GIGA_MAC_VER_36 "RTL8168f/8111f"
> >    RTL_GIGA_MAC_VER_37 "RTL8402"
> >    RTL_GIGA_MAC_VER_38 "RTL8411"
> >    RTL_GIGA_MAC_VER_39 "RTL8106e"
> >    RTL_GIGA_MAC_VER_40 "RTL8168g/8111g"
> > 
> > 3. enable ALDPS after firmware
> >    RTL_GIGA_MAC_VER_34 "RTL8168evl/8111evl"
> >    RTL_GIGA_MAC_VER_35 "RTL8168f/8111f"
> >    RTL_GIGA_MAC_VER_36 "RTL8168f/8111f"
> >    RTL_GIGA_MAC_VER_37 "RTL8402"
> >    RTL_GIGA_MAC_VER_38 "RTL8411"
> >    RTL_GIGA_MAC_VER_39 "RTL8106e"
> >    RTL_GIGA_MAC_VER_40 "RTL8168g/8111g"
> > 
> > The disable/enable ALDPS code is not trivially balanced.
> > 
> > Do we exactly perform the required ALDPS operations ? Nothing more,
> > nothing less ?
> > 
> 
> Because the different design between the giga nic and 10/100M nic, the behavior
> would be different. For example, you couldn't disable the ALDPS of the giga nic
> directly just like the 10/100M (8136 series) one. The giga nic would disable
> ALDPS automatically when loading firmware, but the 8136 series wouldn't.

It would be nice to state these things in the commit message, namely:
- ALDPS should never be enabled for the RTL8105e
- none of the firmware-free chipsets support ALDPS
- neither do the RTL8168d/8111d

-- 
Ueimor

  reply	other threads:[~2012-10-23 19:53 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-22  8:05 [PATCH net-next 1/2] r8169: enable ALDPS for power saving Hayes Wang
2012-10-22 17:59 ` David Miller
2012-10-22 19:27 ` Francois Romieu
2012-10-23  2:24   ` hayeswang
2012-10-23 19:31     ` Francois Romieu [this message]
2012-10-24  5:55       ` hayeswang
2012-10-24 20:05         ` Francois Romieu
2012-10-23  6:47 ` [PATCH v2 " Hayes Wang
2012-10-23  6:47   ` [PATCH v2 net-next 2/2] r8169: update the settings for RTL8111G Hayes Wang
2012-10-23 19:41     ` Francois Romieu
2012-10-23 19:33   ` [PATCH v2 net-next 1/2] r8169: enable ALDPS for power saving Francois Romieu
2012-10-24  5:55     ` hayeswang
2012-10-24  6:24 ` [PATCH v3 net-next] " Hayes Wang
2012-10-24 21:20   ` Francois Romieu
2012-10-26  6:15     ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121023193146.GA10463@electric-eye.fr.zoreil.com \
    --to=romieu@fr.zoreil.com \
    --cc=hayeswang@realtek.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).