From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755708AbcFIGUb (ORCPT ); Thu, 9 Jun 2016 02:20:31 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:64058 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753622AbcFIGU2 (ORCPT ); Thu, 9 Jun 2016 02:20:28 -0400 Subject: Re: [PATCH v3 1/1] net: ethernet: Add TSE PCS support to dwmac-socfpga To: Tien Hock Loh References: <1465268516-31480-1-git-send-email-thloh@altera.com> <1465451310.4467.21.camel@ubuntu> CC: , , , , , , , , , From: Giuseppe CAVALLARO Message-ID: Date: Thu, 9 Jun 2016 08:20:05 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <1465451310.4467.21.camel@ubuntu> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.52.139.54] X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-06-08_11:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. [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. [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. [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. Peppe