Hello Peppe,
> Hello Byungho
>
> sorry for my late reply. I'm attaching two patches built for net-next
> as we had clarified. I had written the first patch long time ago
> and on top of it I have added some part of your code below with some
> fixes (see also the comments inline below).
>
> This work is not yet completed but I do hope these two patches will
> help you to complete all. Unfortunately, I cannot do any tests
> because I have no HW that supports PCS. :-(
>
> In the second patch, take a look at the comment that reports
> the missing parts. For example, ethtool, SGMII etc.
>
> I wonder if you could review/test the two patches on your side.
Looks good to me and it works fine on my hardware platform. Just note, I
tested them with some additional stuff which is in stmmac_init_phy() and
stmmac_mdio_register() for SGMII.
> Also I hope you can improve all adding the missing features (that is
> what you were already doing).
>
> If you agree, you could also re-send *all* to the mailing list to
> be finally reviewed.
>
Anyway, in my opinion, you can take them in your tree for now with my
tested-by if you want. Of course, I'll implement additional patches as you
requested on top of them for remained stuff such as SGMII, TBI, ethtool and
so on.
I think those should be separated for each purpose and we can add and modify
after those two patches.
On 2/22/2013 10:418 PM, Giuseppe CAVALLARO wrote:
> On 1/25/2013 11:01 PM, Byungho An wrote:
> >
> > On 1/23/2013 1:43 PM, Giuseppe CAVALLARO write:
>
> [snip]
>
> >>
> > I modified this part like below
> >
> > @@ -1044,12 +1046,8 @@ static int stmmac_open(struct net_device *dev)
> > priv->hw->mac->core_init(priv->ioaddr);
> >
> > /* Enable auto-negotiation for SGMII, TBI or 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);
> > - }
> > + if (priv->dma_cap.pcs)
> > + priv->hw->mac->ctrl_ane(priv->ioaddr, 0);
> >
> > /* Request the IRQ lines */
> > ret = request_irq(dev->irq, stmmac_interrupt,
> >
> > As you may know, auto-negotiation is essential for SGMII, TBI, or RTBI
> > interface.
> >
>
> good, add it on top of the second patch.
this part is already in the second patch. I think restart flag should be 0
because auto-negotiation does not restarted in the stmmac_open function.
>
> > And ctrl_ane funciont is like that
> >
> > @@ -311,6 +317,18 @@ static void dwmac1000_set_autoneg(void __iomem
> *ioaddr)
> > writel(value, ioaddr + GMAC_AN_CTRL);
> > }
> >
> > +static void dwmac1000_ctrl_ane(void __iomem *ioaddr, bool restart)
> > +{
> > + u32 value;
> > +
> > + value = readl(ioaddr + GMAC_AN_CTRL);
> > + /* auto negotiation enable and External Loopback enable */
> > + value = GMAC_AN_CTRL__ANE | GMAC_AN_CTRL__ELE;
> > +
> > + if (restart)
> > + value |= GMAC_AN_CTRL_RAN;
> > +
> > + writel(value, ioaddr + GMAC_AN_CTRL);
> > +}
> >
> > static const struct stmmac_ops dwmac1000_ops = {
> > .core_init = dwmac1000_core_init,
>
> well done and added in the second patch.
>
> [snip]
> > I've changed restart AN like below.
> >
> > @@ -610,6 +607,27 @@ static int stmmac_set_coalesce(struct net_device
> *dev,
> > return 0;
> > }
> >
> > +static int
> > +stmmac_nway_reset(struct net_device *netdev)
> > +{
> > + struct stmmac_priv *priv = netdev_priv(netdev);
> > + struct phy_device *phy = priv->phydev;
> > + int ret = 0;
> > +
> > + spin_lock(&priv->lock);
> > +
> > + if (netif_running(netdev)) {
> > + phy_stop(phy);
> > + phy_start(phy);
> > + ret = phy_start_aneg(phy);
> > + if (priv->dma_cap.pcs)
> > + priv->hw->mac->ctrl_ane(priv->ioaddr, true);
> > + }
> > +
> > + spin_unlock(&priv->lock);
> > + return ret;
> > +}
> > +
> > static const struct ethtool_ops stmmac_ethtool_ops = {
> > .begin = stmmac_check_if_running,
> > .get_drvinfo = stmmac_ethtool_getdrvinfo,
> >
> >
>
> we have to review this. I expect to have a new patch that includes the
> ethtool support but, at first glance, the stmmac_nway_reset should only
> call the ctrl_ane.
>
> pay attention that also some other ethtool functions have to be fixed
> for this support.
>
I agree with you, I need more time to test ethtool support including
advertisement support.
> [snip]
>
> > I think whenever link is changed, phy->state is changed and call
> > stmmac_adjust_link. It would update gmac's link.
> > I can get autonegotiation complete and link change irqs if we need
> something
> > we can add code in the handler but I'm not sure which one is need yet.
> > As of now I just added phy_state = PHY_CHANGELINK as a test code in the
> link
> > change irq handler.
> >
> > @@ -1624,8 +1629,43 @@ 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_autoneg_complete)
> > + priv->core_pcs_an = true;
> > + if (status & core_irq_pcs_link_status_change) {
> > + priv->core_pcs_link_change = true;
> > + status = readl(priv->ioaddr +
> > GMAC_AN_STATUS);
> > + if (status & GMAC_AN_STATUS_LS)
> > + if ((priv->speed != phy->speed)
||
> > (priv->oldduplex != phy->duplex))
> > + phy->state = PHY_CHANGELINK; /*
for
> > test */
> > + }
> >
> > /* For LPI we need to save the tx status */
> > if (status & core_irq_tx_path_in_lpi_mode) {
> >
> > I have a question, how to hand over phy's irq number, as I analyzed
> > phydev->irq is irqlist and irqlist is like below but I can not find a
> way to
> > set phy's irq number.
> > How to register or set priv->mii_irq or mdio_bus_data->irqs.
> >
> > if (mdio_bus_data->irqs)
> > irqlist = mdio_bus_data->irqs;
> > else
> > irqlist = priv->mii_irq;
>
> I had added something in my first patch that should be ok.
>
> > I added some defines for AN like below
> > @@ -97,6 +97,20 @@ enum power_event {
> > #define GMAC_TBI 0x000000d4 /* TBI extend status */
> > #define GMAC_GMII_STATUS 0x000000d8 /* S/R-GMII status */
> >
> > +/* AN Configuration defines */
> > +#define GMAC_AN_CTRL_RAN 0x00000200 /* Restart
Auto-Negotiation
> > */
> > +#define GMAC_AN_CTRL_ANE 0x00001000 /* Auto-Negotiation
Enable
> > */
> > +#define GMAC_AN_CTRL_ELE 0x00004000 /* External Loopback
Enable
> > */
> > +#define GMAC_AN_CTRL_ECD 0x00010000 /* Enable Comma Detect
*/
> > +#define GMAC_AN_CTRL_LR 0x00020000 /* Lock to Reference */
> > +#define GMAC_AN_CTRL_SGMRAL 0x00040000 /* SGMII RAL Control */
> > +
> > +/* AN Status defines */
> > +#define GMAC_AN_STATUS_LS 0x00000004 /* Link Status 0:down
1:up
> > */
> > +#define GMAC_AN_STATUS_ANA 0x00000008 /* Auto-Negotiation
> Ability
> > */
> > +#define GMAC_AN_STATUS_ANC 0x00000020 /* Auto-Negotiation
> Complete
> > */
> > +#define GMAC_AN_STATUS_ES 0x00000100 /* Extended Status */
> > +
> > /* GMAC Configuration defines */
> > #define GMAC_CONTROL_TC 0x01000000 /* Transmit Conf. in
> > RGMII/SGMII */
> > #define GMAC_CONTROL_WD 0x00800000 /* Disable Watchdog on
> > receive */
>
> ok, these are in the second patch
On 3/6/2013 5:13 AM, Byungho An wrote:
> Hello Peppe,
>> If you agree, you could also re-send *all* to the mailing list to
>> be finally reviewed.
>>
>
> Anyway, in my opinion, you can take them in your tree for now with my
> tested-by if you want. Of course, I'll implement additional patches as you
> requested on top of them for remained stuff such as SGMII, TBI, ethtool and
> so on.
> I think those should be separated for each purpose and we can add and modify
> after those two patches.
Ok Byungho, I'm preparing a new update for the stmmac that will also
include the initial support for SGMII and RGMII. Thx to have tested it!
Obviously on top of these patches, feel free to add further code.
TBI and RTBI are not supported yet.
P.S. in this update I've also added the ethtool missing stuff ;-)
Peppe