From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932532AbcHIV3U (ORCPT ); Tue, 9 Aug 2016 17:29:20 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:33273 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932382AbcHIV3O (ORCPT ); Tue, 9 Aug 2016 17:29:14 -0400 Subject: Re: [RFC PATCH v5 3/3] net: phy: Add gmiitorgmii converter support To: Kedareswara rao Appana , robh+dt@kernel.org, mark.rutland@arm.com, michal.simek@xilinx.com, soren.brinkmann@xilinx.com, appanad@xilinx.com, andrew@lunn.ch, punnaia@xilinx.com References: <1470735283-23611-1-git-send-email-appanad@xilinx.com> <1470735283-23611-4-git-send-email-appanad@xilinx.com> Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org From: Florian Fainelli Message-ID: <8919e4fe-69c5-9b16-cc37-e423b757609c@gmail.com> Date: Tue, 9 Aug 2016 14:29:10 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1470735283-23611-4-git-send-email-appanad@xilinx.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/09/2016 02:34 AM, Kedareswara rao Appana wrote: > This patch adds support for gmiitorgmii converter. > > The GMII to RGMII IP core provides the Reduced Gigabit Media > Independent Interface (RGMII) between Ethernet physical media > Devices and the Gigabit Ethernet controller. This core can > Switch dynamically between the three different speed modes of > Operation by configuring the converter register through mdio write. > > MDIO interface is used to set operating speed of Ethernet MAC. > > This converter sits between the MAC and the external phy > MAC <==> GMII2RGMII <==> RGMII_PHY This looks good, just a few things, see below: > > Signed-off-by: Kedareswara rao Appana > --- > Thanks a lot Andrew for your inputs. > Changes for v5: > --> Fixed return values in the probe as suggested by punnaiah. > --> Added a mask for the converter speed as suggested by punnaiah. > Changes for v4: > --> Updated phydev speed for all 3 speeds as suggested by zhuyj. > Changes for v3: > --> Updated the driver as suggested by Andrew. > Changes for v2: > --> Passed struct xphy pointer directly to the fix_mac_speed > API as suggested by the Florian. > --> Added checks for the phy-node fail case as suggested > by the Florian > > drivers/net/phy/Kconfig | 8 +++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/xilinx_gmii2rgmii.c | 121 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 130 insertions(+) > create mode 100644 drivers/net/phy/xilinx_gmii2rgmii.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 1b534ea..c79f347 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -312,6 +312,14 @@ config MICROSEMI_PHY > ---help--- > Currently supports the VSC8531 and VSC8541 PHYs > > +config XILINX_GMII2RGMII > + tristate "Xilinx GMII2RGMII converter driver" > + default y Don't force that, or at least make the default based on the potential users/drivers here. > + ---help--- > + This driver support xilinx GMII to RGMII IP core it provides > + the Reduced Gigabit Media Independent Interface(RGMII) between > + Ethernet physical media devices and the Gigabit Ethernet controller. > + > endif # PHYLIB > > config MICREL_KS8995MA > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index a713bd4..73d65ce 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -50,3 +50,4 @@ obj-$(CONFIG_MDIO_BCM_IPROC) += mdio-bcm-iproc.o > obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o > obj-$(CONFIG_MDIO_HISI_FEMAC) += mdio-hisi-femac.o > obj-$(CONFIG_MDIO_XGENE) += mdio-xgene.o > +obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c > new file mode 100644 > index 0000000..1456e27 > --- /dev/null > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c > @@ -0,0 +1,121 @@ > +/* Xilinx GMII2RGMII Converter driver > + * > + * Copyright (C) 2016 Xilinx, Inc. > + * > + * Author: Kedareswara rao Appana > + * > + * Description: > + * This driver is developed for Xilinx GMII2RGMII Converter > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define XILINX_GMII2RGMII_REG 0x10 > +#define XILINX_GMII2RGMII_SPEED_MASK 0x2040 BMCR_SPEED1000 | BMCR_SPEED100 would be clearer here. > + > +struct gmii2rgmii { > + struct phy_device *phy_dev; > + struct phy_driver *phy_drv; > + struct phy_driver conv_phy_drv; > + int addr; > +}; > + > +static int xgmiitorgmii_read_status(struct phy_device *phydev) > +{ > + struct gmii2rgmii *priv = (struct gmii2rgmii *)phydev->priv; Casting is not required here, priv is void *. > + u16 val = 0; > + > + priv->phy_drv->read_status(phydev); > + > + val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG); > + val &= XILINX_GMII2RGMII_SPEED_MASK; > + > + switch (phydev->speed) { > + case SPEED_1000: > + val |= BMCR_SPEED1000; Is the fall through really intentional here? See genphy_setup_forced() for instance. > + case SPEED_100: > + val |= BMCR_SPEED100; > + case SPEED_10: > + val |= BMCR_SPEED10; > + } > + > + mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val); > + > + return 0; > +} [snip] > +static int __init xgmiitorgmii_init(void) > +{ > + return mdio_driver_register(&xgmiitorgmii_driver); > +} > +module_init(xgmiitorgmii_init); > + > +static void __exit xgmiitorgmii_cleanup(void) > +{ > + mdio_driver_unregister(&xgmiitorgmii_driver); > +} > +module_exit(xgmiitorgmii_cleanup); mdio_module_driver() does eliminate a bit of this boilerplate code. -- Florian