Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[David Wu]

Hi Shawn,

Thanks for the suggestion, the most is okay.

å 2018å05æ16æ 14:34, Shawn Lin åé:
> Hi David,
>
> On 2018/5/16 11:38, David Wu wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index 13133b3..4b2ab71 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > @@ -61,6 +61,7 @@ struct rk_priv_data {
> > ÂÂÂÂÂ struct clk *mac_clk_tx;
> > ÂÂÂÂÂ struct clk *clk_mac_ref;
> > ÂÂÂÂÂ struct clk *clk_mac_refout;
> > +ÂÂÂ struct clk *clk_mac_speed;
>
> No need to do anything now but it seems you could consider doing some
> cleanup by using clk bulk APIs in the future.

The use of this may seem to be less applicable because there are many 
scenarios using different clocks.

> > ÂÂÂÂÂ struct clk *aclk_mac;
> > ÂÂÂÂÂ struct clk *pclk_mac;
> > ÂÂÂÂÂ struct clk *clk_phy;
> > @@ -83,6 +84,64 @@ struct rk_priv_data {
> > ÂÂÂÂÂ (((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : 
> > soc##_GMAC_TXCLK_DLY_DISABLE) | \
> > ÂÂÂÂÂÂ ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : 
> > soc##_GMAC_RXCLK_DLY_DISABLE))
> > +#define PX30_GRF_GMAC_CON1ÂÂÂÂÂÂÂ 0X0904
>
> s/0X0904/0x0904 , since the other constants in this file follow the
> same format.
>
> > +
> > +/* PX30_GRF_GMAC_CON1 */
> > +#define PX30_GMAC_PHY_INTF_SEL_RMIIÂÂÂ (GRF_CLR_BIT(4) | 
> > GRF_CLR_BIT(5) | \
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GRF_BIT(6))
> > +#define PX30_GMAC_SPEED_10MÂÂÂÂÂÂÂ GRF_CLR_BIT(2)
> > +#define PX30_GMAC_SPEED_100MÂÂÂÂÂÂÂ GRF_BIT(2)
> > +
> > +static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
> > +{
> > +ÂÂÂ struct device *dev = &bsp_priv->pdev->dev;
> > +
> > +ÂÂÂ if (IS_ERR(bsp_priv->grf)) {
> > +ÂÂÂÂÂÂÂ dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
> > +ÂÂÂÂÂÂÂ return;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ PX30_GMAC_PHY_INTF_SEL_RMII);
> > +}
> > +
> > +static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int 
> > speed)
> > +{
> > +ÂÂÂ struct device *dev = &bsp_priv->pdev->dev;
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ if (IS_ERR(bsp_priv->clk_mac_speed)) {
> > +ÂÂÂÂÂÂÂ dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
> > +ÂÂÂÂÂÂÂ return;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ if (speed == 10) {
> > +ÂÂÂÂÂÂÂ regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PX30_GMAC_SPEED_10M);
> > +
> > +ÂÂÂÂÂÂÂ ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500000);
> > +ÂÂÂÂÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "%s: set clk_mac_speed rate 2500000 failed: 
> > %d\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__, ret);
> > +ÂÂÂ } else if (speed == 100) {
> > +ÂÂÂÂÂÂÂ regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PX30_GMAC_SPEED_100M);
> > +
> > +ÂÂÂÂÂÂÂ ret = clk_set_rate(bsp_priv->clk_mac_speed, 25000000);
> > +ÂÂÂÂÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "%s: set clk_mac_speed rate 25000000 failed: 
> > %d\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__, ret);
>
> I know it follows the existing examples, but IMHO it duplicates
> unnecessary code as all the difference is PX30_GMAC_SPEED_*

i think the difference is the register offset and bits.

> > +
> > +ÂÂÂ } else {
> > +ÂÂÂÂÂÂÂ dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
> > +ÂÂÂ }
> > +}
> > +
> > +static const struct rk_gmac_ops px30_ops = {
> > +ÂÂÂ .set_to_rmii = px30_set_to_rmii,
> > +ÂÂÂ .set_rmii_speed = px30_set_rmii_speed,
> > +};
> > +
> > Â #define RK3128_GRF_MAC_CON0ÂÂÂ 0x0168
> > Â #define RK3128_GRF_MAC_CON1ÂÂÂ 0x016c
> > @@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct 
> > plat_stmmacenet_data *plat)
> > ÂÂÂÂÂÂÂÂÂ }
> > ÂÂÂÂÂ }
> > +ÂÂÂ bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
>
> Mightbe it'd be better to use "mac-speed" in DT bindings.
>
> > +ÂÂÂ if (IS_ERR(bsp_priv->clk_mac_speed))
> > +ÂÂÂÂÂÂÂ dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
> > +
>
> Would you like to handle deferred probe >

No,

> > ÂÂÂÂÂ if (bsp_priv->clock_input) {
> > ÂÂÂÂÂÂÂÂÂ dev_info(dev, "clock input from PHY\n");
> > ÂÂÂÂÂ } else {
> > @@ -1424,6 +1487,7 @@ static int rk_gmac_resume(struct device *dev)
> > Â static SIMPLE_DEV_PM_OPS(rk_gmac_pm_ops, rk_gmac_suspend, 
> > rk_gmac_resume);
> > Â static const struct of_device_id rk_gmac_dwmac_match[] = {
> > +ÂÂÂ { .compatible = "rockchip,px30-gmac",ÂÂÂ .data = &px30_opsÂÂ },
> > ÂÂÂÂÂ { .compatible = "rockchip,rk3128-gmac", .data = &rk3128_ops },
> > ÂÂÂÂÂ { .compatible = "rockchip,rk3228-gmac", .data = &rk3228_ops },
> > ÂÂÂÂÂ { .compatible = "rockchip,rk3288-gmac", .data = &rk3288_ops },