[PATCH v2 0/4] Add Ethernet support on STM32F429

[PATCH v2 0/4] Add Ethernet support on STM32F429

[Alexandre TORGUE]

STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
This series:
 -enhance current stmmac driver to control it (code already
available) and adds basic glue for STM32F429 chip.
 -Enable basic Net config in kernel.

Note that DT patches are not present because STM32 pinctrl code is not
yet avalaible.

Changes since v1:
 -Fix Kbuild issue in Kconfig.
 -Remove init/exit callbacks. Suspend/Resume and remove driver is no more
driven in stmmac_pltfr but directly in dwmac-stm32 glue driver.
 -Take into account Joachim review.

Regards.

Alexandre.

Alexandre TORGUE (4):
  net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
  Documentation: Bindings: Add STM32 DWMAC glue
  net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP
  ARM: STM32: Enable Ethernet in stm32_defconfig

 .../devicetree/bindings/net/stm32-dwmac.txt        |  41 ++++
 arch/arm/configs/stm32_defconfig                   |   9 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  | 208 +++++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   1 +
 6 files changed, 272 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/stm32-dwmac.txt
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c

-- 
1.9.1

[PATCH v2 0/4] Add Ethernet support on STM32F429

[Alexandre TORGUE]

STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
This series:
 -enhance current stmmac driver to control it (code already
available) and adds basic glue for STM32F429 chip.
 -Enable basic Net config in kernel.

Note that DT patches are not present because STM32 pinctrl code is not
yet avalaible.

Changes since v1:
 -Fix Kbuild issue in Kconfig.
 -Remove init/exit callbacks. Suspend/Resume and remove driver is no more
driven in stmmac_pltfr but directly in dwmac-stm32 glue driver.
 -Take into account Joachim review.

Regards.

Alexandre.

Alexandre TORGUE (4):
  net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
  Documentation: Bindings: Add STM32 DWMAC glue
  net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP
  ARM: STM32: Enable Ethernet in stm32_defconfig

 .../devicetree/bindings/net/stm32-dwmac.txt        |  41 ++++
 arch/arm/configs/stm32_defconfig                   |   9 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 ++
 drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  | 208 +++++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   1 +
 6 files changed, 272 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/stm32-dwmac.txt
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c

-- 
1.9.1

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

[Alexandre TORGUE]

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@gmail.com>

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
 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
new file mode 100644
index 0000000..0b48ee7
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -0,0 +1,208 @@
+/*
+ * dwmac-stm32.c - DWMAC Specific Glue layer for STM32 MCU
+ *
+ * Copyright (C) Alexandre Torgue 2015
+ * Author:  Alexandre Torgue <alexandre.torgue@gmail.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define MII_PHY_SEL_MASK	BIT(23)
+
+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)
+{
+	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;
+
+	clk_disable_unprepare(dwmac->clk_tx);
+	clk_disable_unprepare(dwmac->clk_rx);
+}
+
+static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
+				  struct platform_device *pdev)
+{
+	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;
+
+	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);
+}
+
+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;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops, stm32_dwmac_suspend, stm32_dwmac_resume);
+
+static const struct of_device_id stm32_dwmac_match[] = {
+	{ .compatible = "st,stm32-dwmac"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
+
+static struct platform_driver stm32_dwmac_driver = {
+	.probe  = stm32_dwmac_probe,
+	.remove = stm32_dwmac_remove,
+	.driver = {
+		.name           = "stm32-dwmac",
+		.pm		= &stm32_dwmac_pm_ops,
+		.of_match_table = stm32_dwmac_match,
+	},
+};
+module_platform_driver(stm32_dwmac_driver);
+
+MODULE_AUTHOR("Alexandre Torgue <alexandre.torgue@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Alexandre TORGUE]

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@gmail.com>

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
 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
new file mode 100644
index 0000000..0b48ee7
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -0,0 +1,208 @@
+/*
+ * dwmac-stm32.c - DWMAC Specific Glue layer for STM32 MCU
+ *
+ * Copyright (C) Alexandre Torgue 2015
+ * Author:  Alexandre Torgue <alexandre.torgue@gmail.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define MII_PHY_SEL_MASK	BIT(23)
+
+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)
+{
+	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;
+
+	clk_disable_unprepare(dwmac->clk_tx);
+	clk_disable_unprepare(dwmac->clk_rx);
+}
+
+static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
+				  struct platform_device *pdev)
+{
+	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;
+
+	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);
+}
+
+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;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops, stm32_dwmac_suspend, stm32_dwmac_resume);
+
+static const struct of_device_id stm32_dwmac_match[] = {
+	{ .compatible = "st,stm32-dwmac"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
+
+static struct platform_driver stm32_dwmac_driver = {
+	.probe  = stm32_dwmac_probe,
+	.remove = stm32_dwmac_remove,
+	.driver = {
+		.name           = "stm32-dwmac",
+		.pm		= &stm32_dwmac_pm_ops,
+		.of_match_table = stm32_dwmac_match,
+	},
+};
+module_platform_driver(stm32_dwmac_driver);
+
+MODULE_AUTHOR("Alexandre Torgue <alexandre.torgue@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[Alexandre TORGUE]

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@gmail.com>

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
 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
new file mode 100644
index 0000000..0b48ee7
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -0,0 +1,208 @@
+/*
+ * dwmac-stm32.c - DWMAC Specific Glue layer for STM32 MCU
+ *
+ * Copyright (C) Alexandre Torgue 2015
+ * Author:  Alexandre Torgue <alexandre.torgue@gmail.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define MII_PHY_SEL_MASK	BIT(23)
+
+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)
+{
+	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;
+
+	clk_disable_unprepare(dwmac->clk_tx);
+	clk_disable_unprepare(dwmac->clk_rx);
+}
+
+static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
+				  struct platform_device *pdev)
+{
+	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;
+
+	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);
+}
+
+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;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops, stm32_dwmac_suspend, stm32_dwmac_resume);
+
+static const struct of_device_id stm32_dwmac_match[] = {
+	{ .compatible = "st,stm32-dwmac"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
+
+static struct platform_driver stm32_dwmac_driver = {
+	.probe  = stm32_dwmac_probe,
+	.remove = stm32_dwmac_remove,
+	.driver = {
+		.name           = "stm32-dwmac",
+		.pm		= &stm32_dwmac_pm_ops,
+		.of_match_table = stm32_dwmac_match,
+	},
+};
+module_platform_driver(stm32_dwmac_driver);
+
+MODULE_AUTHOR("Alexandre Torgue <alexandre.torgue@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre TORGUE]

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
new file mode 100644
index 0000000..18734b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
@@ -0,0 +1,41 @@
+STMicroelectronics STM32 / MCU DWMAC glue layer controller
+
+This file documents platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+The device node has following properties.
+
+Required properties:
+- compatible:  Should be "st,stm32-dwmac" to select glue, and
+	       "snps,dwmac-3.50a" to select IP vesrion.
+- clocks: Should contain the GMAC main clock, and tx clock
+- compatible:  Should be "st,stm32-dwmac" to select glue and
+	       "snps,dwmac-3.50a" to select IP version.
+- clocks: Should contain the MAC main clock
+- clock-names: Should contain the clock names "stmmaceth".
+- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
+	      encompases the glue register, and the offset of the control register.
+
+Optional properties:
+- clocks: Could contain:
+	- the tx clock,
+	- the rx clock
+- clock-names: Could contain the clock names "tx-clk", "rx-clk"
+
+Example:
+
+		ethernet0: dwmac@40028000 {
+			device_type = "network";
+			compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
+			status = "disabled";
+			reg = <0x40028000 0x8000>;
+			reg-names = "stmmaceth";
+			interrupts = <0 61 0>, <0 62 0>;
+			interrupt-names = "macirq", "eth_wake_irq";
+			clock-names = "stmmaceth", "tx-clk", "rx-clk";
+			clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
+			st,syscon = <&syscfg 0x4>;
+			snps,pbl = <32>;
+			snps,mixed-burst;
+			dma-ranges;
+		};
-- 
1.9.1

[PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre TORGUE]

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
new file mode 100644
index 0000000..18734b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
@@ -0,0 +1,41 @@
+STMicroelectronics STM32 / MCU DWMAC glue layer controller
+
+This file documents platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+The device node has following properties.
+
+Required properties:
+- compatible:  Should be "st,stm32-dwmac" to select glue, and
+	       "snps,dwmac-3.50a" to select IP vesrion.
+- clocks: Should contain the GMAC main clock, and tx clock
+- compatible:  Should be "st,stm32-dwmac" to select glue and
+	       "snps,dwmac-3.50a" to select IP version.
+- clocks: Should contain the MAC main clock
+- clock-names: Should contain the clock names "stmmaceth".
+- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
+	      encompases the glue register, and the offset of the control register.
+
+Optional properties:
+- clocks: Could contain:
+	- the tx clock,
+	- the rx clock
+- clock-names: Could contain the clock names "tx-clk", "rx-clk"
+
+Example:
+
+		ethernet0: dwmac at 40028000 {
+			device_type = "network";
+			compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
+			status = "disabled";
+			reg = <0x40028000 0x8000>;
+			reg-names = "stmmaceth";
+			interrupts = <0 61 0>, <0 62 0>;
+			interrupt-names = "macirq", "eth_wake_irq";
+			clock-names = "stmmaceth", "tx-clk", "rx-clk";
+			clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
+			st,syscon = <&syscfg 0x4>;
+			snps,pbl = <32>;
+			snps,mixed-burst;
+			dma-ranges;
+		};
-- 
1.9.1

[PATCH v2 3/4] net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP

[Alexandre TORGUE]

Adds support of Synopsys 3.50a MAC IP in stmmac driver.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 6a52fa1..6cca626 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -178,6 +178,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
 	 * once needed on other platforms.
 	 */
 	if (of_device_is_compatible(np, "st,spear600-gmac") ||
+		of_device_is_compatible(np, "snps,dwmac-3.50a") ||
 		of_device_is_compatible(np, "snps,dwmac-3.70a") ||
 		of_device_is_compatible(np, "snps,dwmac")) {
 		/* Note that the max-frame-size parameter as defined in the
-- 
1.9.1

[PATCH v2 3/4] net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP

[Alexandre TORGUE]

Adds support of Synopsys 3.50a MAC IP in stmmac driver.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 6a52fa1..6cca626 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -178,6 +178,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
 	 * once needed on other platforms.
 	 */
 	if (of_device_is_compatible(np, "st,spear600-gmac") ||
+		of_device_is_compatible(np, "snps,dwmac-3.50a") ||
 		of_device_is_compatible(np, "snps,dwmac-3.70a") ||
 		of_device_is_compatible(np, "snps,dwmac")) {
 		/* Note that the max-frame-size parameter as defined in the
-- 
1.9.1

[PATCH v2 4/4] ARM: STM32: Enable Ethernet in stm32_defconfig

[Alexandre TORGUE]

Enable basic Ethernet support (IPV4) for stm32 defconfig.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index ec52505..8b8abe0 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -33,11 +33,20 @@ CONFIG_XIP_PHYS_ADDR=0x08008000
 CONFIG_BINFMT_FLAT=y
 CONFIG_BINFMT_SHARED_FLAT=y
 # CONFIG_COREDUMP is not set
+CONFIG_NET=y
+CONFIG_INET=y
+# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
+# CONFIG_INET_XFRM_MODE_TUNNEL is not set
+# CONFIG_INET_XFRM_MODE_BEET is not set
+# CONFIG_IPV6 is not set
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 # CONFIG_FW_LOADER is not set
 # CONFIG_BLK_DEV is not set
 CONFIG_EEPROM_93CX6=y
+CONFIG_NETDEVICES=y
+CONFIG_STMMAC_ETH=y
+# CONFIG_WLAN is not set
 # CONFIG_INPUT is not set
 # CONFIG_SERIO is not set
 # CONFIG_VT is not set
-- 
1.9.1

[PATCH v2 4/4] ARM: STM32: Enable Ethernet in stm32_defconfig

[Alexandre TORGUE]

Enable basic Ethernet support (IPV4) for stm32 defconfig.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index ec52505..8b8abe0 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -33,11 +33,20 @@ CONFIG_XIP_PHYS_ADDR=0x08008000
 CONFIG_BINFMT_FLAT=y
 CONFIG_BINFMT_SHARED_FLAT=y
 # CONFIG_COREDUMP is not set
+CONFIG_NET=y
+CONFIG_INET=y
+# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
+# CONFIG_INET_XFRM_MODE_TUNNEL is not set
+# CONFIG_INET_XFRM_MODE_BEET is not set
+# CONFIG_IPV6 is not set
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 # CONFIG_FW_LOADER is not set
 # CONFIG_BLK_DEV is not set
 CONFIG_EEPROM_93CX6=y
+CONFIG_NETDEVICES=y
+CONFIG_STMMAC_ETH=y
+# CONFIG_WLAN is not set
 # CONFIG_INPUT is not set
 # CONFIG_SERIO is not set
 # CONFIG_VT is not set
-- 
1.9.1

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

[Joachim Eastwood]

Hi Alexandre,

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> 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@gmail.com>
>
> 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

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


>  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().


> +{
> +       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.


> +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.

> +{
> +       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.


> +       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.

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.


> +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?

This could be written:
    ret = stm32_dwmac_init(priv->plat->bsp_priv);
    if (ret)
       return ret;

    return stmmac_resume(ndev);


regards,
Joachim Eastwood

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

[Joachim Eastwood]

Hi Alexandre,

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> 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@gmail.com>
>
> 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

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


>  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().


> +{
> +       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.


> +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.

> +{
> +       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.


> +       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.

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.


> +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?

This could be written:
    ret = stm32_dwmac_init(priv->plat->bsp_priv);
    if (ret)
       return ret;

    return stmmac_resume(ndev);


regards,
Joachim Eastwood

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

[Joachim Eastwood]

Hi Alexandre,

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> 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@gmail.com>
>
> 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

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


>  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().


> +{
> +       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.


> +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.

> +{
> +       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.


> +       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.

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.


> +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?

This could be written:
    ret = stm32_dwmac_init(priv->plat->bsp_priv);
    if (ret)
       return ret;

    return stmmac_resume(ndev);


regards,
Joachim Eastwood

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Joachim Eastwood]

Hi Alexandre,

You should copy 'devicetree@vger.kernel.org' on bindings doc. Adding cc here.

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> wrote:
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 0000000..18734b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
> +              "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible:  Should be "st,stm32-dwmac" to select glue and
> +              "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
> +             encompases the glue register, and the offset of the control register.
> +
> +Optional properties:
> +- clocks: Could contain:
> +       - the tx clock,
> +       - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> +               ethernet0: dwmac@40028000 {
> +                       device_type = "network";

What is this 'device_type = "network"' for?
It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Maybe Rob could enlighten us?

> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> +                       status = "disabled";
> +                       reg = <0x40028000 0x8000>;
> +                       reg-names = "stmmaceth";
> +                       interrupts = <0 61 0>, <0 62 0>;
> +                       interrupt-names = "macirq", "eth_wake_irq";
> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
> +                       st,syscon = <&syscfg 0x4>;
> +                       snps,pbl = <32>;

Regarding snps,pbl; using 32 here might not give you what you would except.
See comment in dwmac1000_dma_init().

The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.


regards,
Joachim Eastwood

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Joachim Eastwood]

Hi Alexandre,

You should copy 'devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org' on bindings doc. Adding cc here.

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Signed-off-by: Alexandre TORGUE <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 0000000..18734b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
> +              "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible:  Should be "st,stm32-dwmac" to select glue and
> +              "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
> +             encompases the glue register, and the offset of the control register.
> +
> +Optional properties:
> +- clocks: Could contain:
> +       - the tx clock,
> +       - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> +               ethernet0: dwmac@40028000 {
> +                       device_type = "network";

What is this 'device_type = "network"' for?
It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Maybe Rob could enlighten us?

> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> +                       status = "disabled";
> +                       reg = <0x40028000 0x8000>;
> +                       reg-names = "stmmaceth";
> +                       interrupts = <0 61 0>, <0 62 0>;
> +                       interrupt-names = "macirq", "eth_wake_irq";
> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
> +                       st,syscon = <&syscfg 0x4>;
> +                       snps,pbl = <32>;

Regarding snps,pbl; using 32 here might not give you what you would except.
See comment in dwmac1000_dma_init().

The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.


regards,
Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Joachim Eastwood]

Hi Alexandre,

You should copy 'devicetree at vger.kernel.org' on bindings doc. Adding cc here.

On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.torgue@gmail.com> wrote:
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 0000000..18734b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
> +              "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible:  Should be "st,stm32-dwmac" to select glue and
> +              "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
> +             encompases the glue register, and the offset of the control register.
> +
> +Optional properties:
> +- clocks: Could contain:
> +       - the tx clock,
> +       - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> +               ethernet0: dwmac at 40028000 {
> +                       device_type = "network";

What is this 'device_type = "network"' for?
It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Maybe Rob could enlighten us?

> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> +                       status = "disabled";
> +                       reg = <0x40028000 0x8000>;
> +                       reg-names = "stmmaceth";
> +                       interrupts = <0 61 0>, <0 62 0>;
> +                       interrupt-names = "macirq", "eth_wake_irq";
> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
> +                       st,syscon = <&syscfg 0x4>;
> +                       snps,pbl = <32>;

Regarding snps,pbl; using 32 here might not give you what you would except.
See comment in dwmac1000_dma_init().

The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.


regards,
Joachim Eastwood

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> You should copy 'devicetree@vger.kernel.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac@40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.
>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
Hi Joachim,

My fault, It is not working with 32. I will modify it (should be 8 for example)

Regards

Alexandre

>
> regards,
> Joachim Eastwood

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Hi Alexandre,
>
> You should copy 'devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac@40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.
>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
Hi Joachim,

My fault, It is not working with 32. I will modify it (should be 8 for example)

Regards

Alexandre

>
> regards,
> Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> You should copy 'devicetree at vger.kernel.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac at 40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.
>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
Hi Joachim,

My fault, It is not working with 32. I will modify it (should be 8 for example)

Regards

Alexandre

>
> regards,
> Joachim Eastwood

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@gmail.com>:
> Hi Alexandre,
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> 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@gmail.com>
>>
>> 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

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@gmail.com>:
> Hi Alexandre,
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> 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@gmail.com>
>>
>> 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

[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@gmail.com>:
> Hi Alexandre,
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> 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@gmail.com>
>>
>> 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

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

Hi Joachim,

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> You should copy 'devicetree@vger.kernel.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac@40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Actually it seems that this entry is deprecated for ARM
(http://lists.openwall.net/linux-kernel/2013/12/11/372)
Concerning stmmac driver I don't see an issue to remove it (tested).
So If Rob and Peppe could confirm, I will remove it in my V3.

Br

Alexandre


>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
>
> regards,
> Joachim Eastwood

Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

Hi Joachim,

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> You should copy 'devicetree@vger.kernel.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac@40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Actually it seems that this entry is deprecated for ARM
(http://lists.openwall.net/linux-kernel/2013/12/11/372)
Concerning stmmac driver I don't see an issue to remove it (tested).
So If Rob and Peppe could confirm, I will remove it in my V3.

Br

Alexandre


>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
>
> regards,
> Joachim Eastwood

[PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue

[Alexandre Torgue]

Hi Joachim,

2016-02-23 23:37 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> Hi Alexandre,
>
> You should copy 'devicetree at vger.kernel.org' on bindings doc. Adding cc here.
>
> On 23 February 2016 at 16:10, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> new file mode 100644
>> index 0000000..18734b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>> @@ -0,0 +1,41 @@
>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>> +
>> +This file documents platform glue layer for stmmac.
>> +Please see stmmac.txt for the other unchanged properties.
>> +
>> +The device node has following properties.
>> +
>> +Required properties:
>> +- compatible:  Should be "st,stm32-dwmac" to select glue, and
>> +              "snps,dwmac-3.50a" to select IP vesrion.
>> +- clocks: Should contain the GMAC main clock, and tx clock
>> +- compatible:  Should be "st,stm32-dwmac" to select glue and
>> +              "snps,dwmac-3.50a" to select IP version.
>> +- clocks: Should contain the MAC main clock
>> +- clock-names: Should contain the clock names "stmmaceth".
>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>> +             encompases the glue register, and the offset of the control register.
>> +
>> +Optional properties:
>> +- clocks: Could contain:
>> +       - the tx clock,
>> +       - the rx clock
>> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
>> +
>> +Example:
>> +
>> +               ethernet0: dwmac at 40028000 {
>> +                       device_type = "network";
>
> What is this 'device_type = "network"' for?
> It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.

Actually it seems that this entry is deprecated for ARM
(http://lists.openwall.net/linux-kernel/2013/12/11/372)
Concerning stmmac driver I don't see an issue to remove it (tested).
So If Rob and Peppe could confirm, I will remove it in my V3.

Br

Alexandre


>
> Maybe Rob could enlighten us?
>
>> +                       compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>> +                       status = "disabled";
>> +                       reg = <0x40028000 0x8000>;
>> +                       reg-names = "stmmaceth";
>> +                       interrupts = <0 61 0>, <0 62 0>;
>> +                       interrupt-names = "macirq", "eth_wake_irq";
>> +                       clock-names = "stmmaceth", "tx-clk", "rx-clk";
>> +                       clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>> +                       st,syscon = <&syscfg 0x4>;
>> +                       snps,pbl = <32>;
>
> Regarding snps,pbl; using 32 here might not give you what you would except.
> See comment in dwmac1000_dma_init().
>
> The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
>
>
> regards,
> Joachim Eastwood

Re: [PATCH v2 0/4] Add Ethernet support on STM32F429

[Maxime Coquelin]

Hi Alex,

On 02/23/2016 04:10 PM, Alexandre TORGUE wrote:
> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
> This series:
>   -enhance current stmmac driver to control it (code already
> available) and adds basic glue for STM32F429 chip.
>   -Enable basic Net config in kernel.
>
> Note that DT patches are not present because STM32 pinctrl code is not
> yet avalaible.
>
> Changes since v1:
>   -Fix Kbuild issue in Kconfig.
>   -Remove init/exit callbacks. Suspend/Resume and remove driver is no more
> driven in stmmac_pltfr but directly in dwmac-stm32 glue driver.
>   -Take into account Joachim review.
>
> Regards.
>
> Alexandre.
>
> Alexandre TORGUE (4):
>    net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
>    Documentation: Bindings: Add STM32 DWMAC glue
>    net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP
>    ARM: STM32: Enable Ethernet in stm32_defconfig
>
>   .../devicetree/bindings/net/stm32-dwmac.txt        |  41 ++++
>   arch/arm/configs/stm32_defconfig                   |   9 +
>   drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 ++
>   drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
>   drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  | 208 +++++++++++++++++++++
>   .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   1 +
>   6 files changed, 272 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/net/stm32-dwmac.txt
>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>

You can add my:
Tested-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks!
Maxime

[PATCH v2 0/4] Add Ethernet support on STM32F429

[Maxime Coquelin]

Hi Alex,

On 02/23/2016 04:10 PM, Alexandre TORGUE wrote:
> STM32F429 Chip embeds a Synopsys 3.50a MAC IP.
> This series:
>   -enhance current stmmac driver to control it (code already
> available) and adds basic glue for STM32F429 chip.
>   -Enable basic Net config in kernel.
>
> Note that DT patches are not present because STM32 pinctrl code is not
> yet avalaible.
>
> Changes since v1:
>   -Fix Kbuild issue in Kconfig.
>   -Remove init/exit callbacks. Suspend/Resume and remove driver is no more
> driven in stmmac_pltfr but directly in dwmac-stm32 glue driver.
>   -Take into account Joachim review.
>
> Regards.
>
> Alexandre.
>
> Alexandre TORGUE (4):
>    net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
>    Documentation: Bindings: Add STM32 DWMAC glue
>    net: ethernet: stmmac: add support of Synopsys 3.50a MAC IP
>    ARM: STM32: Enable Ethernet in stm32_defconfig
>
>   .../devicetree/bindings/net/stm32-dwmac.txt        |  41 ++++
>   arch/arm/configs/stm32_defconfig                   |   9 +
>   drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 ++
>   drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
>   drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c  | 208 +++++++++++++++++++++
>   .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   1 +
>   6 files changed, 272 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/net/stm32-dwmac.txt
>   create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>

You can add my:
Tested-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks!
Maxime