Messages in this thread | | | Date | Wed, 23 Jan 2013 10:42:44 +0100 | From | Giuseppe CAVALLARO <> | Subject | Re: [PATCH net-next 1/3] net: stmmac: add gmac autoneg set for SGMII, TBI, and RTBI |
| |
Hello Byungho
On 1/18/2013 6:41 PM, Byungho An wrote: > Hello Peppe, > > On 1/15/2013 11:28 PM, Giuseppe CAVALLARO write: >> On 1/15/2013 10:45 PM, Byungho An wrote: >>> >>> This patch adds gmac autoneg set function for SGMII, TBI, or RTBI >>> interface. In case of PHY's autoneg is set, gmac's autoneg enable bit >>> should set. After checking phy's autoneg if phydev's autoneg is '1' >>> gmac's ANE bit set for those interface. >> >> Sorry I've some doubts about these patches. >> >> Firstly if the MAC is able to manage RGMII/SGMII etc this should be > verified >> by looking at the HW cap register: i.e. PCS bit. >> >> (I have no HW that support this so I cannot do any tests). >> > I agree with you and actually I tried to use a PCS bit previously. > (PCS bit indicates TBI, SGMII, or RTBI PHY interface) > But I was confused which one I should use among PSC, ACTPHYIF or phydev to > recognize the interface. > At this time, I want to add auto-negotiation enable because sgmii interface > is not working without ANE using PCS bit(in H/W feature register). >
really strange, from the synopsys databook we should only look at the RO bit in the HW cap reg just to understand if the feature is supported.
>> In case of this feature is actually supported then the driver could manage >> everything bypassing the MDIO. >> IMO, we could not need to call the stmmac_phy_init and we should not use >> the PHYLIB. >> I mean if we have the PCS module we could have a minimal support to get >> link status, restart ANE etc w/o using at all the PHYLIB (so w/o touching > the >> PHY regs via the MDIO/MDC). >> > Yes. For example SGMII/RGMII/SMII Status Register is useful to know status > so we can use this register to manage status of SGMII/RGMII/SMII > > >> It could also be useful to complete the support with the RGMII... no >> extra effort should be needed while adding SGMII if you look at the >> core registers. >> If you add the RGMII on some platforms we could guarantee to manage >> the fix_mac_speed (see stmmac doc). >> > Unfortunately I have only the SGMII.
no problem, I'll complete it by my hand later.
> >> Looking at the other your patches, the ethtool support is not completed. I >> expected to restart ANE, get/set link property etc. >> > What is link property? It means link up, speed and duplex mode. > Are there anything else? > I think get link property is already working using ethtool. > In case of SGMII/RGMII/SMII, I think we can use SGMII/RGMII/SMII Status > Register for link status. > > I have a question regarding it. > In the mainline code, there is hardcode interrupt mask in > dwmac1000_core_init function. > When I try to get interrupt for link change, interrupt is not occurred > because of this mask. > Can we modify that?
yes you can. I had wrote an initial patch that started doing something. I have tested it on my boards and I can send it to the mailing list to be reviewed. Maybe you can build your patch on top of it.
>> Also pay attention to properly treat EEE. Maybe, as first stage we should >> disable the feature in this case. We will see it later. >> The question is that we could not be able to use some extra features that >> currently need to dialog more with the PHY device. I mean what we actually >> do by using PHYLIB. > OK. As I understood, using gmac register instead of phydev or phylib... > I agree. In my opinion gmac and phy communicate using mdio each other, after > that gmac can get updated status.
yep, this is the point :-)
>> >>> >>> Signed-off-by: Byungho An <bh74.an@samsung.com> >>> --- >>> drivers/net/ethernet/stmicro/stmmac/common.h | 1 + >>> drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c | 11 >> +++++++++++ >>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 +++++++++ >>> 3 files changed, 21 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h >>> b/drivers/net/ethernet/stmicro/stmmac/common.h >>> index 186d148..72ba769 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h >>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h >>> @@ -344,6 +344,7 @@ struct stmmac_ops { >>> void (*reset_eee_mode) (void __iomem *ioaddr); >>> void (*set_eee_timer) (void __iomem *ioaddr, int ls, int tw); >>> void (*set_eee_pls) (void __iomem *ioaddr, int link); >>> + void (*set_autoneg) (void __iomem *ioaddr); >>> }; >>> >>> struct mac_link { >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> index bfe0226..a0737b39 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c >>> @@ -297,6 +297,16 @@ static void dwmac1000_set_eee_timer(void >> __iomem >>> *ioaddr, int ls, int tw) >>> writel(value, ioaddr + LPI_TIMER_CTRL); >>> } >>> >>> +static void dwmac1000_set_autoneg(void __iomem *ioaddr) { >>> + u32 value; >>> + >>> + value = readl(ioaddr + GMAC_AN_CTRL); >>> + value |= 0x1000; >> >> pls use define instead of raw values ... see below. >> >>> + writel(value, ioaddr + GMAC_AN_CTRL); } >>> + >>> + >>> static const struct stmmac_ops dwmac1000_ops = { >>> .core_init = dwmac1000_core_init, >>> .rx_ipc = dwmac1000_rx_ipc_enable, >>> @@ -311,6 +321,7 @@ static const struct stmmac_ops dwmac1000_ops = { >>> .reset_eee_mode = dwmac1000_reset_eee_mode, >>> .set_eee_timer = dwmac1000_set_eee_timer, >>> .set_eee_pls = dwmac1000_set_eee_pls, >>> + .set_autoneg = dwmac1000_set_autoneg, >>> }; >>> >>> struct mac_device_info *dwmac1000_setup(void __iomem *ioaddr) diff >>> --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> index f07c061..3e28934 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >>> @@ -1007,6 +1007,7 @@ static int stmmac_open(struct net_device *dev) >>> { >>> struct stmmac_priv *priv = netdev_priv(dev); >>> int ret; >>> + int interface = priv->plat->interface; >>> >>> clk_prepare_enable(priv->stmmac_clk); >>> >>> @@ -1041,6 +1042,14 @@ static int stmmac_open(struct net_device *dev) >>> /* Initialize the MAC Core */ >>> priv->hw->mac->core_init(priv->ioaddr); >>> >>> + /* If phy autoneg is on, set gmac autoneg for SGMII, TBI and RTBI*/ >>> + if (interface == PHY_INTERFACE_MODE_SGMII || >>> + interface == PHY_INTERFACE_MODE_TBI || >>> + interface == PHY_INTERFACE_MODE_RTBI) { >>> + if (priv->phydev->autoneg) >>> + priv->hw->mac->set_autoneg(priv->ioaddr); >>> + } >> >> we could use the following instead of priv->hw->mac->set_autoneg: >> >> static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool restart) { >> int value = GMAC_CTRL_ANE_EN; >> >> if (restart) >> value |= GMAC_CTRL_ANE_RESTART; >> >> writel(value, ioaddr +GMAC_AN_CTRL); } >> >> where we should defines all the missing macros for the registers 48, 49 > ... > Actually I tested this code for restart autonegotiation, but even though I > changed link (1Gb -> 100Mb) phy's link is not changed. > For more detail, first time 1Gb I changed the link during system working and > then check speed using ethtool/mii-tool. > I think phy also should do autonegotiation (I'm not sure this is just for > the SGMII). Without phy auto negotiation gmac is not working. > > OK, I'll try to check functionality and make macros. > Anyway as a first step, I want to complete to the code regarding ANE (ctrl > function, macros, ethtool support for link status etc..) > I'll send new code before making patch soon.
thanks you
>> >> /* RGMI/SGMII defines */ >> #define GMAC_CTRL_ANE_SGMII_RAL (1 << 18) >> #define GMAC_CTRL_ANE_EN (1 << 12) >> #define GMAC_CTRL_ANE_RESTART (1 << 9) >> >> The handler should store the link status that will be used to pass the > info to >> the ethtool for example. We need to manage speed and duplex etc. >> >> What happens if you run "ethtool eth0" command? >> Or if you run mii-tool? >> I expect to get the right speed and duplex at least. > So far, I can get right data speed and duplex and autoneg. > >> >> Concerning the stmmac_set_pauseparam I'm also not sure you are doing the >> right thing. Note that you are not restarting the ANE at all ... (this is > what the >> phy_start_aneg does). If set the bit 12 then you are enabling the ane. To >> restart it, you need to set the bit 9 in the reg 48. >> >> I can support you on all the points above. Let me know. >> >> BR, >> peppe >> > OK, thank you and happy new year.
Also to you :-)
let me know for any progresses etc.
Peppe
> Byungho An > >>> + >>> /* Request the IRQ lines */ >>> ret = request_irq(dev->irq, stmmac_interrupt, >>> IRQF_SHARED, dev->name, dev); >>> > > >
| |