Date:	Wed, 23 Jan 2013 10:42:44 +0100
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Byungho An <bh74.an@...sung.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	davem@...emloft.net, jeffrey.t.kirsher@...el.com,
	kgene.kim@...sung.com, cpgs@...sung.com
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@...sung.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);
>>>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists