Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip

[Alexandre Torgue]

2016-02-23 23:16 GMT+01:00 Joachim  Eastwood <manabian@xxxxxxxxx>:
> Hi Alexandre,
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@xxxxxxxxx> wrote:
>> stm324xx family chips support Synopsys MAC 3.510 IP.
>> This patch adds settings for logical glue logic:
>> -clocks
>> -mode selection MII or RMII.
>>
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@xxxxxxxxx>
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> index cec147d..f63bdcf 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> @@ -114,6 +114,18 @@ config DWMAC_SUNXI
>>           This selects Allwinner SoC glue layer support for the
>>           stmmac device driver. This driver is used for A20/A31
>>           GMAC ethernet controller.
>> +
>> +config DWMAC_STM32
>> +       tristate "STM32 DWMAC support"
>> +       default ARCH_STM32
>> +       depends on OF && HAS_IOMEM
>> +       select MFD_SYSCON
>> +       ---help---
>> +         Support for ethernet controller on STM32 SOCs.
>> +
>> +         This selects STM32 SoC glue layer support for the stmmac
>> +         device driver. This driver is used on for the STM32 series
>> +         SOCs GMAC ethernet controller.
>>  endif
>>
>>  config STMMAC_PCI
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
>> index b390161..559086d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_DWMAC_ROCKCHIP)  += dwmac-rk.o
>>  obj-$(CONFIG_DWMAC_SOCFPGA)    += dwmac-socfpga.o
>>  obj-$(CONFIG_DWMAC_STI)                += dwmac-sti.o
>>  obj-$(CONFIG_DWMAC_SUNXI)      += dwmac-sunxi.o
>> +obj-$(CONFIG_DWMAC_STM32)      += dwmac-stm32.o
>

Hi Joachim,

> Put them in alphabetic order. Same goes for the KConfig entry.

Ok

>
>
>>  obj-$(CONFIG_DWMAC_GENERIC)    += dwmac-generic.o
>>  stmmac-platform-objs:= stmmac_platform.o
> ...
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> +struct stm32_dwmac {
>> +       int interface;          /* MII interface */
>> +       struct clk *clk_tx;
>> +       struct clk *clk_rx;
>> +       u32 mode_reg;           /* MAC glue-logic mode register */
>> +       struct regmap *regmap;
>> +       u32 speed;
>> +};
>> +
>> +static int stm32_dwmac_init(void *priv)
>
> If you used 'struct stm32_dwmac *' instead of 'void *' you could skip
> the local variable assignment.
>
> Even better; you could pass 'struct plat_stmmacenet_data *' and use
> it's 'interface' member to set the phy mode. Then you could drop the
> interface member in your priv data struct and remove of_get_phy_mode()
> in stm32_dwmac_parse_data().

Yes, interesting.
>
>
>> +{
>> +       struct stm32_dwmac *dwmac = priv;
>> +       struct regmap *regmap = dwmac->regmap;
>> +       int ret, iface = dwmac->interface;
>> +       u32 reg = dwmac->mode_reg;
>> +       u32 val;
>> +
>> +       ret = clk_prepare_enable(dwmac->clk_tx);
>> +       if (ret)
>> +               goto out;
>> +
>> +       ret = clk_prepare_enable(dwmac->clk_rx);
>> +       if (ret)
>> +               goto out_disable_clk_tx;
>> +
>> +       val = (iface == PHY_INTERFACE_MODE_MII) ? 0 : 1;
>> +       ret = regmap_update_bits(regmap, reg, MII_PHY_SEL_MASK, val);
>> +       if (ret)
>> +               goto out_disable_clk_tx_rx;
>> +
>> +       return 0;
>> +
>> +out_disable_clk_tx_rx:
>> +       clk_disable_unprepare(dwmac->clk_rx);
>> +out_disable_clk_tx:
>> +       clk_disable_unprepare(dwmac->clk_tx);
>> +out:
>> +       return ret;
>> +}
>> +
>> +static void stm32_dwmac_exit(void *priv)
>> +{
>> +       struct stm32_dwmac *dwmac = priv;
>
> Again; instead of 'void *' use 'struct stm32_dwmac *' to avoid the
> local assignment.
>
>
>> +
>> +       clk_disable_unprepare(dwmac->clk_tx);
>> +       clk_disable_unprepare(dwmac->clk_rx);
>> +}
>
> To be honest I really don't see the point in having a function with
> just two other function calls in it. Consider dropping the function
> altogether and place the clk_disable_unprepare() calls where it's
> called from. If you still want to keep it, please put a more
> descriptive name on it.

It was just to avoid redundant code, but yes there is not a big
interest to do it.

>
>
>> +static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
>> +                                 struct platform_device *pdev)
>
> Since you are only interested in *dev and not *pdev you could pass a
> 'struct dev *' instead.

ok

>
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct regmap *regmap;
>> +       int err;
>> +
>> +       /*  Get TX/RX clocks */
>> +       dwmac->clk_tx = devm_clk_get(dev, "tx-clk");
>> +       if (IS_ERR(dwmac->clk_tx)) {
>> +               dev_warn(dev, "No tx clock provided...\n");
>> +               dwmac->clk_tx = NULL;
>> +       }
>> +       dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
>> +       if (IS_ERR(dwmac->clk_rx)) {
>> +               dev_warn(dev, "No rx clock provided...\n");
>> +               dwmac->clk_rx = NULL;
>> +       }
>> +
>> +       /* Get mode register */
>> +       regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       err = of_property_read_u32_index(np, "st,syscon", 1, &dwmac->mode_reg);
>> +       if (err) {
>> +               dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
>> +               return err;
>> +       }
>> +
>> +       dwmac->interface = of_get_phy_mode(np);
>> +       dwmac->regmap = regmap;
>
> Why the temporary local regmap variable?
>
> Assigning dwmac->regmap with syscon_regmap_lookup_by_phandle() should
> not exceed 80 chars if that is what you are worried about.

yes you are right.

>
>
>> +       return 0;
>> +}
>> +
>> +static int stm32_dwmac_probe(struct platform_device *pdev)
>> +{
>> +       struct plat_stmmacenet_data *plat_dat;
>> +       struct stmmac_resources stmmac_res;
>> +       struct stm32_dwmac *dwmac;
>> +       int ret;
>> +
>> +       ret = stmmac_get_platform_resources(pdev, &stmmac_res);
>> +       if (ret)
>> +               return ret;
>> +
>> +       plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac);
>> +       if (IS_ERR(plat_dat))
>> +               return PTR_ERR(plat_dat);
>> +
>> +       dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
>> +       if (!dwmac)
>> +               return -ENOMEM;
>> +
>> +       ret = stm32_dwmac_parse_data(dwmac, pdev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Unable to parse OF data\n");
>> +               return ret;
>> +       }
>> +
>> +       plat_dat->bsp_priv = dwmac;
>> +
>> +       ret = stm32_dwmac_init(plat_dat->bsp_priv);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
>
> Note that stmmac_dvr_probe() can fail and if so you should disable
> your tx/rx clks before you return.
>
> Consider putting the clk_prepare_enable() directly here and use goto
> labels for the clean up like most other drivers do in probe.

Ok catch.

>
> Also if you put regmap_update_bits() for phy mode above the
> clk_prepare_enable() calls you remove one of the gotos.
> I assume you don't need to enable tx/rx clock before you write to syscon.

I will check.
>
>
>> +static int stm32_dwmac_remove(struct platform_device *pdev)
>> +{
>> +       struct net_device *ndev = platform_get_drvdata(pdev);
>> +       struct stmmac_priv *priv = netdev_priv(ndev);
>> +       int ret = stmmac_dvr_remove(ndev);
>> +
>> +       stm32_dwmac_exit(priv->plat->bsp_priv);
>> +
>> +       return ret;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int stm32_dwmac_suspend(struct device *dev)
>> +{
>> +       struct net_device *ndev = dev_get_drvdata(dev);
>> +       struct stmmac_priv *priv = netdev_priv(ndev);
>> +       int ret;
>> +
>> +       ret = stmmac_suspend(ndev);
>> +       stm32_dwmac_exit(priv->plat->bsp_priv);
>> +
>> +       return ret;
>> +}
>> +
>> +static int stm32_dwmac_resume(struct device *dev)
>> +{
>> +       struct net_device *ndev = dev_get_drvdata(dev);
>> +       struct stmmac_priv *priv = netdev_priv(ndev);
>> +       int ret;
>> +
>> +       ret = stm32_dwmac_init(priv->plat->bsp_priv);
>> +       if (ret)
>> +               goto out_regmap;
>> +
>> +       ret = stmmac_resume(ndev);
>> +
>> +out_regmap:
>> +       return ret;
>
> Why the goto?

Sorry no sens. I thought that it was better to avoid multiple return
but it this case it is stupid.

Best regards.

Alexandre

>
> This could be written:
>     ret = stm32_dwmac_init(priv->plat->bsp_priv);
>     if (ret)
>        return ret;
>
>     return stmmac_resume(ndev);
>
>
> regards,
> Joachim Eastwood