From mboxrd@z Thu Jan  1 00:00:00 1970
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S965768AbeAOPjG (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Mon, 15 Jan 2018 10:39:06 -0500
Received: from vps0.lunn.ch ([185.16.172.187]:48679 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S933581AbeAOPjD (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 15 Jan 2018 10:39:03 -0500
Date: Mon, 15 Jan 2018 16:38:42 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <kernel@pengutronix.de>,
        Fabio Estevam <fabio.estevam@nxp.com>,
        Ian Ray <ian.ray@ge.com>, Nandor Han <nandor.han@ge.com>,
        Rob Herring <robh+dt@kernel.org>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv2 1/5] net: dsa: Support internal phy on 'cpu' port
Message-ID: <20180115153842.GF21450@lunn.ch>
References: <20180115121508.14544-1-sebastian.reichel@collabora.co.uk>
 <20180115121508.14544-2-sebastian.reichel@collabora.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180115121508.14544-2-sebastian.reichel@collabora.co.uk>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

On Mon, Jan 15, 2018 at 01:15:04PM +0100, Sebastian Reichel wrote:
> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3,  B650v3 and B450v3, which have a
> built-in MV88E6240 switch connected to a PCIe based network card.
> Without this patch the link does not come up and no traffic can be
> routed through the switch.
> 
> The PHY interface, that is being used on the above test systems is
> part of the MV88E6240 and since mv88e6xxx driver resets the chip
> during probe, it is definitely disabled without this patch.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Hi Sebastian

I was not very happy about the original implementation. This is a bit
better, but i still think it needs improvement. The enum
PHY_INTERFACE_MODE_INTERNAL means the PHY is connected to the MAC
using something other than traditional MII. We don't use this for any
other of the PHYs inside the Marvell switch, or for any other vendors
switch which has internal PHYs, since they are connected by MII.

You also make the assumption that the PHY for the CPU port actually is
internal. It does not need to be. It could be an external PHY.

The key thing here is to know there is a PHY for the CPU port. The
standard way to represent that is to have a phy-handle. That
phy-handle also tells you which PHY it is, without making any
assumptions.

Please can you re-write this patch to look for a phy-handle in the cpu
node.

Thanks
	Andrew