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=-2.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 2F3FDC433DF for ; Wed, 17 Jun 2020 11:16:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 05FAC20734 for ; Wed, 17 Jun 2020 11:16:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=intenta.de header.i=@intenta.de header.b="reENZBcU" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725901AbgFQLQC (ORCPT ); Wed, 17 Jun 2020 07:16:02 -0400 Received: from mail.intenta.de ([178.249.25.132]:42855 "EHLO mail.intenta.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725554AbgFQLPn (ORCPT ); Wed, 17 Jun 2020 07:15:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=intenta.de; s=dkim1; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:CC:To:From:Date; bh=9ms7Wgh1QZ7oVZDkbZOFznET7IscPNYrYLXyEq5EI0I=; b=reENZBcUqQ2gxFcRWzyrJehly+o2ry/h/0jmIPsqWArDkYukmdkzL2FWjRKXQmK6GhzwH1qCGBRZ8UTLw+a76xnzU1J33g8zwoSYDBUaJn0qrTVhx9AlRCY6dFbvXjazcINdnNpK6uMdV2dW7DWstHyO1wKsiwf6jOIhqsbeVx6+UsS8zuGNzxm/S03O7XmWS/vXmGWsildLxHtyLlKqGvZK1yNazW0Q8Ry7d/YTbXyCNxC7Izomu+zEQl3P5iyJkukmFyGgBPhQs8Aohfl0lRIqVIcxZQsjZgrwWgkUWdQ3PGVRYSaIVN30t41hptQK1qP1f4MnnA1ERnDVS8sdOQ==; Date: Wed, 17 Jun 2020 13:15:32 +0200 From: Helmut Grohne To: Vladimir Oltean CC: Nicolas Ferre , "David S. Miller" , Jakub Kicinski , Russell King , Palmer Dabbelt , Paul Walmsley , netdev Subject: Re: [PATCH] net: macb: reject unsupported rgmii delays Message-ID: <20200617111532.GA28783@laureti-dev> References: <20200616074955.GA9092@laureti-dev> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-ClientProxiedBy: ICSMA002.intenta.de (10.10.16.48) To ICSMA002.intenta.de (10.10.16.48) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Vladimir, On Wed, Jun 17, 2020 at 11:57:23AM +0200, Vladimir Oltean wrote: > On Tue, 16 Jun 2020 at 11:00, Helmut Grohne wrote: > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config, > > state->interface != PHY_INTERFACE_MODE_RMII && > > state->interface != PHY_INTERFACE_MODE_GMII && > > state->interface != PHY_INTERFACE_MODE_SGMII && > > - !phy_interface_mode_is_rgmii(state->interface)) { > > + state->interface != PHY_INTERFACE_MODE_RGMII_ID) { > > I don't think this change is correct though? > What if there were PCB traces in place, for whatever reason? Then the > driver would need to accept a phy with rgmii-txid, rgmii-rxid or > rgmii. I must confess that after rereading the discussion and the other discussions at https://patchwork.ozlabs.org/project/netdev/patch/20190410005700.31582-19-olteanv@gmail.com/ and https://patchwork.ozlabs.org/project/netdev/patch/20190413012822.30931-21-olteanv@gmail.com/, this is less and less clear to me. I think we can agree that the current definition of phy-mode is confusing when it comes to rgmii delays. The documentation doesn't even mention the possibility of using serpentines. Your interpretation seems to be that this value is solely meant for the PHY to operate on and that the MAC should not act upon it (at least the delay part). That interpetation is consistent with previous discussions and mostly consistent with the documentation. The phy-mode property is documented in ethernet-controller.yaml, which suggests that it should apply to the MAC somehow. Following this interpretation, I think it would be good to also document it in ethernet-phy.yaml. Thank you for the review. I agree that the hunk should be dropped. However, in the fixed-link case (below) the interpretation regarding serpentines seems to be well-defined: If serpentines are present, both MACs should specify rgmii. > > bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); > > return; > > } > > @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp) > > struct phy_device *phydev; > > int ret; > > > > + if (of_phy_is_fixed_link(dn) && > > + phy_interface_mode_is_rgmii(bp->phy_interface) && > > + bp->phy_interface != PHY_INTERFACE_MODE_RGMII) { > > + netdev_err(dev, "RGMII delays are not supported\n"); > > + return -EINVAL; > > + } > > + > > Have you checked that this doesn't break any existing in-tree users? It seems like all the in-tree users of this driver that do specify a phy-mode, specify rmii. If possible breakage is to be minimized, I'd be happy with using netdev_warn instead. The major benefit here is getting a clear indication of why things don't work. My emphasis is on getting something to dmesg, not to make things fail. So what should we prefer here? Failure or warning? In the long run, it would be nice to refactor the way to denote delays in a way that is consistently defined for MAC to PHY and MAC to MAC connections while accounting for serpentines. Helmut