[PATCH v3 0/8] Add Allwinner A20 GMAC ethernet support

[PATCH v3 0/8] Add Allwinner A20 GMAC ethernet support

[Chen-Yu Tsai]

Hi,

This is the remaining part of v3 of the Allwinner A20 GMAC glue layer for
stmmac. The stmmac driver changes have been merged through net-next. The
remaining bits are clock and DT patches. The patches should be applied
over my clock renaming patches.

The Allwinner A20 SoC integrates an early version of dwmac
IP from Synopsys. On top of that is a hardware glue layer.
This layer needs to be configured before the dwmac can be
used.

Part of the glue layer is a clock mux, which controls the
source and direction of the TX clock used by GMAC.

Changes since v2:

  * Added more comments on GMAC clock driver
  * Drop CLK_SET_PARENT_GATE in GMAC clock driver
  * Use macro for max clock parents
  * Line wrapping

Changes since v1:

  * Added optional reset control to stmmac driver core
  * Added non CONFIG_RESET_CONROLLER routines for the above change
  * Extended callback API, as discussed with Srinivas
  * Used new stmmac_of_data to pass features and callbacks,
    instead of platform data, as discussed
  * Seperated clock module glue layer into clock driver

Cheers,
ChenYu


Chen-Yu Tsai (8):
  clk: sunxi: Add Allwinner A20/A31 GMAC clock unit
  ARM: dts: sun7i: Add GMAC clock node to sun7i DTSI
  ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI
  ARM: dts: sun7i: Add pin muxing options for the GMAC
  ARM: dts: sun7i: cubietruck: Enable the GMAC
  ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC
  ARM: dts: sun7i: olinuxino-micro: Enable GMAC instead of EMAC
  ARM: dts: sun7i: Add ethernet alias for GMAC

 Documentation/devicetree/bindings/clock/sunxi.txt | 26 +++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts       | 27 ++++----
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts        | 12 ++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts   | 27 ++++----
 arch/arm/boot/dts/sun7i-a20.dtsi                  | 71 ++++++++++++++++++-
 drivers/clk/sunxi/clk-sunxi.c                     | 83 +++++++++++++++++++++++
 6 files changed, 215 insertions(+), 31 deletions(-)

-- 
1.9.rc1

[PATCH v3 1/8] clk: sunxi: Add Allwinner A20/A31 GMAC clock unit

[Chen-Yu Tsai]

The Allwinner A20/A31 clock module controls the transmit clock source
and interface type of the GMAC ethernet controller. Model this as
a single clock for GMAC drivers to use.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 Documentation/devicetree/bindings/clock/sunxi.txt | 26 +++++++
 drivers/clk/sunxi/clk-sunxi.c                     | 83 +++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
index 0cf679b..f43b4c0 100644
--- a/Documentation/devicetree/bindings/clock/sunxi.txt
+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
@@ -37,6 +37,7 @@ Required properties:
 	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
 	"allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
 	"allwinner,sun7i-a20-out-clk" - for the external output clocks
+	"allwinner,sun7i-a20-gmac-clk" - for the GMAC clock module on A20/A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.
@@ -50,6 +51,9 @@ Required properties for all clocks:
 	If the clock module only has one output, the name shall be the
 	module name.
 
+For "allwinner,sun7i-a20-gmac-clk", the parent clocks shall be fixed rate
+dummy clocks at 25 MHz and 125 MHz, respectively. See example.
+
 Clock consumers should specify the desired clocks they use with a
 "clocks" phandle cell. Consumers that are using a gated clock should
 provide an additional ID in their clock property. This ID is the
@@ -96,3 +100,25 @@ mmc0_clk: clk@01c20088 {
 	clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
 	clock-output-names = "mmc0";
 };
+
+mii_phy_tx_clk: clk@2 {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <25000000>;
+	clock-output-names = "mii_phy_tx";
+};
+
+gmac_int_tx_clk: clk@3 {
+	#clock-cells = <0>;
+	compatible = "fixed-clock";
+	clock-frequency = <125000000>;
+	clock-output-names = "gmac_int_tx";
+};
+
+gmac_clk: clk@01c20164 {
+	#clock-cells = <0>;
+	compatible = "allwinner,sun7i-a20-gmac-clk";
+	reg = <0x01c20164 0x4>;
+	clocks = <&mii_phy_tx_clk>, <&gmac_int_tx_clk>;
+	clock-output-names = "gmac";
+};
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 736fb60..0b361d2 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -379,6 +379,89 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32 parent_rate,
 
 
 /**
+ * sun7i_a20_gmac_clk_setup - Setup function for A20/A31 GMAC clock module
+ *
+ * This clock looks something like this
+ *                               ________________________
+ *  MII TX clock from PHY >-----|___________    _________|----> to GMAC core
+ *  GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
+ *  Ext. 125MHz RGMII TX clk >--|__divider__/            |
+ *                              |________________________|
+ *
+ * The external 125 MHz reference is optional, i.e. GMAC can use its
+ * internal TX clock just fine. The A31 GMAC clock module does not have
+ * the divider controls for the external reference.
+ *
+ * To keep it simple, let the GMAC use either the MII TX clock for MII mode,
+ * and its internal TX clock for GMII and RGMII modes. The GMAC driver should
+ * select the appropriate source and gate/ungate the output to the PHY.
+ *
+ * Only the GMAC should use this clock. Altering the clock so that it doesn't
+ * match the GMAC's operation parameters will result in the GMAC not being
+ * able to send traffic out. The GMAC driver should set the clock rate and
+ * enable/disable this clock to configure the required state. The clock
+ * driver then responds by auto-reparenting the clock.
+ */
+
+#define SUN7I_A20_GMAC_GPIT	2
+#define SUN7I_A20_GMAC_MASK	0x3
+#define SUN7I_A20_GMAC_MAX_PARENTS	2
+
+static void __init sun7i_a20_gmac_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	struct clk_mux *mux;
+	struct clk_gate *gate;
+	const char *clk_name = node->name;
+	const char *parents[SUN7I_A20_GMAC_MAX_PARENTS];
+	void *reg;
+	int i = 0;
+
+	/* allocate mux and gate clock structs */
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux)
+		return;
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate) {
+		kfree(mux);
+		return;
+	}
+
+	reg = of_iomap(node, 0);
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	while (i < SUN7I_A20_GMAC_MAX_PARENTS &&
+			(parents[i] = of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	/* set up gate and fixed rate properties */
+	gate->reg = reg;
+	gate->bit_idx = SUN7I_A20_GMAC_GPIT;
+	gate->lock = &clk_lock;
+	mux->reg = reg;
+	mux->mask = SUN7I_A20_GMAC_MASK;
+	mux->flags = CLK_MUX_INDEX_BIT;
+	mux->lock = &clk_lock;
+
+	clk = clk_register_composite(NULL, clk_name,
+			parents, i,
+			&mux->hw, &clk_mux_ops,
+			NULL, NULL,
+			&gate->hw, &clk_gate_ops,
+			0);
+
+	if (!IS_ERR(clk)) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		clk_register_clkdev(clk, clk_name, NULL);
+	}
+}
+CLK_OF_DECLARE(sun7i_a20_gmac, "allwinner,sun7i-a20-gmac-clk",
+		sun7i_a20_gmac_clk_setup);
+
+
+
+/**
  * sunxi_factors_clk_setup() - Setup function for factor clocks
  */
 
-- 
1.9.rc1

[PATCH v3 2/8] ARM: dts: sun7i: Add GMAC clock node to sun7i DTSI

[Chen-Yu Tsai]

The GMAC uses 1 of 2 sources for its transmit clock, depending on the
PHY interface mode. Add both sources as dummy clocks, and as parents
to the GMAC clock node.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 1595e9a..fc7f470 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -314,6 +314,34 @@
 		};
 
 		/*
+		 * The following two are dummy clocks, placeholders used
+		 * on gmac_tx clock. The actual frequency and availability
+		 * depends on the external PHY, operation mode and link
+		 * speed.
+		 */
+		mii_phy_tx_clk: clk@2 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <25000000>;
+			clock-output-names = "mii_phy_tx";
+		};
+
+		gmac_int_tx_clk: clk@3 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <125000000>;
+			clock-output-names = "gmac_int_tx";
+		};
+
+		gmac_tx_clk: clk@01c20164 {
+			#clock-cells = <0>;
+			compatible = "allwinner,sun7i-a20-gmac-clk";
+			reg = <0x01c20164 0x4>;
+			clocks = <&mii_phy_tx_clk>, <&gmac_int_tx_clk>;
+			clock-output-names = "gmac_tx";
+		};
+
+		/*
 		 * Dummy clock used by output clocks
 		 */
 		osc24M_32k: clk@1 {
-- 
1.9.rc1

[PATCH v3 3/8] ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index fc7f470..5fbac23 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -630,6 +630,21 @@
 			status = "disabled";
 		};
 
+		gmac: ethernet@01c50000 {
+			compatible = "allwinner,sun7i-a20-gmac";
+			reg = <0x01c50000 0x10000>;
+			interrupts = <0 85 4>;
+			interrupt-names = "macirq";
+			clocks = <&ahb_gates 49>, <&gmac_tx_clk>;
+			clock-names = "stmmaceth", "allwinner_gmac_tx";
+			snps,pbl = <2>;
+			snps,fixed-burst;
+			snps,force_sf_dma_mode;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		hstimer@01c60000 {
 			compatible = "allwinner,sun7i-a20-hstimer";
 			reg = <0x01c60000 0x1000>;
-- 
1.9.rc1

[PATCH v3 4/8] ARM: dts: sun7i: Add pin muxing options for the GMAC

[Chen-Yu Tsai]

The A20 has EMAC and GMAC muxed on the same pins.
Add pin sets with gmac function for MII and RGMII mode to the DTSI.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 5fbac23..65fb8d0 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -469,6 +469,32 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			gmac_pins_mii_a: gmac_mii@0 {
+				allwinner,pins = "PA0", "PA1", "PA2",
+						"PA3", "PA4", "PA5", "PA6",
+						"PA7", "PA8", "PA9", "PA10",
+						"PA11", "PA12", "PA13", "PA14",
+						"PA15", "PA16";
+				allwinner,function = "gmac";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			gmac_pins_rgmii_a: gmac_rgmii@0 {
+				allwinner,pins = "PA0", "PA1", "PA2",
+						"PA3", "PA4", "PA5", "PA6",
+						"PA7", "PA8", "PA10",
+						"PA11", "PA12", "PA13",
+						"PA15", "PA16";
+				allwinner,function = "gmac";
+				/*
+				 * data lines in RGMII mode use DDR mode
+				 * and need a higher signal drive strength
+				 */
+				allwinner,drive = <3>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		timer@01c20c00 {
-- 
1.9.rc1

[PATCH v3 5/8] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Chen-Yu Tsai]

The CubieTruck uses the GMAC with an RGMII phy.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index f9dcb61..025ce52 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -51,6 +51,18 @@
 			pinctrl-0 = <&i2c2_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_rgmii_a>;
+			phy = <&phy1>;
+			phy-mode = "rgmii";
+			status = "okay";
+
+			phy1: ethernet-phy@1 {
+				reg = <1>;
+			};
+		};
 	};
 
 	leds {
-- 
1.9.rc1

[PATCH v3 6/8] ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC

[Chen-Yu Tsai]

GMAC has better performance and fewer hardware issues.
Use the GMAC in MII mode for ethernet instead of the EMAC.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 5c51cb8..7bf4935 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -19,21 +19,6 @@
 	compatible = "cubietech,cubieboard2", "allwinner,sun7i-a20";
 
 	soc@01c00000 {
-		emac: ethernet@01c0b000 {
-			pinctrl-names = "default";
-			pinctrl-0 = <&emac_pins_a>;
-			phy = <&phy1>;
-			status = "okay";
-		};
-
-		mdio@01c0b080 {
-			status = "okay";
-
-			phy1: ethernet-phy@1 {
-				reg = <1>;
-			};
-		};
-
 		pinctrl@01c20800 {
 			led_pins_cubieboard2: led_pins@0 {
 				allwinner,pins = "PH20", "PH21";
@@ -60,6 +45,18 @@
 			pinctrl-0 = <&i2c1_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_mii_a>;
+			phy = <&phy1>;
+			phy-mode = "mii";
+			status = "okay";
+
+			phy1: ethernet-phy@1 {
+				reg = <1>;
+			};
+		};
 	};
 
 	leds {
-- 
1.9.rc1

[PATCH v3 7/8] ARM: dts: sun7i: olinuxino-micro: Enable GMAC instead of EMAC

[Chen-Yu Tsai]

GMAC has better performance and fewer hardware issues.
Use the GMAC in MII mode for ethernet instead of the EMAC.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 27 +++++++++++--------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index ead3013..b02a796 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -19,21 +19,6 @@
 	compatible = "olimex,a20-olinuxino-micro", "allwinner,sun7i-a20";
 
 	soc@01c00000 {
-		emac: ethernet@01c0b000 {
-			pinctrl-names = "default";
-			pinctrl-0 = <&emac_pins_a>;
-			phy = <&phy1>;
-			status = "okay";
-		};
-
-		mdio@01c0b080 {
-			status = "okay";
-
-			phy1: ethernet-phy@1 {
-				reg = <1>;
-			};
-		};
-
 		pinctrl@01c20800 {
 			led_pins_olinuxino: led_pins@0 {
 				allwinner,pins = "PH2";
@@ -78,6 +63,18 @@
 			pinctrl-0 = <&i2c2_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_mii_a>;
+			phy = <&phy1>;
+			phy-mode = "mii";
+			status = "okay";
+
+			phy1: ethernet-phy@1 {
+				reg = <1>;
+			};
+		};
 	};
 
 	leds {
-- 
1.9.rc1

[PATCH v3 8/8] ARM: dts: sun7i: Add ethernet alias for GMAC

[Chen-Yu Tsai]

U-Boot will insert MAC address into the device tree image.
It looks up ethernet[0-5] aliases to find the ethernet nodes.
Alias GMAC as ethernet0, as it is the only ethernet controller used.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 65fb8d0..c48fb11 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -17,7 +17,7 @@
 	interrupt-parent = <&gic>;
 
 	aliases {
-		ethernet0 = &emac;
+		ethernet0 = &gmac;
 	};
 
 	cpus {
-- 
1.9.rc1

Re: [PATCH v3 1/8] clk: sunxi: Add Allwinner A20/A31 GMAC clock unit

[Maxime Ripard]

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

Hi,

On Mon, Feb 03, 2014 at 11:32:19AM +0800, Chen-Yu Tsai wrote:
> The Allwinner A20/A31 clock module controls the transmit clock source
> and interface type of the GMAC ethernet controller. Model this as
> a single clock for GMAC drivers to use.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/clock/sunxi.txt | 26 +++++++
>  drivers/clk/sunxi/clk-sunxi.c                     | 83 +++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index 0cf679b..f43b4c0 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -37,6 +37,7 @@ Required properties:
>  	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
>  	"allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
>  	"allwinner,sun7i-a20-out-clk" - for the external output clocks
> +	"allwinner,sun7i-a20-gmac-clk" - for the GMAC clock module on A20/A31
>  
>  Required properties for all clocks:
>  - reg : shall be the control register address for the clock.
> @@ -50,6 +51,9 @@ Required properties for all clocks:
>  	If the clock module only has one output, the name shall be the
>  	module name.
>  
> +For "allwinner,sun7i-a20-gmac-clk", the parent clocks shall be fixed rate
> +dummy clocks at 25 MHz and 125 MHz, respectively. See example.
> +
>  Clock consumers should specify the desired clocks they use with a
>  "clocks" phandle cell. Consumers that are using a gated clock should
>  provide an additional ID in their clock property. This ID is the
> @@ -96,3 +100,25 @@ mmc0_clk: clk@01c20088 {
>  	clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
>  	clock-output-names = "mmc0";
>  };
> +
> +mii_phy_tx_clk: clk@2 {
> +	#clock-cells = <0>;
> +	compatible = "fixed-clock";
> +	clock-frequency = <25000000>;
> +	clock-output-names = "mii_phy_tx";
> +};
> +
> +gmac_int_tx_clk: clk@3 {
> +	#clock-cells = <0>;
> +	compatible = "fixed-clock";
> +	clock-frequency = <125000000>;
> +	clock-output-names = "gmac_int_tx";
> +};
> +
> +gmac_clk: clk@01c20164 {
> +	#clock-cells = <0>;
> +	compatible = "allwinner,sun7i-a20-gmac-clk";
> +	reg = <0x01c20164 0x4>;
> +	clocks = <&mii_phy_tx_clk>, <&gmac_int_tx_clk>;

You should also document in which order you expect the parents to
be. Or it will probably be easier to just use clock-names here.

> +	clock-output-names = "gmac";
> +};
> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index 736fb60..0b361d2 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -379,6 +379,89 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32 parent_rate,
>  
>  
>  /**
> + * sun7i_a20_gmac_clk_setup - Setup function for A20/A31 GMAC clock module
> + *
> + * This clock looks something like this
> + *                               ________________________
> + *  MII TX clock from PHY >-----|___________    _________|----> to GMAC core
> + *  GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
> + *  Ext. 125MHz RGMII TX clk >--|__divider__/            |
> + *                              |________________________|
> + *
> + * The external 125 MHz reference is optional, i.e. GMAC can use its
> + * internal TX clock just fine. The A31 GMAC clock module does not have
> + * the divider controls for the external reference.
> + *
> + * To keep it simple, let the GMAC use either the MII TX clock for MII mode,
> + * and its internal TX clock for GMII and RGMII modes. The GMAC driver should
> + * select the appropriate source and gate/ungate the output to the PHY.
> + *
> + * Only the GMAC should use this clock. Altering the clock so that it doesn't
> + * match the GMAC's operation parameters will result in the GMAC not being
> + * able to send traffic out. The GMAC driver should set the clock rate and
> + * enable/disable this clock to configure the required state. The clock
> + * driver then responds by auto-reparenting the clock.
> + */
> +
> +#define SUN7I_A20_GMAC_GPIT	2
> +#define SUN7I_A20_GMAC_MASK	0x3
> +#define SUN7I_A20_GMAC_MAX_PARENTS	2
> +
> +static void __init sun7i_a20_gmac_clk_setup(struct device_node *node)
> +{
> +	struct clk *clk;
> +	struct clk_mux *mux;
> +	struct clk_gate *gate;
> +	const char *clk_name = node->name;
> +	const char *parents[SUN7I_A20_GMAC_MAX_PARENTS];
> +	void *reg;
> +	int i = 0;
> +
> +	/* allocate mux and gate clock structs */
> +	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
> +	if (!mux)
> +		return;

Newline.

> +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> +	if (!gate) {
> +		kfree(mux);
> +		return;
> +	}
> +
> +	reg = of_iomap(node, 0);

You should check for the return code here.

> +	of_property_read_string(node, "clock-output-names", &clk_name);

And here too, since you made the clock-output-names property mandatory

> +	while (i < SUN7I_A20_GMAC_MAX_PARENTS &&
> +			(parents[i] = of_clk_get_parent_name(node, i)) != NULL)

You should check for an error here too, but if you switch to using
clock-names, that will probably be refactored anyway.

> +		i++;
> +
> +	/* set up gate and fixed rate properties */
> +	gate->reg = reg;
> +	gate->bit_idx = SUN7I_A20_GMAC_GPIT;
> +	gate->lock = &clk_lock;
> +	mux->reg = reg;
> +	mux->mask = SUN7I_A20_GMAC_MASK;
> +	mux->flags = CLK_MUX_INDEX_BIT;
> +	mux->lock = &clk_lock;
> +
> +	clk = clk_register_composite(NULL, clk_name,
> +			parents, i,
> +			&mux->hw, &clk_mux_ops,
> +			NULL, NULL,
> +			&gate->hw, &clk_gate_ops,
> +			0);
> +
> +	if (!IS_ERR(clk)) {
> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +		clk_register_clkdev(clk, clk_name, NULL);
> +	}
> +}
> +CLK_OF_DECLARE(sun7i_a20_gmac, "allwinner,sun7i-a20-gmac-clk",
> +		sun7i_a20_gmac_clk_setup);
> +
> +
> +
> +/**
>   * sunxi_factors_clk_setup() - Setup function for factor clocks
>   */
>  
> -- 
> 1.9.rc1
> 

It looks fine otherwise.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/8] ARM: dts: sun7i: Add GMAC clock node to sun7i DTSI

[Maxime Ripard]

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

On Mon, Feb 03, 2014 at 11:32:20AM +0800, Chen-Yu Tsai wrote:
> The GMAC uses 1 of 2 sources for its transmit clock, depending on the
> PHY interface mode. Add both sources as dummy clocks, and as parents
> to the GMAC clock node.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20.dtsi | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 1595e9a..fc7f470 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -314,6 +314,34 @@
>  		};
>  
>  		/*
> +		 * The following two are dummy clocks, placeholders used
> +		 * on gmac_tx clock. The actual frequency and availability
> +		 * depends on the external PHY, operation mode and link
> +		 * speed.
> +		 */

If it depends on the external PHY, I guess that means it also depends
on the board, right? Or is the GMAC supposed to always have that clock
running at 25MHz, no matter what PHY is connected to it?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 8/8] ARM: dts: sun7i: Add ethernet alias for GMAC

[Maxime Ripard]

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

On Mon, Feb 03, 2014 at 11:32:26AM +0800, Chen-Yu Tsai wrote:
> U-Boot will insert MAC address into the device tree image.
> It looks up ethernet[0-5] aliases to find the ethernet nodes.
> Alias GMAC as ethernet0, as it is the only ethernet controller used.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 65fb8d0..c48fb11 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -17,7 +17,7 @@
>  	interrupt-parent = <&gic>;
>  
>  	aliases {
> -		ethernet0 = &emac;
> +		ethernet0 = &gmac;
>  	};

I'm not very fond of this patch.

People might rely on the fact that ethernet0 is actually the emac, and
are expecting u-boot to fill the ethaddr variable to the emac, and not
the gmac.

Since u-boot is totally able to deal with several ethernet addresses,
please add it as ethernet1.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/8] clk: sunxi: Add Allwinner A20/A31 GMAC clock unit

[Chen-Yu Tsai]

Hi,

On Tue, Feb 4, 2014 at 3:31 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Mon, Feb 03, 2014 at 11:32:19AM +0800, Chen-Yu Tsai wrote:
>> The Allwinner A20/A31 clock module controls the transmit clock source
>> and interface type of the GMAC ethernet controller. Model this as
>> a single clock for GMAC drivers to use.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/clock/sunxi.txt | 26 +++++++
>>  drivers/clk/sunxi/clk-sunxi.c                     | 83 +++++++++++++++++++++++
>>  2 files changed, 109 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index 0cf679b..f43b4c0 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -37,6 +37,7 @@ Required properties:
>>       "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
>>       "allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
>>       "allwinner,sun7i-a20-out-clk" - for the external output clocks
>> +     "allwinner,sun7i-a20-gmac-clk" - for the GMAC clock module on A20/A31
>>
>>  Required properties for all clocks:
>>  - reg : shall be the control register address for the clock.
>> @@ -50,6 +51,9 @@ Required properties for all clocks:
>>       If the clock module only has one output, the name shall be the
>>       module name.
>>


>> +For "allwinner,sun7i-a20-gmac-clk", the parent clocks shall be fixed rate
>> +dummy clocks at 25 MHz and 125 MHz, respectively. See example.
>> +


>>  Clock consumers should specify the desired clocks they use with a
>>  "clocks" phandle cell. Consumers that are using a gated clock should
>>  provide an additional ID in their clock property. This ID is the
>> @@ -96,3 +100,25 @@ mmc0_clk: clk@01c20088 {
>>       clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
>>       clock-output-names = "mmc0";
>>  };
>> +
>> +mii_phy_tx_clk: clk@2 {
>> +     #clock-cells = <0>;
>> +     compatible = "fixed-clock";
>> +     clock-frequency = <25000000>;
>> +     clock-output-names = "mii_phy_tx";
>> +};
>> +
>> +gmac_int_tx_clk: clk@3 {
>> +     #clock-cells = <0>;
>> +     compatible = "fixed-clock";
>> +     clock-frequency = <125000000>;
>> +     clock-output-names = "gmac_int_tx";
>> +};
>> +
>> +gmac_clk: clk@01c20164 {
>> +     #clock-cells = <0>;
>> +     compatible = "allwinner,sun7i-a20-gmac-clk";
>> +     reg = <0x01c20164 0x4>;
>> +     clocks = <&mii_phy_tx_clk>, <&gmac_int_tx_clk>;
>
> You should also document in which order you expect the parents to
> be. Or it will probably be easier to just use clock-names here.

Is it not clear from the "Required properties" section above?

>
>> +     clock-output-names = "gmac";
>> +};
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 736fb60..0b361d2 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -379,6 +379,89 @@ static void sun7i_a20_get_out_factors(u32 *freq, u32 parent_rate,
>>
>>
>>  /**
>> + * sun7i_a20_gmac_clk_setup - Setup function for A20/A31 GMAC clock module
>> + *
>> + * This clock looks something like this
>> + *                               ________________________
>> + *  MII TX clock from PHY >-----|___________    _________|----> to GMAC core
>> + *  GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
>> + *  Ext. 125MHz RGMII TX clk >--|__divider__/            |
>> + *                              |________________________|
>> + *
>> + * The external 125 MHz reference is optional, i.e. GMAC can use its
>> + * internal TX clock just fine. The A31 GMAC clock module does not have
>> + * the divider controls for the external reference.
>> + *
>> + * To keep it simple, let the GMAC use either the MII TX clock for MII mode,
>> + * and its internal TX clock for GMII and RGMII modes. The GMAC driver should
>> + * select the appropriate source and gate/ungate the output to the PHY.
>> + *
>> + * Only the GMAC should use this clock. Altering the clock so that it doesn't
>> + * match the GMAC's operation parameters will result in the GMAC not being
>> + * able to send traffic out. The GMAC driver should set the clock rate and
>> + * enable/disable this clock to configure the required state. The clock
>> + * driver then responds by auto-reparenting the clock.
>> + */
>> +
>> +#define SUN7I_A20_GMAC_GPIT  2
>> +#define SUN7I_A20_GMAC_MASK  0x3
>> +#define SUN7I_A20_GMAC_MAX_PARENTS   2
>> +
>> +static void __init sun7i_a20_gmac_clk_setup(struct device_node *node)
>> +{
>> +     struct clk *clk;
>> +     struct clk_mux *mux;
>> +     struct clk_gate *gate;
>> +     const char *clk_name = node->name;
>> +     const char *parents[SUN7I_A20_GMAC_MAX_PARENTS];
>> +     void *reg;
>> +     int i = 0;
>> +
>> +     /* allocate mux and gate clock structs */
>> +     mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
>> +     if (!mux)
>> +             return;
>
> Newline.
>
>> +     gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
>> +     if (!gate) {
>> +             kfree(mux);
>> +             return;
>> +     }
>> +
>> +     reg = of_iomap(node, 0);
>
> You should check for the return code here.
>
>> +     of_property_read_string(node, "clock-output-names", &clk_name);
>
> And here too, since you made the clock-output-names property mandatory
>
>> +     while (i < SUN7I_A20_GMAC_MAX_PARENTS &&
>> +                     (parents[i] = of_clk_get_parent_name(node, i)) != NULL)
>
> You should check for an error here too, but if you switch to using
> clock-names, that will probably be refactored anyway.
>
>> +             i++;
>> +
>> +     /* set up gate and fixed rate properties */
>> +     gate->reg = reg;
>> +     gate->bit_idx = SUN7I_A20_GMAC_GPIT;
>> +     gate->lock = &clk_lock;
>> +     mux->reg = reg;
>> +     mux->mask = SUN7I_A20_GMAC_MASK;
>> +     mux->flags = CLK_MUX_INDEX_BIT;
>> +     mux->lock = &clk_lock;
>> +
>> +     clk = clk_register_composite(NULL, clk_name,
>> +                     parents, i,
>> +                     &mux->hw, &clk_mux_ops,
>> +                     NULL, NULL,
>> +                     &gate->hw, &clk_gate_ops,
>> +                     0);
>> +
>> +     if (!IS_ERR(clk)) {
>> +             of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +             clk_register_clkdev(clk, clk_name, NULL);
>> +     }
>> +}
>> +CLK_OF_DECLARE(sun7i_a20_gmac, "allwinner,sun7i-a20-gmac-clk",
>> +             sun7i_a20_gmac_clk_setup);
>> +
>> +
>> +
>> +/**
>>   * sunxi_factors_clk_setup() - Setup function for factor clocks
>>   */
>>
>> --
>> 1.9.rc1
>>
>
> It looks fine otherwise.

I'll fix the rest.


Cheers
ChenYu

Re: [PATCH v3 2/8] ARM: dts: sun7i: Add GMAC clock node to sun7i DTSI

[Chen-Yu Tsai]

On Tue, Feb 4, 2014 at 3:34 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, Feb 03, 2014 at 11:32:20AM +0800, Chen-Yu Tsai wrote:
>> The GMAC uses 1 of 2 sources for its transmit clock, depending on the
>> PHY interface mode. Add both sources as dummy clocks, and as parents
>> to the GMAC clock node.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20.dtsi | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
>> index 1595e9a..fc7f470 100644
>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> @@ -314,6 +314,34 @@
>>               };
>>
>>               /*
>> +              * The following two are dummy clocks, placeholders used
>> +              * on gmac_tx clock. The actual frequency and availability
>> +              * depends on the external PHY, operation mode and link
>> +              * speed.
>> +              */
>
> If it depends on the external PHY, I guess that means it also depends
> on the board, right? Or is the GMAC supposed to always have that clock
> running at 25MHz, no matter what PHY is connected to it?

What I meant in the comment is that we cannot control the actual clock
rate of the TX clock. We can only select the source, and this is what
gmac_tx clock does. It is just a clock mux. The 125MHz and 25MHz clock
rates are used by the clk_set_rate in the stmmac glue layer to do
auto-reparenting.

The board dependent factor is what _type_ of PHY it is using, i.e.
MII, GMII, or RGMII. If it's MII, the PHY should provide the clock.
If it's RGMII, the internal clock would be used. GMII is a mix of
both. The actual clock rate depends on the link speed.

I should rephrase the comment along the lines of:

The following two are dummy clocks, placeholders used in the gmac_tx
clock. The gmac driver will choose one parent depending on the PHY
interface mode, using clk_set_rate auto-reparenting.
The actual TX clock rate is not controlled by the gmac_tx clock.


Cheers
ChenYu

Re: [PATCH v3 2/8] ARM: dts: sun7i: Add GMAC clock node to sun7i DTSI

[Maxime Ripard]

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

On Tue, Feb 04, 2014 at 11:06:24AM +0800, Chen-Yu Tsai wrote:
> On Tue, Feb 4, 2014 at 3:34 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Mon, Feb 03, 2014 at 11:32:20AM +0800, Chen-Yu Tsai wrote:
> >> The GMAC uses 1 of 2 sources for its transmit clock, depending on the
> >> PHY interface mode. Add both sources as dummy clocks, and as parents
> >> to the GMAC clock node.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> ---
> >>  arch/arm/boot/dts/sun7i-a20.dtsi | 28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> >> index 1595e9a..fc7f470 100644
> >> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> >> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> >> @@ -314,6 +314,34 @@
> >>               };
> >>
> >>               /*
> >> +              * The following two are dummy clocks, placeholders used
> >> +              * on gmac_tx clock. The actual frequency and availability
> >> +              * depends on the external PHY, operation mode and link
> >> +              * speed.
> >> +              */
> >
> > If it depends on the external PHY, I guess that means it also depends
> > on the board, right? Or is the GMAC supposed to always have that clock
> > running at 25MHz, no matter what PHY is connected to it?
> 
> What I meant in the comment is that we cannot control the actual clock
> rate of the TX clock. We can only select the source, and this is what
> gmac_tx clock does. It is just a clock mux. The 125MHz and 25MHz clock
> rates are used by the clk_set_rate in the stmmac glue layer to do
> auto-reparenting.
> 
> The board dependent factor is what _type_ of PHY it is using, i.e.
> MII, GMII, or RGMII. If it's MII, the PHY should provide the clock.
> If it's RGMII, the internal clock would be used. GMII is a mix of
> both. The actual clock rate depends on the link speed.
> 
> I should rephrase the comment along the lines of:
> 
> The following two are dummy clocks, placeholders used in the gmac_tx
> clock. The gmac driver will choose one parent depending on the PHY
> interface mode, using clk_set_rate auto-reparenting.
> The actual TX clock rate is not controlled by the gmac_tx clock.

Ok, thanks for the clarification.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/8] clk: sunxi: Add Allwinner A20/A31 GMAC clock unit

[Maxime Ripard]

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

On Tue, Feb 04, 2014 at 10:43:33AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Feb 4, 2014 at 3:31 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Hi,
> >
> > On Mon, Feb 03, 2014 at 11:32:19AM +0800, Chen-Yu Tsai wrote:
> >> The Allwinner A20/A31 clock module controls the transmit clock source
> >> and interface type of the GMAC ethernet controller. Model this as
> >> a single clock for GMAC drivers to use.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> ---
> >>  Documentation/devicetree/bindings/clock/sunxi.txt | 26 +++++++
> >>  drivers/clk/sunxi/clk-sunxi.c                     | 83 +++++++++++++++++++++++
> >>  2 files changed, 109 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> >> index 0cf679b..f43b4c0 100644
> >> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> >> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> >> @@ -37,6 +37,7 @@ Required properties:
> >>       "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
> >>       "allwinner,sun4i-mod0-clk" - for the module 0 family of clocks
> >>       "allwinner,sun7i-a20-out-clk" - for the external output clocks
> >> +     "allwinner,sun7i-a20-gmac-clk" - for the GMAC clock module on A20/A31
> >>
> >>  Required properties for all clocks:
> >>  - reg : shall be the control register address for the clock.
> >> @@ -50,6 +51,9 @@ Required properties for all clocks:
> >>       If the clock module only has one output, the name shall be the
> >>       module name.
> >>
> 
> 
> >> +For "allwinner,sun7i-a20-gmac-clk", the parent clocks shall be fixed rate
> >> +dummy clocks at 25 MHz and 125 MHz, respectively. See example.
> >> +
> 
> 
> >>  Clock consumers should specify the desired clocks they use with a
> >>  "clocks" phandle cell. Consumers that are using a gated clock should
> >>  provide an additional ID in their clock property. This ID is the
> >> @@ -96,3 +100,25 @@ mmc0_clk: clk@01c20088 {
> >>       clocks = <&osc24M>, <&pll6 1>, <&pll5 1>;
> >>       clock-output-names = "mmc0";
> >>  };
> >> +
> >> +mii_phy_tx_clk: clk@2 {
> >> +     #clock-cells = <0>;
> >> +     compatible = "fixed-clock";
> >> +     clock-frequency = <25000000>;
> >> +     clock-output-names = "mii_phy_tx";
> >> +};
> >> +
> >> +gmac_int_tx_clk: clk@3 {
> >> +     #clock-cells = <0>;
> >> +     compatible = "fixed-clock";
> >> +     clock-frequency = <125000000>;
> >> +     clock-output-names = "gmac_int_tx";
> >> +};
> >> +
> >> +gmac_clk: clk@01c20164 {
> >> +     #clock-cells = <0>;
> >> +     compatible = "allwinner,sun7i-a20-gmac-clk";
> >> +     reg = <0x01c20164 0x4>;
> >> +     clocks = <&mii_phy_tx_clk>, <&gmac_int_tx_clk>;
> >
> > You should also document in which order you expect the parents to
> > be. Or it will probably be easier to just use clock-names here.
> 
> Is it not clear from the "Required properties" section above?

I'd make it clearer. But again, using clock-names would avoid any
ambiguity, and be more flexible.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 8/8] ARM: dts: sun7i: Add ethernet alias for GMAC

[Chen-Yu Tsai]

On Tue, Feb 4, 2014 at 3:38 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, Feb 03, 2014 at 11:32:26AM +0800, Chen-Yu Tsai wrote:
>> U-Boot will insert MAC address into the device tree image.
>> It looks up ethernet[0-5] aliases to find the ethernet nodes.
>> Alias GMAC as ethernet0, as it is the only ethernet controller used.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
>> index 65fb8d0..c48fb11 100644
>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> @@ -17,7 +17,7 @@
>>       interrupt-parent = <&gic>;
>>
>>       aliases {
>> -             ethernet0 = &emac;
>> +             ethernet0 = &gmac;
>>       };
>
> I'm not very fond of this patch.
>
> People might rely on the fact that ethernet0 is actually the emac, and
> are expecting u-boot to fill the ethaddr variable to the emac, and not
> the gmac.
>
> Since u-boot is totally able to deal with several ethernet addresses,
> please add it as ethernet1.

Actually I think we should override this in the board dts.
The boards we currently support can only use emac or gmac,
and in our u-boot tree, they have been converted to using
gmac. If any future boards support both emac and gmac, we
can address the ordering then. And the ordering should
match u-boot.

What do you think?


Cheers
ChenYu

Re: [PATCH v3 8/8] ARM: dts: sun7i: Add ethernet alias for GMAC

[Maxime Ripard]

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

On Wed, Feb 05, 2014 at 12:43:18PM +0800, Chen-Yu Tsai wrote:
> On Tue, Feb 4, 2014 at 3:38 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Mon, Feb 03, 2014 at 11:32:26AM +0800, Chen-Yu Tsai wrote:
> >> U-Boot will insert MAC address into the device tree image.
> >> It looks up ethernet[0-5] aliases to find the ethernet nodes.
> >> Alias GMAC as ethernet0, as it is the only ethernet controller used.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> ---
> >>  arch/arm/boot/dts/sun7i-a20.dtsi | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> >> index 65fb8d0..c48fb11 100644
> >> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> >> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> >> @@ -17,7 +17,7 @@
> >>       interrupt-parent = <&gic>;
> >>
> >>       aliases {
> >> -             ethernet0 = &emac;
> >> +             ethernet0 = &gmac;
> >>       };
> >
> > I'm not very fond of this patch.
> >
> > People might rely on the fact that ethernet0 is actually the emac, and
> > are expecting u-boot to fill the ethaddr variable to the emac, and not
> > the gmac.
> >
> > Since u-boot is totally able to deal with several ethernet addresses,
> > please add it as ethernet1.
> 
> Actually I think we should override this in the board dts.
> The boards we currently support can only use emac or gmac,
> and in our u-boot tree, they have been converted to using
> gmac. If any future boards support both emac and gmac, we
> can address the ordering then. And the ordering should
> match u-boot.
> 
> What do you think?

That what u-boot does is not a good argument (especially when it's not
even mainline u-boot we're talking about). Otherwise, I can just cook
up a bootloader of my own, do crazy stuff in it, and use that as an
argument to get crazy things in the kernel.

And you can set eth1addr just fine in u-boot.

But thinking a bit more about your patch, I guess that this patch
makes some kind of sense anyway..

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]