[PATCH net-next 00/12] ARM: sun8i: r40: Add Ethernet support

[PATCH net-next 00/12] ARM: sun8i: r40: Add Ethernet support

[Chen-Yu Tsai]

Hi everyone,

This series adds support for the DWMAC based Ethernet controller found
on the Allwinner R40 SoC. The controller is either a DWMAC clone or
DWMAC core with its registers rearranged. This is already supported by
the dwmac-sun8i driver. The glue layer control registers, unlike other
sun8i family SoCs, is not in the system controller region, but in the
clock control unit, like with the older A20 and A31 SoCs.

Mark (Brown), could you take a look at the regmap bits in patches 2 and
7? While we reuse the bindings for dwmac-sun8i using a syscon phandle
reference, we need some custom plumbing for the clock driver to export
a regmap that only allows access to the GMAC register to the dwmac-sun8i
driver. An alternative would be to allow drivers to register custom
syscon devices with their own regmap and locking.

Patch 1 converts the CLK_OF_DECLARE style clock driver to a platform
one, so the regmap introduced later has a struct device to tie to.

Patch 2 adds a regmap that is exported by the clock driver for the
dwmac-sun8i driver to use.

Patch 3 and 4 clean up the dwmac-sun8i binding.

Patch 5 adds device tree binding for Allwinner R40's Ethernet
controller.

Patch 6 converts regmap access of the syscon region in the dwmac-sun8i
driver to regmap_field, in anticipation of different field widths on
the R40.

Patch 7 introduces custom plumbing in the dwmac-sun8i driver to fetch
a regmap from the clock module, by looking up a device via a phandle,
then getting the regmap associated with that device.

Patch 8 adds support for different or absent TX/RX delay chain ranges
to the dwmac-sun8i driver.

Patch 9 adds support for the R40's ethernet controller.

Patch 10 cleans up the Bananapi M2 Ultra device tree file.

Patch 11 adds a GMAC device node and RGMII mode pinmux setting for the
R40.

Patch 12 enables Ethernet on the Bananapi M2 Ultra.


Please have a look.

Regards
ChenYu

Chen-Yu Tsai (10):
  dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions
  dt-bindings: net: dwmac-sun8i: Sort syscon compatibles by alphabetical
    order
  dt-bindings: net: dwmac-sun8i: Add binding for GMAC on Allwinner R40
    SoC
  net: stmmac: dwmac-sun8i: Use regmap_field for syscon register access
  net: stmmac: dwmac-sun8i: Allow getting syscon regmap from CCU device
  net: stmmac: dwmac-sun8i: Support different ranges for TX/RX delay
    chains
  net: stmmac: dwmac-sun8i: Add support for GMAC on Allwinner R40 SoC
  ARM: dts: sun8i: r40: bananapi-m2-ultra: Sort device node dereferences
  ARM: dts: sun8i: r40: Add device node and RGMII pinmux node for GMAC
  ARM: dts: sun8i: r40: bananapi-m2-ultra: Enable GMAC ethernet
    controller

Icenowy Zheng (2):
  clk: sunxi-ng: r40: rewrite init code to a platform driver
  clk: sunxi-ng: r40: export a regmap to access the GMAC register

 .../devicetree/bindings/net/dwmac-sun8i.txt        |  17 ++-
 arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts  |  99 ++++++++++------
 arch/arm/boot/dts/sun8i-r40.dtsi                   |  34 ++++++
 drivers/clk/sunxi-ng/ccu-sun8i-r40.c               |  70 +++++++++--
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  | 128 +++++++++++++++++----
 5 files changed, 278 insertions(+), 70 deletions(-)

-- 
2.16.2

[PATCH net-next 01/12] clk: sunxi-ng: r40: rewrite init code to a platform driver

[Chen-Yu Tsai]

From: Icenowy Zheng <icenowy@aosc.io>

As we need to register a regmap on the R40 CCU, there needs to be a
device structure bound to the CCU device node.

Rewrite the R40 CCU driver initial code to make it a proper platform
driver, thus we will have a platform device bound to it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/clk/sunxi-ng/ccu-sun8i-r40.c | 39 ++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
index 933f2e68f42a..c3aa839a453d 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
@@ -12,7 +12,8 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
 
 #include "ccu_common.h"
 #include "ccu_reset.h"
@@ -1250,17 +1251,17 @@ static struct ccu_mux_nb sun8i_r40_cpu_nb = {
 	.bypass_index	= 1, /* index of 24 MHz oscillator */
 };
 
-static void __init sun8i_r40_ccu_setup(struct device_node *node)
+static int sun8i_r40_ccu_probe(struct platform_device *pdev)
 {
+	struct resource *res;
 	void __iomem *reg;
 	u32 val;
+	int ret;
 
-	reg = of_io_request_and_map(node, 0, of_node_full_name(node));
-	if (IS_ERR(reg)) {
-		pr_err("%s: Could not map the clock registers\n",
-		       of_node_full_name(node));
-		return;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
 
 	/* Force the PLL-Audio-1x divider to 4 */
 	val = readl(reg + SUN8I_R40_PLL_AUDIO_REG);
@@ -1277,7 +1278,9 @@ static void __init sun8i_r40_ccu_setup(struct device_node *node)
 	val &= ~GENMASK(25, 20);
 	writel(val, reg + SUN8I_R40_USB_CLK_REG);
 
-	sunxi_ccu_probe(node, reg, &sun8i_r40_ccu_desc);
+	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun8i_r40_ccu_desc);
+	if (ret)
+		return ret;
 
 	/* Gate then ungate PLL CPU after any rate changes */
 	ccu_pll_notifier_register(&sun8i_r40_pll_cpu_nb);
@@ -1285,6 +1288,20 @@ static void __init sun8i_r40_ccu_setup(struct device_node *node)
 	/* Reparent CPU during PLL CPU rate changes */
 	ccu_mux_notifier_register(pll_cpu_clk.common.hw.clk,
 				  &sun8i_r40_cpu_nb);
+
+	return 0;
 }
-CLK_OF_DECLARE(sun8i_r40_ccu, "allwinner,sun8i-r40-ccu",
-	       sun8i_r40_ccu_setup);
+
+static const struct of_device_id sun8i_r40_ccu_ids[] = {
+	{ .compatible = "allwinner,sun8i-r40-ccu" },
+	{ }
+};
+
+static struct platform_driver sun8i_r40_ccu_driver = {
+	.probe	= sun8i_r40_ccu_probe,
+	.driver	= {
+		.name	= "sun8i-r40-ccu",
+		.of_match_table	= sun8i_r40_ccu_ids,
+	},
+};
+builtin_platform_driver(sun8i_r40_ccu_driver);
-- 
2.16.2

[PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Chen-Yu Tsai]

From: Icenowy Zheng <icenowy@aosc.io>

There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
the syscon part, in the CCU of R40 SoC.

Export a regmap of the CCU.

Read access is not restricted to all registers, but only the GMAC
register is allowed to be written.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/clk/sunxi-ng/ccu-sun8i-r40.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
index c3aa839a453d..54c7a6106206 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c
@@ -1251,9 +1251,35 @@ static struct ccu_mux_nb sun8i_r40_cpu_nb = {
 	.bypass_index	= 1, /* index of 24 MHz oscillator */
 };
 
+/*
+ * Add a regmap for the GMAC driver (dwmac-sun8i) to access the
+ * GMAC configuration register.
+ * Only this register is allowed to be written, in order to
+ * prevent overriding critical clock configuration.
+ */
+
+#define SUN8I_R40_GMAC_CFG_REG 0x164
+static bool sun8i_r40_ccu_regmap_writeable_reg(struct device *dev,
+					       unsigned int reg)
+{
+	if (reg == SUN8I_R40_GMAC_CFG_REG)
+		return true;
+	return false;
+}
+
+static struct regmap_config sun8i_r40_ccu_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= 0x320, /* PLL_LOCK_CTRL_REG */
+
+	.writeable_reg	= sun8i_r40_ccu_regmap_writeable_reg,
+};
+
 static int sun8i_r40_ccu_probe(struct platform_device *pdev)
 {
 	struct resource *res;
+	struct regmap *regmap;
 	void __iomem *reg;
 	u32 val;
 	int ret;
@@ -1278,6 +1304,11 @@ static int sun8i_r40_ccu_probe(struct platform_device *pdev)
 	val &= ~GENMASK(25, 20);
 	writel(val, reg + SUN8I_R40_USB_CLK_REG);
 
+	regmap = devm_regmap_init_mmio(&pdev->dev, reg,
+				       &sun8i_r40_ccu_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
 	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun8i_r40_ccu_desc);
 	if (ret)
 		return ret;
-- 
2.16.2

[PATCH net-next 03/12] dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions

[Chen-Yu Tsai]

The clock delay chains found in the glue layer for dwmac-sun8i are only
used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
range of possible values for the delay chain.

This patch reformats the delay chain section of the device tree binding
to make it clear that the delay chains only apply to RGMII PHYs, and
make it easier to add the R40-specific bits later.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
index 3d6d5fa0c4d5..b8a3028d6c30 100644
--- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
+++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
@@ -28,10 +28,13 @@ Required properties:
   - allwinner,sun8i-a83t-system-controller
 
 Optional properties:
-- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 0-700. Default is 0)
-- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 0-3100. Default is 0)
-Both delay properties need to be a multiple of 100. They control the delay for
-external PHY.
+- allwinner,tx-delay-ps: TX clock delay chain value in ps.
+			 Range is 0-700. Default is 0.
+- allwinner,rx-delay-ps: RX clock delay chain value in ps.
+			 Range is 0-3100. Default is 0.
+Both delay properties need to be a multiple of 100. They control the
+clock delay for external RGMII PHY. They are do apply to the internal
+PHY or external non-RGMII PHYs.
 
 Optional properties for the following compatibles:
   - "allwinner,sun8i-h3-emac",
-- 
2.16.2

[PATCH net-next 04/12] dt-bindings: net: dwmac-sun8i: Sort syscon compatibles by alphabetical order

[Chen-Yu Tsai]

The A83T syscon compatible was appended to the syscon compatibles list,
instead of inserted in to preserve the ordering.

Move it to the proper place to keep the list sorted.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
index b8a3028d6c30..74b3ef79e57a 100644
--- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
+++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
@@ -22,10 +22,10 @@ Required properties:
 - #size-cells: shall be 0
 - syscon: A phandle to the syscon of the SoC with one of the following
  compatible string:
+  - allwinner,sun8i-a83t-system-controller
   - allwinner,sun8i-h3-system-controller
   - allwinner,sun8i-v3s-system-controller
   - allwinner,sun50i-a64-system-controller
-  - allwinner,sun8i-a83t-system-controller
 
 Optional properties:
 - allwinner,tx-delay-ps: TX clock delay chain value in ps.
-- 
2.16.2

[PATCH net-next 05/12] dt-bindings: net: dwmac-sun8i: Add binding for GMAC on Allwinner R40 SoC

[Chen-Yu Tsai]

The Allwinner R40 SoC has the EMAC controller supported by dwmac-sun8i.
It is named "GMAC", while EMAC refers to the 10/100 Mbps Ethernet
controller supported by sun4i-emac. The controller is the same, but
the R40 has the glue layer controls in the clock control unit (CCU),
with a reduced RX delay chain, and no TX delay chain.

This patch adds the R40 specific bits to the dwmac-sun8i binding.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
index 74b3ef79e57a..fe4a48a1eb50 100644
--- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
+++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
@@ -7,6 +7,7 @@ Required properties:
 - compatible: must be one of the following string:
 		"allwinner,sun8i-a83t-emac"
 		"allwinner,sun8i-h3-emac"
+		"allwinner,sun8i-r40-gmac"
 		"allwinner,sun8i-v3s-emac"
 		"allwinner,sun50i-a64-emac"
 - reg: address and length of the register for the device.
@@ -24,14 +25,17 @@ Required properties:
  compatible string:
   - allwinner,sun8i-a83t-system-controller
   - allwinner,sun8i-h3-system-controller
+  - allwinner,sun8i-r40-ccu
   - allwinner,sun8i-v3s-system-controller
   - allwinner,sun50i-a64-system-controller
 
 Optional properties:
 - allwinner,tx-delay-ps: TX clock delay chain value in ps.
 			 Range is 0-700. Default is 0.
+			 Unavailable for allwinner,sun8i-r40-gmac
 - allwinner,rx-delay-ps: RX clock delay chain value in ps.
 			 Range is 0-3100. Default is 0.
+			 Range is 0-700 for allwinner,sun8i-r40-gmac
 Both delay properties need to be a multiple of 100. They control the
 clock delay for external RGMII PHY. They are do apply to the internal
 PHY or external non-RGMII PHYs.
-- 
2.16.2

[PATCH net-next 06/12] net: stmmac: dwmac-sun8i: Use regmap_field for syscon register access

[Chen-Yu Tsai]

On the Allwinner R40, the "GMAC clock" register is located in the CCU
block, at a different register address than the other SoCs that have
it in the "system control" block.

This patch converts the use of regmap to regmap_field for mapping and
accessing the syscon register, so we can have the register address in
the variants data, and not in the actual register manipulation code.

This patch only converts regmap_read() and regmap_write() calls to
regmap_field_read() and regmap_field_write() calls. There are some
places where it might make sense to switch to regmap_field_update_bits(),
but this is not done here to keep the patch simple.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 42 +++++++++++++++++------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index a3fa65b1ca8e..de93f0faf58d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -42,6 +42,7 @@
  *				This value is used for disabling properly EMAC
  *				and used as a good starting value in case of the
  *				boot process(uboot) leave some stuff.
+ * @syscon_field		reg_field for the syscon's gmac register
  * @soc_has_internal_phy:	Does the MAC embed an internal PHY
  * @support_mii:		Does the MAC handle MII
  * @support_rmii:		Does the MAC handle RMII
@@ -49,6 +50,7 @@
  */
 struct emac_variant {
 	u32 default_syscon_value;
+	const struct reg_field *syscon_field;
 	bool soc_has_internal_phy;
 	bool support_mii;
 	bool support_rmii;
@@ -71,13 +73,21 @@ struct sunxi_priv_data {
 	struct regulator *regulator;
 	struct reset_control *rst_ephy;
 	const struct emac_variant *variant;
-	struct regmap *regmap;
+	struct regmap_field *regmap_field;
 	bool internal_phy_powered;
 	void *mux_handle;
 };
 
+/* EMAC clock register @ 0x30 in the "system control" address range */
+const struct reg_field sun8i_syscon_reg_field = {
+	.reg = 0x30,
+	.lsb = 0,
+	.msb = 31,
+};
+
 static const struct emac_variant emac_variant_h3 = {
 	.default_syscon_value = 0x58000,
+	.syscon_field = &sun8i_syscon_reg_field,
 	.soc_has_internal_phy = true,
 	.support_mii = true,
 	.support_rmii = true,
@@ -86,12 +96,14 @@ static const struct emac_variant emac_variant_h3 = {
 
 static const struct emac_variant emac_variant_v3s = {
 	.default_syscon_value = 0x38000,
+	.syscon_field = &sun8i_syscon_reg_field,
 	.soc_has_internal_phy = true,
 	.support_mii = true
 };
 
 static const struct emac_variant emac_variant_a83t = {
 	.default_syscon_value = 0,
+	.syscon_field = &sun8i_syscon_reg_field,
 	.soc_has_internal_phy = false,
 	.support_mii = true,
 	.support_rgmii = true
@@ -99,6 +111,7 @@ static const struct emac_variant emac_variant_a83t = {
 
 static const struct emac_variant emac_variant_a64 = {
 	.default_syscon_value = 0,
+	.syscon_field = &sun8i_syscon_reg_field,
 	.soc_has_internal_phy = false,
 	.support_mii = true,
 	.support_rmii = true,
@@ -216,7 +229,6 @@ static const struct emac_variant emac_variant_a64 = {
 #define SYSCON_ETCS_MII		0x0
 #define SYSCON_ETCS_EXT_GMII	0x1
 #define SYSCON_ETCS_INT_GMII	0x2
-#define SYSCON_EMAC_REG		0x30
 
 /* sun8i_dwmac_dma_reset() - reset the EMAC
  * Called from stmmac via stmmac_dma_ops->reset
@@ -745,7 +757,7 @@ static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
 	bool need_power_ephy = false;
 
 	if (current_child ^ desired_child) {
-		regmap_read(gmac->regmap, SYSCON_EMAC_REG, &reg);
+		regmap_field_read(gmac->regmap_field, &reg);
 		switch (desired_child) {
 		case DWMAC_SUN8I_MDIO_MUX_INTERNAL_ID:
 			dev_info(priv->device, "Switch mux to internal PHY");
@@ -763,7 +775,7 @@ static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
 				desired_child);
 			return -EINVAL;
 		}
-		regmap_write(gmac->regmap, SYSCON_EMAC_REG, val);
+		regmap_field_write(gmac->regmap_field, val);
 		if (need_power_ephy) {
 			ret = sun8i_dwmac_power_internal_phy(priv);
 			if (ret)
@@ -801,7 +813,7 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 	int ret;
 	u32 reg, val;
 
-	regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
+	regmap_field_read(gmac->regmap_field, &val);
 	reg = gmac->variant->default_syscon_value;
 	if (reg != val)
 		dev_warn(priv->device,
@@ -883,7 +895,7 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 		return -EINVAL;
 	}
 
-	regmap_write(gmac->regmap, SYSCON_EMAC_REG, reg);
+	regmap_field_write(gmac->regmap_field, reg);
 
 	return 0;
 }
@@ -892,7 +904,7 @@ static void sun8i_dwmac_unset_syscon(struct sunxi_priv_data *gmac)
 {
 	u32 reg = gmac->variant->default_syscon_value;
 
-	regmap_write(gmac->regmap, SYSCON_EMAC_REG, reg);
+	regmap_field_write(gmac->regmap_field, reg);
 }
 
 static void sun8i_dwmac_exit(struct platform_device *pdev, void *priv)
@@ -980,6 +992,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 	int ret;
 	struct stmmac_priv *priv;
 	struct net_device *ndev;
+	struct regmap *regmap;
 
 	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
 	if (ret)
@@ -1014,14 +1027,21 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 		gmac->regulator = NULL;
 	}
 
-	gmac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
-						       "syscon");
-	if (IS_ERR(gmac->regmap)) {
-		ret = PTR_ERR(gmac->regmap);
+	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "syscon");
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
 		dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
 		return ret;
 	}
 
+	gmac->regmap_field = devm_regmap_field_alloc(dev, regmap,
+						     *gmac->variant->syscon_field);
+	if (IS_ERR(gmac->regmap_field)) {
+		ret = PTR_ERR(gmac->regmap_field);
+		dev_err(dev, "Unable to map syscon register: %d\n", ret);
+		return ret;
+	}
+
 	plat_dat->interface = of_get_phy_mode(dev->of_node);
 
 	/* platform data specifying hardware features and callbacks.
-- 
2.16.2

[PATCH net-next 07/12] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from CCU device

[Chen-Yu Tsai]

On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU
address space. Using a standard syscon to access it provides no
coordination with the CCU driver for register access. Neither does
it prevent this and other drivers from accessing other, maybe critical,
clock control registers.

Instead, for these types of setups, we let the CCU register a proper
device and a regmap tied to it. We can then get the device from the
phandle, and retrieve the regmap with dev_get_regmap().

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 38 ++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index de93f0faf58d..a51175bcfd11 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -43,6 +43,9 @@
  *				and used as a good starting value in case of the
  *				boot process(uboot) leave some stuff.
  * @syscon_field		reg_field for the syscon's gmac register
+ * @syscon_from_dev		syscon regmap is in ccu address space and
+ *				needs to be retrieved using dev_get_regmap()
+ *				instead of syscon_regmap_lookup_by_phandle()
  * @soc_has_internal_phy:	Does the MAC embed an internal PHY
  * @support_mii:		Does the MAC handle MII
  * @support_rmii:		Does the MAC handle RMII
@@ -51,6 +54,7 @@
 struct emac_variant {
 	u32 default_syscon_value;
 	const struct reg_field *syscon_field;
+	bool syscon_from_dev;
 	bool soc_has_internal_phy;
 	bool support_mii;
 	bool support_rmii;
@@ -983,6 +987,34 @@ static struct mac_device_info *sun8i_dwmac_setup(void *ppriv)
 	return mac;
 }
 
+static struct regmap *sun8i_dwmac_get_syscon_from_dev(struct device_node *node)
+{
+	struct device_node *syscon_node;
+	struct platform_device *syscon_pdev;
+	struct regmap *regmap = NULL;
+
+	syscon_node = of_parse_phandle(node, "syscon", 0);
+	if (!syscon_node)
+		return ERR_PTR(-ENODEV);
+
+	syscon_pdev = of_find_device_by_node(syscon_node);
+	if (!syscon_pdev) {
+		/* platform device might not be probed yet */
+		regmap = ERR_PTR(-EPROBE_DEFER);
+		goto out_put_node;
+	}
+
+	/* If no regmap is found then the other device driver is at fault */
+	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
+	if (!regmap)
+		regmap = ERR_PTR(-EINVAL);
+
+	platform_device_put(syscon_pdev);
+out_put_node:
+	of_node_put(syscon_node);
+	return regmap;
+}
+
 static int sun8i_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
@@ -1027,7 +1059,11 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 		gmac->regulator = NULL;
 	}
 
-	regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "syscon");
+	if (gmac->variant->syscon_from_dev)
+		regmap = sun8i_dwmac_get_syscon_from_dev(pdev->dev.of_node);
+	else
+		regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							 "syscon");
 	if (IS_ERR(regmap)) {
 		ret = PTR_ERR(regmap);
 		dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
-- 
2.16.2

[PATCH net-next 08/12] net: stmmac: dwmac-sun8i: Support different ranges for TX/RX delay chains

[Chen-Yu Tsai]

On the R40 SoC, the RX delay chain only has a range of 0~7 (hundred
picoseconds), instead of 0~31. Also the TX delay chain is completely
absent.

This patch adds support for different ranges by adding per-compatible
maximum values in the variant data. A maximum of 0 indicates that the
delay chain is not supported or absent.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 32 ++++++++++++++++-------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index a51175bcfd11..08d263567a52 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -50,6 +50,12 @@
  * @support_mii:		Does the MAC handle MII
  * @support_rmii:		Does the MAC handle RMII
  * @support_rgmii:		Does the MAC handle RGMII
+ *
+ * @rx_delay_max:		Maximum raw value for RX delay chain
+ * @tx_delay_max:		Maximum raw value for TX delay chain
+ *				These two also indicate the bitmask for
+ *				the RX and TX delay chain registers. A
+ *				value of zero indicates this is not supported.
  */
 struct emac_variant {
 	u32 default_syscon_value;
@@ -59,6 +65,8 @@ struct emac_variant {
 	bool support_mii;
 	bool support_rmii;
 	bool support_rgmii;
+	u8 rx_delay_max;
+	u8 tx_delay_max;
 };
 
 /* struct sunxi_priv_data - hold all sunxi private data
@@ -95,7 +103,9 @@ static const struct emac_variant emac_variant_h3 = {
 	.soc_has_internal_phy = true,
 	.support_mii = true,
 	.support_rmii = true,
-	.support_rgmii = true
+	.support_rgmii = true,
+	.rx_delay_max = 31,
+	.tx_delay_max = 7,
 };
 
 static const struct emac_variant emac_variant_v3s = {
@@ -110,7 +120,9 @@ static const struct emac_variant emac_variant_a83t = {
 	.syscon_field = &sun8i_syscon_reg_field,
 	.soc_has_internal_phy = false,
 	.support_mii = true,
-	.support_rgmii = true
+	.support_rgmii = true,
+	.rx_delay_max = 31,
+	.tx_delay_max = 7,
 };
 
 static const struct emac_variant emac_variant_a64 = {
@@ -119,7 +131,9 @@ static const struct emac_variant emac_variant_a64 = {
 	.soc_has_internal_phy = false,
 	.support_mii = true,
 	.support_rmii = true,
-	.support_rgmii = true
+	.support_rgmii = true,
+	.rx_delay_max = 31,
+	.tx_delay_max = 7,
 };
 
 #define EMAC_BASIC_CTL0 0x00
@@ -223,9 +237,7 @@ static const struct emac_variant emac_variant_a64 = {
 #define SYSCON_RMII_EN		BIT(13) /* 1: enable RMII (overrides EPIT) */
 
 /* Generic system control EMAC_CLK bits */
-#define SYSCON_ETXDC_MASK		GENMASK(2, 0)
 #define SYSCON_ETXDC_SHIFT		10
-#define SYSCON_ERXDC_MASK		GENMASK(4, 0)
 #define SYSCON_ERXDC_SHIFT		5
 /* EMAC PHY Interface Type */
 #define SYSCON_EPIT			BIT(2) /* 1: RGMII, 0: MII */
@@ -851,8 +863,9 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 		}
 		val /= 100;
 		dev_dbg(priv->device, "set tx-delay to %x\n", val);
-		if (val <= SYSCON_ETXDC_MASK) {
-			reg &= ~(SYSCON_ETXDC_MASK << SYSCON_ETXDC_SHIFT);
+		if (val <= gmac->variant->tx_delay_max) {
+			reg &= ~(gmac->variant->tx_delay_max <<
+				 SYSCON_ETXDC_SHIFT);
 			reg |= (val << SYSCON_ETXDC_SHIFT);
 		} else {
 			dev_err(priv->device, "Invalid TX clock delay: %d\n",
@@ -868,8 +881,9 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 		}
 		val /= 100;
 		dev_dbg(priv->device, "set rx-delay to %x\n", val);
-		if (val <= SYSCON_ERXDC_MASK) {
-			reg &= ~(SYSCON_ERXDC_MASK << SYSCON_ERXDC_SHIFT);
+		if (val <= gmac->variant->rx_delay_max) {
+			reg &= ~(gmac->variant->rx_delay_max <<
+				 SYSCON_ERXDC_SHIFT);
 			reg |= (val << SYSCON_ERXDC_SHIFT);
 		} else {
 			dev_err(priv->device, "Invalid RX clock delay: %d\n",
-- 
2.16.2

[PATCH net-next 09/12] net: stmmac: dwmac-sun8i: Add support for GMAC on Allwinner R40 SoC

[Chen-Yu Tsai]

The Allwinner R40 SoC has the EMAC controller supported by dwmac-sun8i.
It is named "GMAC", while EMAC refers to the 10/100 Mbps Ethernet
controller supported by sun4i-emac. The controller is the same, but
the R40 has the glue layer controls in the clock control unit (CCU),
with a reduced RX delay chain, and no TX delay chain.

This patch adds support for it using the framework laid out by previous
patches to map the differences.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 08d263567a52..be6705e89e95 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -97,6 +97,13 @@ const struct reg_field sun8i_syscon_reg_field = {
 	.msb = 31,
 };
 
+/* EMAC clock register @ 0x164 in the CCU address range */
+const struct reg_field sun8i_ccu_reg_field = {
+	.reg = 0x164,
+	.lsb = 0,
+	.msb = 31,
+};
+
 static const struct emac_variant emac_variant_h3 = {
 	.default_syscon_value = 0x58000,
 	.syscon_field = &sun8i_syscon_reg_field,
@@ -125,6 +132,15 @@ static const struct emac_variant emac_variant_a83t = {
 	.tx_delay_max = 7,
 };
 
+static const struct emac_variant emac_variant_r40 = {
+	.default_syscon_value = 0,
+	.syscon_field = &sun8i_ccu_reg_field,
+	.syscon_from_dev = true,
+	.support_mii = true,
+	.support_rgmii = true,
+	.rx_delay_max = 7,
+};
+
 static const struct emac_variant emac_variant_a64 = {
 	.default_syscon_value = 0,
 	.syscon_field = &sun8i_syscon_reg_field,
@@ -1148,6 +1164,8 @@ static const struct of_device_id sun8i_dwmac_match[] = {
 		.data = &emac_variant_v3s },
 	{ .compatible = "allwinner,sun8i-a83t-emac",
 		.data = &emac_variant_a83t },
+	{ .compatible = "allwinner,sun8i-r40-gmac",
+		.data = &emac_variant_r40 },
 	{ .compatible = "allwinner,sun50i-a64-emac",
 		.data = &emac_variant_a64 },
 	{ }
-- 
2.16.2

[PATCH net-next 10/12] ARM: dts: sun8i: r40: bananapi-m2-ultra: Sort device node dereferences

[Chen-Yu Tsai]

The device nodes dereference (&foo) usages should be sorted by the label
names, barring any parsing order issues such as the #include statement
for the PMIC's .dtsi file that must come after the PMIC.

Move the mmc and ohci blocks in front of the PMIC's regulator blocks.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 69 ++++++++++++-----------
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
index 27d9ccd0ef2f..c6da21e43572 100644
--- a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
+++ b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
@@ -114,6 +114,41 @@
 
 #include "axp22x.dtsi"
 
+&mmc0 {
+	vmmc-supply = <&reg_dcdc1>;
+	bus-width = <4>;
+	cd-gpios = <&pio 7 13 GPIO_ACTIVE_HIGH>; /* PH13 */
+	cd-inverted;
+	status = "okay";
+};
+
+&mmc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc1_pg_pins>;
+	vmmc-supply = <&reg_dldo2>;
+	vqmmc-supply = <&reg_dldo1>;
+	mmc-pwrseq = <&wifi_pwrseq>;
+	bus-width = <4>;
+	non-removable;
+	status = "okay";
+};
+
+&mmc2 {
+	vmmc-supply = <&reg_dcdc1>;
+	vqmmc-supply = <&reg_dcdc1>;
+	bus-width = <8>;
+	non-removable;
+	status = "okay";
+};
+
+&ohci1 {
+	status = "okay";
+};
+
+&ohci2 {
+	status = "okay";
+};
+
 &reg_aldo3 {
 	regulator-always-on;
 	regulator-min-microvolt = <2700000>;
@@ -161,40 +196,6 @@
 	regulator-name = "vcc-wifi";
 };
 
-&mmc0 {
-	vmmc-supply = <&reg_dcdc1>;
-	bus-width = <4>;
-	cd-gpios = <&pio 7 13 GPIO_ACTIVE_LOW>; /* PH13 */
-	status = "okay";
-};
-
-&mmc1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&mmc1_pg_pins>;
-	vmmc-supply = <&reg_dldo2>;
-	vqmmc-supply = <&reg_dldo1>;
-	mmc-pwrseq = <&wifi_pwrseq>;
-	bus-width = <4>;
-	non-removable;
-	status = "okay";
-};
-
-&mmc2 {
-	vmmc-supply = <&reg_dcdc1>;
-	vqmmc-supply = <&reg_dcdc1>;
-	bus-width = <8>;
-	non-removable;
-	status = "okay";
-};
-
-&ohci1 {
-	status = "okay";
-};
-
-&ohci2 {
-	status = "okay";
-};
-
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
2.16.2

[PATCH net-next 11/12] ARM: dts: sun8i: r40: Add device node and RGMII pinmux node for GMAC

[Chen-Yu Tsai]

The R40 SoC has a GMAC (gigabit capable Ethernet controller). Add a
device node for it. The only publicly available board for this SoC
uses an RGMII PHY. Add a pinmux node for it as well.

Since this SoC also has an old 10/100 Mbps EMAC, which also has an
MDIO bus controller, the MDIO bus for the GMAC is labeled "gmac_mdio".

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-r40.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
index 173dcc1652d2..bd97ca3dc2fa 100644
--- a/arch/arm/boot/dts/sun8i-r40.dtsi
+++ b/arch/arm/boot/dts/sun8i-r40.dtsi
@@ -265,6 +265,19 @@
 			#interrupt-cells = <3>;
 			#gpio-cells = <3>;
 
+			gmac_rgmii_pins: gmac-rgmii-pins {
+				pins = "PA0", "PA1", "PA2", "PA3",
+				       "PA4", "PA5", "PA6", "PA7",
+				       "PA8", "PA10", "PA11", "PA12",
+				       "PA13", "PA15", "PA16";
+				function = "gmac";
+				/*
+				 * data lines in RGMII mode use DDR mode
+				 * and need a higher signal drive strength
+				 */
+				drive-strength = <40>;
+			};
+
 			i2c0_pins: i2c0-pins {
 				pins = "PB0", "PB1";
 				function = "i2c0";
@@ -451,6 +464,27 @@
 			#size-cells = <0>;
 		};
 
+		gmac: ethernet@1c50000 {
+			compatible = "allwinner,sun8i-r40-gmac";
+			syscon = <&ccu>;
+			reg = <0x01c50000 0x10000>;
+			interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+			resets = <&ccu RST_BUS_GMAC>;
+			reset-names = "stmmaceth";
+			clocks = <&ccu CLK_BUS_GMAC>;
+			clock-names = "stmmaceth";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			gmac_mdio: mdio {
+				compatible = "snps,dwmac-mdio";
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		gic: interrupt-controller@1c81000 {
 			compatible = "arm,gic-400";
 			reg = <0x01c81000 0x1000>,
-- 
2.16.2

[PATCH net-next 12/12] ARM: dts: sun8i: r40: bananapi-m2-ultra: Enable GMAC ethernet controller

[Chen-Yu Tsai]

The Bananapi M2 Ultra has a Realtek RTL8211E RGMII PHY tied to the GMAC.
The PMIC's DC1SW output provides power for the PHY, while the ALDO2
output provides I/O voltages on both sides.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
index c6da21e43572..25fb048c7df2 100644
--- a/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
+++ b/arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dts
@@ -51,6 +51,7 @@
 	compatible = "sinovoip,bpi-m2-ultra", "allwinner,sun8i-r40";
 
 	aliases {
+		ethernet0 = &gmac;
 		serial0 = &uart0;
 	};
 
@@ -101,6 +102,22 @@
 	status = "okay";
 };
 
+&gmac {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac_rgmii_pins>;
+	phy-handle = <&phy1>;
+	phy-mode = "rgmii";
+	phy-supply = <&reg_dc1sw>;
+	status = "okay";
+};
+
+&gmac_mdio {
+	phy1: ethernet-phy@1 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+	};
+};
+
 &i2c0 {
 	status = "okay";
 
@@ -149,6 +166,13 @@
 	status = "okay";
 };
 
+&reg_aldo2 {
+	regulator-always-on;
+	regulator-min-microvolt = <2500000>;
+	regulator-max-microvolt = <2500000>;
+	regulator-name = "vcc-pa";
+};
+
 &reg_aldo3 {
 	regulator-always-on;
 	regulator-min-microvolt = <2700000>;
@@ -156,6 +180,12 @@
 	regulator-name = "avcc";
 };
 
+&reg_dc1sw {
+	regulator-min-microvolt = <3000000>;
+	regulator-max-microvolt = <3000000>;
+	regulator-name = "vcc-gmac-phy";
+};
+
 &reg_dcdc1 {
 	regulator-always-on;
 	regulator-min-microvolt = <3000000>;
-- 
2.16.2

Re: [PATCH net-next 03/12] dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions

[Sergei Shtylyov]

Hello!

On 3/17/2018 12:28 PM, Chen-Yu Tsai wrote:

> The clock delay chains found in the glue layer for dwmac-sun8i are only
> used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
> MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
> range of possible values for the delay chain.
> 
> This patch reformats the delay chain section of the device tree binding
> to make it clear that the delay chains only apply to RGMII PHYs, and
> make it easier to add the R40-specific bits later.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>   Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> index 3d6d5fa0c4d5..b8a3028d6c30 100644
> --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> @@ -28,10 +28,13 @@ Required properties:
>     - allwinner,sun8i-a83t-system-controller
>   
>   Optional properties:
> -- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 0-700. Default is 0)
> -- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 0-3100. Default is 0)
> -Both delay properties need to be a multiple of 100. They control the delay for
> -external PHY.
> +- allwinner,tx-delay-ps: TX clock delay chain value in ps.
> +			 Range is 0-700. Default is 0.
> +- allwinner,rx-delay-ps: RX clock delay chain value in ps.
> +			 Range is 0-3100. Default is 0.
> +Both delay properties need to be a multiple of 100. They control the
> +clock delay for external RGMII PHY. They are do apply to the internal

   s/are do/do not/?

> +PHY or external non-RGMII PHYs.
>   
>   Optional properties for the following compatibles:
>     - "allwinner,sun8i-h3-emac",

MBR, Sergei

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
> 
> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> the syscon part, in the CCU of R40 SoC.
> 
> Export a regmap of the CCU.
> 
> Read access is not restricted to all registers, but only the GMAC
> register is allowed to be written.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Gah, this is crazy. I'm really starting to regret letting that syscon
in in the first place...

And I'm not really looking forward the time where SCPI et al. will be
mature and we'll have the clock controller completely outside of our
control.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Chen-Yu Tsai]

On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>> From: Icenowy Zheng <icenowy@aosc.io>
>>
>> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
>> the syscon part, in the CCU of R40 SoC.
>>
>> Export a regmap of the CCU.
>>
>> Read access is not restricted to all registers, but only the GMAC
>> register is allowed to be written.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> Gah, this is crazy. I'm really starting to regret letting that syscon
> in in the first place...

IMHO syscon is really a better fit. It's part of the glue layer and
most other dwmac user platforms treat it as such and use a syscon.
Plus the controls encompass delays (phase), inverters (polarity),
and even signal routing. It's not really just a group of clock controls,
like what we poorly modeled for A20/A31. I think that was really a
mistake.

As I mentioned in the cover letter, a slightly saner approach would
be to let drivers add custom syscon entries, which would then require
less custom plumbing.

> And I'm not really looking forward the time where SCPI et al. will be
> mature and we'll have the clock controller completely outside of our
> control.

I don't think it's going to happen for any of the older SoCs. The R40
only stands out because the GMAC controls are in the clock controller
address space, presumably to be like the A20.


ChenYu

Re: [PATCH net-next 06/12] net: stmmac: dwmac-sun8i: Use regmap_field for syscon register access

[kbuild test robot]

Hi Chen-Yu,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20180309]
[also build test WARNING on v4.16-rc6]
[cannot apply to v4.16-rc4 v4.16-rc3 v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chen-Yu-Tsai/ARM-sun8i-r40-Add-Ethernet-support/20180318-161723
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c:82:24: sparse: symbol 'sun8i_syscon_reg_field' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] net: stmmac: dwmac-sun8i: sun8i_syscon_reg_field can be static

[kbuild test robot]

Fixes: e3c10deef23c ("net: stmmac: dwmac-sun8i: Use regmap_field for syscon register access")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 dwmac-sun8i.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index de93f0f..bbc0514 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -79,7 +79,7 @@ struct sunxi_priv_data {
 };
 
 /* EMAC clock register @ 0x30 in the "system control" address range */
-const struct reg_field sun8i_syscon_reg_field = {
+static const struct reg_field sun8i_syscon_reg_field = {
 	.reg = 0x30,
 	.lsb = 0,
 	.msb = 31,

[RFC PATCH] net: stmmac: dwmac-sun8i: sun8i_ccu_reg_field can be static

[kbuild test robot]

Fixes: 0e59c15b2797 ("net: stmmac: dwmac-sun8i: Add support for GMAC on Allwinner R40 SoC")
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 dwmac-sun8i.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index be6705e8..622fb2b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -98,7 +98,7 @@ const struct reg_field sun8i_syscon_reg_field = {
 };
 
 /* EMAC clock register @ 0x164 in the CCU address range */
-const struct reg_field sun8i_ccu_reg_field = {
+static const struct reg_field sun8i_ccu_reg_field = {
 	.reg = 0x164,
 	.lsb = 0,
 	.msb = 31,

Re: [PATCH net-next 09/12] net: stmmac: dwmac-sun8i: Add support for GMAC on Allwinner R40 SoC

[kbuild test robot]

Hi Chen-Yu,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20180309]
[also build test WARNING on v4.16-rc6]
[cannot apply to v4.16-rc4 v4.16-rc3 v4.16-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chen-Yu-Tsai/ARM-sun8i-r40-Add-Ethernet-support/20180318-161723
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c:94:24: sparse: symbol 'sun8i_syscon_reg_field' was not declared. Should it be static?
>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c:101:24: sparse: symbol 'sun8i_ccu_reg_field' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [RFC PATCH] net: stmmac: dwmac-sun8i: sun8i_syscon_reg_field can be static

[David Miller]

From: kbuild test robot <lkp@intel.com>
Date: Thu, 22 Mar 2018 15:42:18 +0800

> 
> Fixes: e3c10deef23c ("net: stmmac: dwmac-sun8i: Use regmap_field for syscon register access")
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>

This commit ID and this symbol are both not present in any of my trees.

Please avoid using SHA1 IDs which don't actually exist in an upstream
tree.  When I see a "Fixes: " tag like this, I expect that it is for a
change in my tree.

Re: [PATCH net-next 04/12] dt-bindings: net: dwmac-sun8i: Sort syscon compatibles by alphabetical order

[Rob Herring]

On Sat, Mar 17, 2018 at 05:28:49PM +0800, Chen-Yu Tsai wrote:
> The A83T syscon compatible was appended to the syscon compatibles list,
> instead of inserted in to preserve the ordering.
> 
> Move it to the proper place to keep the list sorted.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH net-next 03/12] dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions

[Chen-Yu Tsai]

On Sun, Mar 18, 2018 at 5:21 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello!
>
>
> On 3/17/2018 12:28 PM, Chen-Yu Tsai wrote:
>
>> The clock delay chains found in the glue layer for dwmac-sun8i are only
>> used with RGMII PHYs. They are not intended for non-RGMII PHYs, such as
>> MII external PHYs or the internal PHY. Also, a recent SoC has a smaller
>> range of possible values for the delay chain.
>>
>> This patch reformats the delay chain section of the device tree binding
>> to make it clear that the delay chains only apply to RGMII PHYs, and
>> make it easier to add the R40-specific bits later.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>   Documentation/devicetree/bindings/net/dwmac-sun8i.txt | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> index 3d6d5fa0c4d5..b8a3028d6c30 100644
>> --- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
>> @@ -28,10 +28,13 @@ Required properties:
>>     - allwinner,sun8i-a83t-system-controller
>>     Optional properties:
>> -- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is
>> 0-700. Default is 0)
>> -- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is
>> 0-3100. Default is 0)
>> -Both delay properties need to be a multiple of 100. They control the
>> delay for
>> -external PHY.
>> +- allwinner,tx-delay-ps: TX clock delay chain value in ps.
>> +                        Range is 0-700. Default is 0.
>> +- allwinner,rx-delay-ps: RX clock delay chain value in ps.
>> +                        Range is 0-3100. Default is 0.
>> +Both delay properties need to be a multiple of 100. They control the
>> +clock delay for external RGMII PHY. They are do apply to the internal
>
>
>   s/are do/do not/?

Fixed. Thanks!

ChenYu

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> >> From: Icenowy Zheng <icenowy@aosc.io>
> >>
> >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> >> the syscon part, in the CCU of R40 SoC.
> >>
> >> Export a regmap of the CCU.
> >>
> >> Read access is not restricted to all registers, but only the GMAC
> >> register is allowed to be written.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> > Gah, this is crazy. I'm really starting to regret letting that syscon
> > in in the first place...
> 
> IMHO syscon is really a better fit. It's part of the glue layer and
> most other dwmac user platforms treat it as such and use a syscon.
> Plus the controls encompass delays (phase), inverters (polarity),
> and even signal routing. It's not really just a group of clock controls,
> like what we poorly modeled for A20/A31. I think that was really a
> mistake.
> 
> As I mentioned in the cover letter, a slightly saner approach would
> be to let drivers add custom syscon entries, which would then require
> less custom plumbing.

A syscon is convenient, sure, but it also bypasses any abstraction
layer we have everywhere else, which means that we'll have to maintain
the register layout in each and every driver that uses it.

So far, it's only be the GMAC, but it can also be others (the SRAM
controller comes to my mind), and then, if there's any difference in
the design in a future SoC, we'll have to maintain that in the GMAC
driver as well.

> > And I'm not really looking forward the time where SCPI et al. will be
> > mature and we'll have the clock controller completely outside of our
> > control.
> 
> I don't think it's going to happen for any of the older SoCs. The R40
> only stands out because the GMAC controls are in the clock controller
> address space, presumably to be like the A20.

SCPI (or equivalent) is a really nice feature to have when it comes to
virtualization, so even if it's less likely, it doesn't make it less
relevant on other SoCs.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> > >> From: Icenowy Zheng <icenowy@aosc.io>
> > >>
> > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> > >> the syscon part, in the CCU of R40 SoC.
> > >>
> > >> Export a regmap of the CCU.
> > >>
> > >> Read access is not restricted to all registers, but only the GMAC
> > >> register is allowed to be written.
> > >>
> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > >
> > > Gah, this is crazy. I'm really starting to regret letting that syscon
> > > in in the first place...
> > 
> > IMHO syscon is really a better fit. It's part of the glue layer and
> > most other dwmac user platforms treat it as such and use a syscon.
> > Plus the controls encompass delays (phase), inverters (polarity),
> > and even signal routing. It's not really just a group of clock controls,
> > like what we poorly modeled for A20/A31. I think that was really a
> > mistake.
> > 
> > As I mentioned in the cover letter, a slightly saner approach would
> > be to let drivers add custom syscon entries, which would then require
> > less custom plumbing.
> 
> A syscon is convenient, sure, but it also bypasses any abstraction
> layer we have everywhere else, which means that we'll have to maintain
> the register layout in each and every driver that uses it.
> 
> So far, it's only be the GMAC, but it can also be others (the SRAM
> controller comes to my mind), and then, if there's any difference in
> the design in a future SoC, we'll have to maintain that in the GMAC
> driver as well.

I guess I forgot to say something, I'm fine with using a syscon we
already have.

I'm just questionning if merging any other driver using one is the
right move.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Icenowy Zheng]

于 2018年4月3日 GMT+08:00 下午5:50:05, Maxime Ripard <maxime.ripard@bootlin.com> 写到:
>On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
>> > <maxime.ripard@bootlin.com> wrote:
>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>> > >> From: Icenowy Zheng <icenowy@aosc.io>
>> > >>
>> > >> There's a GMAC configuration register, which exists on
>A64/A83T/H3/H5 in
>> > >> the syscon part, in the CCU of R40 SoC.
>> > >>
>> > >> Export a regmap of the CCU.
>> > >>
>> > >> Read access is not restricted to all registers, but only the
>GMAC
>> > >> register is allowed to be written.
>> > >>
>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> > >
>> > > Gah, this is crazy. I'm really starting to regret letting that
>syscon
>> > > in in the first place...
>> > 
>> > IMHO syscon is really a better fit. It's part of the glue layer and
>> > most other dwmac user platforms treat it as such and use a syscon.
>> > Plus the controls encompass delays (phase), inverters (polarity),
>> > and even signal routing. It's not really just a group of clock
>controls,
>> > like what we poorly modeled for A20/A31. I think that was really a
>> > mistake.
>> > 
>> > As I mentioned in the cover letter, a slightly saner approach would
>> > be to let drivers add custom syscon entries, which would then
>require
>> > less custom plumbing.
>> 
>> A syscon is convenient, sure, but it also bypasses any abstraction
>> layer we have everywhere else, which means that we'll have to
>maintain
>> the register layout in each and every driver that uses it.
>> 
>> So far, it's only be the GMAC, but it can also be others (the SRAM
>> controller comes to my mind), and then, if there's any difference in
>> the design in a future SoC, we'll have to maintain that in the GMAC
>> driver as well.
>
>I guess I forgot to say something, I'm fine with using a syscon we
>already have.
>
>I'm just questionning if merging any other driver using one is the
>right move.

Even for current SoCs supported by dwnac-sun8i, there
is a syscon/sram-controller problem. They're both at 0x1c00000.

The first examples for the need of sram-controller is
A64, which we need to claim SRAM C for DE2 access.

>
>Maxime

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Chen-Yu Tsai]

On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
>> > <maxime.ripard@bootlin.com> wrote:
>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>> > >> From: Icenowy Zheng <icenowy@aosc.io>
>> > >>
>> > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
>> > >> the syscon part, in the CCU of R40 SoC.
>> > >>
>> > >> Export a regmap of the CCU.
>> > >>
>> > >> Read access is not restricted to all registers, but only the GMAC
>> > >> register is allowed to be written.
>> > >>
>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> > >
>> > > Gah, this is crazy. I'm really starting to regret letting that syscon
>> > > in in the first place...
>> >
>> > IMHO syscon is really a better fit. It's part of the glue layer and
>> > most other dwmac user platforms treat it as such and use a syscon.
>> > Plus the controls encompass delays (phase), inverters (polarity),
>> > and even signal routing. It's not really just a group of clock controls,
>> > like what we poorly modeled for A20/A31. I think that was really a
>> > mistake.
>> >
>> > As I mentioned in the cover letter, a slightly saner approach would
>> > be to let drivers add custom syscon entries, which would then require
>> > less custom plumbing.
>>
>> A syscon is convenient, sure, but it also bypasses any abstraction
>> layer we have everywhere else, which means that we'll have to maintain
>> the register layout in each and every driver that uses it.
>>
>> So far, it's only be the GMAC, but it can also be others (the SRAM
>> controller comes to my mind), and then, if there's any difference in
>> the design in a future SoC, we'll have to maintain that in the GMAC
>> driver as well.
>
> I guess I forgot to say something, I'm fine with using a syscon we
> already have.
>
> I'm just questionning if merging any other driver using one is the
> right move.

Right. So in this case, we are not actually going through the syscon
API. Rather we are exporting a regmap whose properties we actually
define. If it makes you more acceptable to it, we could map just
the GMAC register in the new regmap, and also have it named. This
is all plumbing within the kernel so the device tree stays the same.

ChenYu

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Icenowy Zheng]

于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到:
>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard
><maxime.ripard@bootlin.com> wrote:
>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
>>> > <maxime.ripard@bootlin.com> wrote:
>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>>> > >> From: Icenowy Zheng <icenowy@aosc.io>
>>> > >>
>>> > >> There's a GMAC configuration register, which exists on
>A64/A83T/H3/H5 in
>>> > >> the syscon part, in the CCU of R40 SoC.
>>> > >>
>>> > >> Export a regmap of the CCU.
>>> > >>
>>> > >> Read access is not restricted to all registers, but only the
>GMAC
>>> > >> register is allowed to be written.
>>> > >>
>>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>> > >
>>> > > Gah, this is crazy. I'm really starting to regret letting that
>syscon
>>> > > in in the first place...
>>> >
>>> > IMHO syscon is really a better fit. It's part of the glue layer
>and
>>> > most other dwmac user platforms treat it as such and use a syscon.
>>> > Plus the controls encompass delays (phase), inverters (polarity),
>>> > and even signal routing. It's not really just a group of clock
>controls,
>>> > like what we poorly modeled for A20/A31. I think that was really a
>>> > mistake.
>>> >
>>> > As I mentioned in the cover letter, a slightly saner approach
>would
>>> > be to let drivers add custom syscon entries, which would then
>require
>>> > less custom plumbing.
>>>
>>> A syscon is convenient, sure, but it also bypasses any abstraction
>>> layer we have everywhere else, which means that we'll have to
>maintain
>>> the register layout in each and every driver that uses it.
>>>
>>> So far, it's only be the GMAC, but it can also be others (the SRAM
>>> controller comes to my mind), and then, if there's any difference in
>>> the design in a future SoC, we'll have to maintain that in the GMAC
>>> driver as well.
>>
>> I guess I forgot to say something, I'm fine with using a syscon we
>> already have.
>>
>> I'm just questionning if merging any other driver using one is the
>> right move.
>
>Right. So in this case, we are not actually going through the syscon
>API. Rather we are exporting a regmap whose properties we actually
>define. If it makes you more acceptable to it, we could map just
>the GMAC register in the new regmap, and also have it named. This
>is all plumbing within the kernel so the device tree stays the same.

I think my driver has already restricted the write permission
only to GMAC register.

>
>ChenYu

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Chen-Yu Tsai]

On Tue, Apr 3, 2018 at 5:54 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
>
>
> 于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到:
>>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard
>><maxime.ripard@bootlin.com> wrote:
>>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
>>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
>>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
>>>> > <maxime.ripard@bootlin.com> wrote:
>>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>>>> > >> From: Icenowy Zheng <icenowy@aosc.io>
>>>> > >>
>>>> > >> There's a GMAC configuration register, which exists on
>>A64/A83T/H3/H5 in
>>>> > >> the syscon part, in the CCU of R40 SoC.
>>>> > >>
>>>> > >> Export a regmap of the CCU.
>>>> > >>
>>>> > >> Read access is not restricted to all registers, but only the
>>GMAC
>>>> > >> register is allowed to be written.
>>>> > >>
>>>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>>> > >
>>>> > > Gah, this is crazy. I'm really starting to regret letting that
>>syscon
>>>> > > in in the first place...
>>>> >
>>>> > IMHO syscon is really a better fit. It's part of the glue layer
>>and
>>>> > most other dwmac user platforms treat it as such and use a syscon.
>>>> > Plus the controls encompass delays (phase), inverters (polarity),
>>>> > and even signal routing. It's not really just a group of clock
>>controls,
>>>> > like what we poorly modeled for A20/A31. I think that was really a
>>>> > mistake.
>>>> >
>>>> > As I mentioned in the cover letter, a slightly saner approach
>>would
>>>> > be to let drivers add custom syscon entries, which would then
>>require
>>>> > less custom plumbing.
>>>>
>>>> A syscon is convenient, sure, but it also bypasses any abstraction
>>>> layer we have everywhere else, which means that we'll have to
>>maintain
>>>> the register layout in each and every driver that uses it.
>>>>
>>>> So far, it's only be the GMAC, but it can also be others (the SRAM
>>>> controller comes to my mind), and then, if there's any difference in
>>>> the design in a future SoC, we'll have to maintain that in the GMAC
>>>> driver as well.
>>>
>>> I guess I forgot to say something, I'm fine with using a syscon we
>>> already have.
>>>
>>> I'm just questionning if merging any other driver using one is the
>>> right move.
>>
>>Right. So in this case, we are not actually going through the syscon
>>API. Rather we are exporting a regmap whose properties we actually
>>define. If it makes you more acceptable to it, we could map just
>>the GMAC register in the new regmap, and also have it named. This
>>is all plumbing within the kernel so the device tree stays the same.
>
> I think my driver has already restricted the write permission
> only to GMAC register.

Correct, but it still maps the entire region out, which means the
consumer needs to know which offset to use. Maxime is saying this
is something that is troublesome to maintain. So my proposal was
to create a regmap with a base at the GMAC register offset. That
way, the consumer doesn't need to use an offset to access it.

ChenYu

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3668 bytes --]

On Tue, Apr 03, 2018 at 05:58:05PM +0800, Chen-Yu Tsai wrote:
> On Tue, Apr 3, 2018 at 5:54 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> >
> >
> > 于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到:
> >>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard
> >><maxime.ripard@bootlin.com> wrote:
> >>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
> >>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> >>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> >>>> > <maxime.ripard@bootlin.com> wrote:
> >>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> >>>> > >> From: Icenowy Zheng <icenowy@aosc.io>
> >>>> > >>
> >>>> > >> There's a GMAC configuration register, which exists on
> >>A64/A83T/H3/H5 in
> >>>> > >> the syscon part, in the CCU of R40 SoC.
> >>>> > >>
> >>>> > >> Export a regmap of the CCU.
> >>>> > >>
> >>>> > >> Read access is not restricted to all registers, but only the
> >>GMAC
> >>>> > >> register is allowed to be written.
> >>>> > >>
> >>>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >>>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >>>> > >
> >>>> > > Gah, this is crazy. I'm really starting to regret letting that
> >>syscon
> >>>> > > in in the first place...
> >>>> >
> >>>> > IMHO syscon is really a better fit. It's part of the glue layer
> >>and
> >>>> > most other dwmac user platforms treat it as such and use a syscon.
> >>>> > Plus the controls encompass delays (phase), inverters (polarity),
> >>>> > and even signal routing. It's not really just a group of clock
> >>controls,
> >>>> > like what we poorly modeled for A20/A31. I think that was really a
> >>>> > mistake.
> >>>> >
> >>>> > As I mentioned in the cover letter, a slightly saner approach
> >>would
> >>>> > be to let drivers add custom syscon entries, which would then
> >>require
> >>>> > less custom plumbing.
> >>>>
> >>>> A syscon is convenient, sure, but it also bypasses any abstraction
> >>>> layer we have everywhere else, which means that we'll have to
> >>maintain
> >>>> the register layout in each and every driver that uses it.
> >>>>
> >>>> So far, it's only be the GMAC, but it can also be others (the SRAM
> >>>> controller comes to my mind), and then, if there's any difference in
> >>>> the design in a future SoC, we'll have to maintain that in the GMAC
> >>>> driver as well.
> >>>
> >>> I guess I forgot to say something, I'm fine with using a syscon we
> >>> already have.
> >>>
> >>> I'm just questionning if merging any other driver using one is the
> >>> right move.
> >>
> >>Right. So in this case, we are not actually going through the syscon
> >>API. Rather we are exporting a regmap whose properties we actually
> >>define. If it makes you more acceptable to it, we could map just
> >>the GMAC register in the new regmap, and also have it named. This
> >>is all plumbing within the kernel so the device tree stays the same.
> >
> > I think my driver has already restricted the write permission
> > only to GMAC register.
> 
> Correct, but it still maps the entire region out, which means the
> consumer needs to know which offset to use. Maxime is saying this
> is something that is troublesome to maintain. So my proposal was
> to create a regmap with a base at the GMAC register offset. That
> way, the consumer doesn't need to use an offset to access it.

I guess this is something we can keep in mind if it gets out of
control yse.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Icenowy Zheng]

在 2018-04-03二的 11:50 +0200，Maxime Ripard写道：
> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
> > On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> > > <maxime.ripard@bootlin.com> wrote:
> > > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> > > > > From: Icenowy Zheng <icenowy@aosc.io>
> > > > > 
> > > > > There's a GMAC configuration register, which exists on
> > > > > A64/A83T/H3/H5 in
> > > > > the syscon part, in the CCU of R40 SoC.
> > > > > 
> > > > > Export a regmap of the CCU.
> > > > > 
> > > > > Read access is not restricted to all registers, but only the
> > > > > GMAC
> > > > > register is allowed to be written.
> > > > > 
> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > > 
> > > > Gah, this is crazy. I'm really starting to regret letting that
> > > > syscon
> > > > in in the first place...
> > > 
> > > IMHO syscon is really a better fit. It's part of the glue layer
> > > and
> > > most other dwmac user platforms treat it as such and use a
> > > syscon.
> > > Plus the controls encompass delays (phase), inverters (polarity),
> > > and even signal routing. It's not really just a group of clock
> > > controls,
> > > like what we poorly modeled for A20/A31. I think that was really
> > > a
> > > mistake.
> > > 
> > > As I mentioned in the cover letter, a slightly saner approach
> > > would
> > > be to let drivers add custom syscon entries, which would then
> > > require
> > > less custom plumbing.
> > 
> > A syscon is convenient, sure, but it also bypasses any abstraction
> > layer we have everywhere else, which means that we'll have to
> > maintain
> > the register layout in each and every driver that uses it.
> > 
> > So far, it's only be the GMAC, but it can also be others (the SRAM
> > controller comes to my mind), and then, if there's any difference
> > in
> > the design in a future SoC, we'll have to maintain that in the GMAC
> > driver as well.
> 
> I guess I forgot to say something, I'm fine with using a syscon we
> already have.

As we finally need the SRAM controller on these new SoCs (for VE on all
SoCs targeted by dwmac-sun8i, and for DE on A64), should we deprecate
the syscon and all switch to device regmap (and let sunxi-dram driver
to export a regmap)? (As in the A64 DE2 thread discussed, two device
nodes shouldn't cover the same MMIO region.)

In addition, there might be multiple registers in the syscon/sram zone
that are needed (for example, if a version driver is written, it will
need the "0x24 Version Register"; and GMAC needs "0x30 EMAC Clock
Register"). How to deal with this if we export the syscon/sram zone as
a generic device regmap?

> 
> I'm just questionning if merging any other driver using one is the
> right move.
> 
> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Chen-Yu Tsai]

On Wed, Apr 4, 2018 at 2:45 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> 在 2018-04-03二的 11:50 +0200，Maxime Ripard写道：
>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
>> > On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
>> > > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
>> > > <maxime.ripard@bootlin.com> wrote:
>> > > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
>> > > > > From: Icenowy Zheng <icenowy@aosc.io>
>> > > > >
>> > > > > There's a GMAC configuration register, which exists on
>> > > > > A64/A83T/H3/H5 in
>> > > > > the syscon part, in the CCU of R40 SoC.
>> > > > >
>> > > > > Export a regmap of the CCU.
>> > > > >
>> > > > > Read access is not restricted to all registers, but only the
>> > > > > GMAC
>> > > > > register is allowed to be written.
>> > > > >
>> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> > > >
>> > > > Gah, this is crazy. I'm really starting to regret letting that
>> > > > syscon
>> > > > in in the first place...
>> > >
>> > > IMHO syscon is really a better fit. It's part of the glue layer
>> > > and
>> > > most other dwmac user platforms treat it as such and use a
>> > > syscon.
>> > > Plus the controls encompass delays (phase), inverters (polarity),
>> > > and even signal routing. It's not really just a group of clock
>> > > controls,
>> > > like what we poorly modeled for A20/A31. I think that was really
>> > > a
>> > > mistake.
>> > >
>> > > As I mentioned in the cover letter, a slightly saner approach
>> > > would
>> > > be to let drivers add custom syscon entries, which would then
>> > > require
>> > > less custom plumbing.
>> >
>> > A syscon is convenient, sure, but it also bypasses any abstraction
>> > layer we have everywhere else, which means that we'll have to
>> > maintain
>> > the register layout in each and every driver that uses it.
>> >
>> > So far, it's only be the GMAC, but it can also be others (the SRAM
>> > controller comes to my mind), and then, if there's any difference
>> > in
>> > the design in a future SoC, we'll have to maintain that in the GMAC
>> > driver as well.
>>
>> I guess I forgot to say something, I'm fine with using a syscon we
>> already have.
>
> As we finally need the SRAM controller on these new SoCs (for VE on all
> SoCs targeted by dwmac-sun8i, and for DE on A64), should we deprecate
> the syscon and all switch to device regmap (and let sunxi-dram driver
> to export a regmap)? (As in the A64 DE2 thread discussed, two device
> nodes shouldn't cover the same MMIO region.)

Sounds like a plan.

> In addition, there might be multiple registers in the syscon/sram zone
> that are needed (for example, if a version driver is written, it will
> need the "0x24 Version Register"; and GMAC needs "0x30 EMAC Clock
> Register"). How to deal with this if we export the syscon/sram zone as
> a generic device regmap?

You can have multiple regmaps for a device. And the API allows you to
request them up by name.

ChenYu

>>
>> I'm just questionning if merging any other driver using one is the
>> right move.
>>
>> Maxime
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel