From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424243AbcFIHWd (ORCPT ); Thu, 9 Jun 2016 03:22:33 -0400 Received: from mail-bn1on0054.outbound.protection.outlook.com ([157.56.110.54]:45945 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1161273AbcFIHWb convert rfc822-to-8bit (ORCPT ); Thu, 9 Jun 2016 03:22:31 -0400 Authentication-Results: spf=softfail (sender IP is 66.35.236.227) smtp.mailfrom=altera.com; arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=altera.com; Message-ID: <1465451310.4467.21.camel@ubuntu> Subject: Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga From: Tien Hock Loh To: Giuseppe CAVALLARO CC: , , , , , , , , , , Date: Wed, 8 Jun 2016 22:48:30 -0700 In-Reply-To: References: <1465268516-31480-1-git-send-email-thloh@altera.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Mailer: Evolution 3.2.3-0ubuntu6 MIME-Version: 1.0 X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:66.35.236.227;IPV:NLI;CTRY:US;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(3190300001)(377454003)(377424004)(189002)(54094003)(199003)(52314003)(24454002)(106466001)(68736007)(47776003)(4326007)(5820100001)(76176999)(110136002)(23676002)(50986999)(2906002)(4001430100002)(86362001)(42186005)(33716001)(81156014)(87936001)(97736004)(5008740100001)(6806005)(105596002)(9686002)(81166006)(586003)(8746002)(8936002)(33646002)(107886002)(2950100001)(19580405001)(92566002)(50466002)(15975445007)(8676002)(103116003)(189998001)(19580395003)(50226002)(7099028)(99106002)(2004002)(217873001);DIR:OUT;SFP:1101;SCL:1;SRVR:BN1PR03MB107;H:sj-itexedge03.altera.priv.altera.com;FPR:;SPF:SoftFail;PTR:InfoDomainNonexistent;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BN1AFFO11FD035;1:CIT/heQpPLnAgFS0fVtwxg7A4mk3d9oAy+EjK8TuGoDfVqLZuvKW58gcpqEFzzwku9o5hy6POzrJyvZFyLMUhzX6HN044cydnV4vFpmozG1o7gRXBc63fI1yvxYaomDm/yXVEZFE1D1QtQyK5FU21lzKsvjrlSn6Vx80EKcxRJ2ZEuYvTWyhYxReMKfOBYs03onNGge5wYUFsi9PfxYHDcwYb9GRzDCBo4mmm0SHgSPuBtx5EGa/9k5iZEXE99ORRex9z0BbLWRhvhD/3fGD5NETifyRy9ttn7Leq/eRV5RwWPKHAIESfRhGRsgq2QjL2PI7XeZw0lJPqAHn5ADRP1mViP2i6yNVpRrfephkJV+F7bjXdEPScWuPVyy0FbDSggJGFrQW6xd8DzqTkFetEFJhW9iMWEunD+YRz7O+rlBHjINDm/Mv9SfsK2/0gDfI6gpyqKsePXFvdwd7giiE887E4I+UeF6+2xUaei3bkp7ppnv+I5LS8NyGjnCFOBkMGrIUnlV4eFIB8gsk1TZT58AsihGFUZvYbqDmGYpMksRJ7J5oJfb2kFdOIHd5Dl5Ikxd5dItDezfQTy/esdsZnA== X-MS-Office365-Filtering-Correlation-Id: d03225d1-bbe1-459e-c880-08d39029aef7 X-Microsoft-Exchange-Diagnostics: 1;BN1PR03MB107;2:AJkMRoUNgoVsGxNr/zlqZrdwEa7riTkvq7XWNZFaKDlKrxwBboh56FrGE+8NrgHHtymFEj5Xndb4hTe/RAbocZJr5lmYwwgi407wXwWiJ2tSRHIFyp5yfMf9EGP4mRrdfBw3XbPsLLv9O7MeNaOapBUWn9Y+y8oJoezrWHo4BEd/qvMyGO+SPabWuAnu1Egu;3:zmKjzCiwcIZoz1uTcdOKWU8TwuTX55PoYrMoL6FnfhSo0nam3/vJHnDdnZiCvTgAyMnxcXXhKQqXMgBK7/vfZQBiEnIOZtxS728UWKS76HCHPr8x0ttSxizO/uFs5n7NByDfLl7JoD83jDEAsNUP00M9QRxaTNVn7us8zMY1E3fB8Epti+fPkfAqtRjs0Cu3491t5Hjuc5tulOZCXPqP3RbT7e52Xf4LCvuGO8vr/S0=;25:o4bUFHXyeQuWPEPRbcXeX20d26i/HTL2EqFcpNhBo2oFn/e3aO7x6GgoswBvod440Y1f0zoePLpqeS/vjz/08Is2Nmg0iepLvJGI+Z4lci4EqTOi5lm7ZgK2R9+p9iQrZHQmKSyO8ohJs3xvZfaURdACdysMGRXWMeOR6WzFBJFgmGzKfKK0d7LZGW4FogLDdzR3jFN4KWnSldwj86sZD3z16Ff3rbHK1fIhPM+UteGLd0M3l9B68LjcrgJspnIqLU3hCbgYkE37o1DYkMsRewy63Ryah2b/9G+AJA4n5UVlV6roQCzw6ab1XjZTncr5reIkY9Ozi/bneC1F5AhA7LeG2YsGxQNMT3qDmymyX+lF8yd4E3RXzJ+DJW5ODU4WVq0CGNFQQTsAIEs4mxmcISIXPRtiVLq8gL/m/IzNGuY= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN1PR03MB107; X-Microsoft-Exchange-Diagnostics: 1;BN1PR03MB107;20:fojKQM/4KFw+1ycPxeY3MFGHlqLHgwRXxUFTaA9dlo3H2hFb2/4419csHmZsAKS2k37yxFP88LHPwxCUNVWtZI7pyGxZJTuYcfpZjhtKF24KITBe4tJHvwhMo5ifI3447sHXiOxueJaJdpWrTDT96nXfY+pGm0Coxkmswjbj6sE=;4:W2gXyJ8FDlpC061HVOkE1iD2VXhrWBEQPaD9p6M4gnoJaWF65NaSfayZdvEW5Q/ec4A9AqgkeK7BDj/6c8MmrtGBBYk9zy+RbAb0DxwXMbAO9zbeL4HRyNwYE+I3k/ZBvuz/lURpu//jrbLyd9uDjOvQXJQ+7nqIY2XPhCsQSeNOXIFghW373RSgaN0vQ7M4/kCO5s0sQi4aeSZnv6Ij3sO6RujK42k7vqlGVAvTcYNk33PoVcMOlpkfO8qWlO6HEFUqDTfk71bQp3AZctmBGozfKfhwXjsbc66u6i9LBHJc13Hu52KjPYEXyJ3eAFa/wKQKI6YkKTFk5QPWmov6iy4AY/fPnVe3sUgF38xTCWEoV/S8p8QP4srMCjWQ+cgIrrOunq8II6W6FmOUrh4x+vbpoOlhVZ24H+jhFpYrdMtfILj1rMzaD69bGqKQ1dshl8V5iT0Xq1u5w4dXQMPxOUWDgClqBfPt4gYtvLiLm8HhKxHwHed/D1VI2rftzgMOF8KEPKUEHyjZZz13Q6ukC+de5cn7dQGpdakzJ5uNZzG0qSINBEGtNp8TpZ6m17WyiQF5/RUE4CIRVmR1lcxhpA== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(250305191791016)(22074186197030)(80048183373757); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(13017025)(13015025)(13024025)(13023025)(13018025)(5005006)(8121501046)(3002001)(10201501046)(6055026);SRVR:BN1PR03MB107;BCL:0;PCL:0;RULEID:;SRVR:BN1PR03MB107; X-Forefront-PRVS: 0968D37274 X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTjFQUjAzTUIxMDc7MjM6OXRnMVc5YmowRDM1UXB5YlJTeURMZUFjQkcz?= =?utf-8?B?VGQ4NjNKaU90ZFZjQmNjMnp2RzFEQk5maXNhdkZ5ZThMTkg2SDM1RGkzSDNj?= =?utf-8?B?WFZ1a3pyVUs2bVZMMjh1RWxKL0ZOVWFxUjZORHlhTUtPMEdCcGtNMXJmbWw2?= =?utf-8?B?MUo1aVJhRnJ1REYxTEg3dnQ1OUhpSFgvUEp1Q1hjcDR5TFlLTTBwblVpbHMz?= =?utf-8?B?eHZ2UTZFc0ptWTd3NGdoa0pQd09HQklIKy9tRmJXeDZYd2xlSDJMVUVJajds?= =?utf-8?B?MVNqQlJnOTZldmVJV2RtS0UzWlV5amZ4czY2cFJJY0dUOW1pYWk5bmRJWTZh?= =?utf-8?B?L1dnTWhXMTB2dDFQa0dKQW5MSit6NmY0WXdZWDFSY0FuT1hNYW40UDV1Z3V2?= =?utf-8?B?ZkZ3K0dleUVabHpOZThxckRobmEvVExnMkJSRjhXL2FqTFVQU1k0dzlSTnpj?= =?utf-8?B?dGJwcE5kQXZpVWo3eTZKTGlSRmR6UnZLWktQSlh4R3I2b3I4c2NYY1cydEhp?= =?utf-8?B?bHdLcUlDNFdHQWFmbUhoMDkzS0hHUExWc0JFRHVvb1l1ZXlONUlINGNPU3Rp?= =?utf-8?B?RmJnNG9aYzVwOFJtZGJSTStvbktIL3BNV29DdWM4K1NvZGxWNDdKenVveXQ5?= =?utf-8?B?UDBBekRENy9mQzRROFM4ZFZoenZaazlCUFpXMkx5VUZoWHI1d1RrSmZmemdw?= =?utf-8?B?VWNkVVM5ZmU2V0VYNWFNeisyOWNOMGRXY0dmOHFBL0orKytvK2UvSnFpakIv?= =?utf-8?B?MmZCc1lBemFmd3g0aXhQQmNpek96OWI3ejhpbmQxWXlieE92OXFMNVdJVjJO?= =?utf-8?B?REc3ZW5lb1R1dlVLbTNGbUZSRmdlK2RKWitqZVJSbHhMZVN4THgwR3hCeTh6?= =?utf-8?B?Q0hoc21TWkVad1kvVXJxcHVLdjBkR1JqVVh3dFBEVkwranRrdHRCRkVRUk5z?= =?utf-8?B?ZEVEdFQzbmxSSis3Z3BsOTFrNUl1SEMzSEwrL2ZBMHA1dWdNaEhSWjBDb1J0?= =?utf-8?B?SkZBVU5rN1RxRUlWdGE4V1Bud3ZoYmlYL2RFUEV4Q0JGQXhqK0E1NFpveHJX?= =?utf-8?B?akJLNmZnT2FXemorVnFNM0h5NTF6UTFZTDI2OG9GcnlLNGNTMHd5dE5ZeEx1?= =?utf-8?B?Nm0xbVczT0ZqMUhrcDVIWmxKUkVyckdsYU1mMjVkZVgyS1UrcmRSUjRjN1NU?= =?utf-8?B?bG5uZHdMWUlMb0h5UEJwRk51TE5COFhIUFpJb0lFL2M5SFpZK1Fzcm1CWkxB?= =?utf-8?B?TUFsand4TGljMnNhVEI5dk4yUXU5VHczUUE3QWgvQmd3TlN0S0VlYllRSFVv?= =?utf-8?B?SmRsSkl1cG5ZTmJSWWtWWXlSRm4xMzJwZ041OW9KK2FENGFDb2RiSmlidFpJ?= =?utf-8?B?UTJlckRsdUtzZFo3SXVFUHBoMUozWE5wN0d1WlVCSElOSERrNW9Ob0pjOXZB?= =?utf-8?B?dHczeENJRFJtcFFCK3BCVGhSM0R4S0E2dnZ5TkY3VkZCK0NmNWQ1WmxHeDU1?= =?utf-8?B?Q211bWdPK1NkMDFsWEFSbU9OcG5PSTNXdmZoR0VUYTI1VzBoSG5COEY2K3o0?= =?utf-8?B?UmwwMUZYSXhyK1l2cFN6VU94b2gvUG1kekF0Y1EvN0RsL3pHRzNMdlRvamxs?= =?utf-8?B?Y25BWUpyL1J4T2RHVGVYRE5wSmpKRzROdXUrcXYwaVhOSkdEWExONFJNS0hW?= =?utf-8?B?K29xL0lyS1MwUkMrTU9MT1JmcGNSa3MralR0STZXaG5CR1JxYkRYTkwyQUFS?= =?utf-8?B?MDJkdW5pdmoxNENqVWZnPT0=?= X-Microsoft-Exchange-Diagnostics: 1;BN1PR03MB107;5:bOogytUk8wUHeqYD+vdhtZTwSIqmY1sM0BVEUJzYU1FO85RuCyDq4VShurEdBZe/iBNjvUmYLy2jd/7vA0eLJz5aTzwGr766dG1k+9A882Jxc+AAb92Kvchr0HJCe6KQLJy6mgTlDJTUbH0HismP6g==;24:35ZH+StEQIENbEMXPtitq5lKWEzw8gJUCZF+lGh2zUKWAFB2fuNb93KFCw9h/BI8D/O7OjsexpSZ4W0jD3KcaPUHSgeLfOEfqaUsQ1zEBHk=;7:5Ye+y1YWYHRWPx5CmdjgJpZBoDfvwRcU4c1t85/zcC265z5aTplcz3qAZBqmk3rG40Bq/zt4SJ1Qwj0g4G/uM1ktfDjIEjTxYOqV6ayqPW4B5byrWt/ekDE7CHluJ5Nk6DBXdG06vGQTmzwtJkjXbOkEkaAaBo4fCGxCko+Yh7ey1VZ8eq6DfLnoVt0jfvt9ssAAfVyJMRn2LM/p3geBeA==;20:mSwLRMTn25Ul9a4RgYgO0peJe+TDez0Vh1pyFJiFxA+7B00UEoohxKC9VIfjMyycUmHn0ZdjJqlVtp37m9xp3q8YslLc8fHnpnudL9vABymoREq5kqizy32l8wtw9R8RNEVFXTWfziwRNnoecfhMkc2hdiCP5eGQrdruw26qpEQ= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: altera.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2016 05:48:28.8537 (UTC) X-MS-Exchange-CrossTenant-Id: fbd72e03-d4a5-4110-adce-614d51f2077a X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=fbd72e03-d4a5-4110-adce-614d51f2077a;Ip=[66.35.236.227];Helo=[sj-itexedge03.altera.priv.altera.com] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN1PR03MB107 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peppe, On Tue, 2016-06-07 at 10:30 +0200, Giuseppe CAVALLARO wrote: > Hello > > On 6/7/2016 5:01 AM, thloh@altera.com wrote: > > From: Tien Hock Loh > > > > This adds support for TSE PCS that uses SGMII adapter when the phy-mode of > > the dwmac is set to sgmii > > > > Signed-off-by: Tien Hock Loh > > > > --- > > v2: > > - Refactored the TSE PCS out from the dwmac-socfpga.c file > > - Added binding documentation for TSE PCS sgmii adapter > > v3: > > - Added missing license header for new source files > > - Updated tse_pcs.h include headers > > - Standardize if statements > > --- > > .../devicetree/bindings/net/socfpga-dwmac.txt | 4 + > > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > > .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++-- > > drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++ > > drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++ > > 5 files changed, 419 insertions(+), 24 deletions(-) > > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > I just wonder if it could make sense to rename the > tse_pcs.[hc] files or creating a sub-directory for altera devel. > It seems that tse_pcs.[hc] are common files but this support > is for Altera. > Anyway, I let you decide and I also ask you to update the stmmac.txt > file. Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename them, would altr_tse_pcs.[hc] be good? I don't think creating a sub-directory with only 2 files is necessary though. I see that stmmac.txt is generic, and other vendor's PCS support documents their specific uses in their own *-dwmac.txt (eg. rockchip-dwmac.txt). Is this not the case? > > > diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > index 3a9d679..2bc39f1 100644 > > --- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > +++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > @@ -15,6 +15,8 @@ Required properties: > > Optional properties: > > altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if > > DWMAC controller is connected emac splitter. > > +phy-mode: The phy mode the ethernet operates in > > +altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter > > > > Example: > > > > @@ -28,4 +30,6 @@ gmac0: ethernet@ff700000 { > > mac-address = [00 00 00 00 00 00];/* Filled in by U-Boot */ > > clocks = <&emac_0_clk>; > > clock-names = "stmmaceth"; > > + phy-mode = "sgmii"; > > + altr,gmii_to_sgmii_converter = <&sgmii_1_gmii_to_sgmii_converter_0>; > > }; > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile > > index 73c2715..29c1dee 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile > > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile > > @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \ > > > > obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o > > stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \ > > - dwmac-sti.o dwmac-socfpga.o dwmac-rk.o > > + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o tse_pcs.o > > > > obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o > > stmmac-pci-objs:= stmmac_pci.o > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > index 3f9588e..88fba4e 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > @@ -27,6 +27,8 @@ > > #include "stmmac.h" > > #include "stmmac_platform.h" > > > > +#include "tse_pcs.h" > > + > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0 > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1 > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2 > > @@ -47,48 +49,60 @@ struct socfpga_dwmac { > > struct regmap *sys_mgr_base_addr; > > struct reset_control *stmmac_rst; > > void __iomem *splitter_base; > > + struct tse_pcs pcs; > > }; > > > > static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed) > > { > > struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv; > > void __iomem *splitter_base = dwmac->splitter_base; > > + void __iomem *tse_pcs_base = dwmac->pcs.tse_pcs_base; > > + void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base; > > + struct device *dev = dwmac->dev; > > + struct net_device *ndev = dev_get_drvdata(dev); > > + struct phy_device *phy_dev = ndev->phydev; > > u32 val; > > > > - if (!splitter_base) > > - return; > > - > > - val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG); > > - val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK; > > - > > - switch (speed) { > > - case 1000: > > - val |= EMAC_SPLITTER_CTRL_SPEED_1000; > > - break; > > - case 100: > > - val |= EMAC_SPLITTER_CTRL_SPEED_100; > > - break; > > - case 10: > > - val |= EMAC_SPLITTER_CTRL_SPEED_10; > > - break; > > - default: > > - return; > > + if (splitter_base) { > > + val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG); > > + val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK; > > + > > + switch (speed) { > > + case 1000: > > + val |= EMAC_SPLITTER_CTRL_SPEED_1000; > > + break; > > + case 100: > > + val |= EMAC_SPLITTER_CTRL_SPEED_100; > > + break; > > + case 10: > > + val |= EMAC_SPLITTER_CTRL_SPEED_10; > > + break; > > + default: > > + return; > > + } > > + writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG); > > } > > > > - writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG); > > + if ((tse_pcs_base) && (sgmii_adapter_base)) > > + tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed); > > } > > > > -static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev) > > +static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, > > + struct device *dev) > > { > > struct device_node *np = dev->of_node; > > struct regmap *sys_mgr_base_addr; > > u32 reg_offset, reg_shift; > > - int ret; > > - struct device_node *np_splitter; > > + int ret, index; > > + struct device_node *np_splitter = NULL; > > + struct device_node *np_sgmii_adapter = NULL; > > + > > struct resource res_splitter; > > + struct resource res_tse_pcs; > > + struct resource res_sgmii_adapter; > > > > dwmac->stmmac_rst = devm_reset_control_get(dev, > > - STMMAC_RESOURCE_NAME); > > + STMMAC_RESOURCE_NAME); > > if (IS_ERR(dwmac->stmmac_rst)) { > > dev_info(dev, "Could not get reset control!\n"); > > if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER) > > @@ -99,6 +113,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device * > > dwmac->interface = of_get_phy_mode(np); > > > > sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > > + > > if (IS_ERR(sys_mgr_base_addr)) { > > dev_info(dev, "No sysmgr-syscon node found\n"); > > return PTR_ERR(sys_mgr_base_addr); > > @@ -130,6 +145,74 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device * > > } > > } > > > > + np_sgmii_adapter = of_parse_phandle(np, > > + "altr,gmii_to_sgmii_converter", 0); > > + if (np_sgmii_adapter) { > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "hps_emac_interface_splitter_avalon_slave"); > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_splitter)) { > > + dev_err(dev, "%s: ERROR: missing emac splitter address\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->splitter_base = devm_ioremap_resource( > > + dev, &res_splitter); > > + > > + if (IS_ERR(dwmac->splitter_base)) { > > + dev_err(dev, "%s: ERROR: failed mapping emac splitter\n", > > + __func__); > > + return PTR_ERR(dwmac->splitter_base); > > + } > > + } > > + > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "gmii_to_sgmii_adapter_avalon_slave"); > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_sgmii_adapter)) { > > + dev_err(dev, > > + "%s: ERROR: failed mapping adapter\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->pcs.sgmii_adapter_base = > > + devm_ioremap_resource(dev, &res_sgmii_adapter); > > + > > + if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) { > > + dev_err(dev, "%s: failed to mapping adapter\n", > > + __func__); > > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base); > > + } > > + } > > + > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "eth_tse_control_port"); > > reg-names looks to be specific and mandatory, IMO they should be > documented in the binding. That's the binding for the adapter (the phandle to the sgmii adapter), not the stmac binding itself. Do you mean I should document the sgmii adapter as well? > > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_tse_pcs)) { > > + dev_err(dev, > > + "%s: ERROR: failed mapping tse control port\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->pcs.tse_pcs_base = devm_ioremap_resource( > > + dev, &res_tse_pcs); > > can you check with indentation (scripts/Lindent)? Noted. For some reason check_patch didn't catch this. > > > > + > > + if (IS_ERR(dwmac->pcs.tse_pcs_base)) { > > + dev_err(dev, "%s: ERROR: failed mapping tse control port\n", > > + __func__); > > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base); > > + } > > + } > > + } > > dwmac->reg_offset = reg_offset; > > dwmac->reg_shift = reg_shift; > > dwmac->sys_mgr_base_addr = sys_mgr_base_addr; > > @@ -153,6 +236,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac) > > break; > > case PHY_INTERFACE_MODE_MII: > > case PHY_INTERFACE_MODE_GMII: > > + case PHY_INTERFACE_MODE_SGMII: > > val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII; > > break; > > default: > > @@ -172,6 +256,10 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac) > > ctrl |= val << reg_shift; > > > > regmap_write(sys_mgr_base_addr, reg_offset, ctrl); > > + > > + if (phymode == PHY_INTERFACE_MODE_SGMII) > > + tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs); > > + > > return 0; > > } > > > > @@ -191,6 +279,12 @@ static void *socfpga_dwmac_probe(struct platform_device *pdev) > > return ERR_PTR(ret); > > } > > > > + ret = socfpga_dwmac_setup(dwmac); > > + if (ret) { > > + dev_err(dev, "couldn't setup SoC glue (%d)\n", ret); > > + return ERR_PTR(ret); > > + } > > + > > return dwmac; > > } > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > new file mode 100644 > > index 0000000..1989bd1 > > --- /dev/null > > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > @@ -0,0 +1,261 @@ > > +/* Copyright Altera Corporation (C) 2016. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License, version 2, > > + * as published by the Free Software Foundation. > > + * > > + * 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. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see . > > + * > > + * Author: Tien Hock Loh > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "stmmac.h" > > +#include "stmmac_platform.h" > > +#include "tse_pcs.h" > > + > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003 > > + > > +#define EMAC_SPLITTER_CTRL_REG 0x0 > > +#define EMAC_SPLITTER_CTRL_SPEED_MASK 0x3 > > +#define EMAC_SPLITTER_CTRL_SPEED_10 0x2 > > +#define EMAC_SPLITTER_CTRL_SPEED_100 0x3 > > +#define EMAC_SPLITTER_CTRL_SPEED_1000 0x0 > > pls use BIT and GENMASK. Noted. Will do the next patch. > > ... > > > +static void config_tx_buffer(u16 data, void __iomem *base) > > +{ > > + writew(data, base + SGMII_ADAPTER_CTRL_REG); > > +} > > hmm, is this function really necessary? I think you can use > directly writew or maybe writew_relaxed. > OK will do. > > + > > +static void tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs) > > +{ > > + int counter = 0; > > + u16 val; > > + > > + val = readw(base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_SW_RST_MASK; > > + writew(val, base + TSE_PCS_CONTROL_REG); > > + > > + while (counter < TSE_PCS_SW_RESET_TIMEOUT) { > > + val = readw(base + TSE_PCS_CONTROL_REG); > > + val &= TSE_PCS_SW_RST_MASK; > > + if (val == 0) > > + break; > > + counter++; > > + udelay(1); > > + } > > + if (counter >= TSE_PCS_SW_RESET_TIMEOUT) > > + dev_err(pcs->dev, "PCS could not get out of sw reset\n"); > > +} > > + > > maybe this should return an error to stop the init if reset is NOK. Yeah you're right. I'll fix this in the next patch. > > > > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs) > > +{ > > + writew(0x0001, base + TSE_PCS_IF_MODE_REG); > > + > > + writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG); > > + writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG); > > + > > + tse_pcs_reset(base, pcs); > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, > > + pcs->sgmii_adapter_base); > > +} > > + > > +static void pcs_link_timer_callback(unsigned long data) > > +{ > > + u16 val = 0; > > + > > + struct tse_pcs *pcs = (struct tse_pcs *)data; > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + > > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG); > > + val &= TSE_PCS_STATUS_LINK_MASK; > > + > > + if (val != 0) { > > + dev_dbg(pcs->dev, "Adapter: Link is established\n"); > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base); > > + } else { > > + mod_timer(&pcs->link_timer, jiffies + > > + msecs_to_jiffies(LINK_TIMER)); > > + } > > +} > > + > > +static void auto_nego_timer_callback(unsigned long data) > > +{ > > + u16 val = 0; > > + u16 speed = 0; > > + u16 duplex = 0; > > + > > + struct tse_pcs *pcs = (struct tse_pcs *)data; > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + > > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG); > > + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK; > > + > > + if (val != 0) { > > + dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n"); > > + val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG); > > + speed = val & TSE_PCS_PARTNER_SPEED_MASK; > > + duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK; > > + > > + if (speed == TSE_PCS_PARTNER_SPEED_10 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 10/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_100 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 100/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 1000/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_10 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_100 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else > > + dev_err(pcs->dev, > > + "Adapter: Invalid Partner Speed and Duplex\n"); > > ANE is completed but speed or duplex is NOK. Any failure to signalling? > I see that you then enable the adpter in any case. > > Maybe we could try to restart ANE again or force it (reducing the speed) > I wonder what happens if, for some reason, there is some hw problem. In > that case the timer is always running signalling an invalid Parter > speed. Anyway, this is jus a question... I expect that this error will > always point us to a problem to debug further (if occurs). Let me look at how we can handle the case. Perhaps we could do a restart and register the timer again. I'm just worried it will go into an infinite timer registering hogging up the kernel if the hardware really fails. Maybe I can do a n-time retry here. Looking into this. Let me know if you have any opinions on this. I haven't been able to check for this behaviour though, negative testing on this code isn't too easy to simulate. > > > + > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base); > > + } else { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > + mod_timer(&pcs->an_timer, jiffies + > > + msecs_to_jiffies(AUTONEGO_TIMER)); > > + } > > +} > > + > > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev, > > + unsigned int speed) > > +{ > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + u32 val; > > + > > + config_tx_buffer(SGMII_ADAPTER_DISABLE, sgmii_adapter_base); > > + > > + if (phy_dev->autoneg == AUTONEG_ENABLE) { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_AN_EN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val |= TSE_PCS_USE_SGMII_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > why do you need to reset here? This is done according to the Altera TSE specification, whenever a configuration changes, it is recommended that the PCS gets reset. > > > + > > + setup_timer(&pcs->an_timer, > > + auto_nego_timer_callback, > > + (unsigned long)pcs); > > + mod_timer(&pcs->an_timer, jiffies + > > + msecs_to_jiffies(AUTONEGO_TIMER)); > > + } else if (phy_dev->autoneg == AUTONEG_DISABLE) { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val &= ~TSE_PCS_CONTROL_AN_EN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val &= ~TSE_PCS_USE_SGMII_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val &= ~TSE_PCS_SGMII_SPEED_MASK; > > + > > + switch (speed) { > > + case 1000: > > + val |= TSE_PCS_SGMII_SPEED_1000; > > + break; > > + case 100: > > + val |= TSE_PCS_SGMII_SPEED_100; > > + break; > > + case 10: > > + val |= TSE_PCS_SGMII_SPEED_10; > > + break; > > + default: > > + return; > > + } > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > + > > + setup_timer(&pcs->link_timer, > > + pcs_link_timer_callback, > > + (unsigned long)pcs); > > + mod_timer(&pcs->link_timer, jiffies + > > + msecs_to_jiffies(LINK_TIMER)); > > I wonder if we can have just one timer to manage ANE and LINK. > That would increase the complexity of the code because we would need to check the callback type on when the callback is triggered and call the correct function. > > + } > > +} > > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > new file mode 100644 > > index 0000000..e3d4511 > > --- /dev/null > > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > @@ -0,0 +1,36 @@ > > +/* Copyright Altera Corporation (C) 2016. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License, version 2, > > + * as published by the Free Software Foundation. > > + * > > + * 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. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see . > > + * > > + * Author: Tien Hock Loh > > + */ > > + > > +#ifndef __TSE_PCS_H__ > > +#define __TSE_PCS_H__ > > + > > +#include > > +#include > > + > > +struct tse_pcs { > > + struct device *dev; > > + void __iomem *tse_pcs_base; > > + void __iomem *sgmii_adapter_base; > > + struct timer_list an_timer; > > + struct timer_list link_timer; > > +}; > > + > > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs); > > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev, > > + unsigned int speed); > > + > > +#endif /* __TSE_PCS_H__ */ > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tien Hock Loh Subject: Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga Date: Wed, 8 Jun 2016 22:48:30 -0700 Message-ID: <1465451310.4467.21.camel@ubuntu> References: <1465268516-31480-1-git-send-email-thloh@altera.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Giuseppe CAVALLARO Cc: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, thloh85@gmail.com, cnphoon@altera.com, thloh@altera.com List-Id: devicetree@vger.kernel.org Hi Peppe, On Tue, 2016-06-07 at 10:30 +0200, Giuseppe CAVALLARO wrote: > Hello > > On 6/7/2016 5:01 AM, thloh@altera.com wrote: > > From: Tien Hock Loh > > > > This adds support for TSE PCS that uses SGMII adapter when the phy-mode of > > the dwmac is set to sgmii > > > > Signed-off-by: Tien Hock Loh > > > > --- > > v2: > > - Refactored the TSE PCS out from the dwmac-socfpga.c file > > - Added binding documentation for TSE PCS sgmii adapter > > v3: > > - Added missing license header for new source files > > - Updated tse_pcs.h include headers > > - Standardize if statements > > --- > > .../devicetree/bindings/net/socfpga-dwmac.txt | 4 + > > drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +- > > .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++-- > > drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++ > > drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++ > > 5 files changed, 419 insertions(+), 24 deletions(-) > > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > I just wonder if it could make sense to rename the > tse_pcs.[hc] files or creating a sub-directory for altera devel. > It seems that tse_pcs.[hc] are common files but this support > is for Altera. > Anyway, I let you decide and I also ask you to update the stmmac.txt > file. Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename them, would altr_tse_pcs.[hc] be good? I don't think creating a sub-directory with only 2 files is necessary though. I see that stmmac.txt is generic, and other vendor's PCS support documents their specific uses in their own *-dwmac.txt (eg. rockchip-dwmac.txt). Is this not the case? > > > diff --git a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > index 3a9d679..2bc39f1 100644 > > --- a/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > +++ b/Documentation/devicetree/bindings/net/socfpga-dwmac.txt > > @@ -15,6 +15,8 @@ Required properties: > > Optional properties: > > altr,emac-splitter: Should be the phandle to the emac splitter soft IP node if > > DWMAC controller is connected emac splitter. > > +phy-mode: The phy mode the ethernet operates in > > +altr,sgmii_to_sgmii_converter: phandle to the TSE SGMII converter > > > > Example: > > > > @@ -28,4 +30,6 @@ gmac0: ethernet@ff700000 { > > mac-address = [00 00 00 00 00 00];/* Filled in by U-Boot */ > > clocks = <&emac_0_clk>; > > clock-names = "stmmaceth"; > > + phy-mode = "sgmii"; > > + altr,gmii_to_sgmii_converter = <&sgmii_1_gmii_to_sgmii_converter_0>; > > }; > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile > > index 73c2715..29c1dee 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile > > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile > > @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o \ > > > > obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o > > stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \ > > - dwmac-sti.o dwmac-socfpga.o dwmac-rk.o > > + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o tse_pcs.o > > > > obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o > > stmmac-pci-objs:= stmmac_pci.o > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > index 3f9588e..88fba4e 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c > > @@ -27,6 +27,8 @@ > > #include "stmmac.h" > > #include "stmmac_platform.h" > > > > +#include "tse_pcs.h" > > + > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0 > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1 > > #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2 > > @@ -47,48 +49,60 @@ struct socfpga_dwmac { > > struct regmap *sys_mgr_base_addr; > > struct reset_control *stmmac_rst; > > void __iomem *splitter_base; > > + struct tse_pcs pcs; > > }; > > > > static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed) > > { > > struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv; > > void __iomem *splitter_base = dwmac->splitter_base; > > + void __iomem *tse_pcs_base = dwmac->pcs.tse_pcs_base; > > + void __iomem *sgmii_adapter_base = dwmac->pcs.sgmii_adapter_base; > > + struct device *dev = dwmac->dev; > > + struct net_device *ndev = dev_get_drvdata(dev); > > + struct phy_device *phy_dev = ndev->phydev; > > u32 val; > > > > - if (!splitter_base) > > - return; > > - > > - val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG); > > - val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK; > > - > > - switch (speed) { > > - case 1000: > > - val |= EMAC_SPLITTER_CTRL_SPEED_1000; > > - break; > > - case 100: > > - val |= EMAC_SPLITTER_CTRL_SPEED_100; > > - break; > > - case 10: > > - val |= EMAC_SPLITTER_CTRL_SPEED_10; > > - break; > > - default: > > - return; > > + if (splitter_base) { > > + val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG); > > + val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK; > > + > > + switch (speed) { > > + case 1000: > > + val |= EMAC_SPLITTER_CTRL_SPEED_1000; > > + break; > > + case 100: > > + val |= EMAC_SPLITTER_CTRL_SPEED_100; > > + break; > > + case 10: > > + val |= EMAC_SPLITTER_CTRL_SPEED_10; > > + break; > > + default: > > + return; > > + } > > + writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG); > > } > > > > - writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG); > > + if ((tse_pcs_base) && (sgmii_adapter_base)) > > + tse_pcs_fix_mac_speed(&dwmac->pcs, phy_dev, speed); > > } > > > > -static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device *dev) > > +static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, > > + struct device *dev) > > { > > struct device_node *np = dev->of_node; > > struct regmap *sys_mgr_base_addr; > > u32 reg_offset, reg_shift; > > - int ret; > > - struct device_node *np_splitter; > > + int ret, index; > > + struct device_node *np_splitter = NULL; > > + struct device_node *np_sgmii_adapter = NULL; > > + > > struct resource res_splitter; > > + struct resource res_tse_pcs; > > + struct resource res_sgmii_adapter; > > > > dwmac->stmmac_rst = devm_reset_control_get(dev, > > - STMMAC_RESOURCE_NAME); > > + STMMAC_RESOURCE_NAME); > > if (IS_ERR(dwmac->stmmac_rst)) { > > dev_info(dev, "Could not get reset control!\n"); > > if (PTR_ERR(dwmac->stmmac_rst) == -EPROBE_DEFER) > > @@ -99,6 +113,7 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device * > > dwmac->interface = of_get_phy_mode(np); > > > > sys_mgr_base_addr = syscon_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon"); > > + > > if (IS_ERR(sys_mgr_base_addr)) { > > dev_info(dev, "No sysmgr-syscon node found\n"); > > return PTR_ERR(sys_mgr_base_addr); > > @@ -130,6 +145,74 @@ static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct device * > > } > > } > > > > + np_sgmii_adapter = of_parse_phandle(np, > > + "altr,gmii_to_sgmii_converter", 0); > > + if (np_sgmii_adapter) { > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "hps_emac_interface_splitter_avalon_slave"); > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_splitter)) { > > + dev_err(dev, "%s: ERROR: missing emac splitter address\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->splitter_base = devm_ioremap_resource( > > + dev, &res_splitter); > > + > > + if (IS_ERR(dwmac->splitter_base)) { > > + dev_err(dev, "%s: ERROR: failed mapping emac splitter\n", > > + __func__); > > + return PTR_ERR(dwmac->splitter_base); > > + } > > + } > > + > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "gmii_to_sgmii_adapter_avalon_slave"); > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_sgmii_adapter)) { > > + dev_err(dev, > > + "%s: ERROR: failed mapping adapter\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->pcs.sgmii_adapter_base = > > + devm_ioremap_resource(dev, &res_sgmii_adapter); > > + > > + if (IS_ERR(dwmac->pcs.sgmii_adapter_base)) { > > + dev_err(dev, "%s: failed to mapping adapter\n", > > + __func__); > > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base); > > + } > > + } > > + > > + index = of_property_match_string(np_sgmii_adapter, "reg-names", > > + "eth_tse_control_port"); > > reg-names looks to be specific and mandatory, IMO they should be > documented in the binding. That's the binding for the adapter (the phandle to the sgmii adapter), not the stmac binding itself. Do you mean I should document the sgmii adapter as well? > > > + > > + if (index >= 0) { > > + if (of_address_to_resource(np_sgmii_adapter, index, > > + &res_tse_pcs)) { > > + dev_err(dev, > > + "%s: ERROR: failed mapping tse control port\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + dwmac->pcs.tse_pcs_base = devm_ioremap_resource( > > + dev, &res_tse_pcs); > > can you check with indentation (scripts/Lindent)? Noted. For some reason check_patch didn't catch this. > > > > + > > + if (IS_ERR(dwmac->pcs.tse_pcs_base)) { > > + dev_err(dev, "%s: ERROR: failed mapping tse control port\n", > > + __func__); > > + return PTR_ERR(dwmac->pcs.sgmii_adapter_base); > > + } > > + } > > + } > > dwmac->reg_offset = reg_offset; > > dwmac->reg_shift = reg_shift; > > dwmac->sys_mgr_base_addr = sys_mgr_base_addr; > > @@ -153,6 +236,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac) > > break; > > case PHY_INTERFACE_MODE_MII: > > case PHY_INTERFACE_MODE_GMII: > > + case PHY_INTERFACE_MODE_SGMII: > > val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII; > > break; > > default: > > @@ -172,6 +256,10 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac *dwmac) > > ctrl |= val << reg_shift; > > > > regmap_write(sys_mgr_base_addr, reg_offset, ctrl); > > + > > + if (phymode == PHY_INTERFACE_MODE_SGMII) > > + tse_pcs_init(dwmac->pcs.tse_pcs_base, &dwmac->pcs); > > + > > return 0; > > } > > > > @@ -191,6 +279,12 @@ static void *socfpga_dwmac_probe(struct platform_device *pdev) > > return ERR_PTR(ret); > > } > > > > + ret = socfpga_dwmac_setup(dwmac); > > + if (ret) { > > + dev_err(dev, "couldn't setup SoC glue (%d)\n", ret); > > + return ERR_PTR(ret); > > + } > > + > > return dwmac; > > } > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > new file mode 100644 > > index 0000000..1989bd1 > > --- /dev/null > > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.c > > @@ -0,0 +1,261 @@ > > +/* Copyright Altera Corporation (C) 2016. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License, version 2, > > + * as published by the Free Software Foundation. > > + * > > + * 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. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see . > > + * > > + * Author: Tien Hock Loh > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "stmmac.h" > > +#include "stmmac_platform.h" > > +#include "tse_pcs.h" > > + > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII 0x0 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RGMII 0x1 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_RMII 0x2 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_WIDTH 2 > > +#define SYSMGR_EMACGRP_CTRL_PHYSEL_MASK 0x00000003 > > + > > +#define EMAC_SPLITTER_CTRL_REG 0x0 > > +#define EMAC_SPLITTER_CTRL_SPEED_MASK 0x3 > > +#define EMAC_SPLITTER_CTRL_SPEED_10 0x2 > > +#define EMAC_SPLITTER_CTRL_SPEED_100 0x3 > > +#define EMAC_SPLITTER_CTRL_SPEED_1000 0x0 > > pls use BIT and GENMASK. Noted. Will do the next patch. > > ... > > > +static void config_tx_buffer(u16 data, void __iomem *base) > > +{ > > + writew(data, base + SGMII_ADAPTER_CTRL_REG); > > +} > > hmm, is this function really necessary? I think you can use > directly writew or maybe writew_relaxed. > OK will do. > > + > > +static void tse_pcs_reset(void __iomem *base, struct tse_pcs *pcs) > > +{ > > + int counter = 0; > > + u16 val; > > + > > + val = readw(base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_SW_RST_MASK; > > + writew(val, base + TSE_PCS_CONTROL_REG); > > + > > + while (counter < TSE_PCS_SW_RESET_TIMEOUT) { > > + val = readw(base + TSE_PCS_CONTROL_REG); > > + val &= TSE_PCS_SW_RST_MASK; > > + if (val == 0) > > + break; > > + counter++; > > + udelay(1); > > + } > > + if (counter >= TSE_PCS_SW_RESET_TIMEOUT) > > + dev_err(pcs->dev, "PCS could not get out of sw reset\n"); > > +} > > + > > maybe this should return an error to stop the init if reset is NOK. Yeah you're right. I'll fix this in the next patch. > > > > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs) > > +{ > > + writew(0x0001, base + TSE_PCS_IF_MODE_REG); > > + > > + writew(TSE_PCS_SGMII_LINK_TIMER_0, base + TSE_PCS_LINK_TIMER_0_REG); > > + writew(TSE_PCS_SGMII_LINK_TIMER_1, base + TSE_PCS_LINK_TIMER_1_REG); > > + > > + tse_pcs_reset(base, pcs); > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, > > + pcs->sgmii_adapter_base); > > +} > > + > > +static void pcs_link_timer_callback(unsigned long data) > > +{ > > + u16 val = 0; > > + > > + struct tse_pcs *pcs = (struct tse_pcs *)data; > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + > > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG); > > + val &= TSE_PCS_STATUS_LINK_MASK; > > + > > + if (val != 0) { > > + dev_dbg(pcs->dev, "Adapter: Link is established\n"); > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base); > > + } else { > > + mod_timer(&pcs->link_timer, jiffies + > > + msecs_to_jiffies(LINK_TIMER)); > > + } > > +} > > + > > +static void auto_nego_timer_callback(unsigned long data) > > +{ > > + u16 val = 0; > > + u16 speed = 0; > > + u16 duplex = 0; > > + > > + struct tse_pcs *pcs = (struct tse_pcs *)data; > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + > > + val = readw(tse_pcs_base + TSE_PCS_STATUS_REG); > > + val &= TSE_PCS_STATUS_AN_COMPLETED_MASK; > > + > > + if (val != 0) { > > + dev_dbg(pcs->dev, "Adapter: Auto Negotiation is completed\n"); > > + val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG); > > + speed = val & TSE_PCS_PARTNER_SPEED_MASK; > > + duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK; > > + > > + if (speed == TSE_PCS_PARTNER_SPEED_10 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 10/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_100 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 100/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_FULL) > > + dev_dbg(pcs->dev, > > + "Adapter: Link Partner is Up - 1000/Full\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_10 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_100 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else if (speed == TSE_PCS_PARTNER_SPEED_1000 && > > + duplex == TSE_PCS_PARTNER_DUPLEX_HALF) > > + dev_err(pcs->dev, > > + "Adapter does not support Half Duplex\n"); > > + else > > + dev_err(pcs->dev, > > + "Adapter: Invalid Partner Speed and Duplex\n"); > > ANE is completed but speed or duplex is NOK. Any failure to signalling? > I see that you then enable the adpter in any case. > > Maybe we could try to restart ANE again or force it (reducing the speed) > I wonder what happens if, for some reason, there is some hw problem. In > that case the timer is always running signalling an invalid Parter > speed. Anyway, this is jus a question... I expect that this error will > always point us to a problem to debug further (if occurs). Let me look at how we can handle the case. Perhaps we could do a restart and register the timer again. I'm just worried it will go into an infinite timer registering hogging up the kernel if the hardware really fails. Maybe I can do a n-time retry here. Looking into this. Let me know if you have any opinions on this. I haven't been able to check for this behaviour though, negative testing on this code isn't too easy to simulate. > > > + > > + config_tx_buffer(SGMII_ADAPTER_ENABLE, sgmii_adapter_base); > > + } else { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > + mod_timer(&pcs->an_timer, jiffies + > > + msecs_to_jiffies(AUTONEGO_TIMER)); > > + } > > +} > > + > > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev, > > + unsigned int speed) > > +{ > > + void __iomem *tse_pcs_base = pcs->tse_pcs_base; > > + void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base; > > + u32 val; > > + > > + config_tx_buffer(SGMII_ADAPTER_DISABLE, sgmii_adapter_base); > > + > > + if (phy_dev->autoneg == AUTONEG_ENABLE) { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_AN_EN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val |= TSE_PCS_USE_SGMII_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val |= TSE_PCS_CONTROL_RESTART_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > why do you need to reset here? This is done according to the Altera TSE specification, whenever a configuration changes, it is recommended that the PCS gets reset. > > > + > > + setup_timer(&pcs->an_timer, > > + auto_nego_timer_callback, > > + (unsigned long)pcs); > > + mod_timer(&pcs->an_timer, jiffies + > > + msecs_to_jiffies(AUTONEGO_TIMER)); > > + } else if (phy_dev->autoneg == AUTONEG_DISABLE) { > > + val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG); > > + val &= ~TSE_PCS_CONTROL_AN_EN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val &= ~TSE_PCS_USE_SGMII_AN_MASK; > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + val &= ~TSE_PCS_SGMII_SPEED_MASK; > > + > > + switch (speed) { > > + case 1000: > > + val |= TSE_PCS_SGMII_SPEED_1000; > > + break; > > + case 100: > > + val |= TSE_PCS_SGMII_SPEED_100; > > + break; > > + case 10: > > + val |= TSE_PCS_SGMII_SPEED_10; > > + break; > > + default: > > + return; > > + } > > + writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG); > > + > > + tse_pcs_reset(tse_pcs_base, pcs); > > + > > + setup_timer(&pcs->link_timer, > > + pcs_link_timer_callback, > > + (unsigned long)pcs); > > + mod_timer(&pcs->link_timer, jiffies + > > + msecs_to_jiffies(LINK_TIMER)); > > I wonder if we can have just one timer to manage ANE and LINK. > That would increase the complexity of the code because we would need to check the callback type on when the callback is triggered and call the correct function. > > + } > > +} > > diff --git a/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > new file mode 100644 > > index 0000000..e3d4511 > > --- /dev/null > > +++ b/drivers/net/ethernet/stmicro/stmmac/tse_pcs.h > > @@ -0,0 +1,36 @@ > > +/* Copyright Altera Corporation (C) 2016. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License, version 2, > > + * as published by the Free Software Foundation. > > + * > > + * 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. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see . > > + * > > + * Author: Tien Hock Loh > > + */ > > + > > +#ifndef __TSE_PCS_H__ > > +#define __TSE_PCS_H__ > > + > > +#include > > +#include > > + > > +struct tse_pcs { > > + struct device *dev; > > + void __iomem *tse_pcs_base; > > + void __iomem *sgmii_adapter_base; > > + struct timer_list an_timer; > > + struct timer_list link_timer; > > +}; > > + > > +void tse_pcs_init(void __iomem *base, struct tse_pcs *pcs); > > +void tse_pcs_fix_mac_speed(struct tse_pcs *pcs, struct phy_device *phy_dev, > > + unsigned int speed); > > + > > +#endif /* __TSE_PCS_H__ */ > > >