Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Wed, 15 Nov 2023 15:24:37 +0100 (CET)
From:   Romain Gantois <romain.gantois@bootlin.com>
To:     Andrew Lunn <andrew@lunn.ch>
cc:     Romain Gantois <romain.gantois@bootlin.com>, davem@davemloft.net,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        thomas.petazzoni@bootlin.com,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        linux-arm-kernel@lists.infradead.org,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Luka Perkov <luka.perkov@sartura.hr>,
        Robert Marko <robert.marko@sartura.hr>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>
Subject: Re: [PATCH net-next v3 3/8] net: qualcomm: ipqess: introduce the
 Qualcomm IPQESS driver
In-Reply-To: <f6ce0c15-8b72-4568-8ba2-f0216db84ffd@lunn.ch>
Message-ID: <d60c1fd1-3ee1-3c15-6072-06c70f09d6d2@bootlin.com>
References: <20231114105600.1012056-1-romain.gantois@bootlin.com> <20231114105600.1012056-4-romain.gantois@bootlin.com> <f6ce0c15-8b72-4568-8ba2-f0216db84ffd@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 14 Nov 2023, Andrew Lunn wrote:
...
> > +	phy_support_asym_pause(phy);
> > +
> > +	ipqess_port_set_state_now(port, BR_STATE_FORWARDING, false);
> > +
> > +	if (port->pl)
> > +		phylink_start(port->pl);
> 
> That looks odd. You unconditionally call phy_support_asym_pause() yet
> conditionally call phylink_start(). I would expect there to always be
> a phylink instance.
> 
> Also, you should be telling phylink about the pause capabilities in
> config->mac_capabilities. It is then phylinks problem to tell the PHY,
> or the PCS driving the SFP etc about pause.
You are correct. I probably fumbled this when splitting the calibration code.

> > +		if (tx_pause || port->index == 0)
> > +			reg |= QCA8K_PORT_STATUS_TXFLOW;
> > +	}
> > +
> > +	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
> > +
> > +	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port->index), reg);
> > +}
> 
> qca8k_phylink_mac_link_up() with some refactoring can be
> reused. Please look through the driver and find other instances like
> this where you can reuse more code.
I tried to be conservative with modifying qca8k-common.c when it required 
modifying qca8k-8xxx.c. But I'll factor this code more aggressively since you 
think it is preferable.

Best,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com