[PATCH v2 2/2] sh_eth: add device tree support

[PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Add support of the device tree probing for the Renesas SH-Mobile SoCs
documenting the device tree binding as necessary.

This work is loosely based on an original patch by Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com>.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against Dave's 'net-next.git' repo.
Not posting it to netdev@vger.kernel.org this time, or Dave will scold me. :-)

Changes in version 2:
- added sh_eth_match_table[] entry for "renesas,ether-r8a7778", documented it;
- clarified descriptions of the "reg" and "interrupt" properties;
- moved "interrupt-parent" from required properties to optional;
- mentioned the necessary PHY subnode to the "phy-handle" property description,
  documented the requered "#address-cells" and "#size-cells" properties;
- clarified the types/descriptions of the Renesas specific properties;
- refreshed the patch.

 Documentation/devicetree/bindings/net/sh_eth.txt |   44 +++++++++++++++
 drivers/net/ethernet/renesas/sh_eth.c            |   67 ++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 1 deletion(-)

Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
=================================--- /dev/null
+++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt
@@ -0,0 +1,44 @@
+* Renesas Electronics SH EtherMAC
+
+This file provides information on what the device node for the SH EtherMAC
+interface contains.
+
+Required properties:
+- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC.
+	      "renesas,ether-r8a7778"  if the device is a part of R8A7778 SoC.
+	      "renesas,ether-r8a7779"  if the device is a part of R8A7779 SoC.
+	      "renesas,ether-r8a7790"  if the device is a part of R8A7790 SoC.
+- reg: offset and length of (1) the E-DMAC/feLic register block (required),
+       (2) the TSU register block (optional).
+- interrupts: interrupt specifier for the sole interrupt.
+- phy-mode: string, operation mode of the PHY interface (a string that
+	    of_get_phy_mode() can understand).
+- phy-handle: phandle of the PHY device (there should be a PHY device subnode).
+- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
+- #size-cells: number of size cells on the MDIO bus, must be equal to 0.
+
+Optional properties:
+- interrupt-parent: the phandle for the interrupt controller that services
+		    interrupts for this device.
+- local-mac-address: 6 bytes, MAC address.
+- renesas,no-ether-link: boolean, specify when a board does not provide a proper
+			 Ether LINK signal.
+- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
+  				 active-low instead of normal active-high.
+
+Example (Armadillo800EVA board):
+
+	ethernet@e9a00000 {
+		compatible = "renesas,gether-r8a7740";
+		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 142 0x4>;
+		phy-mode = "mii";
+		phy-handle = <&phy0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		phy0: ethernet-phy@0 {
+			reg = <0>;
+		};
+	};
Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
=================================--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -32,6 +32,9 @@
 #include <linux/platform_device.h>
 #include <linux/mdio-bitbang.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/cache.h>
 #include <linux/io.h>
@@ -2606,6 +2609,53 @@ static const struct net_device_ops sh_et
 	.ndo_change_mtu		= eth_change_mtu,
 };
 
+#ifdef CONFIG_OF
+static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct sh_eth_plat_data *pdata;
+	struct device_node *phy;
+	const char *mac_addr;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->phy_interface = of_get_phy_mode(np);
+
+	phy = of_parse_phandle(np, "phy-handle", 0);
+	if (!phy || of_property_read_u32(phy, "reg", &pdata->phy)) {
+		devm_kfree(dev, pdata);
+		return NULL;
+	}
+
+	mac_addr = of_get_mac_address(np);
+	if (mac_addr)
+		memcpy(pdata->mac_addr, mac_addr, ETH_ALEN);
+
+	pdata->no_ether_link +		of_property_read_bool(np, "renesas,no-ether-link");
+	pdata->ether_link_active_low +		of_property_read_bool(np, "renesas,ether-link-active-low");
+
+	return pdata;
+}
+
+static const struct of_device_id sh_eth_match_table[] = {
+	{ .compatible = "renesas,gether-r8a7740", .data = &r8a7740_data },
+	{ .compatible = "renesas,ether-r8a7778", .data = &r8a777x_data },
+	{ .compatible = "renesas,ether-r8a7779", .data = &r8a777x_data },
+	{ .compatible = "renesas,ether-r8a7790", .data = &r8a7790_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sh_eth_match_table);
+#else
+static inline struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int sh_eth_drv_probe(struct platform_device *pdev)
 {
 	int ret, devno = 0;
@@ -2659,6 +2709,8 @@ static int sh_eth_drv_probe(struct platf
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
+	if (pdev->dev.of_node)
+		pd = sh_eth_parse_dt(&pdev->dev);
 	if (!pd) {
 		dev_err(&pdev->dev, "no platform data\n");
 		ret = -EINVAL;
@@ -2674,7 +2726,19 @@ static int sh_eth_drv_probe(struct platf
 	mdp->ether_link_active_low = pd->ether_link_active_low;
 
 	/* set cpu data */
-	mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
+	if (id) {
+		mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
+	} else	{
+		const struct of_device_id *match;
+
+		match = of_match_device(of_match_ptr(sh_eth_match_table),
+					&pdev->dev);
+		if (!match) {
+			ret = -EINVAL;
+			goto out_release;
+		}
+		mdp->cd = (struct sh_eth_cpu_data *)match->data;
+	}
 	mdp->reg_offset = sh_eth_get_register_offset(mdp->cd->register_type);
 	sh_eth_set_default_cpu_data(mdp->cd);
 
@@ -2815,6 +2879,7 @@ static struct platform_driver sh_eth_dri
 	.driver = {
 		   .name = CARDNAME,
 		   .pm = SH_ETH_PM_OPS,
+		   .of_match_table = of_match_ptr(sh_eth_match_table),
 	},
 };
 

[PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Add support of the device tree probing for the Renesas SH-Mobile SoCs
documenting the device tree binding as necessary.

This work is loosely based on an original patch by Nobuhiro Iwamatsu
<nobuhiro.iwamatsu.yj@renesas.com>.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against Dave's 'net-next.git' repo.
Not posting it to netdev@vger.kernel.org this time, or Dave will scold me. :-)

Changes in version 2:
- added sh_eth_match_table[] entry for "renesas,ether-r8a7778", documented it;
- clarified descriptions of the "reg" and "interrupt" properties;
- moved "interrupt-parent" from required properties to optional;
- mentioned the necessary PHY subnode to the "phy-handle" property description,
  documented the requered "#address-cells" and "#size-cells" properties;
- clarified the types/descriptions of the Renesas specific properties;
- refreshed the patch.

 Documentation/devicetree/bindings/net/sh_eth.txt |   44 +++++++++++++++
 drivers/net/ethernet/renesas/sh_eth.c            |   67 ++++++++++++++++++++++-
 2 files changed, 110 insertions(+), 1 deletion(-)

Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
===================================================================
--- /dev/null
+++ net-next/Documentation/devicetree/bindings/net/sh_eth.txt
@@ -0,0 +1,44 @@
+* Renesas Electronics SH EtherMAC
+
+This file provides information on what the device node for the SH EtherMAC
+interface contains.
+
+Required properties:
+- compatible: "renesas,gether-r8a7740" if the device is a part of R8A7740 SoC.
+	      "renesas,ether-r8a7778"  if the device is a part of R8A7778 SoC.
+	      "renesas,ether-r8a7779"  if the device is a part of R8A7779 SoC.
+	      "renesas,ether-r8a7790"  if the device is a part of R8A7790 SoC.
+- reg: offset and length of (1) the E-DMAC/feLic register block (required),
+       (2) the TSU register block (optional).
+- interrupts: interrupt specifier for the sole interrupt.
+- phy-mode: string, operation mode of the PHY interface (a string that
+	    of_get_phy_mode() can understand).
+- phy-handle: phandle of the PHY device (there should be a PHY device subnode).
+- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
+- #size-cells: number of size cells on the MDIO bus, must be equal to 0.
+
+Optional properties:
+- interrupt-parent: the phandle for the interrupt controller that services
+		    interrupts for this device.
+- local-mac-address: 6 bytes, MAC address.
+- renesas,no-ether-link: boolean, specify when a board does not provide a proper
+			 Ether LINK signal.
+- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
+  				 active-low instead of normal active-high.
+
+Example (Armadillo800EVA board):
+
+	ethernet@e9a00000 {
+		compatible = "renesas,gether-r8a7740";
+		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 142 0x4>;
+		phy-mode = "mii";
+		phy-handle = <&phy0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		phy0: ethernet-phy@0 {
+			reg = <0>;
+		};
+	};
Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -32,6 +32,9 @@
 #include <linux/platform_device.h>
 #include <linux/mdio-bitbang.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/cache.h>
 #include <linux/io.h>
@@ -2606,6 +2609,53 @@ static const struct net_device_ops sh_et
 	.ndo_change_mtu		= eth_change_mtu,
 };
 
+#ifdef CONFIG_OF
+static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct sh_eth_plat_data *pdata;
+	struct device_node *phy;
+	const char *mac_addr;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->phy_interface = of_get_phy_mode(np);
+
+	phy = of_parse_phandle(np, "phy-handle", 0);
+	if (!phy || of_property_read_u32(phy, "reg", &pdata->phy)) {
+		devm_kfree(dev, pdata);
+		return NULL;
+	}
+
+	mac_addr = of_get_mac_address(np);
+	if (mac_addr)
+		memcpy(pdata->mac_addr, mac_addr, ETH_ALEN);
+
+	pdata->no_ether_link =
+		of_property_read_bool(np, "renesas,no-ether-link");
+	pdata->ether_link_active_low =
+		of_property_read_bool(np, "renesas,ether-link-active-low");
+
+	return pdata;
+}
+
+static const struct of_device_id sh_eth_match_table[] = {
+	{ .compatible = "renesas,gether-r8a7740", .data = &r8a7740_data },
+	{ .compatible = "renesas,ether-r8a7778", .data = &r8a777x_data },
+	{ .compatible = "renesas,ether-r8a7779", .data = &r8a777x_data },
+	{ .compatible = "renesas,ether-r8a7790", .data = &r8a7790_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sh_eth_match_table);
+#else
+static inline struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int sh_eth_drv_probe(struct platform_device *pdev)
 {
 	int ret, devno = 0;
@@ -2659,6 +2709,8 @@ static int sh_eth_drv_probe(struct platf
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
+	if (pdev->dev.of_node)
+		pd = sh_eth_parse_dt(&pdev->dev);
 	if (!pd) {
 		dev_err(&pdev->dev, "no platform data\n");
 		ret = -EINVAL;
@@ -2674,7 +2726,19 @@ static int sh_eth_drv_probe(struct platf
 	mdp->ether_link_active_low = pd->ether_link_active_low;
 
 	/* set cpu data */
-	mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
+	if (id) {
+		mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
+	} else	{
+		const struct of_device_id *match;
+
+		match = of_match_device(of_match_ptr(sh_eth_match_table),
+					&pdev->dev);
+		if (!match) {
+			ret = -EINVAL;
+			goto out_release;
+		}
+		mdp->cd = (struct sh_eth_cpu_data *)match->data;
+	}
 	mdp->reg_offset = sh_eth_get_register_offset(mdp->cd->register_type);
 	sh_eth_set_default_cpu_data(mdp->cd);
 
@@ -2815,6 +2879,7 @@ static struct platform_driver sh_eth_dri
 	.driver = {
 		   .name = CARDNAME,
 		   .pm = SH_ETH_PM_OPS,
+		   .of_match_table = of_match_ptr(sh_eth_match_table),
 	},
 };
 

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/06/2013 05:43 PM, Sergei Shtylyov wrote:
> Add support of the device tree probing for the Renesas SH-Mobile SoCs
> documenting the device tree binding as necessary.
> 
> This work is loosely based on an original patch by Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.yj@renesas.com>.

> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt

> +- reg: offset and length of (1) the E-DMAC/feLic register block (required),
> +       (2) the TSU register block (optional).

There's an argument that you should specify reg-names entries to allow
arbitrary ordering or entries within reg, if there's more than one
entry. But, I don't think it's mandatory, just something to consider at
present.

> +- interrupts: interrupt specifier for the sole interrupt.
> +- phy-mode: string, operation mode of the PHY interface (a string that
> +	    of_get_phy_mode() can understand).

DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
Please spell out the complete list of supported values here, without
reference to Linux-specific code or documentation.

> +- phy-handle: phandle of the PHY device (there should be a PHY device subnode).

Is this a custom property, or part of some generic PHY subsystem
binding? If the latter, please reference the DT binding document for
that subsystem. If it's custom, what kinds of nodes can this phandle
point at; what kind of interface must the referenced DT node provide (is
a #phy-cells property required, must its compatible value be one of a
specific set of values).

> +- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
> +- #size-cells: number of size cells on the MDIO bus, must be equal to 0.

If this node is expected to contain child nodes, you need to specify
details of those child nodes. Can you reference the filename of a
generic MDIO binding? What do the address values mean (perhaps that'd be
covered by the MDIO binding already, if it's applicable).

> +Optional properties:
> +- interrupt-parent: the phandle for the interrupt controller that services
> +		    interrupts for this device.
> +- local-mac-address: 6 bytes, MAC address.
> +- renesas,no-ether-link: boolean, specify when a board does not provide a proper
> +			 Ether LINK signal.
> +- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
> +  				 active-low instead of normal active-high.

Presumably the link signal is some dedicated signal in the Ethernet MAC
HW block, and not a generic GPIO? If it's a GPIO, there should be a flag
in the GPIO property/binding you can use for active low.

Do you need any clocks properties, IP block reset signals, power domains?

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/06/2013 05:43 PM, Sergei Shtylyov wrote:
> Add support of the device tree probing for the Renesas SH-Mobile SoCs
> documenting the device tree binding as necessary.
> 
> This work is loosely based on an original patch by Nobuhiro Iwamatsu
> <nobuhiro.iwamatsu.yj@renesas.com>.

> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt

> +- reg: offset and length of (1) the E-DMAC/feLic register block (required),
> +       (2) the TSU register block (optional).

There's an argument that you should specify reg-names entries to allow
arbitrary ordering or entries within reg, if there's more than one
entry. But, I don't think it's mandatory, just something to consider at
present.

> +- interrupts: interrupt specifier for the sole interrupt.
> +- phy-mode: string, operation mode of the PHY interface (a string that
> +	    of_get_phy_mode() can understand).

DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
Please spell out the complete list of supported values here, without
reference to Linux-specific code or documentation.

> +- phy-handle: phandle of the PHY device (there should be a PHY device subnode).

Is this a custom property, or part of some generic PHY subsystem
binding? If the latter, please reference the DT binding document for
that subsystem. If it's custom, what kinds of nodes can this phandle
point at; what kind of interface must the referenced DT node provide (is
a #phy-cells property required, must its compatible value be one of a
specific set of values).

> +- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
> +- #size-cells: number of size cells on the MDIO bus, must be equal to 0.

If this node is expected to contain child nodes, you need to specify
details of those child nodes. Can you reference the filename of a
generic MDIO binding? What do the address values mean (perhaps that'd be
covered by the MDIO binding already, if it's applicable).

> +Optional properties:
> +- interrupt-parent: the phandle for the interrupt controller that services
> +		    interrupts for this device.
> +- local-mac-address: 6 bytes, MAC address.
> +- renesas,no-ether-link: boolean, specify when a board does not provide a proper
> +			 Ether LINK signal.
> +- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
> +  				 active-low instead of normal active-high.

Presumably the link signal is some dedicated signal in the Ethernet MAC
HW block, and not a generic GPIO? If it's a GPIO, there should be a flag
in the GPIO property/binding you can use for active low.

Do you need any clocks properties, IP block reset signals, power domains?

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 10-09-2013 1:02, Stephen Warren wrote:

>> Add support of the device tree probing for the Renesas SH-Mobile SoCs
>> documenting the device tree binding as necessary.

>> This work is loosely based on an original patch by Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.yj@renesas.com>.

>> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt

>> +- reg: offset and length of (1) the E-DMAC/feLic register block (required),
>> +       (2) the TSU register block (optional).

> There's an argument that you should specify reg-names entries to allow
> arbitrary ordering or entries within reg, if there's more than one
> entry. But, I don't think it's mandatory, just something to consider at
> present.

    I also don't think it's needed, the driver has happily worked without 
resource names so far.

>> +- interrupts: interrupt specifier for the sole interrupt.
>> +- phy-mode: string, operation mode of the PHY interface (a string that
>> +	    of_get_phy_mode() can understand).

> DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
> Please spell out the complete list of supported values here, without
> reference to Linux-specific code or documentation.

    I don't think listing the numerous values of this standrd property in 
every file describing an Ethernet device is practical. How about I create file 
ethernet.txt in the same directory (don't know why I'm the first to do it 
despite many other driver using this property)?

>> +- phy-handle: phandle of the PHY device (there should be a PHY device subnode).

> Is this a custom property, or part of some generic PHY subsystem
> binding?

    The latter.

> If the latter, please reference the DT binding document for
> that subsystem.

    Unfortunately, phy.txt in the same directory describes only properties of 
the "ethernet-phy" nodes. I think the new ethernet.txt would be more fitting 
for the purpose.

> If it's custom, what kinds of nodes can this phandle
> point at; what kind of interface must the referenced DT node provide (is
> a #phy-cells property required, must its compatible value be one of a
> specific set of values).

    This is described by the phy.txt in the same directory.

>> +- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
>> +- #size-cells: number of size cells on the MDIO bus, must be equal to 0.

> If this node is expected to contain child nodes, you need to specify
> details of those child nodes.

    They are specified in phy.txt (this file is somewhat obsolete though).

> Can you reference the filename of a generic MDIO binding?

    OK.

> What do the address values mean (perhaps that'd be
> covered by the MDIO binding already, if it's applicable).

>> +Optional properties:
>> +- interrupt-parent: the phandle for the interrupt controller that services
>> +		    interrupts for this device.
>> +- local-mac-address: 6 bytes, MAC address.
>> +- renesas,no-ether-link: boolean, specify when a board does not provide a proper
>> +			 Ether LINK signal.
>> +- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
>> +  				 active-low instead of normal active-high.

> Presumably the link signal is some dedicated signal in the Ethernet MAC
> HW block, and not a generic GPIO?

    Indeed.

> Do you need any clocks properties, IP block reset signals, power domains?

    Currently not.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 10-09-2013 1:02, Stephen Warren wrote:

>> Add support of the device tree probing for the Renesas SH-Mobile SoCs
>> documenting the device tree binding as necessary.

>> This work is loosely based on an original patch by Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.yj@renesas.com>.

>> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt

>> +- reg: offset and length of (1) the E-DMAC/feLic register block (required),
>> +       (2) the TSU register block (optional).

> There's an argument that you should specify reg-names entries to allow
> arbitrary ordering or entries within reg, if there's more than one
> entry. But, I don't think it's mandatory, just something to consider at
> present.

    I also don't think it's needed, the driver has happily worked without 
resource names so far.

>> +- interrupts: interrupt specifier for the sole interrupt.
>> +- phy-mode: string, operation mode of the PHY interface (a string that
>> +	    of_get_phy_mode() can understand).

> DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
> Please spell out the complete list of supported values here, without
> reference to Linux-specific code or documentation.

    I don't think listing the numerous values of this standrd property in 
every file describing an Ethernet device is practical. How about I create file 
ethernet.txt in the same directory (don't know why I'm the first to do it 
despite many other driver using this property)?

>> +- phy-handle: phandle of the PHY device (there should be a PHY device subnode).

> Is this a custom property, or part of some generic PHY subsystem
> binding?

    The latter.

> If the latter, please reference the DT binding document for
> that subsystem.

    Unfortunately, phy.txt in the same directory describes only properties of 
the "ethernet-phy" nodes. I think the new ethernet.txt would be more fitting 
for the purpose.

> If it's custom, what kinds of nodes can this phandle
> point at; what kind of interface must the referenced DT node provide (is
> a #phy-cells property required, must its compatible value be one of a
> specific set of values).

    This is described by the phy.txt in the same directory.

>> +- #address-cells: number of address cells for the MDIO bus, must be equal to 1.
>> +- #size-cells: number of size cells on the MDIO bus, must be equal to 0.

> If this node is expected to contain child nodes, you need to specify
> details of those child nodes.

    They are specified in phy.txt (this file is somewhat obsolete though).

> Can you reference the filename of a generic MDIO binding?

    OK.

> What do the address values mean (perhaps that'd be
> covered by the MDIO binding already, if it's applicable).

>> +Optional properties:
>> +- interrupt-parent: the phandle for the interrupt controller that services
>> +		    interrupts for this device.
>> +- local-mac-address: 6 bytes, MAC address.
>> +- renesas,no-ether-link: boolean, specify when a board does not provide a proper
>> +			 Ether LINK signal.
>> +- renesas,ether-link-active-low: boolean, specify when the Ether LINK signal is
>> +  				 active-low instead of normal active-high.

> Presumably the link signal is some dedicated signal in the Ethernet MAC
> HW block, and not a generic GPIO?

    Indeed.

> Do you need any clocks properties, IP block reset signals, power domains?

    Currently not.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 08:39 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 10-09-2013 1:02, Stephen Warren wrote:
> 
>>> Add support of the device tree probing for the Renesas SH-Mobile SoCs
>>> documenting the device tree binding as necessary.
> 
>>> This work is loosely based on an original patch by Nobuhiro Iwamatsu
>>> <nobuhiro.iwamatsu.yj@renesas.com>.
> 
>>> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
> 
>>> +- reg: offset and length of (1) the E-DMAC/feLic register block
>>> (required),
>>> +       (2) the TSU register block (optional).
> 
>> There's an argument that you should specify reg-names entries to allow
>> arbitrary ordering or entries within reg, if there's more than one
>> entry. But, I don't think it's mandatory, just something to consider at
>> present.
> 
>    I also don't think it's needed, the driver has happily worked without
> resource names so far.
> 
>>> +- interrupts: interrupt specifier for the sole interrupt.
>>> +- phy-mode: string, operation mode of the PHY interface (a string that
>>> +        of_get_phy_mode() can understand).
> 
>> DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
>> Please spell out the complete list of supported values here, without
>> reference to Linux-specific code or documentation.
> 
>    I don't think listing the numerous values of this standrd property in
> every file describing an Ethernet device is practical. How about I
> create file ethernet.txt in the same directory (don't know why I'm the
> first to do it despite many other driver using this property)?

We must list the values somewhere. Creating an ethernet.txt, documenting
them there, and modifying the description of this property to reference
ethernet.txt would be fine.

>>> +- phy-handle: phandle of the PHY device (there should be a PHY
>>> device subnode).
> 
>> Is this a custom property, or part of some generic PHY subsystem
>> binding?
> 
>    The latter.
> 
>> If the latter, please reference the DT binding document for
>> that subsystem.
> 
>    Unfortunately, phy.txt in the same directory describes only
> properties of the "ethernet-phy" nodes. I think the new ethernet.txt
> would be more fitting for the purpose.

That's fine, so long as it's documented somewhere, and this document
references that place.

>> Do you need any clocks properties, IP block reset signals, power domains?
> 
>    Currently not.

What does "currently" mean? Does that mean that the Linux driver simply
doesn't touch those entities at present? If so, that's not enough to say
that those entities should not be described in the DT binding. We should
strive to make the binding completely describe all aspects of the HW,
irrespective of whether a particular driver happens to use that
information at present.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 08:39 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 10-09-2013 1:02, Stephen Warren wrote:
> 
>>> Add support of the device tree probing for the Renesas SH-Mobile SoCs
>>> documenting the device tree binding as necessary.
> 
>>> This work is loosely based on an original patch by Nobuhiro Iwamatsu
>>> <nobuhiro.iwamatsu.yj@renesas.com>.
> 
>>> Index: net-next/Documentation/devicetree/bindings/net/sh_eth.txt
> 
>>> +- reg: offset and length of (1) the E-DMAC/feLic register block
>>> (required),
>>> +       (2) the TSU register block (optional).
> 
>> There's an argument that you should specify reg-names entries to allow
>> arbitrary ordering or entries within reg, if there's more than one
>> entry. But, I don't think it's mandatory, just something to consider at
>> present.
> 
>    I also don't think it's needed, the driver has happily worked without
> resource names so far.
> 
>>> +- interrupts: interrupt specifier for the sole interrupt.
>>> +- phy-mode: string, operation mode of the PHY interface (a string that
>>> +        of_get_phy_mode() can understand).
> 
>> DT bindings should be OS-agnostic. of_get_phy_mode() is Linux-specific.
>> Please spell out the complete list of supported values here, without
>> reference to Linux-specific code or documentation.
> 
>    I don't think listing the numerous values of this standrd property in
> every file describing an Ethernet device is practical. How about I
> create file ethernet.txt in the same directory (don't know why I'm the
> first to do it despite many other driver using this property)?

We must list the values somewhere. Creating an ethernet.txt, documenting
them there, and modifying the description of this property to reference
ethernet.txt would be fine.

>>> +- phy-handle: phandle of the PHY device (there should be a PHY
>>> device subnode).
> 
>> Is this a custom property, or part of some generic PHY subsystem
>> binding?
> 
>    The latter.
> 
>> If the latter, please reference the DT binding document for
>> that subsystem.
> 
>    Unfortunately, phy.txt in the same directory describes only
> properties of the "ethernet-phy" nodes. I think the new ethernet.txt
> would be more fitting for the purpose.

That's fine, so long as it's documented somewhere, and this document
references that place.

>> Do you need any clocks properties, IP block reset signals, power domains?
> 
>    Currently not.

What does "currently" mean? Does that mean that the Linux driver simply
doesn't touch those entities at present? If so, that's not enough to say
that those entities should not be described in the DT binding. We should
strive to make the binding completely describe all aspects of the HW,
irrespective of whether a particular driver happens to use that
information at present.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/10/2013 07:15 PM, Stephen Warren wrote:

>>> Do you need any clocks properties, IP block reset signals, power domains?

>>     Currently not.

> What does "currently" mean? Does that mean that the Linux driver simply
> doesn't touch those entities at present?

    There's Ether clock but the driver doesn't manipulate it directly, 
assumingly it does this thru the runtime PM interface. As for the others, I 
simply don't know.

> If so, that's not enough to say
> that those entities should not be described in the DT binding. We should
> strive to make the binding completely describe all aspects of the HW,
> irrespective of whether a particular driver happens to use that
> information at present.

    There's no DT representation for the clocks in SH-Mobile subarch yet.
The same applies to the other entities you mentioned.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/10/2013 07:15 PM, Stephen Warren wrote:

>>> Do you need any clocks properties, IP block reset signals, power domains?

>>     Currently not.

> What does "currently" mean? Does that mean that the Linux driver simply
> doesn't touch those entities at present?

    There's Ether clock but the driver doesn't manipulate it directly, 
assumingly it does this thru the runtime PM interface. As for the others, I 
simply don't know.

> If so, that's not enough to say
> that those entities should not be described in the DT binding. We should
> strive to make the binding completely describe all aspects of the HW,
> irrespective of whether a particular driver happens to use that
> information at present.

    There's no DT representation for the clocks in SH-Mobile subarch yet.
The same applies to the other entities you mentioned.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 12:01 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/10/2013 07:15 PM, Stephen Warren wrote:
> 
>>>> Do you need any clocks properties, IP block reset signals, power
>>>> domains?
> 
>>>     Currently not.
> 
>> What does "currently" mean? Does that mean that the Linux driver simply
>> doesn't touch those entities at present?
> 
>    There's Ether clock but the driver doesn't manipulate it directly,
> assumingly it does this thru the runtime PM interface. As for the
> others, I simply don't know.

If there's a clock, it should be represented in DT, even if the kernel
somehow gets access to the clock through some means other than parsing DT.

>> If so, that's not enough to say
>> that those entities should not be described in the DT binding. We should
>> strive to make the binding completely describe all aspects of the HW,
>> irrespective of whether a particular driver happens to use that
>> information at present.
> 
>    There's no DT representation for the clocks in SH-Mobile subarch yet.
> The same applies to the other entities you mentioned.

You can still write the binding to say that the appropriate clock
property must be present; the overall format of this property won't be
affected by the representation chosen for the SH-Mobile clocks.

It seems like it'd be best to get the basic resources (like clocks)
represented in DT before trying to build blocks that use them.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 12:01 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/10/2013 07:15 PM, Stephen Warren wrote:
> 
>>>> Do you need any clocks properties, IP block reset signals, power
>>>> domains?
> 
>>>     Currently not.
> 
>> What does "currently" mean? Does that mean that the Linux driver simply
>> doesn't touch those entities at present?
> 
>    There's Ether clock but the driver doesn't manipulate it directly,
> assumingly it does this thru the runtime PM interface. As for the
> others, I simply don't know.

If there's a clock, it should be represented in DT, even if the kernel
somehow gets access to the clock through some means other than parsing DT.

>> If so, that's not enough to say
>> that those entities should not be described in the DT binding. We should
>> strive to make the binding completely describe all aspects of the HW,
>> irrespective of whether a particular driver happens to use that
>> information at present.
> 
>    There's no DT representation for the clocks in SH-Mobile subarch yet.
> The same applies to the other entities you mentioned.

You can still write the binding to say that the appropriate clock
property must be present; the overall format of this property won't be
affected by the representation chosen for the SH-Mobile clocks.

It seems like it'd be best to get the basic resources (like clocks)
represented in DT before trying to build blocks that use them.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/11/2013 12:07 AM, Stephen Warren wrote:

>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>> domains?

>>>>      Currently not.

>>> What does "currently" mean? Does that mean that the Linux driver simply
>>> doesn't touch those entities at present?

>>     There's Ether clock but the driver doesn't manipulate it directly,
>> assumingly it does this thru the runtime PM interface. As for the
>> others, I simply don't know.

> If there's a clock, it should be represented in DT, even if the kernel
> somehow gets access to the clock through some means other than parsing DT.

    Frankly speaking, I don't see the point.

>>> If so, that's not enough to say
>>> that those entities should not be described in the DT binding. We should
>>> strive to make the binding completely describe all aspects of the HW,
>>> irrespective of whether a particular driver happens to use that
>>> information at present.

>>     There's no DT representation for the clocks in SH-Mobile subarch yet.
>> The same applies to the other entities you mentioned.

> You can still write the binding to say that the appropriate clock
> property must be present; the overall format of this property won't be
> affected by the representation chosen for the SH-Mobile clocks.

    Where can I find an example of such property, independent of the parent 
clock node? All I could find with quick search refers with a phandle to a 
clock node (we don't have now).

> It seems like it'd be best to get the basic resources (like clocks)
> represented in DT before trying to build blocks that use them.

    We don't use them directly. And we need the Ether device tree support 
*now*, while clock-related work will probably take months (there are plans to 
switch Sh-Mobile to CCF in like 6 months).

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/11/2013 12:07 AM, Stephen Warren wrote:

>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>> domains?

>>>>      Currently not.

>>> What does "currently" mean? Does that mean that the Linux driver simply
>>> doesn't touch those entities at present?

>>     There's Ether clock but the driver doesn't manipulate it directly,
>> assumingly it does this thru the runtime PM interface. As for the
>> others, I simply don't know.

> If there's a clock, it should be represented in DT, even if the kernel
> somehow gets access to the clock through some means other than parsing DT.

    Frankly speaking, I don't see the point.

>>> If so, that's not enough to say
>>> that those entities should not be described in the DT binding. We should
>>> strive to make the binding completely describe all aspects of the HW,
>>> irrespective of whether a particular driver happens to use that
>>> information at present.

>>     There's no DT representation for the clocks in SH-Mobile subarch yet.
>> The same applies to the other entities you mentioned.

> You can still write the binding to say that the appropriate clock
> property must be present; the overall format of this property won't be
> affected by the representation chosen for the SH-Mobile clocks.

    Where can I find an example of such property, independent of the parent 
clock node? All I could find with quick search refers with a phandle to a 
clock node (we don't have now).

> It seems like it'd be best to get the basic resources (like clocks)
> represented in DT before trying to build blocks that use them.

    We don't use them directly. And we need the Ether device tree support 
*now*, while clock-related work will probably take months (there are plans to 
switch Sh-Mobile to CCF in like 6 months).

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 03:48 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/11/2013 12:07 AM, Stephen Warren wrote:
> 
>>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>>> domains?
> 
>>>>>      Currently not.
> 
>>>> What does "currently" mean? Does that mean that the Linux driver simply
>>>> doesn't touch those entities at present?
> 
>>>     There's Ether clock but the driver doesn't manipulate it directly,
>>> assumingly it does this thru the runtime PM interface. As for the
>>> others, I simply don't know.
> 
>> If there's a clock, it should be represented in DT, even if the kernel
>> somehow gets access to the clock through some means other than parsing
>> DT.
> 
>    Frankly speaking, I don't see the point.

To be honest, that doesn't exactly make me care about reviewing the
binding then.

>>>> If so, that's not enough to say
>>>> that those entities should not be described in the DT binding. We
>>>> should
>>>> strive to make the binding completely describe all aspects of the HW,
>>>> irrespective of whether a particular driver happens to use that
>>>> information at present.
> 
>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
>>> The same applies to the other entities you mentioned.
> 
>> You can still write the binding to say that the appropriate clock
>> property must be present; the overall format of this property won't be
>> affected by the representation chosen for the SH-Mobile clocks.
> 
>    Where can I find an example of such property, independent of the
> parent clock node? All I could find with quick search refers with a
> phandle to a clock node (we don't have now).

All clock property definitions are independent of the source of the clock.

Now the actual value you put in the DT file is dependent on which source
it's describing.

>> It seems like it'd be best to get the basic resources (like clocks)
>> represented in DT before trying to build blocks that use them.
> 
>    We don't use them directly. And we need the Ether device tree support *now*, while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months). 

What/who is "we". If it's just a matter of the driver-vs-runtime-pm
code, then it's not relevant.

I don't think needing something now is a good excuse.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Stephen Warren]

On 09/10/2013 03:48 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/11/2013 12:07 AM, Stephen Warren wrote:
> 
>>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>>> domains?
> 
>>>>>      Currently not.
> 
>>>> What does "currently" mean? Does that mean that the Linux driver simply
>>>> doesn't touch those entities at present?
> 
>>>     There's Ether clock but the driver doesn't manipulate it directly,
>>> assumingly it does this thru the runtime PM interface. As for the
>>> others, I simply don't know.
> 
>> If there's a clock, it should be represented in DT, even if the kernel
>> somehow gets access to the clock through some means other than parsing
>> DT.
> 
>    Frankly speaking, I don't see the point.

To be honest, that doesn't exactly make me care about reviewing the
binding then.

>>>> If so, that's not enough to say
>>>> that those entities should not be described in the DT binding. We
>>>> should
>>>> strive to make the binding completely describe all aspects of the HW,
>>>> irrespective of whether a particular driver happens to use that
>>>> information at present.
> 
>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
>>> The same applies to the other entities you mentioned.
> 
>> You can still write the binding to say that the appropriate clock
>> property must be present; the overall format of this property won't be
>> affected by the representation chosen for the SH-Mobile clocks.
> 
>    Where can I find an example of such property, independent of the
> parent clock node? All I could find with quick search refers with a
> phandle to a clock node (we don't have now).

All clock property definitions are independent of the source of the clock.

Now the actual value you put in the DT file is dependent on which source
it's describing.

>> It seems like it'd be best to get the basic resources (like clocks)
>> represented in DT before trying to build blocks that use them.
> 
>    We don't use them directly. And we need the Ether device tree support *now*, while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months). 

What/who is "we". If it's just a matter of the driver-vs-runtime-pm
code, then it's not relevant.

I don't think needing something now is a good excuse.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/11/2013 02:21 AM, Stephen Warren wrote:

>>>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>>>> domains?

>>>>>>       Currently not.

>>>>> What does "currently" mean? Does that mean that the Linux driver simply
>>>>> doesn't touch those entities at present?

>>>>      There's Ether clock but the driver doesn't manipulate it directly,
>>>> assumingly it does this thru the runtime PM interface. As for the
>>>> others, I simply don't know.

>>> If there's a clock, it should be represented in DT, even if the kernel
>>> somehow gets access to the clock through some means other than parsing
>>> DT.

>>     Frankly speaking, I don't see the point.

> To be honest, that doesn't exactly make me care about reviewing the
> binding then.

    Well, that would seem to make my task easier unless you mean that you'd 
just NAK my further attempts of pushing this patch. :-)
    It seems somewhat unfair to require clock DT support for the Ethernet 
driver (which doesn't even use clocks directly), while R-Car I2C driver DT 
support has been queued successfully at the same time (this driver uses clock 
API directly).

>>>>> If so, that's not enough to say
>>>>> that those entities should not be described in the DT binding. We
>>>>> should
>>>>> strive to make the binding completely describe all aspects of the HW,
>>>>> irrespective of whether a particular driver happens to use that
>>>>> information at present.

>>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
>>>> The same applies to the other entities you mentioned.

>>> You can still write the binding to say that the appropriate clock
>>> property must be present; the overall format of this property won't be
>>> affected by the representation chosen for the SH-Mobile clocks.

>>     Where can I find an example of such property, independent of the
>> parent clock node? All I could find with quick search refers with a
>> phandle to a clock node (we don't have now).

> All clock property definitions are independent of the source of the clock.

> Now the actual value you put in the DT file is dependent on which source
> it's describing.

    AFAIU, we'd need the "root" clock node still to which the clock properties 
would refer via phandle.

>>> It seems like it'd be best to get the basic resources (like clocks)
>>> represented in DT before trying to build blocks that use them.

>>     We don't use them directly. And we need the Ether device tree support *now*,
>> while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months).

> What/who is "we".

    Our customer Renesas, and therefore we, Cogent Embedded. Note that clock 
DT support is not the part of our tasks.

> If it's just a matter of the driver-vs-runtime-pm
> code, then it's not relevant.

> I don't think needing something now is a good excuse.

    Hm, perhaps we still can do things in the planned order? Magnus, could you 
clarify again what are the plans on clock DT support for the R8A777x/R8A779x SoCs?
    I'm still hoping we can deal with the Ethernet DT support first and then 
we'd add the clock property to the binding when it would become availble.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Sergei Shtylyov]

Hello.

On 09/11/2013 02:21 AM, Stephen Warren wrote:

>>>>>>> Do you need any clocks properties, IP block reset signals, power
>>>>>>> domains?

>>>>>>       Currently not.

>>>>> What does "currently" mean? Does that mean that the Linux driver simply
>>>>> doesn't touch those entities at present?

>>>>      There's Ether clock but the driver doesn't manipulate it directly,
>>>> assumingly it does this thru the runtime PM interface. As for the
>>>> others, I simply don't know.

>>> If there's a clock, it should be represented in DT, even if the kernel
>>> somehow gets access to the clock through some means other than parsing
>>> DT.

>>     Frankly speaking, I don't see the point.

> To be honest, that doesn't exactly make me care about reviewing the
> binding then.

    Well, that would seem to make my task easier unless you mean that you'd 
just NAK my further attempts of pushing this patch. :-)
    It seems somewhat unfair to require clock DT support for the Ethernet 
driver (which doesn't even use clocks directly), while R-Car I2C driver DT 
support has been queued successfully at the same time (this driver uses clock 
API directly).

>>>>> If so, that's not enough to say
>>>>> that those entities should not be described in the DT binding. We
>>>>> should
>>>>> strive to make the binding completely describe all aspects of the HW,
>>>>> irrespective of whether a particular driver happens to use that
>>>>> information at present.

>>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
>>>> The same applies to the other entities you mentioned.

>>> You can still write the binding to say that the appropriate clock
>>> property must be present; the overall format of this property won't be
>>> affected by the representation chosen for the SH-Mobile clocks.

>>     Where can I find an example of such property, independent of the
>> parent clock node? All I could find with quick search refers with a
>> phandle to a clock node (we don't have now).

> All clock property definitions are independent of the source of the clock.

> Now the actual value you put in the DT file is dependent on which source
> it's describing.

    AFAIU, we'd need the "root" clock node still to which the clock properties 
would refer via phandle.

>>> It seems like it'd be best to get the basic resources (like clocks)
>>> represented in DT before trying to build blocks that use them.

>>     We don't use them directly. And we need the Ether device tree support *now*,
>> while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months).

> What/who is "we".

    Our customer Renesas, and therefore we, Cogent Embedded. Note that clock 
DT support is not the part of our tasks.

> If it's just a matter of the driver-vs-runtime-pm
> code, then it's not relevant.

> I don't think needing something now is a good excuse.

    Hm, perhaps we still can do things in the planned order? Magnus, could you 
clarify again what are the plans on clock DT support for the R8A777x/R8A779x SoCs?
    I'm still hoping we can deal with the Ethernet DT support first and then 
we'd add the clock property to the binding when it would become availble.

WBR, Sergei

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Mark Rutland]

On Wed, Oct 16, 2013 at 11:41:59PM +0100, Sergei Shtylyov wrote:
> Hello.

Hello,

> 
> On 09/11/2013 02:21 AM, Stephen Warren wrote:
> 
> >>>>>>> Do you need any clocks properties, IP block reset signals, power
> >>>>>>> domains?
> 
> >>>>>>       Currently not.
> 
> >>>>> What does "currently" mean? Does that mean that the Linux driver simply
> >>>>> doesn't touch those entities at present?
> 
> >>>>      There's Ether clock but the driver doesn't manipulate it directly,
> >>>> assumingly it does this thru the runtime PM interface. As for the
> >>>> others, I simply don't know.
> 
> >>> If there's a clock, it should be represented in DT, even if the kernel
> >>> somehow gets access to the clock through some means other than parsing
> >>> DT.
> 
> >>     Frankly speaking, I don't see the point.
> 
> > To be honest, that doesn't exactly make me care about reviewing the
> > binding then.
> 
>     Well, that would seem to make my task easier unless you mean that you'd 
> just NAK my further attempts of pushing this patch. :-)
>     It seems somewhat unfair to require clock DT support for the Ethernet 
> driver (which doesn't even use clocks directly), while R-Car I2C driver DT 
> support has been queued successfully at the same time (this driver uses clock 
> API directly).

To me it seems entirely fair that to support a device we require support
for the devices it depends upon.

The fact that the R-Car I2C driver doesn't acquire its clocks from DT is
a deficiency of the R-Car I2C binding and driver that should be fixed.
It does not excuse making the same mistake again.

We know now that we will have to describe the clocks at some point in
the future. Taking a shortcut now by relying on the board files to
provide the clocks or assuming they're handled elsewhere, and not
providing a way to provide them via the DT is lazy and actively harmful.

We _will_ have to change this in future, we know exactly how we will
have to change it, and we know that said changes will break previously
working DTBs. People _will_ complain when their board no longer
functions, and if we're serious about not having functional regressions,
we cannot remove all the ad-hoc support code that allows the incomplete
device trees to boot a useful kernel.

The current approach for converting platforms to device tree is a train
wreck, and the practice of relying on board files during the conversion
is fundamentally flawed. It allows us to take shortcuts today that we
know will cause pain for everyone further down the line, rather than
ensuring everything is working and stable from the outset.

We've admittedly been inconsistent in the review of bindings. That is a
procedural failing that we need to sort out, but that's orthogonal to
the issue at hand.

> 
> >>>>> If so, that's not enough to say
> >>>>> that those entities should not be described in the DT binding. We
> >>>>> should
> >>>>> strive to make the binding completely describe all aspects of the HW,
> >>>>> irrespective of whether a particular driver happens to use that
> >>>>> information at present.
> 
> >>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
> >>>> The same applies to the other entities you mentioned.
> 
> >>> You can still write the binding to say that the appropriate clock
> >>> property must be present; the overall format of this property won't be
> >>> affected by the representation chosen for the SH-Mobile clocks.
> 
> >>     Where can I find an example of such property, independent of the
> >> parent clock node? All I could find with quick search refers with a
> >> phandle to a clock node (we don't have now).
> 
> > All clock property definitions are independent of the source of the clock.
> 
> > Now the actual value you put in the DT file is dependent on which source
> > it's describing.
> 
>     AFAIU, we'd need the "root" clock node still to which the clock properties 
> would refer via phandle.

The binding for the device is independent from the source.The binding
can say:

- clocks: A list of clock-specifiers. Should contain a clock-specifier
          for each entry in clock-names
- clock-names: A list of strings. Should contain (in any order):
    * "apb_pclk" for the clock driving the AMBA interface (optional)
    * "refclk" for the reference clock
    * "syncclk" for the synchronisation clock, if wired up (optional)

The implementation of the clock sources has no bearing on the consumer.

The driver can do its best to try to make something work with the clocks
it is given. Some clocks may be necessary because we really need to know
their rate to do anything useful with the hardware. Some we might be
able to ignore if not present (e.g. we just assume they're always on).
Some we might not need now (maybe we don't use a portion of a device),
but might use in future.

You must have some reference manual for the hardware, so you should know
the set of clock inputs the hardware has. If you know that, you can add
the clock inputs to the binding.

> 
> >>> It seems like it'd be best to get the basic resources (like clocks)
> >>> represented in DT before trying to build blocks that use them.
> 
> >>     We don't use them directly. And we need the Ether device tree support *now*,
> >> while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months).
> 
> > What/who is "we".
> 
>     Our customer Renesas, and therefore we, Cogent Embedded. Note that clock 
> DT support is not the part of our tasks.

There are plenty of businesses involved in Linux development. Note that
their commercial interests do not justify the addition of infrastructure
with known problems. Note that maintaining DT support is part of my
tasks.

> 
> > If it's just a matter of the driver-vs-runtime-pm
> > code, then it's not relevant.
> 
> > I don't think needing something now is a good excuse.
> 
>     Hm, perhaps we still can do things in the planned order? Magnus, could you 
> clarify again what are the plans on clock DT support for the R8A777x/R8A779x SoCs?
>     I'm still hoping we can deal with the Ethernet DT support first and then 
> we'd add the clock property to the binding when it would become availble.

I would hope that in future we build DT support without crutches. If $X
depends on $Y, get $Y working, then get $X working.

Thanks,
Mark.

Re: [PATCH v2 2/2] sh_eth: add device tree support

[Mark Rutland]

On Wed, Oct 16, 2013 at 11:41:59PM +0100, Sergei Shtylyov wrote:
> Hello.

Hello,

> 
> On 09/11/2013 02:21 AM, Stephen Warren wrote:
> 
> >>>>>>> Do you need any clocks properties, IP block reset signals, power
> >>>>>>> domains?
> 
> >>>>>>       Currently not.
> 
> >>>>> What does "currently" mean? Does that mean that the Linux driver simply
> >>>>> doesn't touch those entities at present?
> 
> >>>>      There's Ether clock but the driver doesn't manipulate it directly,
> >>>> assumingly it does this thru the runtime PM interface. As for the
> >>>> others, I simply don't know.
> 
> >>> If there's a clock, it should be represented in DT, even if the kernel
> >>> somehow gets access to the clock through some means other than parsing
> >>> DT.
> 
> >>     Frankly speaking, I don't see the point.
> 
> > To be honest, that doesn't exactly make me care about reviewing the
> > binding then.
> 
>     Well, that would seem to make my task easier unless you mean that you'd 
> just NAK my further attempts of pushing this patch. :-)
>     It seems somewhat unfair to require clock DT support for the Ethernet 
> driver (which doesn't even use clocks directly), while R-Car I2C driver DT 
> support has been queued successfully at the same time (this driver uses clock 
> API directly).

To me it seems entirely fair that to support a device we require support
for the devices it depends upon.

The fact that the R-Car I2C driver doesn't acquire its clocks from DT is
a deficiency of the R-Car I2C binding and driver that should be fixed.
It does not excuse making the same mistake again.

We know now that we will have to describe the clocks at some point in
the future. Taking a shortcut now by relying on the board files to
provide the clocks or assuming they're handled elsewhere, and not
providing a way to provide them via the DT is lazy and actively harmful.

We _will_ have to change this in future, we know exactly how we will
have to change it, and we know that said changes will break previously
working DTBs. People _will_ complain when their board no longer
functions, and if we're serious about not having functional regressions,
we cannot remove all the ad-hoc support code that allows the incomplete
device trees to boot a useful kernel.

The current approach for converting platforms to device tree is a train
wreck, and the practice of relying on board files during the conversion
is fundamentally flawed. It allows us to take shortcuts today that we
know will cause pain for everyone further down the line, rather than
ensuring everything is working and stable from the outset.

We've admittedly been inconsistent in the review of bindings. That is a
procedural failing that we need to sort out, but that's orthogonal to
the issue at hand.

> 
> >>>>> If so, that's not enough to say
> >>>>> that those entities should not be described in the DT binding. We
> >>>>> should
> >>>>> strive to make the binding completely describe all aspects of the HW,
> >>>>> irrespective of whether a particular driver happens to use that
> >>>>> information at present.
> 
> >>>> There's no DT representation for the clocks in SH-Mobile subarch yet.
> >>>> The same applies to the other entities you mentioned.
> 
> >>> You can still write the binding to say that the appropriate clock
> >>> property must be present; the overall format of this property won't be
> >>> affected by the representation chosen for the SH-Mobile clocks.
> 
> >>     Where can I find an example of such property, independent of the
> >> parent clock node? All I could find with quick search refers with a
> >> phandle to a clock node (we don't have now).
> 
> > All clock property definitions are independent of the source of the clock.
> 
> > Now the actual value you put in the DT file is dependent on which source
> > it's describing.
> 
>     AFAIU, we'd need the "root" clock node still to which the clock properties 
> would refer via phandle.

The binding for the device is independent from the source.The binding
can say:

- clocks: A list of clock-specifiers. Should contain a clock-specifier
          for each entry in clock-names
- clock-names: A list of strings. Should contain (in any order):
    * "apb_pclk" for the clock driving the AMBA interface (optional)
    * "refclk" for the reference clock
    * "syncclk" for the synchronisation clock, if wired up (optional)

The implementation of the clock sources has no bearing on the consumer.

The driver can do its best to try to make something work with the clocks
it is given. Some clocks may be necessary because we really need to know
their rate to do anything useful with the hardware. Some we might be
able to ignore if not present (e.g. we just assume they're always on).
Some we might not need now (maybe we don't use a portion of a device),
but might use in future.

You must have some reference manual for the hardware, so you should know
the set of clock inputs the hardware has. If you know that, you can add
the clock inputs to the binding.

> 
> >>> It seems like it'd be best to get the basic resources (like clocks)
> >>> represented in DT before trying to build blocks that use them.
> 
> >>     We don't use them directly. And we need the Ether device tree support *now*,
> >> while clock-related work will probably take months (there are plans to switch Sh-Mobile to CCF in like 6 months).
> 
> > What/who is "we".
> 
>     Our customer Renesas, and therefore we, Cogent Embedded. Note that clock 
> DT support is not the part of our tasks.

There are plenty of businesses involved in Linux development. Note that
their commercial interests do not justify the addition of infrastructure
with known problems. Note that maintaining DT support is part of my
tasks.

> 
> > If it's just a matter of the driver-vs-runtime-pm
> > code, then it's not relevant.
> 
> > I don't think needing something now is a good excuse.
> 
>     Hm, perhaps we still can do things in the planned order? Magnus, could you 
> clarify again what are the plans on clock DT support for the R8A777x/R8A779x SoCs?
>     I'm still hoping we can deal with the Ethernet DT support first and then 
> we'd add the clock property to the binding when it would become availble.

I would hope that in future we build DT support without crutches. If $X
depends on $Y, get $Y working, then get $X working.

Thanks,
Mark.