Re: [PATCH 1/1] net: ethernet: Add SGMII support to dwmac-socfpga

[Giuseppe CAVALLARO]

Hello Loh Tien

On 5/31/2016 11:10 AM, Loh Tien Hock wrote:
> Hi Peppe,
>
> Sorry for the late reply.

no pbl at all.

> I believe my patch's title is a little confusing. The patch is to
> enable Altera TSE PCS SGMII support, not to add SGMII support to
> stmmac (well that in a way tests SGMII for stmmac though)

ok

> I went through the code and it seems like if we were to refactor PCS
> defines (and codes) into common code it would bloat up the common
> codes.
> Would it be better if we create new PCS source files (eg.
> altr_tse_pcs.c/.h)? This means the rest of the PCS supports should
> also be refactored out as well for consistency purposes.

yes agree.

Just an info to better understand the case; after enabling the SGMII on
Altera, do you test in some way the PCS SGMII mode in the main layer?

Regards
Peppe

> Thanks
>
> On Fri, May 13, 2016 at 3:47 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@xxxxxx> wrote:
> > Hello Tien
> >
> > On 5/13/2016 9:01 AM, thloh@xxxxxxxxxx wrote:
> > > From: Tien Hock Loh <thloh@xxxxxxxxxx>
> > >
> > > Adds SGMII support for dwmac-socfpga to enable the SGMII PHY when phy-mode
> > > of the dwmac is set to sgmii.
> >
> >
> > I wonder if part of this patch can be unified to the common code
> > reusing (or improving) existent PCS support (see, for example the
> > defines inside the common.h and dwmac1000.h header files).
> > I had added some code for RGMII but never tested the SGMII indeed.
> >
> > what is your feeling about that?
> >
> > Peppe
> >
> >
> > > Signed-off-by: Tien Hock Loh <thloh@xxxxxxxxxx>
> > > ---
> > >   .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c    | 339
> > > ++++++++++++++++++++-
> > >   1 file changed, 329 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > > index 41f4c58..a59d590 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> > > @@ -40,6 +40,43 @@
> > >   #define EMAC_SPLITTER_CTRL_SPEED_100          0x3
> > >   #define EMAC_SPLITTER_CTRL_SPEED_1000         0x0
> > >
> > > +#define TSE_PCS_CONTROL_AN_EN_MASK                     0x1000
> > > +#define TSE_PCS_CONTROL_REG                            0x00
> > > +#define TSE_PCS_CONTROL_RESTART_AN_MASK                0x0200
> > > +#define TSE_PCS_IF_MODE_REG                            0x28
> > > +#define TSE_PCS_LINK_TIMER_0_REG                       0x24
> > > +#define TSE_PCS_LINK_TIMER_1_REG                       0x26
> > > +#define TSE_PCS_SIZE                                   0x40
> > > +#define TSE_PCS_STATUS_AN_COMPLETED_MASK               0x0020
> > > +#define TSE_PCS_STATUS_LINK_MASK                       0x0004
> > > +#define TSE_PCS_STATUS_REG                             0x02
> > > +#define TSE_PCS_SGMII_SPEED_1000                       0x8
> > > +#define TSE_PCS_SGMII_SPEED_100                        0x4
> > > +#define TSE_PCS_SGMII_SPEED_10                         0x0
> > > +#define TSE_PCS_SW_RST_MASK                            0x8000
> > > +#define TSE_PCS_PARTNER_ABILITY_REG                    0x0A
> > > +#define TSE_PCS_PARTNER_DUPLEX_FULL                    0x1000
> > > +#define TSE_PCS_PARTNER_DUPLEX_HALF                    0x0000
> > > +#define TSE_PCS_PARTNER_DUPLEX_MASK                    0x1000
> > > +#define TSE_PCS_PARTNER_SPEED_MASK                     0x0c00
> > > +#define TSE_PCS_PARTNER_SPEED_1000                     0x0800
> > > +#define TSE_PCS_PARTNER_SPEED_100                      0x0400
> > > +#define TSE_PCS_PARTNER_SPEED_10                       0x0000
> > > +#define TSE_PCS_PARTNER_SPEED_1000                     0x0800
> > > +#define TSE_PCS_PARTNER_SPEED_100                      0x0400
> > > +#define TSE_PCS_PARTNER_SPEED_10                       0x0000
> > > +#define TSE_PCS_SGMII_SPEED_MASK                       0x000C
> > > +#define TSE_PCS_SGMII_LINK_TIMER_0                     0x0D40
> > > +#define TSE_PCS_SGMII_LINK_TIMER_1                     0x0003
> > > +#define TSE_PCS_SW_RESET_TIMEOUT                       100
> > > +#define TSE_PCS_USE_SGMII_AN_MASK                      0x0002
> > > +
> > > +#define SGMII_ADAPTER_CTRL_REG                         0x00
> > > +#define SGMII_ADAPTER_DISABLE                          0x0001
> > > +#define SGMII_ADAPTER_ENABLE                           0x0000
> > > +#define LINK_TIMER                                     20
> > > +#define AUTONEGO_TIMER                                 20
> > > +
> > >   struct socfpga_dwmac {
> > >         int     interface;
> > >         u32     reg_offset;
> > > @@ -49,18 +86,17 @@ struct socfpga_dwmac {
> > >         struct reset_control *stmmac_rst;
> > >         void __iomem *splitter_base;
> > >         bool f2h_ptp_ref_clk;
> > > +       void __iomem *tse_pcs_base;
> > > +       void __iomem *sgmii_adapter_base;
> > > +       struct timer_list an_timer;
> > > +       struct timer_list link_timer;
> > >   };
> > >
> > > -static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
> > > +static void config_emac_splitter_speed(void __iomem *base, unsigned int
> > > speed)
> > >   {
> > > -       struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
> > > -       void __iomem *splitter_base = dwmac->splitter_base;
> > >         u32 val;
> > >
> > > -       if (!splitter_base)
> > > -               return;
> > > -
> > > -       val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> > > +       val = readl(base + EMAC_SPLITTER_CTRL_REG);
> > >         val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> > >
> > >         switch (speed) {
> > > @@ -76,8 +112,214 @@ static void socfpga_dwmac_fix_mac_speed(void *priv,
> > > unsigned int speed)
> > >         default:
> > >                 return;
> > >         }
> > > +       writel(val, base + EMAC_SPLITTER_CTRL_REG);
> > > +}
> > > +
> > > +static void config_tx_buffer(u16 data, void __iomem *base)
> > > +{
> > > +       writew(data, base + SGMII_ADAPTER_CTRL_REG);
> > > +}
> > > +
> > > +static void tse_pcs_reset(void __iomem *base, struct socfpga_dwmac
> > > *dwmac)
> > > +{
> > > +       int counter = 0;
> > > +       u16 val;
> > > +
> > > +       val = readw(base + TSE_PCS_CONTROL_REG);
> > > +       val |= TSE_PCS_SW_RST_MASK;
> > > +       writew(val, base + TSE_PCS_CONTROL_REG);
> > > +
> > > +       while (counter < TSE_PCS_SW_RESET_TIMEOUT) {
> > > +               val = readw(base + TSE_PCS_CONTROL_REG);
> > > +               val &= TSE_PCS_SW_RST_MASK;
> > > +               if (val == 0)
> > > +                       break;
> > > +               counter++;
> > > +               udelay(1);
> > > +       }
> > > +       if (counter >= TSE_PCS_SW_RESET_TIMEOUT)
> > > +               dev_err(dwmac->dev, "PCS could not get out of sw
> > > reset\n");
> > > +}
> > > +
> > > +static void tse_pcs_init(void __iomem *base, struct socfpga_dwmac *dwmac)
> > > +{
> > > +       writew(0x0001, base + TSE_PCS_IF_MODE_REG);
> > > +
> > > +       writew(TSE_PCS_SGMII_LINK_TIMER_0, base +
> > > TSE_PCS_LINK_TIMER_0_REG);
> > > +       writew(TSE_PCS_SGMII_LINK_TIMER_1, base +
> > > TSE_PCS_LINK_TIMER_1_REG);
> > > +
> > > +       tse_pcs_reset(base, dwmac);
> > > +}
> > > +
> > > +static void pcs_link_timer_callback(unsigned long data)
> > > +{
> > > +       u16 val = 0;
> > > +
> > > +       struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)data;
> > > +       void __iomem *tse_pcs_base = dwmac->tse_pcs_base;
> > > +       void __iomem *sgmii_adapter_base = dwmac->sgmii_adapter_base;
> > > +
> > > +       val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> > > +       val &=  TSE_PCS_STATUS_LINK_MASK;
> > > +
> > > +       if (val != 0) {
> > > +               dev_dbg(dwmac->dev, "Adapter: Link is established\n");
> > > +               config_tx_buffer(SGMII_ADAPTER_ENABLE,
> > > sgmii_adapter_base);
> > > +       } else {
> > > +               mod_timer(&dwmac->link_timer, jiffies +
> > > +                               msecs_to_jiffies(LINK_TIMER));
> > > +       }
> > > +}
> > > +
> > > +static void auto_nego_timer_callback(unsigned long data)
> > > +{
> > > +       u16 val = 0;
> > > +       u16 speed = 0;
> > > +       u16 duplex = 0;
> > > +
> > > +       struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)data;
> > > +       void __iomem *tse_pcs_base = dwmac->tse_pcs_base;
> > > +       void __iomem *sgmii_adapter_base = dwmac->sgmii_adapter_base;
> > > +
> > > +       val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
> > > +       val &=  TSE_PCS_STATUS_AN_COMPLETED_MASK;
> > > +
> > > +       if (val != 0) {
> > > +               dev_dbg(dwmac->dev, "Adapter: Auto Negotiation is
> > > completed\n");
> > > +               val = readw(tse_pcs_base + TSE_PCS_PARTNER_ABILITY_REG);
> > > +               speed = val & TSE_PCS_PARTNER_SPEED_MASK;
> > > +               duplex = val & TSE_PCS_PARTNER_DUPLEX_MASK;
> > > +
> > > +               if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> > > +                   duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > > +                       dev_dbg(dwmac->dev,
> > > +                               "Adapter: Link Partner is Up -
> > > 10/Full\n");
> > > +               else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> > > +                        duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > > +                       dev_dbg(dwmac->dev,
> > > +                               "Adapter: Link Partner is Up -
> > > 100/Full\n");
> > > +               else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> > > +                        duplex == TSE_PCS_PARTNER_DUPLEX_FULL)
> > > +                       dev_dbg(dwmac->dev,
> > > +                               "Adapter: Link Partner is Up -
> > > 1000/Full\n");
> > > +               else if (speed == TSE_PCS_PARTNER_SPEED_10 &&
> > > +                        duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > > +                       dev_err(dwmac->dev,
> > > +                               "Adapter does not support Half Duplex\n");
> > > +               else if (speed == TSE_PCS_PARTNER_SPEED_100 &&
> > > +                        duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > > +                       dev_err(dwmac->dev,
> > > +                               "Adapter does not support Half Duplex\n");
> > > +               else if (speed == TSE_PCS_PARTNER_SPEED_1000 &&
> > > +                        duplex == TSE_PCS_PARTNER_DUPLEX_HALF)
> > > +                       dev_err(dwmac->dev,
> > > +                               "Adapter does not support Half Duplex\n");
> > > +               else
> > > +                       dev_err(dwmac->dev,
> > > +                               "Adapter: Invalid Partner Speed and
> > > Duplex\n");
> > > +
> > > +               config_tx_buffer(SGMII_ADAPTER_ENABLE,
> > > sgmii_adapter_base);
> > > +       } else {
> > > +               val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
> > > +               val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> > > +               writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > > +
> > > +               tse_pcs_reset(tse_pcs_base, dwmac);
> > > +               mod_timer(&dwmac->an_timer, jiffies +
> > > +                         msecs_to_jiffies(AUTONEGO_TIMER));
> > > +       }
> > > +}
> > > +
> > > +static void socfpga_dwmac_fix_mac_speed(void *priv, unsigned int speed)
> > > +{
> > > +       struct socfpga_dwmac *dwmac = (struct socfpga_dwmac *)priv;
> > > +       void __iomem *splitter_base = dwmac->splitter_base;
> > > +       void __iomem *tse_pcs_base = dwmac->tse_pcs_base;
> > > +       void __iomem *sgmii_adapter_base = dwmac->sgmii_adapter_base;
> > > +       struct device *dev = dwmac->dev;
> > > +       struct net_device *ndev = dev_get_drvdata(dev);
> > > +       struct phy_device *phy_dev = ndev->phydev;
> > > +       u32 val;
> > >
> > > -       writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> > > +       if ((tse_pcs_base) && (sgmii_adapter_base)) {
> > > +               config_tx_buffer(SGMII_ADAPTER_DISABLE,
> > > sgmii_adapter_base);
> > > +               if (splitter_base)
> > > +                       config_emac_splitter_speed(splitter_base, speed);
> > > +
> > > +               if (phy_dev->autoneg == AUTONEG_ENABLE) {
> > > +                       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_CONTROL_REG);
> > > +                       val |= TSE_PCS_CONTROL_RESTART_AN_MASK;
> > > +                       writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
> > > +
> > > +                       tse_pcs_reset(tse_pcs_base, dwmac);
> > > +
> > > +                       setup_timer(&dwmac->an_timer,
> > > +                                   auto_nego_timer_callback,
> > > +                                   (unsigned long)dwmac);
> > > +                       mod_timer(&dwmac->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, dwmac);
> > > +
> > > +                       setup_timer(&dwmac->link_timer,
> > > +                                   pcs_link_timer_callback,
> > > +                                               (unsigned long)dwmac);
> > > +                       mod_timer(&dwmac->link_timer, jiffies +
> > > +                                 msecs_to_jiffies(LINK_TIMER));
> > > +               }
> > > +       } else if (splitter_base) {
> > > +               val = readl(splitter_base + EMAC_SPLITTER_CTRL_REG);
> > > +               val &= ~EMAC_SPLITTER_CTRL_SPEED_MASK;
> > > +
> > > +               switch (speed) {
> > > +               case 1000:
> > > +                       val |= EMAC_SPLITTER_CTRL_SPEED_1000;
> > > +                       break;
> > > +               case 100:
> > > +                       val |= EMAC_SPLITTER_CTRL_SPEED_100;
> > > +                       break;
> > > +               case 10:
> > > +                       val |= EMAC_SPLITTER_CTRL_SPEED_10;
> > > +                       break;
> > > +               default:
> > > +                       return;
> > > +               }
> > > +               writel(val, splitter_base + EMAC_SPLITTER_CTRL_REG);
> > > +       }
> > >   }
> > >
> > >   static int socfpga_dwmac_parse_data(struct socfpga_dwmac *dwmac, struct
> > > device *dev)
> > > @@ -85,9 +327,13 @@ static int socfpga_dwmac_parse_data(struct
> > > socfpga_dwmac *dwmac, struct device *
> > >         struct device_node *np = dev->of_node;
> > >         struct regmap *sys_mgr_base_addr;
> > >         u32 reg_offset, reg_shift;
> > > -       int ret;
> > > -       struct device_node *np_splitter;
> > > +       int ret, index;
> > > +       struct device_node *np_splitter = NULL;
> > > +       struct device_node *np_sgmii_adapter = NULL;
> > > +
> > >         struct resource res_splitter;
> > > +       struct resource res_tse_pcs;
> > > +       struct resource res_sgmii_adapter;
> > >
> > >         dwmac->stmmac_rst = devm_reset_control_get(dev,
> > >                                                   STMMAC_RESOURCE_NAME);
> > > @@ -134,6 +380,72 @@ static int socfpga_dwmac_parse_data(struct
> > > socfpga_dwmac *dwmac, struct device *
> > >                 }
> > >         }
> > >
> > > +       np_sgmii_adapter = of_parse_phandle(np,
> > > +
> > > "altr,gmii_to_sgmii_converter", 0);
> > > +       if (np_sgmii_adapter) {
> > > +               index = of_property_match_string(np_sgmii_adapter,
> > > "reg-names",
> > > +
> > > "hps_emac_interface_splitter_avalon_slave");
> > > +
> > > +               if (index >= 0) {
> > > +                       if (of_address_to_resource(np_sgmii_adapter,
> > > index,
> > > +                                                  &res_splitter)) {
> > > +                               dev_err(dev, "%s: ERROR: missing emac
> > > splitter "
> > > +                                       "address\n", __func__);
> > > +                               return -EINVAL;
> > > +                       }
> > > +
> > > +                       dwmac->splitter_base = devm_ioremap_resource(
> > > +                               dev, &res_splitter);
> > > +
> > > +                       if (IS_ERR(dwmac->splitter_base)) {
> > > +                               dev_err(dev, "%s: ERROR: failed to mapping
> > > emac"
> > > +                                       " splitter\n", __func__);
> > > +                               return PTR_ERR(dwmac->splitter_base);
> > > +                       }
> > > +               }
> > > +
> > > +               index = of_property_match_string(np_sgmii_adapter,
> > > "reg-names",
> > > +
> > > "gmii_to_sgmii_adapter_avalon_slave");
> > > +
> > > +               if (index >= 0) {
> > > +                       if (of_address_to_resource(np_sgmii_adapter,
> > > index,
> > > +                                                  &res_sgmii_adapter)) {
> > > +                               dev_err(dev, "%s: ERROR: failed to mapping
> > > adapter\n",
> > > +                                       __func__);
> > > +                               return -EINVAL;
> > > +                       }
> > > +
> > > +                       dwmac->sgmii_adapter_base = devm_ioremap_resource(
> > > +                               dev, &res_sgmii_adapter);
> > > +
> > > +                       if (IS_ERR(dwmac->sgmii_adapter_base)) {
> > > +                               dev_err(dev, "%s: failed to mapping
> > > adapter\n",
> > > +                                       __func__);
> > > +                               return PTR_ERR(dwmac->sgmii_adapter_base);
> > > +                       }
> > > +               }
> > > +
> > > +               index = of_property_match_string(np_sgmii_adapter,
> > > "reg-names",
> > > +                                                "eth_tse_control_port");
> > > +
> > > +               if (index >= 0) {
> > > +                       if (of_address_to_resource(np_sgmii_adapter,
> > > index,
> > > +                                                  &res_tse_pcs)) {
> > > +                               dev_err(dev, "%s: ERROR: failed mapping
> > > tse control port\n",
> > > +                                       __func__);
> > > +                               return -EINVAL;
> > > +                       }
> > > +
> > > +                       dwmac->tse_pcs_base = devm_ioremap_resource(
> > > +                       dev, &res_tse_pcs);
> > > +
> > > +                       if (IS_ERR(dwmac->tse_pcs_base)) {
> > > +                               dev_err(dev, "%s: failed to mapping tse
> > > control port\n",
> > > +                                       __func__);
> > > +                               return PTR_ERR(dwmac->sgmii_adapter_base);
> > > +                       }
> > > +               }
> > > +       }
> > >         dwmac->reg_offset = reg_offset;
> > >         dwmac->reg_shift = reg_shift;
> > >         dwmac->sys_mgr_base_addr = sys_mgr_base_addr;
> > > @@ -157,6 +469,7 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac
> > > *dwmac)
> > >                 break;
> > >         case PHY_INTERFACE_MODE_MII:
> > >         case PHY_INTERFACE_MODE_GMII:
> > > +       case PHY_INTERFACE_MODE_SGMII:
> > >                 val = SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII;
> > >                 break;
> > >         default:
> > > @@ -181,6 +494,12 @@ static int socfpga_dwmac_setup(struct socfpga_dwmac
> > > *dwmac)
> > >                 ctrl &= ~(SYSMGR_EMACGRP_CTRL_PTP_REF_CLK_MASK <<
> > > (reg_shift / 2));
> > >
> > >         regmap_write(sys_mgr_base_addr, reg_offset, ctrl);
> > > +
> > > +       if (phymode == PHY_INTERFACE_MODE_SGMII) {
> > > +               tse_pcs_init(dwmac->tse_pcs_base, dwmac);
> > > +               config_tx_buffer(SGMII_ADAPTER_ENABLE,
> > > +                                dwmac->sgmii_adapter_base);
> > > +       }
> > >         return 0;
> > >   }