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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5FC9DCCF9EC for ; Wed, 25 Sep 2024 19:42:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WjdEpakeAEueAFF38g825g9x6ZCC7yRWFkL/TDSCzf4=; b=jfYc7kOHApP/PdOp8DkW20aryG 7dm5HdI8pc1eUsAn5EVbCaobQdDdTjaSOEGmHL8tfygRuBaXaP+xvlFx9/w3fNYfA06yd8OXTExNr j0KzfYG4h3F09pwPjfcqAC2ELPn06DPMt462yzlBabEK+fwHNSaVGoDWsEFKrz6RUx2bgRVtgb6tK 2kNdAONpQf1gj45+PiE0eR/i1dacjxQoBRQULYB//NEOVRxlBDFJgsnMXzCNm5W5p7UvfRxwbwEbq xoXk61/4ui32qYn/hzlVc6sFpFwjetQlr+RwtMimwGee5Mh8DvN8L3gvAmChIRggD5KhZpElQiU/T M245nFMQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1stXtj-00000006QUX-2rCi; Wed, 25 Sep 2024 19:42:03 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1stXsZ-00000006Q38-3jpJ for linux-arm-kernel@lists.infradead.org; Wed, 25 Sep 2024 19:40:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=WjdEpakeAEueAFF38g825g9x6ZCC7yRWFkL/TDSCzf4=; b=dB4xz0D+ZLsZ8rRZRuHk2g7f0F om8LUD3GKpWwh2Fs3G+4EdP2q5XG9Yz8gjo2G7DEG1f4Bd1JMACALUonuEavPkLlVPPyA1xG2FS6I YcEPo0BdJgmWGPwQAGeRzveSDy9jrICmq+bDQ5kiaARL9eHZKgJwsXB2kqeA7motkxvr86YlPHUYE QdcJaeLGrMUYxDdgqpMGAjohUyzfCRInu5FqJo04rczLL1VDqd6fXb7t9VEUUJlPY6rEujKppCpu5 9t7/oMgI0pqFFUtir6y1iPINXshZIY6vIePXkf3jyA+fmiGxzikOGI4H37jYDSI/nMe4GenncV4Zn fzc2N26g==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:35972) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1stXqJ-0007AJ-1I; Wed, 25 Sep 2024 20:38:31 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.96) (envelope-from ) id 1stXqB-0007bY-1T; Wed, 25 Sep 2024 20:38:23 +0100 Date: Wed, 25 Sep 2024 20:38:23 +0100 From: "Russell King (Oracle)" To: Vladimir Oltean Cc: Andrew Lunn , Heiner Kallweit , Alexandre Torgue , "David S. Miller" , Eric Dumazet , Florian Fainelli , Jakub Kicinski , Jiawen Wu , Jose Abreu , Jose Abreu , linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, Maxime Coquelin , Mengyuan Lou , netdev@vger.kernel.org, Paolo Abeni Subject: Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload Message-ID: References: <20240925131517.s562xmc5ekkslkhp@skbuf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240925131517.s562xmc5ekkslkhp@skbuf> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240925_124051_955813_425FFD3E X-CRM114-Status: GOOD ( 33.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 25, 2024 at 04:15:17PM +0300, Vladimir Oltean wrote: > On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote: > > +static int sja1105_set_port_speed(struct sja1105_private *priv, int port, > > + int speed_mbps) > > { > > struct sja1105_mac_config_entry *mac; > > - struct device *dev = priv->ds->dev; > > I think if you could keep this line in the new sja1105_set_port_speed() > function.. > > > u64 speed; > > - int rc; > > > > /* On P/Q/R/S, one can read from the device via the MAC reconfiguration > > * tables. On E/T, MAC reconfig tables are not readable, only writable. > > @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > > speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; > > break; > > default: > > - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); > > + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps); > > you could also get rid of this unnecessary line change. Yes, maybe I'll move it to another patch, but the reason I made the change is that I don't see much point to the local variable existing for just one user (there were multiple users prior to this patch.) However... > > return -EINVAL; > > } > > There are 2 more changes which I believe should be made in sja1105_set_port_speed(): > - since it isn't called from mac_config() anymore but from mac_link_up() > (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN > - we can trust that phylink will not call mac_link_up() with a speed > outside what we provided in mac_capabilities, so we can remove the > -EINVAL "default" speed_mbps case, and make this method return void, > as it can never truly cause an error > > But I believe these are incremental changes which should be done after > this patch. I've made a note of them and will create 2 patches on top > when I have the spare time. ... if we were to make those changes prior to this patch, then the dev_err() will no longer be there and thus this becomes a non-issue. So I'd suggest a patch prior to this one to make the changes you state here, thus eliminating the need for this hunk in this patch. > > +/* Set link speed in the MAC configuration for a specific port. */ > > Could this comment state this instead? "Write the MAC Configuration > Table entry and, if necessary, the CGU settings, after a link speed > change for this port." Done. > > @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > > { > > struct ptp_system_timestamp ptp_sts_before; > > struct ptp_system_timestamp ptp_sts_after; > > - int speed_mbps[SJA1105_MAX_NUM_PORTS]; > > + u64 mac_speed[SJA1105_MAX_NUM_PORTS]; > > Could you move this line lower to preserve the ordering by decreasing line length? > > > u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0}; Probably didn't notice that due to not having full clear sight for the screen yet (a few more weeks to go before that happens!) Thanks for spotting that. > > - /* Back up the dynamic link speed changed by sja1105_adjust_port_config > > + /* Back up the dynamic link speed changed by sja1105_set_port_speed > > Could you please put () after the function name? I know the original > code did not have it, but my coding style has changed in the past 5 years. Done. Thanks! -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!