Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
Hi Andrew
On Fri, 2017-03-24 at 15:19 +0100, Andrew Lunn wrote:
> On Tue, Mar 21, 2017 at 05:35:10PM +0800, sean.w...@mediatek.com wrote:
>
> Hi Sean
>
> > + /* Lower Tx Driving */
> > + for (i = 0 ; i < 6 ; i++)
>
> Could MT7530_CPU_PORT be used here?
>
I should create meaningful definition instead of avoiding hard coding
where 6 should be corrected into 5 which is meant for the number for
control register to configure TRGMII path
> > + mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
> > +TD_DM_DRVP(8) | TD_DM_DRVN(8));
> > +
> > + /* Setup MT7530 core clock */
> > + if (!trgint) {
> > + /* Disable MT7530 core clock */
> > + core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > +
> > + /* Disable MT7530 PLL, since phy_device has not yet been
> > +* created when this function is called. So we provide
> > +* core_write_mmd_indirect to complete this function
> > +*/
> > + core_write_mmd_indirect(priv,
> > + CORE_GSWPLL_GRP1,
> > + MDIO_MMD_VEND2,
> > + 0);
> > +
> > + /* Setup MT7530 core clock into 500Mhz */
> > + core_write(priv, CORE_GSWPLL_GRP2,
> > + RG_GSWPLL_POSDIV_500M(1) |
> > + RG_GSWPLL_FBKDIV_500M(25));
> > +
> > + /* Enable MT7530 PLL */
> > + core_write(priv, CORE_GSWPLL_GRP1,
> > + RG_GSWPLL_EN_PRE |
> > + RG_GSWPLL_POSDIV_200M(2) |
> > + RG_GSWPLL_FBKDIV_200M(32));
> > +
> > + /* Enable MT7530 core clock */
> > + core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > + }
> > +
> > + /* Setup the MT7530 TRGMII Tx Clock */
> > + core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> > + core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> > + core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> > + core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> > + core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> > + core_write(priv, CORE_PLL_GROUP4,
> > + RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
> > + RG_SYSPLL_BIAS_LPF_EN);
> > + core_write(priv, CORE_PLL_GROUP2,
> > + RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
> > + RG_SYSPLL_POSDIV(1));
> > + core_write(priv, CORE_PLL_GROUP7,
> > + RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
> > + RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> > + core_set(priv, CORE_TRGMII_GSW_CLK_CG,
> > +REG_GSWCK_EN | REG_TRGMIICK_EN);
> > +
> > + if (!trgint)
> > + for (i = 0 ; i < 5 ; i++)
>
> Why only 5 here? All other similar loops are to 6. Replacing 5 with a
> #define might help make this more readable.
the same as above, it should be corrected
> > + mt7530_rmw(priv, MT7530_TRGMII_RD(i),
> > + RD_TAP_MASK, RD_TAP(16));
> > + else
> > + mt7623_trgmii_set(priv, GSW_INTF_MODE, INTF_MODE_TRGMII);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +mt7623_pad_clk_setup(struct dsa_switch *ds)
> > +{
> > + struct mt7530_priv *priv = ds->priv;
> > + int i;
> > +
> > + for (i = 0 ; i < 6; i++)
>
> MT7530_CPU_PORT?
the same as above , it should be corrected
> > +
> > +/* Registers to mac forward conrol for unknown frames */
>
> /conrol/control
>
will fix it up
> > +
> > +/* Registor for port control */
>
> Register
>
will fix it up
> > +/* Regiser for TOP signal control */
>
> Register
>
will fix it up
> > +/* struct mt7530_priv -This is the main datasructure for holding the
> > state
>
> data structure
>
> > + * of the driver
> > + * @dev: The device pointer
> > + * @ds:The pointer to the dsa core structure
> > + * @bus: The bus used for the device and built-in PHY
> > + * @ethsys:The regmap used for enabling the necessary PLL
> > + * @ethernet: The regmap used for access TRGMII-based
> > registers
> > + * @core_pwr: The power supplied into the core
> > + * @io_pwr:The power supplied into the I/O
> > + * @mcm: Flag for distinguishing if standalone IC or module
> > + * coupling
> > + * @reset: The descriptor for GPIO line tied to its reset pin
> > + * @phy_mode: The xMII for cpu port used
> > + * @ports: Holding the state amongs ports
>
> among
will fix it up
thank for your careful reviewing again!!
> Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
Hi Andrew,
Add comment as below inline
On Fri, 2017-03-24 at 15:02 +0100, Andrew Lunn wrote:
> Hi Sean
>
> > + regmap = devm_regmap_init(ds->dev, NULL, priv,
> > + &mt7530_regmap_config);
> > + if (IS_ERR(regmap))
> > + dev_warn(priv->dev, "phy regmap initialization failed");
> > +
>
> Shouldn't this be a fatal error? If you keep going when there is an
> error, what happens when you actually try to use the regmap?
>
Initial thought is that it is just for debugging purpose so if it fails,
it should break any functionality of switch and only have implication as
a warning message.
Anyway, i will remove it in the next one since it seems better being
kept in private place as you suggested in the previous mail.
> > + phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> > + if (phy_mode < 0) {
> > + dev_err(priv->dev, "Can't find phy-mode for master device\n");
> > + return phy_mode;
> > + }
> > + dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);
>
> dev_dbg?
>
Sorry. i forgot turned it into dev_dbg
> > +
> > + id = mt7530_read(priv, MT7530_CREV);
> > + id >>= CHIP_NAME_SHIFT;
> > + if (id != MT7530_ID)
> > + return -ENODEV;
>
> It might be helpful to say what ID has been found, if it is not the
> supported ID.
I will fix it up to make users know what was going on
thanks for taking your time on those reviewing again!
> Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
Hi Florian,
Thank for taking your time on reviewing. Add comment as inline.
On Wed, 2017-03-22 at 11:39 -0700, Florian Fainelli wrote:
> On 03/21/2017 02:35 AM, sean.w...@mediatek.com wrote:
> > From: Sean Wang
> >
> > MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
> > Mediatek router platforms such as MT7623A or MT7623N platform which
> > includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
> > Among these ports, The port from 0 to 4 are the user ports connecting
> > with the remote devices while the port 5 and 6 are the CPU ports
> > connecting into Mediatek Ethernet GMAC.
> >
> > For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
> > through either the TRGMII or RGMII which could be controlled by phy-mode
> > in the dt-bindings to specify which mode is preferred to use. And for
> > port 5, only RGMII can be specified. However, currently, only port 6 is
> > being supported in this DSA driver.
> >
> > The driver is made with the reference to qca8k and other existing DSA
> > driver. The most of the essential callbacks of the DSA are already
> > support in the driver, including tag insert for user port distinguishing,
> > port control, bridge offloading, STP setup and ethtool operation to allow
> > DSA to model each user port into a standalone netdevice as the other DSA
> > driver had done.
>
> Overall, this looks pretty nice and clean, a few comments below
>
> >
> > Signed-off-by: Sean Wang
> > Signed-off-by: Landen Chao
> > ---
>
> > +static void
> > +mt7530_fdb_read(struct mt7530_priv *priv, struct mt7530_fdb *fdb)
> > +{
> > + u32 reg[3];
> > + int i;
> > +
> > + /* Read from ARL table into an array */
> > + for (i = 0; i < 3; i++) {
> > + reg[i] = mt7530_read(priv, MT7530_TSRA1 + (i * 4));
> > +
> > + dev_dbg(priv->dev, "%s(%d) reg[%d]=0x%x\n",
> > + __func__, __LINE__, i, reg[i]);
> > + }
> > +
> > + /* vid - 11:0 on reg[1] */
> > + fdb->vid = (reg[1] >> 0) & 0xfff;
> > + /* aging - 31:24 on reg[2] */
> > + fdb->aging = (reg[2] >> 24) & 0xff;
> > + /* portmask - 11:4 on reg[2] */
> > + fdb->port_mask = (reg[2] >> 4) & 0xff;
> > + /* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> > + fdb->mac[0] = (reg[0] >> 24) & 0xff;
> > + fdb->mac[1] = (reg[0] >> 16) & 0xff;
> > + fdb->mac[2] = (reg[0] >> 8) & 0xff;
> > + fdb->mac[3] = (reg[0] >> 0) & 0xff;
> > + fdb->mac[4] = (reg[1] >> 24) & 0xff;
> > + fdb->mac[5] = (reg[1] >> 16) & 0xff;
> > + /* noarp - 3:2 on reg[2] */
> > + fdb->noarp = ((reg[2] >> 2) & 0x3) == STATIC_ENT;
>
> Could you add some definitions for the bits and masks that you are
> shifting here?
>
Okay, I'll make into proper macro for readability
> > +}
> > +
> > +static void
> > +mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> > +u8 port_mask, const u8 *mac,
> > +u8 aging, u8 type)
> > +{
> > + u32 reg[3] = { 0 };
> > + int i;
> > +
> > + /* vid - 11:0 on reg[1] */
> > + reg[1] |= (vid & 0xfff) << 0;
> > + /* aging - 31:25 on reg[2] */
> > + reg[2] |= (aging & 0xff) << 24;
> > + /* portmask - 11:4 on reg[2] */
> > + reg[2] |= (port_mask & 0xff) << 4;
> > + /* type - 3 indicate that entry is static wouldn't
> > +* be aged out and 0 specified as erasing an entry
> > +*/
> > + reg[2] |= (type & 0x3) << 2;
> > + /* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> > + reg[1] |= mac[5] << 16;
> > + reg[1] |= mac[4] << 24;
> > + reg[0] |= mac[3] << 0;
> > + reg[0] |= mac[2] << 8;
> > + reg[0] |= mac[1] << 16;
> > + reg[0] |= mac[0] << 24;
> > +
> > + /* Wrirte array into the ARL table */
> > + for (i = 0; i < 3; i++)
> > + mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
> > +}
>
> Same here.
>
As above. I will improve them.
> > +
> > +static int
> > +mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> > +{
> > + struct mt7530_priv *priv = ds->priv;
> > + u32 ncpo1, ssc_delta, trgint, i;
> > +
> > + switch (mode) {
> > + case PHY_INTERFACE_MODE_RGMII:
> > + trgint = 0;
> > + ncpo1 = 0x0c80;
> > + ssc_delta = 0x87;
> > + break;
> > + case PHY_INTERFACE_MODE_TRGMII:
> > + trgint = 1;
> > + ncpo1 = 0x1400;
> > + ssc_delta = 0x57;
> > + break;
> > + default:
> > + pr_err("xMII mode %d not supported\n", mode);
> > + return -EINVAL;
> > + }
>
> You may be able to move this to an adjust_link callback that the PHY
> library would call when the PHY gets setup and the port is finally used,
> as opposed to doing this upfront during driver initialization.
>
Good point. i will follow up
>
> > +mt7530_setup(struct dsa_switch *ds)
> > +{
> > + struct mt7530_priv *priv = ds->priv;
> > + int ret, i, phy_mode;
> > + u8 cpup_mask = 0;
> > + u32 id, val;
> > + struct regmap *regmap;
> > + struct device_node *dn;
> > +
> > + /* Make sure that c
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On Tue, Mar 21, 2017 at 05:35:10PM +0800, sean.w...@mediatek.com wrote:
Hi Sean
> + /* Lower Tx Driving */
> + for (i = 0 ; i < 6 ; i++)
Could MT7530_CPU_PORT be used here?
> + mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
> + TD_DM_DRVP(8) | TD_DM_DRVN(8));
> +
> + /* Setup MT7530 core clock */
> + if (!trgint) {
> + /* Disable MT7530 core clock */
> + core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> +
> + /* Disable MT7530 PLL, since phy_device has not yet been
> + * created when this function is called. So we provide
> + * core_write_mmd_indirect to complete this function
> + */
> + core_write_mmd_indirect(priv,
> + CORE_GSWPLL_GRP1,
> + MDIO_MMD_VEND2,
> + 0);
> +
> + /* Setup MT7530 core clock into 500Mhz */
> + core_write(priv, CORE_GSWPLL_GRP2,
> +RG_GSWPLL_POSDIV_500M(1) |
> +RG_GSWPLL_FBKDIV_500M(25));
> +
> + /* Enable MT7530 PLL */
> + core_write(priv, CORE_GSWPLL_GRP1,
> +RG_GSWPLL_EN_PRE |
> +RG_GSWPLL_POSDIV_200M(2) |
> +RG_GSWPLL_FBKDIV_200M(32));
> +
> + /* Enable MT7530 core clock */
> + core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> + }
> +
> + /* Setup the MT7530 TRGMII Tx Clock */
> + core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
> + core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> + core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> + core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> + core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> + core_write(priv, CORE_PLL_GROUP4,
> +RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
> +RG_SYSPLL_BIAS_LPF_EN);
> + core_write(priv, CORE_PLL_GROUP2,
> +RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
> +RG_SYSPLL_POSDIV(1));
> + core_write(priv, CORE_PLL_GROUP7,
> +RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
> +RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> + core_set(priv, CORE_TRGMII_GSW_CLK_CG,
> + REG_GSWCK_EN | REG_TRGMIICK_EN);
> +
> + if (!trgint)
> + for (i = 0 ; i < 5 ; i++)
Why only 5 here? All other similar loops are to 6. Replacing 5 with a
#define might help make this more readable.
> + mt7530_rmw(priv, MT7530_TRGMII_RD(i),
> +RD_TAP_MASK, RD_TAP(16));
> + else
> + mt7623_trgmii_set(priv, GSW_INTF_MODE, INTF_MODE_TRGMII);
> +
> + return 0;
> +}
> +
> +static int
> +mt7623_pad_clk_setup(struct dsa_switch *ds)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + int i;
> +
> + for (i = 0 ; i < 6; i++)
MT7530_CPU_PORT?
> +
> +/* Registers to mac forward conrol for unknown frames */
/conrol/control
> +
> +/* Registor for port control */
Register
> +/* Regiser for TOP signal control */
Register
> +/* struct mt7530_priv - This is the main datasructure for holding the
> state
data structure
> + * of the driver
> + * @dev: The device pointer
> + * @ds: The pointer to the dsa core structure
> + * @bus: The bus used for the device and built-in PHY
> + * @ethsys: The regmap used for enabling the necessary PLL
> + * @ethernet:The regmap used for access TRGMII-based
> registers
> + * @core_pwr:The power supplied into the core
> + * @io_pwr: The power supplied into the I/O
> + * @mcm: Flag for distinguishing if standalone IC or module
> + * coupling
> + * @reset: The descriptor for GPIO line tied to its reset pin
> + * @phy_mode:The xMII for cpu port used
> + * @ports: Holding the state amongs ports
among
Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
Hi Sean
> + regmap = devm_regmap_init(ds->dev, NULL, priv,
> + &mt7530_regmap_config);
> + if (IS_ERR(regmap))
> + dev_warn(priv->dev, "phy regmap initialization failed");
> +
Shouldn't this be a fatal error? If you keep going when there is an
error, what happens when you actually try to use the regmap?
> + phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> + if (phy_mode < 0) {
> + dev_err(priv->dev, "Can't find phy-mode for master device\n");
> + return phy_mode;
> + }
> + dev_info(priv->dev, "phy-mode for master device = %x\n", phy_mode);
dev_dbg?
> +
> + id = mt7530_read(priv, MT7530_CREV);
> + id >>= CHIP_NAME_SHIFT;
> + if (id != MT7530_ID)
> + return -ENODEV;
It might be helpful to say what ID has been found, if it is not the
supported ID.
Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On Thu, Mar 23, 2017 at 04:06:56PM +0800, Sean Wang wrote: > Hi Andrew, > > The purpose for the regmap table registered is to > > provide a way which helps us to look up a specific > > register on the switch through regmap-debugfs. > > > And not all ranges of register is defined > > so I only include the meaningful ones in a sparse way > > for the table. Many of there registers can be dumped using existing tools. You can dump the port registers using ethtool -r, if you implement get_regs/get_regs_len in your driver. mii-tool can dump the PHY registers. What you cannot see are global registers, so i can understand the usage of regmap-debugfs. However, i have a hard time with you only actually using the regmap for get/set for one small subset of registers. Either you need to use regmap to get/set all registers, or you remove it from the mainline driver, and keep it as a private patch which you use for your development work. For the Marvell driver we have an out of tree patch which exports a log of information via debugfs. Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On 2017-03-23 15:25, John Crispin wrote: > > > On 23/03/17 15:09, Felix Fietkau wrote: >> On 2017-03-23 09:06, Sean Wang wrote: >>> Hi Andrew, >>> >>> The purpose for the regmap table registered is to >>> >>> provide a way which helps us to look up a specific >>> >>> register on the switch through regmap-debugfs. >>> >>> >>> And not all ranges of register is defined >>> >>> so I only include the meaningful ones in a sparse way >>> >>> for the table. >> I think in that case it might be nice to make regmap support optional in >> order to avoid pulling in bloat on platforms that don't need it. >> >> - Felix >> > The 2 relevant platforms are mips/ralink and arm/mediatek. both require > regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth > driver which is a prereq for mt7530. so regmap cannot be optional here. Makes sense, thanks. - Felix
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On 23/03/17 15:09, Felix Fietkau wrote: On 2017-03-23 09:06, Sean Wang wrote: Hi Andrew, The purpose for the regmap table registered is to provide a way which helps us to look up a specific register on the switch through regmap-debugfs. And not all ranges of register is defined so I only include the meaningful ones in a sparse way for the table. I think in that case it might be nice to make regmap support optional in order to avoid pulling in bloat on platforms that don't need it. - Felix The 2 relevant platforms are mips/ralink and arm/mediatek. both require regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth driver which is a prereq for mt7530. so regmap cannot be optional here. John ___ Linux-mediatek mailing list linux-media...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On 2017-03-23 09:06, Sean Wang wrote: > Hi Andrew, > > The purpose for the regmap table registered is to > > provide a way which helps us to look up a specific > > register on the switch through regmap-debugfs. > > > And not all ranges of register is defined > > so I only include the meaningful ones in a sparse way > > for the table. I think in that case it might be nice to make regmap support optional in order to avoid pulling in bloat on platforms that don't need it. - Felix
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
Hi Andrew,
The purpose for the regmap table registered is to
provide a way which helps us to look up a specific
register on the switch through regmap-debugfs.
And not all ranges of register is defined
so I only include the meaningful ones in a sparse way
for the table.
Sean
On Thu, 2017-03-23 at 08:22 +0100, Andrew Lunn wrote:
> > +static int
> > +mt7623_trgmii_write(struct mt7530_priv *priv, u32 reg, u32 val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(priv->ethernet, TRGMII_BASE(reg), val);
> > + if (ret < 0)
> > + dev_err(priv->dev,
> > + "failed to priv write register\n");
> > + return ret;
> > +}
> > +
> > +static u32
> > +mt7623_trgmii_read(struct mt7530_priv *priv, u32 reg)
> > +{
> > + int ret;
> > + u32 val;
> > +
> > + ret = regmap_read(priv->ethernet, TRGMII_BASE(reg), &val);
> > + if (ret < 0) {
> > + dev_err(priv->dev,
> > + "failed to priv read register\n");
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
>
> Hi Sean
>
> These appear to be the only two accessors which use the regmap.
>
> > +static int
> > +mt7530_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > +{
> > + struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
> > +
> > + /* BIT(15) is used as indication for pseudo registers
> > +* which would be translated into the general MDIO
> > +* access to leverage the unique regmap sys interface.
> > +*/
> > + if (reg & BIT(15))
> > + *val = mdiobus_read_nested(priv->bus,
> > + (reg & 0xf00) >> 8,
> > + (reg & 0xff) >> 2);
> > + else
> > + *val = mt7530_read(priv, reg);
> > +
> > + return 0;
> > +}
>
> .
>
> > +static const struct regmap_range mt7530_readable_ranges[] = {
> > + regmap_reg_range(0x, 0x00ac), /* Global control */
> > + regmap_reg_range(0x2000, 0x202c), /* Port Control - P0 */
> > + regmap_reg_range(0x2100, 0x212c), /* Port Control - P1 */
> > + regmap_reg_range(0x2200, 0x222c), /* Port Control - P2 */
> > + regmap_reg_range(0x2300, 0x232c), /* Port Control - P3 */
> > + regmap_reg_range(0x2400, 0x242c), /* Port Control - P4 */
> > + regmap_reg_range(0x2500, 0x252c), /* Port Control - P5 */
> > + regmap_reg_range(0x2600, 0x262c), /* Port Control - P6 */
> > + regmap_reg_range(0x30e0, 0x30f8), /* Port MAC - SYS */
> > + regmap_reg_range(0x3000, 0x3014), /* Port MAC - P0 */
> > + regmap_reg_range(0x3100, 0x3114), /* Port MAC - P1 */
> > + regmap_reg_range(0x3200, 0x3214), /* Port MAC - P2*/
> > + regmap_reg_range(0x3300, 0x3314), /* Port MAC - P3*/
> > + regmap_reg_range(0x3400, 0x3414), /* Port MAC - P4 */
> > + regmap_reg_range(0x3500, 0x3514), /* Port MAC - P5 */
> > + regmap_reg_range(0x3600, 0x3614), /* Port MAC - P6 */
> > + regmap_reg_range(0x4000, 0x40d4), /* MIB - P0 */
> > + regmap_reg_range(0x4100, 0x41d4), /* MIB - P1 */
> > + regmap_reg_range(0x4200, 0x42d4), /* MIB - P2 */
> > + regmap_reg_range(0x4300, 0x43d4), /* MIB - P3 */
> > + regmap_reg_range(0x4400, 0x44d4), /* MIB - P4 */
> > + regmap_reg_range(0x4500, 0x45d4), /* MIB - P5 */
> > + regmap_reg_range(0x4600, 0x46d4), /* MIB - P6 */
> > + regmap_reg_range(0x4fe0, 0x4ff4), /* SYS */
> > + regmap_reg_range(0x7000, 0x700c), /* SYS 2 */
> > + regmap_reg_range(0x7018, 0x7028), /* SYS 3 */
> > + regmap_reg_range(0x7800, 0x7830), /* SYS 4 */
> > + regmap_reg_range(0x7a00, 0x7a7c), /* TRGMII */
> > + regmap_reg_range(0x8000, 0x8078), /* Psedo address for Phy - P0 */
> > + regmap_reg_range(0x8100, 0x8178), /* Psedo address for Phy - P1 */
> > + regmap_reg_range(0x8200, 0x8278), /* Psedo address for Phy - P2 */
> > + regmap_reg_range(0x8300, 0x8378), /* Psedo address for Phy - P3 */
> > + regmap_reg_range(0x8400, 0x8478), /* Psedo address for Phy - P4 */
> > +};
>
> It looks like your regmap accessor are only used for 0x7a00 to 0x7a7c.
>
> It is not clear why you even bother with a regmap. If you have it, why
> not use it for all registers within the regmap?
>
> Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
> +static int
> +mt7623_trgmii_write(struct mt7530_priv *priv, u32 reg, u32 val)
> +{
> + int ret;
> +
> + ret = regmap_write(priv->ethernet, TRGMII_BASE(reg), val);
> + if (ret < 0)
> + dev_err(priv->dev,
> + "failed to priv write register\n");
> + return ret;
> +}
> +
> +static u32
> +mt7623_trgmii_read(struct mt7530_priv *priv, u32 reg)
> +{
> + int ret;
> + u32 val;
> +
> + ret = regmap_read(priv->ethernet, TRGMII_BASE(reg), &val);
> + if (ret < 0) {
> + dev_err(priv->dev,
> + "failed to priv read register\n");
> + return ret;
> + }
> +
> + return val;
> +}
Hi Sean
These appear to be the only two accessors which use the regmap.
> +static int
> +mt7530_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> +{
> + struct mt7530_priv *priv = (struct mt7530_priv *)ctx;
> +
> + /* BIT(15) is used as indication for pseudo registers
> + * which would be translated into the general MDIO
> + * access to leverage the unique regmap sys interface.
> + */
> + if (reg & BIT(15))
> + *val = mdiobus_read_nested(priv->bus,
> +(reg & 0xf00) >> 8,
> +(reg & 0xff) >> 2);
> + else
> + *val = mt7530_read(priv, reg);
> +
> + return 0;
> +}
.
> +static const struct regmap_range mt7530_readable_ranges[] = {
> + regmap_reg_range(0x, 0x00ac), /* Global control */
> + regmap_reg_range(0x2000, 0x202c), /* Port Control - P0 */
> + regmap_reg_range(0x2100, 0x212c), /* Port Control - P1 */
> + regmap_reg_range(0x2200, 0x222c), /* Port Control - P2 */
> + regmap_reg_range(0x2300, 0x232c), /* Port Control - P3 */
> + regmap_reg_range(0x2400, 0x242c), /* Port Control - P4 */
> + regmap_reg_range(0x2500, 0x252c), /* Port Control - P5 */
> + regmap_reg_range(0x2600, 0x262c), /* Port Control - P6 */
> + regmap_reg_range(0x30e0, 0x30f8), /* Port MAC - SYS */
> + regmap_reg_range(0x3000, 0x3014), /* Port MAC - P0 */
> + regmap_reg_range(0x3100, 0x3114), /* Port MAC - P1 */
> + regmap_reg_range(0x3200, 0x3214), /* Port MAC - P2*/
> + regmap_reg_range(0x3300, 0x3314), /* Port MAC - P3*/
> + regmap_reg_range(0x3400, 0x3414), /* Port MAC - P4 */
> + regmap_reg_range(0x3500, 0x3514), /* Port MAC - P5 */
> + regmap_reg_range(0x3600, 0x3614), /* Port MAC - P6 */
> + regmap_reg_range(0x4000, 0x40d4), /* MIB - P0 */
> + regmap_reg_range(0x4100, 0x41d4), /* MIB - P1 */
> + regmap_reg_range(0x4200, 0x42d4), /* MIB - P2 */
> + regmap_reg_range(0x4300, 0x43d4), /* MIB - P3 */
> + regmap_reg_range(0x4400, 0x44d4), /* MIB - P4 */
> + regmap_reg_range(0x4500, 0x45d4), /* MIB - P5 */
> + regmap_reg_range(0x4600, 0x46d4), /* MIB - P6 */
> + regmap_reg_range(0x4fe0, 0x4ff4), /* SYS */
> + regmap_reg_range(0x7000, 0x700c), /* SYS 2 */
> + regmap_reg_range(0x7018, 0x7028), /* SYS 3 */
> + regmap_reg_range(0x7800, 0x7830), /* SYS 4 */
> + regmap_reg_range(0x7a00, 0x7a7c), /* TRGMII */
> + regmap_reg_range(0x8000, 0x8078), /* Psedo address for Phy - P0 */
> + regmap_reg_range(0x8100, 0x8178), /* Psedo address for Phy - P1 */
> + regmap_reg_range(0x8200, 0x8278), /* Psedo address for Phy - P2 */
> + regmap_reg_range(0x8300, 0x8378), /* Psedo address for Phy - P3 */
> + regmap_reg_range(0x8400, 0x8478), /* Psedo address for Phy - P4 */
> +};
It looks like your regmap accessor are only used for 0x7a00 to 0x7a7c.
It is not clear why you even bother with a regmap. If you have it, why
not use it for all registers within the regmap?
Andrew
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
On 03/21/2017 02:35 AM, sean.w...@mediatek.com wrote:
> From: Sean Wang
>
> MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
> Mediatek router platforms such as MT7623A or MT7623N platform which
> includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
> Among these ports, The port from 0 to 4 are the user ports connecting
> with the remote devices while the port 5 and 6 are the CPU ports
> connecting into Mediatek Ethernet GMAC.
>
> For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
> through either the TRGMII or RGMII which could be controlled by phy-mode
> in the dt-bindings to specify which mode is preferred to use. And for
> port 5, only RGMII can be specified. However, currently, only port 6 is
> being supported in this DSA driver.
>
> The driver is made with the reference to qca8k and other existing DSA
> driver. The most of the essential callbacks of the DSA are already
> support in the driver, including tag insert for user port distinguishing,
> port control, bridge offloading, STP setup and ethtool operation to allow
> DSA to model each user port into a standalone netdevice as the other DSA
> driver had done.
Overall, this looks pretty nice and clean, a few comments below
>
> Signed-off-by: Sean Wang
> Signed-off-by: Landen Chao
> ---
> +static void
> +mt7530_fdb_read(struct mt7530_priv *priv, struct mt7530_fdb *fdb)
> +{
> + u32 reg[3];
> + int i;
> +
> + /* Read from ARL table into an array */
> + for (i = 0; i < 3; i++) {
> + reg[i] = mt7530_read(priv, MT7530_TSRA1 + (i * 4));
> +
> + dev_dbg(priv->dev, "%s(%d) reg[%d]=0x%x\n",
> + __func__, __LINE__, i, reg[i]);
> + }
> +
> + /* vid - 11:0 on reg[1] */
> + fdb->vid = (reg[1] >> 0) & 0xfff;
> + /* aging - 31:24 on reg[2] */
> + fdb->aging = (reg[2] >> 24) & 0xff;
> + /* portmask - 11:4 on reg[2] */
> + fdb->port_mask = (reg[2] >> 4) & 0xff;
> + /* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> + fdb->mac[0] = (reg[0] >> 24) & 0xff;
> + fdb->mac[1] = (reg[0] >> 16) & 0xff;
> + fdb->mac[2] = (reg[0] >> 8) & 0xff;
> + fdb->mac[3] = (reg[0] >> 0) & 0xff;
> + fdb->mac[4] = (reg[1] >> 24) & 0xff;
> + fdb->mac[5] = (reg[1] >> 16) & 0xff;
> + /* noarp - 3:2 on reg[2] */
> + fdb->noarp = ((reg[2] >> 2) & 0x3) == STATIC_ENT;
Could you add some definitions for the bits and masks that you are
shifting here?
> +}
> +
> +static void
> +mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
> + u8 port_mask, const u8 *mac,
> + u8 aging, u8 type)
> +{
> + u32 reg[3] = { 0 };
> + int i;
> +
> + /* vid - 11:0 on reg[1] */
> + reg[1] |= (vid & 0xfff) << 0;
> + /* aging - 31:25 on reg[2] */
> + reg[2] |= (aging & 0xff) << 24;
> + /* portmask - 11:4 on reg[2] */
> + reg[2] |= (port_mask & 0xff) << 4;
> + /* type - 3 indicate that entry is static wouldn't
> + * be aged out and 0 specified as erasing an entry
> + */
> + reg[2] |= (type & 0x3) << 2;
> + /* mac - 31:0 on reg[0] and 31:16 on reg[1] */
> + reg[1] |= mac[5] << 16;
> + reg[1] |= mac[4] << 24;
> + reg[0] |= mac[3] << 0;
> + reg[0] |= mac[2] << 8;
> + reg[0] |= mac[1] << 16;
> + reg[0] |= mac[0] << 24;
> +
> + /* Wrirte array into the ARL table */
> + for (i = 0; i < 3; i++)
> + mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
> +}
Same here.
> +
> +static int
> +mt7530_pad_clk_setup(struct dsa_switch *ds, int mode)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 ncpo1, ssc_delta, trgint, i;
> +
> + switch (mode) {
> + case PHY_INTERFACE_MODE_RGMII:
> + trgint = 0;
> + ncpo1 = 0x0c80;
> + ssc_delta = 0x87;
> + break;
> + case PHY_INTERFACE_MODE_TRGMII:
> + trgint = 1;
> + ncpo1 = 0x1400;
> + ssc_delta = 0x57;
> + break;
> + default:
> + pr_err("xMII mode %d not supported\n", mode);
> + return -EINVAL;
> + }
You may be able to move this to an adjust_link callback that the PHY
library would call when the PHY gets setup and the port is finally used,
as opposed to doing this upfront during driver initialization.
> +mt7530_setup(struct dsa_switch *ds)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + int ret, i, phy_mode;
> + u8 cpup_mask = 0;
> + u32 id, val;
> + struct regmap *regmap;
> + struct device_node *dn;
> +
> + /* Make sure that cpu port specfied on the dt is appropriate */
> + if (!dsa_is_cpu_port(ds, MT7530_CPU_PORT)) {
> + dev_err(priv->dev, "port not matched with the CPU port\n");
> + return -EINVAL;
> + }
This is kind of a hard error, in that case, a sensible thing to do could
be issue a warning to the user telling that the configuration does not
permit the use of Mediatek
Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
> +static int
> +core_read_mmd_indirect(struct mt7530_priv *priv, int prtad, int devad)
> +{
> + struct mii_bus *bus = priv->bus;
> + int value, ret;
> +
> + /* Write the desired MMD Devad */
> + ret = bus->write(bus, 0, MII_MMD_CTRL, devad);
> + if (ret < 0)
> + goto err;
> +
> + /* Write the desired MMD register address */
> + ret = bus->write(bus, 0, MII_MMD_DATA, prtad);
> + if (ret < 0)
> + goto err;
> +
> + /* Select the Function : DATA with no post increment */
> + ret = bus->write(bus, 0, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
> + if (ret < 0)
> + goto err;
Hi Sean
This appear to be a copy of mmd_phy_indirect(). There are patches from
Russell King which made this a public function. Once Russell patches
make net-next, it would be nice to use mmd_phy_indirect(). But that
might be as a follow up patch, rather than now, depending on the order
of acceptance of the patches.
Andrew
[PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch
From: Sean Wang
MT7530 is a 7-ports Gigabit Ethernet Switch that could be found on
Mediatek router platforms such as MT7623A or MT7623N platform which
includes 7-port Gigabit Ethernet MAC and 5-port Gigabit Ethernet PHY.
Among these ports, The port from 0 to 4 are the user ports connecting
with the remote devices while the port 5 and 6 are the CPU ports
connecting into Mediatek Ethernet GMAC.
For port 6, it can communicate with the CPU via Mediatek Ethernet GMAC
through either the TRGMII or RGMII which could be controlled by phy-mode
in the dt-bindings to specify which mode is preferred to use. And for
port 5, only RGMII can be specified. However, currently, only port 6 is
being supported in this DSA driver.
The driver is made with the reference to qca8k and other existing DSA
driver. The most of the essential callbacks of the DSA are already
support in the driver, including tag insert for user port distinguishing,
port control, bridge offloading, STP setup and ethtool operation to allow
DSA to model each user port into a standalone netdevice as the other DSA
driver had done.
Signed-off-by: Sean Wang
Signed-off-by: Landen Chao
---
drivers/net/dsa/Kconfig |8 +
drivers/net/dsa/Makefile |2 +-
drivers/net/dsa/mt7530.c | 1172 ++
drivers/net/dsa/mt7530.h | 382 +++
4 files changed, 1563 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/dsa/mt7530.c
create mode 100644 drivers/net/dsa/mt7530.h
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 0659846..5b322b4 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -34,4 +34,12 @@ config NET_DSA_QCA8K
This enables support for the Qualcomm Atheros QCA8K Ethernet
switch chips.
+config NET_DSA_MT7530
+ tristate "Mediatek MT7530 Ethernet switch support"
+ depends on NET_DSA
+ select NET_DSA_TAG_MTK
+ ---help---
+ This enables support for the Mediatek MT7530 Ethernet switch
+ chip.
+
endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index a3c9416..8e629c1 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -2,6 +2,6 @@ obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
obj-$(CONFIG_NET_DSA_BCM_SF2) += bcm-sf2.o
bcm-sf2-objs := bcm_sf2.o bcm_sf2_cfp.o
obj-$(CONFIG_NET_DSA_QCA8K)+= qca8k.o
-
+obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
obj-y += b53/
obj-y += mv88e6xxx/
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
new file mode 100644
index 000..99e5485
--- /dev/null
+++ b/drivers/net/dsa/mt7530.c
@@ -0,0 +1,1172 @@
+/*
+ * Mediatek MT7530 DSA Switch driver
+ * Copyright (C) 2017 Sean Wang
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "mt7530.h"
+
+/* String, offset, and register size in bytes if different from 4 bytes */
+static const struct mt7530_mib_desc mt7530_mib[] = {
+ MIB_DESC(1, 0x00, "TxDrop"),
+ MIB_DESC(1, 0x04, "TxCrcErr"),
+ MIB_DESC(1, 0x08, "TxUnicast"),
+ MIB_DESC(1, 0x0c, "TxMulticast"),
+ MIB_DESC(1, 0x10, "TxBroadcast"),
+ MIB_DESC(1, 0x14, "TxCollision"),
+ MIB_DESC(1, 0x18, "TxSingleCollision"),
+ MIB_DESC(1, 0x1c, "TxMultipleCollision"),
+ MIB_DESC(1, 0x20, "TxDeferred"),
+ MIB_DESC(1, 0x24, "TxLateCollision"),
+ MIB_DESC(1, 0x28, "TxExcessiveCollistion"),
+ MIB_DESC(1, 0x2c, "TxPause"),
+ MIB_DESC(1, 0x30, "TxPktSz64"),
+ MIB_DESC(1, 0x34, "TxPktSz65To127"),
+ MIB_DESC(1, 0x38, "TxPktSz128To255"),
+ MIB_DESC(1, 0x3c, "TxPktSz256To511"),
+ MIB_DESC(1, 0x40, "TxPktSz512To1023"),
+ MIB_DESC(1, 0x44, "Tx1024ToMax"),
+ MIB_DESC(2, 0x48, "TxBytes"),
+ MIB_DESC(1, 0x60, "RxDrop"),
+ MIB_DESC(1, 0x64, "RxFiltering"),
+ MIB_DESC(1, 0x6c, "RxMulticast"),
+ MIB_DESC(1, 0x70, "RxBroadcast"),
+ MIB_DESC(1, 0x74, "RxAlignErr"),
+ MIB_DESC(1, 0x78, "RxCrcErr"),
+ MIB_DESC(1, 0x7c, "RxUnderSizeErr"),
+ MIB_DESC(1, 0x80, "RxFragErr"),
+ MIB_DESC(1, 0x84, "RxOverSzErr"),
+ MIB_DESC(1, 0x88, "RxJabberErr"),
+ MIB_DESC(1, 0x8c, "RxPause"),
+ MIB_DESC(1, 0x90, "RxPktSz64"),
+ MIB_DESC(1, 0x94, "RxPktSz65To127"),
+ MIB_DESC(1, 0x98, "RxPktSz128To255"),
+ MIB_DESC(1, 0
