| Message ID | 20170724134848.19330-4-antoine.tenart@free-electrons.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello! On 07/24/2017 04:48 PM, Antoine Tenart wrote: > When connecting to the PHY, explicitly set the SMI PHY address in the > controller registers to configure a given port to be connected to the > selected PHY. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/net/ethernet/marvell/mvpp2.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c > index 1e592abc9067..6ffff929b22a 100644 > --- a/drivers/net/ethernet/marvell/mvpp2.c > +++ b/drivers/net/ethernet/marvell/mvpp2.c [...] > @@ -5954,6 +5958,16 @@ static int mvpp2_phy_connect(struct mvpp2_port *port) > port->duplex = 0; > port->speed = 0; > > + if (priv->hw_version != MVPP22) > + return 0; > + > + /* Set the SMI PHY address */ > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > + netdev_err(port->dev, "cannot find the PHY address\n"); > + return -EINVAL; Wny not propagte the error from of_property_read_u32()? [...] MBR, Sergei
Hi Sergei, On Mon, Jul 24, 2017 at 07:40:01PM +0300, Sergei Shtylyov wrote: > On 07/24/2017 04:48 PM, Antoine Tenart wrote: > > > + /* Set the SMI PHY address */ > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > > + netdev_err(port->dev, "cannot find the PHY address\n"); > > + return -EINVAL; > > Wny not propagte the error from of_property_read_u32()? I could do this, you're right. Antoine
On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote: > When connecting to the PHY, explicitly set the SMI PHY address in the > controller registers to configure a given port to be connected to the > selected PHY. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/net/ethernet/marvell/mvpp2.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c > index 1e592abc9067..6ffff929b22a 100644 > --- a/drivers/net/ethernet/marvell/mvpp2.c > +++ b/drivers/net/ethernet/marvell/mvpp2.c > @@ -359,6 +359,8 @@ > #define MVPP22_SMI_MISC_CFG_REG 0x1204 > #define MVPP22_SMI_POLLING_EN BIT(10) > > +#define MVPP22_SMI_PHY_ADDR(port) (0x120c + (port) * 0x4) > + > #define MVPP22_GMAC_BASE(port) (0x7000 + (port) * 0x1000 + 0xe00) > > #define MVPP2_CAUSE_TXQ_SENT_DESC_ALL_MASK 0xff > @@ -5939,7 +5941,9 @@ static void mvpp21_get_mac_address(struct mvpp2_port *port, unsigned char *addr) > > static int mvpp2_phy_connect(struct mvpp2_port *port) > { > + struct mvpp2 *priv = port->priv; > struct phy_device *phy_dev; > + u32 phy_addr; > > phy_dev = of_phy_connect(port->dev, port->phy_node, mvpp2_link_event, 0, > port->phy_interface); > @@ -5954,6 +5958,16 @@ static int mvpp2_phy_connect(struct mvpp2_port *port) > port->duplex = 0; > port->speed = 0; > > + if (priv->hw_version != MVPP22) > + return 0; > + > + /* Set the SMI PHY address */ > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > + netdev_err(port->dev, "cannot find the PHY address\n"); > + return -EINVAL; > + } > + > + writel(phy_addr, priv->iface_base + MVPP22_SMI_PHY_ADDR(port->gop_id)); > return 0; > } Hi Antoine You could use phy_dev->mdiodev->addr, rather than parse the DT. Why does the MAC need to know this address? The phylib and PHY driver should be the only thing accessing the PHY, otherwise you are asking for trouble. What if the PHY is hanging off some other mdio bus? I've got a freescale board with dual ethernets and a Marvell switch on the hardware MDIO bus and a PHY on a bit-banging MDIO bus. Andrew
Hi Andrew, On Wed, Jul 26, 2017 at 06:08:06PM +0200, Andrew Lunn wrote: > On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote: > > > > + if (priv->hw_version != MVPP22) > > + return 0; > > + > > + /* Set the SMI PHY address */ > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > > + netdev_err(port->dev, "cannot find the PHY address\n"); > > + return -EINVAL; > > + } > > + > > + writel(phy_addr, priv->iface_base + MVPP22_SMI_PHY_ADDR(port->gop_id)); > > return 0; > > } > > You could use phy_dev->mdiodev->addr, rather than parse the DT. OK. > Why does the MAC need to know this address? The phylib and PHY driver > should be the only thing accessing the PHY, otherwise you are asking > for trouble. This is part of the SMI/xSMI interface. I added into the mvpp2 driver and not in the mvmdio one because the GoP port number must be known to set this register (so that would be even less clean to do it). > What if the PHY is hanging off some other mdio bus? I've got a > freescale board with dual ethernets and a Marvell switch on the > hardware MDIO bus and a PHY on a bit-banging MDIO bus. Then it wouldn't be controlled by the PPv2 SMI/xSMI interface, so we wouldn't need to set the this register. Thanks! Antoine
On Thu, Jul 27, 2017 at 06:49:05PM -0700, Antoine Tenart wrote: > Hi Andrew, > > On Wed, Jul 26, 2017 at 06:08:06PM +0200, Andrew Lunn wrote: > > On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote: > > > > > > + if (priv->hw_version != MVPP22) > > > + return 0; > > > + > > > + /* Set the SMI PHY address */ > > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > > > + netdev_err(port->dev, "cannot find the PHY address\n"); > > > + return -EINVAL; > > > + } > > > + > > > + writel(phy_addr, priv->iface_base + MVPP22_SMI_PHY_ADDR(port->gop_id)); > > > return 0; > > > } > > > > You could use phy_dev->mdiodev->addr, rather than parse the DT. > > OK. > > > Why does the MAC need to know this address? The phylib and PHY driver > > should be the only thing accessing the PHY, otherwise you are asking > > for trouble. > > This is part of the SMI/xSMI interface. I added into the mvpp2 driver > and not in the mvmdio one because the GoP port number must be known to > set this register (so that would be even less clean to do it). Hi Antoine It is still not clear to my why you need to program the address into the hardware. Is the hardware talking to the PHY? Andrew
Hi Andrew, On Fri, Jul 28, 2017 at 06:21:53AM +0200, Andrew Lunn wrote: > On Thu, Jul 27, 2017 at 06:49:05PM -0700, Antoine Tenart wrote: > > On Wed, Jul 26, 2017 at 06:08:06PM +0200, Andrew Lunn wrote: > > > On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote: > > > > > > > > + if (priv->hw_version != MVPP22) > > > > + return 0; > > > > + > > > > + /* Set the SMI PHY address */ > > > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > > > > + netdev_err(port->dev, "cannot find the PHY address\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + writel(phy_addr, priv->iface_base + MVPP22_SMI_PHY_ADDR(port->gop_id)); > > > > return 0; > > > > } > > > > > > Why does the MAC need to know this address? The phylib and PHY driver > > > should be the only thing accessing the PHY, otherwise you are asking > > > for trouble. > > > > This is part of the SMI/xSMI interface. I added into the mvpp2 driver > > and not in the mvmdio one because the GoP port number must be known to > > set this register (so that would be even less clean to do it). > > It is still not clear to my why you need to program the address into > the hardware. Is the hardware talking to the PHY? Sorry for the answer delay, I was out of the office... This PHY address configuration should be done in the mvmdio driver as this is not directly related to the PPv2 (well, the mvmdio driver is only an abstraction to reuse the mdio code, using registers exposed by PPv2 in this case anyway). But two values must be known in order to do this: the PHY address and the GoP port number. Getting the last one from the mvmdio driver would be really ugly as we would need to read the PPv2 dt node. This is why this patch adds it in the PPv2 driver, but I know it's not perfect. I'll resend a series very soon, with this patch still included. We can continue the discussion there I guess, if needed. Thanks! Antoine
> > It is still not clear to my why you need to program the address into > > the hardware. Is the hardware talking to the PHY? > > Sorry for the answer delay, I was out of the office... > > This PHY address configuration should be done in the mvmdio driver as > this is not directly related to the PPv2 (well, the mvmdio driver is > only an abstraction to reuse the mdio code, using registers exposed by > PPv2 in this case anyway). But two values must be known in order to do > this: the PHY address and the GoP port number. Getting the last one from > the mvmdio driver would be really ugly as we would need to read the PPv2 > dt node. This is why this patch adds it in the PPv2 driver, but I know > it's not perfect. > > I'll resend a series very soon, with this patch still included. We can > continue the discussion there I guess, if needed. > > Thanks! Hi Antoine You have still not explained why PPv2 needs to know the PHY address. What i'm worried about is that the PPv2 is actually talking to the PHY. Is it polling link state? Does it take the PHY mutex when it does this poll? Andrew
> -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Friday, July 28, 2017 7:22 AM > To: Antoine Tenart <antoine.tenart@free-electrons.com> > Cc: davem@davemloft.net; jason@lakedaemon.net; gregory.clement@free- > electrons.com; sebastian.hesselbarth@gmail.com; thomas.petazzoni@free- > electrons.com; Nadav Haklai <nadavh@marvell.com>; linux@armlinux.org.uk; > mw@semihalf.com; Stefan Chulski <stefanc@marvell.com>; > netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: [EXT] Re: [PATCH net-next 03/18] net: mvpp2: set the SMI PHY address > when connecting to the PHY > > External Email > > ---------------------------------------------------------------------- > On Thu, Jul 27, 2017 at 06:49:05PM -0700, Antoine Tenart wrote: > > Hi Andrew, > > > > On Wed, Jul 26, 2017 at 06:08:06PM +0200, Andrew Lunn wrote: > > > On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote: > > > > > > > > + if (priv->hw_version != MVPP22) > > > > + return 0; > > > > + > > > > + /* Set the SMI PHY address */ > > > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { > > > > + netdev_err(port->dev, "cannot find the PHY address\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + writel(phy_addr, priv->iface_base + > > > > +MVPP22_SMI_PHY_ADDR(port->gop_id)); > > > > return 0; > > > > } > > > > > > You could use phy_dev->mdiodev->addr, rather than parse the DT. > > > > OK. > > > > > Why does the MAC need to know this address? The phylib and PHY > > > driver should be the only thing accessing the PHY, otherwise you are > > > asking for trouble. > > > > This is part of the SMI/xSMI interface. I added into the mvpp2 driver > > and not in the mvmdio one because the GoP port number must be known to > > set this register (so that would be even less clean to do it). > > Hi Antoine > > It is still not clear to my why you need to program the address into the > hardware. Is the hardware talking to the PHY? > > Andrew Hi Andrew, This register configures SMI(Serial Management Interface) hardware unit, not PPv2(Packet Processor) hardware unit. The SB incorporates the following SMI management interfaces: MDC - Serial Management Interface Clock , MDIO - Serial Management Interface Data and complies with IEEE 802.3 Clause 22. SMI interface used for: 1. PHY register read/write. The device provides a mechanism for PHY registers read and write access. 2. Auto-Negotiation with PHY devices connected to the GMAC ports. The device uses a standard master Serial Management Interface for reading from/writing to the PHY registers. In addition, the PHY polling unit performs Auto-Negotiation status update with PHY devices attached to the Network ports via the Master SMI Interface. The device polls the Status register of each PHY in a round-robin manner. If the device detects a change in the link from down to up on 1 of the ports, it performs a series of register reads from the PHY and updates the Auto-Negotiation results in the device's registers. The Port MAC Status register is updated with these results only if Auto-Negotiation is enabled. So SMI interface should know GoP(MAC) id. Regards, Stefan.
> 2. Auto-Negotiation with PHY devices connected to the GMAC ports. > The device uses a standard master Serial Management Interface for reading from/writing to the PHY > registers. In addition, the PHY polling unit performs Auto-Negotiation status update with PHY devices attached > to the Network ports via the Master SMI Interface. > The device polls the Status register of each PHY in a round-robin manner. > If the device detects a change in the link from down to up on 1 of the ports, it performs a series of > register reads from the PHY and updates the Auto-Negotiation results in the device's registers. The > Port MAC Status register is updated with these results only if Auto-Negotiation is enabled. Hi Stefan That is what i was afraid off. How clever is this phy polling hardware? e.g. Say somebody reads the PHY temperature sensor: commit 0b04680fdae464ee51409b8cb36005f6ef8bd689 Author: Andrew Lunn <andrew@lunn.ch> Date: Fri Jan 20 01:37:49 2017 +0100 phy: marvell: Add support for temperature sensor Some Marvell PHYs have an inbuilt temperature sensor. Add hwmon support for this sensor. There are two different variants. The simpler, older chips have a 5 degree accuracy. The newer devices have 1 degree accuracy. Signed-off-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: David S. Miller <davem@davemloft.net> This requires changing the PHY page to 0x1a. Any reads the polling unit does at that time are going to get registers from page 0x1a, not 0x0. And there are other examples where the page may change, e.g. configuring WOL, LEDs. Cable test is not yet supported, but it is on my todo list. In order to safely read/write the PHY, you need to hold the PHY mutex. Unless the hardware is very smart, please don't enable this. Let the phylib and the appropriate PHY driver do the work. Andrew
> In order to safely read/write the PHY, you need to hold the PHY mutex. > Unless the hardware is very smart, please don't enable this. Let the phylib and > the appropriate PHY driver do the work. > > Andrew Hi Andrew, This feature work only for Out-of-Band Auto-Negotiation in SGMII Mode. Current GoP(MAC) code configure SGMII In-band Auto-Negotiation performed by the PCS layer without PHY polling. Regards, Stefan.
> This feature work only for Out-of-Band Auto-Negotiation in SGMII Mode. > Current GoP(MAC) code configure SGMII In-band Auto-Negotiation performed by the PCS layer > without PHY polling. Hi Stefan So there is no need to configure the address then, leave PHY polling turned off and we avoid all the issues. Andrew
diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c index 1e592abc9067..6ffff929b22a 100644 --- a/drivers/net/ethernet/marvell/mvpp2.c +++ b/drivers/net/ethernet/marvell/mvpp2.c @@ -359,6 +359,8 @@ #define MVPP22_SMI_MISC_CFG_REG 0x1204 #define MVPP22_SMI_POLLING_EN BIT(10) +#define MVPP22_SMI_PHY_ADDR(port) (0x120c + (port) * 0x4) + #define MVPP22_GMAC_BASE(port) (0x7000 + (port) * 0x1000 + 0xe00) #define MVPP2_CAUSE_TXQ_SENT_DESC_ALL_MASK 0xff @@ -5939,7 +5941,9 @@ static void mvpp21_get_mac_address(struct mvpp2_port *port, unsigned char *addr) static int mvpp2_phy_connect(struct mvpp2_port *port) { + struct mvpp2 *priv = port->priv; struct phy_device *phy_dev; + u32 phy_addr; phy_dev = of_phy_connect(port->dev, port->phy_node, mvpp2_link_event, 0, port->phy_interface); @@ -5954,6 +5958,16 @@ static int mvpp2_phy_connect(struct mvpp2_port *port) port->duplex = 0; port->speed = 0; + if (priv->hw_version != MVPP22) + return 0; + + /* Set the SMI PHY address */ + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) { + netdev_err(port->dev, "cannot find the PHY address\n"); + return -EINVAL; + } + + writel(phy_addr, priv->iface_base + MVPP22_SMI_PHY_ADDR(port->gop_id)); return 0; }
When connecting to the PHY, explicitly set the SMI PHY address in the controller registers to configure a given port to be connected to the selected PHY. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/net/ethernet/marvell/mvpp2.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)