From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752658AbcFJGM4 (ORCPT ); Fri, 10 Jun 2016 02:12:56 -0400 Received: from mail-bl2on0100.outbound.protection.outlook.com ([65.55.169.100]:8867 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751068AbcFJGMw convert rfc822-to-8bit (ORCPT ); Fri, 10 Jun 2016 02:12:52 -0400 X-Greylist: delayed 85820 seconds by postgrey-1.27 at vger.kernel.org; Fri, 10 Jun 2016 02:12:52 EDT 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: <1465539145.4467.42.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: , , , , , , , , , Chee Nouk Phoon Date: Thu, 9 Jun 2016 23:12:25 -0700 In-Reply-To: References: <1465268516-31480-1-git-send-email-thloh@altera.com> <1465451310.4467.21.camel@ubuntu> 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)(199003)(377454003)(377424004)(54094003)(52314003)(24454002)(189002)(33646002)(50986999)(47776003)(50466002)(81156014)(4326007)(8746002)(105596002)(2906002)(106466001)(76176999)(8676002)(6806005)(33716001)(11100500001)(81166006)(2950100001)(110136002)(92566002)(42186005)(5820100001)(103116003)(87936001)(107886002)(5008740100001)(189998001)(19580395003)(68736007)(86362001)(9686002)(586003)(93886004)(50226002)(8936002)(97736004)(23676002)(4001430100002)(7099028)(99106002)(217873001);DIR:OUT;SFP:1101;SCL:1;SRVR:BLUPR0301MB1537;H:sj-itexedge03.altera.priv.altera.com;FPR:;SPF:SoftFail;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BY2FFO11FD017;1:K0aSUvVlGuMiFlEct0s6JldTJ0ePi1LNWdMKHigM028gd9OTTWn7kJ86qqR2Ei+nSaYwNGfOUm9f+699lu3gF3tTRLRn+SZB+MPVLEyXTnGbsIe1DHwgePQesDoE5w2uQ8WSzVTbcMDPckCKit2mG3nH8cXmvC84gpFm4b6l1WfUmzJ+X/OuwU1hoZkbKqJZInh/CCaZyTk8hdRMn3mN9Msy3eA3HYbBF6sGlO2Z8bFOKa6+54/PSRZZ416uLEnbHVf3G7EN3JU81oox5iGUFldtJXmIkkGWrFm2xDUkKUHV/4MKlgO5itV9iFPMYFs2B9VzeKwZbwm8mgG/1TEBJvxjOgL0VXl1HExdx39DRwWSMemn/bQt/y+jLvDubEbxXkkzfu8Wz3CZE7lTVZNcKOwrGSgG2uOR4yLEoUFwaJy/Zaj0SOlrX2zGPmp+Iy0k4lq5w/5zH8bWSbbeggLIWE0h2Kk2VV35vFkJ0J1vUneXJdgtxxccuwDSL4n4o4wq7ioPgsXCS5QkwEdtYOSgEimpQI2j5lIGuX/BplVCg+kfpXHTE6DdB3uKgiIdGdWMcIVNhsBiNBsgXhJZNszD/w== X-MS-Office365-Filtering-Correlation-Id: 1371f071-a67e-4051-1bc6-08d390f63f81 X-Microsoft-Exchange-Diagnostics: 1;BLUPR0301MB1537;2:ZvlqsD5E5O3z8AVbfv2SRxWKp5ByZ+9lIvaT5P2GsfbcZOT9GlqEVaczmy7ON5tZ9L/EZ/69mZf51OAEMvQSw6o9NV4PI0DwiIX/NNfyR+64IBZt2+hwZQW+A6PQQDvGVgzJZSFZyT08XtOb73ojqaAgVIvca4Nz1Sa2achtFK18vAOlC6XW90RwNUKDfwRH;3:I9qNCuSrZzNYeAnLXr0Ob8XbFcGJZSjZN7c0Rvlk9R/uhA8G1azBgRrQzvdLXLT6Dll79GdwnwoybNABlLKYLzz2Kt/PwTIqlrH0MtAtQr/hUbh8ABpZ7qLVaiewfH2ygMjT/9RuhyvrbbGAa/BbPOFqabJ44V6O7H8HzPICcXu2uRF6UCRFXqL8QYhD3bC+dGR2Cwo6MMX1ju2YdSF+aKbBqoj8qr40L+JNyDxtYnc=;25:ZC4fSmw02r47r+vPfjze4rff4Q9ZriTa2ATC7TLkl566FQp+r4XBJeVmvwZLHiFl/ZoMr34GHQNODWTbRzYBJaZy5Pz24we3T3IGdPzDyT3t5eljsPkIJ+nEz2X9wa0n/kAQlNaKG4OAeRTl2LX3zotEBk5mn2smwSZEqTcAC+ZNlKKJY9myTbN4qa9xsszCNIMcmKy/ap5WwnmjcLWSh812FfFSNCxS+DLUdATl5jRSFgbmD/KAxEFuE4vPHeTXgiDH4SVDw8zO5/Yeyq3kfE04BVtn6V0+YuJvbXT4q7rQCjzv1pZ1z99TtR5Wtoj/31SEvEwpvWFI5mYOH8E5s9fOCoOb6kU2yFr3w5SmzP+rjAuaIZLeGBUocoUzXRuI/3kQVTSV/i30XQRru27UA32DzqQLZCCpweEeTAmd98w= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BLUPR0301MB1537; X-Microsoft-Exchange-Diagnostics: 1;BLUPR0301MB1537;20:7StJ5Y0fwWRawKN36zwWC1D3DG36BdY9iRFYatQiUdX8XkO0elTszIxeQ5dTd/svOnuw54FPtnYSvcbTyGsbxDJjYNMKyYN/XqwjazGZgYtqj6jmFaJ+1xttWJC1mJjhGBgqnS+Rt6Sf/alXWZtnZd36+vc4evDS4W8//ULRTwQ=;4:soL4KwTBwSzv9MOqgWs7z1YSLsGvm9tmladEkHfFJR/eO0l2aKE1PwWDf57mBndw8u03sHffcWrs5iweJAlucE0w8GYXGh6R8ifjnhx3Ht5vxCBJfXOLxgzeeQMT/dK0vBDqmSR6qRGq71HyyxuI7i5aGQIeNR8Ar6TuZwY2aRVBYBAUI4XKpx9uhofQb2Ssxk7tltIVkG+LMgbpGzpGIpcltma85nmTu9uCTJSJyoyTd+7JrRKuNrxdTBncixcBr+t80n0Tq/GJkMM9aEdwFZCHlCeGRPZxUoeuJmcCzeivm93OJh9OYUyY8WWxiXKxAOxfQE7SAE1BrDKow0ve0JbC0hu6PGdMbAwI8Vu+yFpgt7/3E8fw+vFSb/VCRk2WHyCICz8NFXnFmNJspn+G5R8GOBqzWko/xBol3FWVHw+43nWLlXOkOGGAqcMTyJBTDNzDVFszKq30coEwpYHIVTJnktEEvybaxPqH9h+GkLw= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(13017025)(5005006)(13015025)(8121501046)(13024025)(13018025)(13023025)(3002001)(10201501046)(6055026);SRVR:BLUPR0301MB1537;BCL:0;PCL:0;RULEID:;SRVR:BLUPR0301MB1537; X-Forefront-PRVS: 096943F07A X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTFVQUjAzMDFNQjE1Mzc7MjM6WGxnbW1zRGF4MytHdDNEcFNKNUpQUldx?= =?utf-8?B?VzdFMmp5cjBrSThhSjdONXQ3SGMrQ0ZjOEk4ajM1Q2taeXVuZzErVmRBL0VI?= =?utf-8?B?d2FDZ2RlRjhaUVRReW5qVXhhRnI5aXJ0NkZIQnBwbTJabFRVRW16UmdOU3V4?= =?utf-8?B?T1ZjWkhaZEZVVkVlZ09rd3JMTmVMc2xYTXBySUdmOEVDcUhYSy9GQk5jNGF1?= =?utf-8?B?NldZVE5UWWhTRVZCdkoxQXltSUV0bjY1cjg2OGMzMmZMblpScmlUeWN1djZO?= =?utf-8?B?NzJ4akMzY2labDd1WkRnOFI5Y0FHaHJhSEIzTzcwRDdaVzhxdVMxKzFjRVM1?= =?utf-8?B?WTRiNkp6RXJ3Z3ZWQ0p5VVB6KzhVQS9OSkhyVUdHSkI1QWZCMWYwRENGcy9E?= =?utf-8?B?dUhJcEUwQXhIUGlkRjRzclFKUFlCNUFvakRqcHZVMVAxSVRkYW1LU2FHTXhh?= =?utf-8?B?dzUvVjMxdmF2ZThScnEzZTJ1NFYrOHhtbFdvUHNkNStCRjRySTk4THpWVnVz?= =?utf-8?B?eWJGRExtSWF1M0QwVHJtd2t1cUFVVWdCTG5sYlFnYjNIalhFbGxkNHBYYjVR?= =?utf-8?B?L05JMWN2WnA3YXBtRkRnL250K245Zy95cWdaUGhlZ3N4Vjg5MWltdW1Calcy?= =?utf-8?B?dTV6aFFuRy9JVlpZRjQwblRheEJaaTJxMmp1RW5SYVRWR2NvK1hlZ29sSGQv?= =?utf-8?B?UDJYNGE4S0pMTEM5YmQxbDVuSEREbHBnRy9zWGFxWS9pZUg0QUlRS0tEb3RE?= =?utf-8?B?eU92bUx0eklWdTd1eHkzLys0THdXTFN3ZGM1bUR0bHVqQnZXSnhuWDRhaUtY?= =?utf-8?B?OFlaWURPZTcvZ2dDSXhOOGRzM2syRjlZdVFjR0k3b0JEZmRvQnRQa3ZYY0tX?= =?utf-8?B?S3BsU1AzYTIrbndGb0pybERlZUJwT3ZyNTMwam03ZDBDVUlaSFczMVdnT3Zn?= =?utf-8?B?WXFJOVNKUmoxdDd6LzVzci9qRWlMdUJGN3lmWkNSSkk2UDc0RHREYWxyODc0?= =?utf-8?B?VzZnL0Fhd2FrVE5VMiszZG13WGREVWhIbC8zZWVBQXF1QXRXMzl5c0Q4RVlp?= =?utf-8?B?cWRWQmN0WUFuc3VZVkFuZTAxcVIwZ3lGWkZ2ZXlHSkU2ZmRKUlgyUmViZW1U?= =?utf-8?B?TGhOVUhrT2xOOTY2d0tEVFYrOGxVRitEZlJsdEpSamRqUG1USjdsRC95aUI3?= =?utf-8?B?YitjaURmbkQ5WFBUU21KZGhzczRaZjY1TTZ0MTZkME5UbWdXQkhiZmd6TEw4?= =?utf-8?B?M3hIRVljNHRiOFNoUkxkeHV5bTJWVUw2dkMwSU5NMmFoZXBZYUtnajhXeCth?= =?utf-8?B?ZlRFdkJTQlZ3VWtTY0M4bEw5UGppemo5Y1VSNE9EUE9QNHp1OFFhQmk4WkVt?= =?utf-8?B?cEhCYWJicStkckFXZk5QRVFmalFDRWkySnM3VStQK3JEN0RLWFVUbUs0MFY3?= =?utf-8?B?T3BSSTlwK2xTWVpUNXhGVzNuYUY0cnZLRkFRR1N2US8zNHd1dCtyaU5OZWlW?= =?utf-8?B?dEQyaTNQNmZEbklWR24wdnBhYWFRM3V1UkJqRTJaeTNFYk1tL3VBN3pGTDRB?= =?utf-8?B?MkZMdUwwMWltdU52dkYrakoxaGQvcXFSZklhSTVnTklNR1BIZDJhZHZ1NGpW?= =?utf-8?B?WTAyMlpUK1dQb1B1THhnRk5ITkZUYzdFZUJXcEU0UEhJVmZEV3FGeUkybE1v?= =?utf-8?Q?UUpWAsNjXyeX+3POi2ie9Xd4Y/hcgHbwTQC6Ugw47?= X-Microsoft-Exchange-Diagnostics: 1;BLUPR0301MB1537;5:KJ2iTOg8xyGBSAbwZejqcnVBLUKhj7FBiKj41xH/2w58aKe6IEVoY0/sRqAfoEgXwDbacuTdRao73QTdGQwxaU9n1ZxVC12LfGPSK/JJGBRZYMqXAmjoQSxlmzxY7+YBwu4Z7cS/Gkc7FShVsTjlug==;24:Pva35WQ6/XqIvghkhDas2u9DuziGMQ6XwC/sLAJbS7Nf/qkMlyIBW1RSpjQRXrEoqumLOp80/Cd8aiEhSDUqLSvYt3pIMOYlOa98WVWxe8A=;7:AV6yTq0mf5GdyZed957SIkhCQe//4Tu4ZJN1xSgO4o0OhVVCrvQLLxuCR8O40+U8DIXUwG86GyAcszuFXEQ1mGHMn0SVveX33s4MKS/KVsoWWEuPZ/ylwTMEkf/3lEVMr/bBmqw/WhqULLjV/bS5RnFfmIFi9ZUYCnpUaNKdiLBZ2VQYqqsXuNH3v5rWD2B1Ih2BvsoMXLg1IucJ9+1oSQ==;20:T26tNWG5Kd96OuYo2y3Q3ZwK0hkAw4f202SE4o/qVpZ71LKXzeIBt26luIuBA7b2gxCQsB3/KA9C9vh5uise41xVMIyqt/V1n6Vy6l5NoqwaqnCg32Vy3/3Q8trKqScQYpkbgFYpbQlh0QtweSo4L1loMi1jd5hP9gkGakjrCbg= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: altera.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2016 06:12:48.8968 (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: BLUPR0301MB1537 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peppe, On Wed, 2016-06-08 at 23:20 +0000, Giuseppe CAVALLARO wrote: > Hello Tien Hock > > On 6/9/2016 7:48 AM, Tien Hock Loh wrote: > > [snip] > > >>> .../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. > > ok for two files w/o sub-dir. > > > > > 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? > > yes you can use this name convention. I let you decide. What I meant was we've documented the bindings in socfpga-dwmac.txt for platform specific bindings, and I won't be updating stmmac.txt because that is the generic driver binding. Agree? > > [snip] > > > >>> + > >>> + 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? > > no I just meant for the adapter binding, I had understood that > eth_tse_control_port and gmii_to_sgmii_adapter_avalon_slave > were not documented and these seem to be mandatory. OK noted. > > [snip] > > >>> + > >>> +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; > > [snip] > > >> > >> 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. > > yes and I expect this can occur on hw / conf problems. Take a look at > how the Physical Abstraction Layer manages this. > Indeed, we can try to restart ANE for a while and then just report the > failure (dev_err). Or we can try to fix other speed or duplex. But not > sure this is a good solution. We just mask a problem. Done some read up on the generic PHY in Physical Abstraction Layer and it halts the PHY on aneg failure. I guess we can do do the same then, to not enable the SGMII adapter. > > [snip] > > >>> + > >>> + 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. > > hmm, in that case, you have two timers and no lock protection: > I suspect there could be some hidden problem. The link goes down > and a timer polls this then another one try to restart ANE and > both timers read the TSE_PCS_STATUS_REG. > IMO, a timer is enough and you could keep the code to manage ANE and > LINK in two different functions. Pls take a look at if this is feasible. Yeah you're right about the lock protection. I'll patch them to use one timer. > > Peppe Tien Hock