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 060F9C47080 for ; Tue, 1 Jun 2021 16:55:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E48EC610C9 for ; Tue, 1 Jun 2021 16:55:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233971AbhFAQ4t (ORCPT ); Tue, 1 Jun 2021 12:56:49 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:39152 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230288AbhFAQ4r (ORCPT ); Tue, 1 Jun 2021 12:56:47 -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=HUgJUFHWhFqGGVtr0GW02t4yLZoPpe5HYrZEnVB3RYs=; b=KO2bJeUOO5ItiycNjOLAH6dEbk Q2xez1L5M/44oMRO8X8dgwVxC4fHke0BPcZm9zyUf43trlKbwGobNFO3Pn/VcxbE7R9d30x88nGNA j9+iXndu2PWQUNYU9AuD4i1nAEL+Zt7thbJd35epKCiOEQSHotc3K1af2FT0bZjO5cfs=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1lo7fT-007JcI-6s; Tue, 01 Jun 2021 18:55:03 +0200 Date: Tue, 1 Jun 2021 18:55:03 +0200 From: Andrew Lunn To: Liang Xu Cc: "hkallweit1@gmail.com" , "netdev@vger.kernel.org" , "davem@davemloft.net" , "kuba@kernel.org" , "linux@armlinux.org.uk" , Hauke Mehrtens , Thomas Mohren Subject: Re: [PATCH] phy: maxlinear: add Maxlinear GPY115/21x/24x driver Message-ID: References: <20210601074427.40990-1-lxu@maxlinear.com> <050c9cd2-ba6e-332d-d235-4fa9364b461b@maxlinear.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <050c9cd2-ba6e-332d-d235-4fa9364b461b@maxlinear.com> Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > >> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > >> + phydev->supported, > >> + ret & MDIO_PMA_NG_EXTABLE_5GBT); > >> + > > Does genphy_c45_pma_read_abilities() do the wrong thing here? What > > does it get wrong? > > The problem comes from condition "phydev->c45_ids.mmds_present & > MDIO_DEVS_AN". > > Our product supports both C22 and C45. > > In the real system, we found C22 was used by customers (with indirect > access to C45 registers when necessary). > > Then during probe, in API "get_phy_device", it skips reading C45 IDs. > > So that genphy_c45_pma_read_abilities skip the supported flag > ETHTOOL_LINK_MODE_Autoneg_BIT. This sounds like a generic problem, which will affect any PHY which has both C22 and C45. I wounder if it makes sense to add a helper function which a PHY driver can call to get the phydev->c45_ids.mmds_present populated? > >> +static int gpy_read_status(struct phy_device *phydev) > >> +{ > >> + int ret; > >> + > >> + ret = genphy_update_link(phydev); > >> + if (ret) > >> + return ret; > >> + > >> + phydev->speed = SPEED_UNKNOWN; > >> + phydev->duplex = DUPLEX_UNKNOWN; > >> + phydev->pause = 0; > >> + phydev->asym_pause = 0; > >> + > >> + if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { > >> + ret = genphy_c45_read_lpa(phydev); > >> + if (ret < 0) > >> + return ret; > >> + > >> + /* Read the link partner's 1G advertisement */ > >> + ret = phy_read(phydev, MII_STAT1000); > >> + if (ret < 0) > >> + return ret; > >> + mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret); > > can genphy_read_lpa() be used here? > > 2.5G is not covered in genphy_read_lpa. > > If I use genphy_c45_read_lpa first then genphy_read_lpa after, it seems > a bit redundant. I'm just trying to avoid repeating code which is in helpers. I think this is the first PHY driver which uses a mixture of C22 and C45 like this. So it could be the helpers need small modifications to make them work. We should make those modifications, since your PHY is not likely to be the only mixed C22 and C45 device. Andrew