From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58396)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1X2MFb-0006MI-Ux
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 11:15:19 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1X2MFS-00051x-97
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 11:15:11 -0400
Received: from mx1.redhat.com ([209.132.183.28]:25048)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1X2MFR-00050x-Vu
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 11:15:02 -0400
Date: Wed, 2 Jul 2014 18:17:05 +0300
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20140702151705.GB6649@redhat.com>
References: <1404147350-28904-1-git-send-email-somlo@cmu.edu>
	<1404147350-28904-3-git-send-email-somlo@cmu.edu>
	<20140630175554.GA2871@redhat.com>
	<C0C63132-40FB-4D69-9281-57C7FBE8181E@suse.de>
	<20140702090207.GA2945@crash.ini.cmu.edu>
	<20140702093316.GA3692@redhat.com>
	<20140702142133.GW1688@ERROL.INI.CMU.EDU>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20140702142133.GW1688@ERROL.INI.CMU.EDU>
Subject: Re: [Qemu-devel] [RFC PATCH v1 2/2] e1000: adjust initial autoneg
 timing (for piix/osx)
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>, Alexander Graf <agraf@suse.de>, "stefanha@redhat.com" <stefanha@redhat.com>, "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

On Wed, Jul 02, 2014 at 10:21:34AM -0400, Gabriel L. Somlo wrote:
> On Wed, Jul 02, 2014 at 12:33:16PM +0300, Michael S. Tsirkin wrote:
> > Just poking around the spec I find more things
> > we don't implement correctly wrt to auto-negotiation.
> > For example, MII_SR_AUTONEG_CAPS isn't set, is it?
> > Maybe that's why your guest doesn't work:
> > it doesn't expect to get autonegotation at all?
> > 
> > So I have a question: does your patch actually help any guests?
> > If not, maybe we should defer it to after release,
> > and try to clean up autonegotiation more thouroughly for 2.2?
> 
> I'll re-submit after 2.1 is officially out. But, since we're talking
> about MII_SR_AUTONEG_CAPS:  PHY_STATUS is initialized to 0x794d, which
> includes setting the MII_SR_AUTONEG_CAPS bit (|= 0x8).
> Did you mean: we should check for MII_SR_AUTONEG_CAPS in have_autoneg() ?
> (i.e., on the chance it gets turned off by a guest-side write to
> PHY_STATUS) ?

PHY_STATUS isn't writeable is it?
No I just got confused with the binary math.
We really should use symbolic constants there,
we already have them defined.

> 
> Thx,
> --G
> 
> PS. Maybe also spell out the individual bits in phy_reg_init[] ? Like,
> instead of:
> 
>     [PHY_STATUS] = 0x794d,
> 
> do this:
> 
>     [PHY_STATUS] = MII_SR_EXTENDED_CAPS |
>                    MII_SR_LINK_STATUS |
>                    MII_SR_AUTONEG_CAPS | 
>                    MII_SR_PREAMBLE_SUPPRESS | 
>                    MII_SR_EXTENDED_STATUS | 
>                    MII_SR_10T_HD_CAPS | 
>                    MII_SR_10T_FD_CAPS | 
>                    MII_SR_100X_HD_CAPS | 
>                    MII_SR_100X_FD_CAPS,
> 
> ... for all registers ? Much more verbose, but IMHO that'd be a good
> thing :)
> 
>