Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 5 Aug 2023 08:45:40 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>
Cc:     "christophe.jaillet@wanadoo.fr" <christophe.jaillet@wanadoo.fr>,
        "simon.horman@corigine.com" <simon.horman@corigine.com>,
        "Verdun, Jean-Marie" <verdun@hpe.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "conor+dt@kernel.org" <conor+dt@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/5] net: hpe: Add GXP UMAC Driver
Message-ID: <61c541c9-be30-4a43-aa85-53816d5848f9@lunn.ch>
References: <20230802201824.3683-1-nick.hawkins@hpe.com>
 <20230802201824.3683-5-nick.hawkins@hpe.com>
 <fb656c31-ecc3-408a-a719-cba65a6aa984@lunn.ch>
 <933D6861-A193-4145-9533-A7EE8E6DD32F@hpe.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <933D6861-A193-4145-9533-A7EE8E6DD32F@hpe.com>
Precedence: bulk

On Fri, Aug 04, 2023 at 08:55:58PM +0000, Hawkins, Nick wrote:
> Greetings Andrew,
> 
> For some reason I do not see your replies for v1 of this patch or
> the mdio driver on lore.kernel. Apologies as I did not intend to
> not address your previous review comments. My mistake.
> 
> >> +static int umac_int_phy_init(struct umac_priv *umac)
> >> +{
> >> + struct phy_device *phy_dev = umac->int_phy_dev;
> >> + unsigned int value;
> >> +
> >> + value = phy_read(phy_dev, 0);
> >> + if (value & 0x4000)
> >> + pr_info("Internal PHY loopback is enabled - clearing\n");
> 
> > How is the PHY getting into loopback mode? The MAC driver should never
> > touch the PHY, because you have no idea what the PHY actually is,
> > unless it is internal. 
> 
> It would only be in loopback mode if it was previously configured
> that way. I will remove it. The PHY is internal to the ASIC 
> and is always the same. Given that information is it acceptable

Hi Nick

So what you call a PHY is probably a PCS. Please look at the API used
in driver/net/pcs/. The real PHYs are external.

Given that this is a BMC, you probably have lots of i2c busses. So you
can support an SFP on the SERDES. So it would be better if you used
the phylink interface, not phylib. This should also solve your
interface mode switching.

    Andrew