RE: [PATCH 1/3] net: stmmac: change GMAC control register for SGMII

[Byungho An]

On 12/04/2012 06:35 AM, Giuseppe CAVALLARO wrote:
>On 11/28/2012 11:57 AM, Byungho An wrote:
>> On 11/26/2012 07:31 PM, Giuseppe CABALLARO wrote:
>>> On 11/23/2012 10:04 AM, Byungho An wrote:
>>>>
>>>> This patch changes GMAC control register (TC(Transmit
>>>> Configuration) and PS(Port Selection) bit for SGMII.
>>>> In case of SGMII, TC bit is '1' and PS bit is 0.
>>>
>>> IMO this new support that should be released for net-next and further
>>> effort is actually needed.
>>>
>> OK, I see but if possible, I want to support the new features which is
>> included in this patch from v3.8
>
>ok I agree and I can support you.
>
>>
>>> The availability of the PCS registers is given by looking at the HW
>>> feature register. In fact, these are optional registers.
>>> I don't want to break the compatibility with old chips.
>>>
>> It means that old chip doesn't have this bit or this register? If that,
how
>> about using compatible in DT blob like snps,dwmac-3.70a and then in just
>> this case trying to read this bit and this register.
>
>The driver also works on mac 10/100 Databook 2.0 that has not these
>registers.
>
>>> I do not see why we have to use Kconfig macro to select ANE etc (as
>>> you do in your patches).
>> OK. I agree with you.
>
>we have to use the HW feature reg.
>
>>
>>> The driver could directly manage the phy device by itself if possible
>>> and the stmmac_init_phy should be reworked.
>>>
>> Could you explain more detail? As I understood, after set ANE bit in MAC
>> side then PHY auto-negotiation can be enabled. If I'm wrong let me know.
>> According to your mention, MAC and PHY auto-negotiation can be managed
>in
>> stmmac_init_phy?
>
>Currently the driver uses the Physical Abstraction Layer (PAL) to dialog
>with a PHY. On all the platforms supported (not only ST) we have always
>used it. Personally, I tested several phy devices with different MII
>interfaces (MII/RMII/GMII/RGMII ... ) but not TBI/RTBI/SGMII interfaces.
>
Currently I'm testing on SGMII interface and so far it's working fine.

>>> There are several things that need to be implemented. For example:
>>>
>>> The ISR (e.g. priv->hw->mac->host_irq_status) should be able to manage
>>> these new interrupts.
>> I think that there would be two additional interrupts."PCS
Auto-Negotiation
>> Complete" and "PCS Link Status Changed". These two interrupts are added
>to
>> "stmmac_interrupt". In my opinion, there are no specific processing for
>> these two irqs. What do you think about it?
>
>if the link changes this has to be logged in the driver.
>For example, depending on the link speed on some platforms we need to
>call dedicated call-back to set sysconfig registers or custom clocks.
>
>>> The code has to be able to maintain the user interface.
>>> For example if you want to enable ANE or manage Advertisement caps.
>>>
>> Does it mean that command line or other network command(e.g. ifconfig...)
>or
>> ioctol? Actually I don't understand exact user interface way. Could you
>> recommend the method for user interface?
>
>Using ethtool or mii-tool the user want to know the link status. So
>these kind of information have to be maintained.
>
As I know, user can set speed, duplex mode and auto-negotiation on/off using
ethtool.
If user wants to set auto-negotiation on, I think GMAC's ANE bit should be
ON as well.
For example, in stmmac_ethtool.c file, I try to add GMAC ANE bit enable
setting.
I think stmmac_set_pauseparam function is suitable for it.

	if (phy->autoneg) {
		if (netif_running(netdev)) {
 			ret = phy_start_aneg(phy);
+			value = readl(priv->ioaddr + GMAC_AN_CTRL);
+			value |= 0x1000;
+			writel(value, priv->ioaddr + GMAC_AN_CTRL);
+		}

What's your opinion?

>Take a look at the stmmac_adjust_link that is called by the PAL.
>
And for speed setting, if user changes speed to 1Gb using ethtool, link
status will be changed and then interrupt is occurred.
In the ISR, If interface is SGMII, I want to add SGMRAL setting and PS(port
selection)for guarantee 1Gb speed.
In the dwmac1000_core.c file.

+	if (unlikely(intr_status & pcs_link_irq)) {
+		readl(ioaddr + GMAC_AN_STATUS);
+		status |= core_irq_pcs_link;

+		if (priv->phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+			value = readl(priv->ioaddr);
+			/* GMAC_CONTROL_PS : Port Selection for GMII */
+			value &= ~GMAC_CONTROL_PS;
+			writel(value, priv->ioaddr);
+			value = readl(priv->ioaddr + GMAC_AN_CTRL);
+			value = |= 0x40000;
+			writel(value, priv->ioaddr + GMAC_AN_CTRL);
+		}		
+	}

Then stmmac_interrupt in stmmac_main.c file call stmmac_adjust_link when
status is core_irq_pcs_link.
 
For auto-negotiation complete interrupt, I try to make patch like below

drivers/net/ethernet/stmicro/stmmac/common.h
@@ -184,6 +184,7 @@ enum core_specific_irq_mask {
 	core_irq_tx_path_exit_lpi_mode = 32,
 	core_irq_rx_path_in_lpi_mode = 64,
 	core_irq_rx_path_exit_lpi_mode = 128,
+	core_irq_rx_pcs_an = 256,
 };
 
 /* DMA HW capabilities */

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -231,6 +231,11 @@ static int dwmac1000_irq_status(void __iomem *ioaddr)
 		readl(ioaddr + GMAC_PMT);
 		status |= core_irq_receive_pmt_irq;
 	}
+	if (unlikely(intr_status & pcs_ane_irq)) {
+		CHIP_DBG(KERN_INFO "GMAC: PCS Auto-negotiation complete\n");
+		readl(ioaddr + GMAC_AN_STATUS);
+		status |= core_irq_pcs_an;
+	}
 	/* MAC trx/rx EEE LPI entry/exit interrupts */
 	if (intr_status & lpiis_irq) {
 		/* Clean LPI interrupt by reading the Reg 12 */

drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -93,6 +93,7 @@ struct stmmac_priv {
 	int eee_enabled;
 	int eee_active;
 	int tx_lpi_timer;
+	bool core_pcs_an;
 };
 
 extern int phyaddr;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1658,6 +1658,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
*dev_id)
 				priv->xstats.mmc_rx_csum_offload_irq_n++;
 			if (status & core_irq_receive_pmt_irq)
 				priv->xstats.irq_receive_pmt_irq_n++;
+			if (status & core_irq_pcs_an)
+				priv->core_pcs_an = true;
 
 			/* For LPI we need to save the tx status */
 			if (status & core_irq_tx_path_in_lpi_mode) {

>>>> Signed-off-by: Byungho An <bh74.an@xxxxxxxxxxx>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +	if (priv->phydev->interface == PHY_INTERFACE_MODE_SGMII) {
>>>> +		value = readl(priv->ioaddr);
>>>> +		/* GMAC_CONTROL_TC : transmit config in RGMII/SGMII */
>>>> +		value |= 0x1000000;
>>>> +		/* GMAC_CONTROL_PS : Port Selection for GMII */
>>>> +		value &= ~(0x8000);
>>>> +		writel(value, priv->ioaddr);
>>>> +	}
>>>> +
>>>
>>>
>>> This parts of code have to be moved in
>>> drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
>>>
>> OK.
>>
I moved this code todwmac1000_core.c using hw feature register like below

+#include "dwmac_dma.h"
 
 static void dwmac1000_core_init(void __iomem *ioaddr)
 {
 	u32 value = readl(ioaddr + GMAC_CONTROL);
+	u32 hw_feature = readl(ioaddr + DMA_HW_FEATURE);
 	value |= GMAC_CORE_INIT;
 	writel(value, ioaddr + GMAC_CONTROL);
+	hw_feature &= DMA_HW_FEAT_ACTPHYIF;
+
+	if ((hw_feature >> 28 < 2) && (hw_feature != 0) {
+		/* transmit config in RGMII/SGMII */
+		value |= GMAC_CONTROL_TC;
+	}
+	writel(value, ioaddr + GMAC_CONTROL);

>>> Pls, do not use value |= 0x1000000 but provide the appropriate defines.
>>>
>> OK.
>>
>>>>    	/* Request the IRQ lines */
>>>>    	ret = request_irq(dev->irq, stmmac_interrupt,
>>>>    			 IRQF_SHARED, dev->name, dev);
>>>>
>> Thank you.
>
>you are welcome
>Peppe
>
>> Byungho An.
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
Thank you.
Byungho An.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/