From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD252C433F5 for ; Wed, 22 Sep 2021 23:56:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AB9236115A for ; Wed, 22 Sep 2021 23:56:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238585AbhIVX57 (ORCPT ); Wed, 22 Sep 2021 19:57:59 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:55406 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238631AbhIVX55 (ORCPT ); Wed, 22 Sep 2021 19:57:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=zqD1G9bG2/r+O+hBeEr/nyWacbzUh1+NrgW2dIOn62o=; b=1xakWK6plu+TLbKlULUR5cvP+f xyie3mAVuMLRtzsdyRy1BwzcS5/uE1o15ssAXDaPyKtI8e26nRfK/eeR25djg/eC2dcklDgmR7Xi+ LwlolTuo9WVfJ+jdu+3Z6jkA/JFFr2oPHl0swSCIlgWfQzOZGclI8OyFel8KQuY01tnM=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1mTC6A-007qe3-2X; Thu, 23 Sep 2021 01:56:22 +0200 Date: Thu, 23 Sep 2021 01:56:22 +0200 From: Andrew Lunn To: "Russell King (Oracle)" Cc: Heiner Kallweit , "David S. Miller" , netdev@vger.kernel.org, Marek Beh__n , Jakub Kicinski Subject: Re: [PATCH net-next] net: phy: marvell10g: add downshift tunable support Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Sep 22, 2021 at 01:00:31PM +0100, Russell King (Oracle) wrote: > On Fri, Sep 17, 2021 at 03:58:01PM +0100, Russell King (Oracle) wrote: > > On Fri, Sep 17, 2021 at 04:45:03PM +0200, Andrew Lunn wrote: > > > > +static int mv3310_set_downshift(struct phy_device *phydev, u8 ds) > > > > +{ > > > > + struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev); > > > > + u16 val; > > > > + int err; > > > > + > > > > + if (!priv->has_downshift) > > > > + return -EOPNOTSUPP; > > > > + > > > > + if (ds == DOWNSHIFT_DEV_DISABLE) > > > > + return phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, MV_PCS_DSC1, > > > > + MV_PCS_DSC1_ENABLE); > > > > + > > > > + /* FIXME: The default is disabled, so should we disable? */ > > > > + if (ds == DOWNSHIFT_DEV_DEFAULT_COUNT) > > > > + ds = 2; > > > > > > Hi Russell > > > > > > Rather than a FIXME, maybe just document that the hardware default is > > > disabled, which does not make too much sense, so default to 2 attempts? > > > > Sadly, the downshift parameters aren't documented at all in the kernel, > > and one has to dig into the ethtool source to find out what they mean: > > > > DOWNSHIFT_DEV_DEFAULT_COUNT - > > ethtool --set-phy-tunable ethN downshift on > > DOWNSHIFT_DEV_DISABLE - > > ethtool --set-phy-tunable ethN downshift off > > otherwise: > > ethtool --set-phy-tunable ethN downshift count N > > > > This really needs to be documented somewhere in the kernel. > > I was hoping that this would cause further discussion on what the > exact meaning of "DOWNSHIFT_DEV_DEFAULT_COUNT" is. Clearly, it's > meant to turn downshift on, but what does "default" actually mean? I guess this comes from the fact every other PHY has a bit to enable downshift, and a counter from saying how many attempts to make. And the counter has a documented default value. > If we define "default" as "whatever the hardware defaults to" then > for this phy, that would be turning off downshift. Which does not make sense. > So, should we rename "DOWNSHIFT_DEV_DEFAULT_COUNT" to be > "DOWNSHIFT_DEV_ENABLE" rather than trying to imply that it's > some kind of default that may need to be made up? The value is made up anyway. Normally the silicon vendor picks a value, and that is what you get after a reset. Does it make that much difference if in this case if you pick the value, rather than Marvell? None of this is standardised as far as i know, there is no correct value. Andrew